Add X11 opt-in function for device events #2195

chrisduerr · 2022-02-16T20:16:34Z

Previously on X11, by default all global events were broadcasted to
every Winit application. This unnecessarily drains battery due to
excessive CPU usage when moving the mouse.

To resolve this, device events are now ignored by default and users must
manually opt into it using EventLoop::set_filter_device_events.

Fixes (#1634) on Linux.

kchibisov

Could you make it part of the event loop builder. ~~And probably a unix extension only for x11? I'm not sure that we'd need that for other platforms.~~ Also, have you verified that it fixes the underlying issue?

Ah, it's required for all platforms, so I'd assume just putting it on window builder should be enough.

chrisduerr · 2022-02-17T10:08:52Z

And probably a unix extension only for x11? I'm not sure that we'd need that for other platforms.

Just to clarify this: Even if it was only needed on X11 I'd try to make it available on all platforms just so downstream doesn't have to put it behind a cfg. Unless the functionality can't be implemented on some platforms, there's no reason not to offer it. In this case it would just be no-op without any issues obviously.

Also, have you verified that it fixes the underlying issue?

Yup, tested that it works at startup, tested that it works manually activating/deactivating repeatedly. And verified that the CPU usage does actually stay at zero whenever device events are filtered out. So it works like a charm.

This is also why I'm not sure if a builder is the right place for it though: It is possible to change this at runtime without any problem, so why would we force people to only set it at startup? And there's no reason to have both really because you don't get events right after event loop creation anyway so doing it in a separate statement right after EventLoop::new doesn't change anything.

So what benefit do we gain by putting it in the builder? As far as I can tell, none.

madsmtm

A benefit of having it on the EventLoopBuilder is that we expose slightly less functionality, giving us more freedom further down the line (as an example, a proper macOS implementation possibly requires changing whether a method is implemented or not, and things can definitely be made more efficient).

Also, please ensure the examples that use DeviceEvent work after this change.

madsmtm · 2022-02-17T12:49:00Z

src/event_loop.rs

+    /// Change [`DeviceEvent`] filter mode.
+    ///
+    /// Since the [`DeviceEvent`] capture can lead to high CPU usage for unfocused windows, winit
+    /// will ignore them by default on Linux/BSD. This method allows changing this filter at


Wayland is not implemented yet

Suggested change

/// will ignore them by default on Linux/BSD. This method allows changing this filter at

/// will ignore them by default on X11. This method allows changing this filter at

I've intentionally chosen this wording because they already get ignored on Wayland, simply because there is no other option.

madsmtm · 2022-02-17T12:49:36Z

CHANGELOG.md

@@ -13,6 +13,7 @@ And please only add new entries to the top of this list, right below the `# Unre
 - **Breaking:** Bump `ndk` version to 0.6, ndk-sys to `v0.3`, `ndk-glue` to `0.6`.
 - Remove no longer needed `WINIT_LINK_COLORSYNC` environment variable.
 - **Breaking:** Rename the `Exit` variant of `ControlFlow` to `ExitWithCode`, which holds a value to control the exit code after running. Add an `Exit` constant which aliases to `ExitWithCode(0)` instead to avoid major breakage. This shouldn't affect most existing programs.
+- **Breaking:** On X11, device events are now ignored by default, use `EventLoop::set_filter_device_events` to opt-into device events again.


I'm not sure making it disabled by default is a good idea while implementations for the other platforms don't exist.

Disabling it is the way it should have always been and the way it already is on Wayland. This just improves the consistency with the Linux backends.

It's not really true, we have some device events on Wayland, like pointer motion information which is something used by e.g. games.

The "correct" default would be to have device events be tied to window focus, which (AIUI) is what we get on Wayland. You'd then opt out of this behaviour on platforms where that's allowed.

src/event_loop.rs

chrisduerr · 2022-02-17T19:13:56Z

A benefit of having it on the EventLoopBuilder is that we expose slightly less functionality

I wouldn't say providing less functionality is a benefit.

more freedom further down the line (as an example, a proper macOS implementation possibly requires changing whether a method is implemented or not, and things can definitely be made more efficient).

I don't know anything about macOS, but as a maintainer I'd assume you should be able to tell if this API can be made to work with macOS? This is the best API for the platform I've written the patch for, I can't say anything about the other platforms.

Also, please ensure the examples that use DeviceEvent work after this change.

I've verified this works against Alacritty. Both changing it at startup and at runtime does exactly what you'd expect.

src/event_loop.rs

maroider · 2022-02-24T08:42:47Z

CHANGELOG.md

@@ -13,6 +13,7 @@ And please only add new entries to the top of this list, right below the `# Unre
 - **Breaking:** Bump `ndk` version to 0.6, ndk-sys to `v0.3`, `ndk-glue` to `0.6`.
 - Remove no longer needed `WINIT_LINK_COLORSYNC` environment variable.
 - **Breaking:** Rename the `Exit` variant of `ControlFlow` to `ExitWithCode`, which holds a value to control the exit code after running. Add an `Exit` constant which aliases to `ExitWithCode(0)` instead to avoid major breakage. This shouldn't affect most existing programs.
+- **Breaking:** On X11, device events are now ignored by default, use `EventLoop::set_filter_device_events` to opt-into device events again.


The "correct" default would be to have device events be tied to window focus, which (AIUI) is what we get on Wayland. You'd then opt out of this behaviour on platforms where that's allowed.

kchibisov · 2022-02-24T08:49:40Z

Could probably do the following:

pub enum DeviceEventFilter {
    /// Always filter device events.
    Always
    /// Only receive device events when the window is being focused.
    Focused
    /// Don't filter device events.
    Never
}

And then use Focused by default. The naming could change, but we certainly could go beyond a simple boolean.

chrisduerr · 2022-04-09T22:06:13Z

Should now only filter device events for unfocused windows by default.

kchibisov · 2022-04-09T22:54:20Z

CHANGELOG.md

@@ -32,6 +32,7 @@ And please only add new entries to the top of this list, right below the `# Unre
 - On Wayland, fix polling during consecutive `EventLoop::run_return` invocations.
 - On Windows, fix race issue creating fullscreen windows with `WindowBuilder::with_fullscreen`
 - On Android, `virtual_keycode` for `KeyboardInput` events is now filled in where a suitable match is found.
+- **Breaking:** On X11, device events are now ignored for unfocused windows by default, use `EventLoop::set_device_event_filter` to set the filter level.


Should we mention that it's on EventLoopWindowTarget, I think it's correct for that, since the run and run_return callbacks take target?

madsmtm · 2022-04-17T15:21:15Z

src/event_loop.rs

+    /// Change [`DeviceEvent`] filter mode.
+    ///
+    /// Since the [`DeviceEvent`] capture can lead to high CPU usage for unfocused windows, winit
+    /// will ignore them by default for unfocused windows on Linux/BSD. This method allows changing


Just checked, unfocused applications do not receive DeviceEvents on macOS either (apart from DeviceEvent::MouseWheel when hovering over an unfocused window)

madsmtm

Re putting this on EventLoopBuilder (static configuration before creation) vs. EventLoop (dynamically after creation, but before running) vs. EventLoopWindowTarget (dynamically while running):

Of course it is possible to change this setting dynamically on macOS, but given that the reason we're even making this change is for efficiency reasons, the question becomes more "would it be somewhat efficient?".

The answer here is a bit more nuanced, and I haven't actually implemented it so I can't say for sure, but I know it would be at least somewhat nontrivial to ensure thread safety of the underlying functions needed like CFRunLoopAddSource/CFRunLoopRemoveSource / method swizzling. I also do know that only having the possibility to change this once, before the event loop is created, is definitely the most efficient option.

But I guess my objection mostly arises from different values: I want to expose the minimal set of APIs that solves people's problems - if we then find a concrete use-case that need us to expose something more powerful, I'm up for it, but until then the more free to change implementation details we can be, the better.

(Aside: set_device_event_filter(DeviceEventFilter::Always) would probably require root access on macOS, so maybe it should return a Result?)

kchibisov · 2022-05-17T13:47:33Z

(Aside: set_device_event_filter(DeviceEventFilter::Always) would probably require root access on macOS, so maybe it should return a Result?)

We can say that it's a no-op. Given that on Wayland you'd need something like that as well + direct libinput access. So I don't think we should return result, just saying that it's not-supported is the way to go, given that running things under root isn't something anyone should do.

Changing on the fly could be required for global hotkeys like options, so if the user added global hotkey it must be changed on the fly.

Previously on X11, by default all global events were broadcasted to every Winit application. This unnecessarily drains battery due to excessive CPU usage when moving the mouse. To resolve this, device events are now ignored by default and users must manually opt into it using `EventLoopWindowTarget::set_filter_device_events`. Fixes (rust-windowing#1634) on Linux.

wusyong · 2022-07-06T06:06:17Z

src/event_loop.rs

+    /// - **Wayland / Windows / macOS / iOS / Android / Web**: Unsupported.
+    ///
+    /// [`DeviceEvent`]: crate::event::DeviceEvent
+    pub fn set_device_event_filter(&mut self, _filter: DeviceEventFilter) {


There isn't a DerefMut implementation for EventLoop to EventLoopWindowTarget.
Do you think we should add it? Or keep it immutable reference only and find workaround.

This was addressed in #2323

maroider added DS - x11 C - waiting on maintainer A maintainer must review this code labels Feb 16, 2022

kchibisov requested changes Feb 16, 2022

View reviewed changes

madsmtm requested changes Feb 17, 2022

View reviewed changes

kchibisov approved these changes Feb 24, 2022

View reviewed changes

maroider reviewed Feb 24, 2022

View reviewed changes

chrisduerr force-pushed the no_device_events branch 2 times, most recently from 62f2090 to 9efeefc Compare April 9, 2022 22:05

chrisduerr force-pushed the no_device_events branch from 7ae0217 to 81383df Compare April 9, 2022 22:34

kchibisov reviewed Apr 9, 2022

View reviewed changes

kchibisov approved these changes Apr 9, 2022

View reviewed changes

kchibisov requested review from maroider and madsmtm April 9, 2022 23:11

kchibisov added this to the Version 0.27 milestone Apr 11, 2022

madsmtm reviewed Apr 17, 2022

View reviewed changes

madsmtm requested changes Apr 17, 2022

View reviewed changes

kchibisov force-pushed the no_device_events branch from 8b658a9 to a927863 Compare June 7, 2022 20:52

kchibisov force-pushed the no_device_events branch from a927863 to e232ec1 Compare June 7, 2022 20:53

kchibisov merged commit f10a984 into rust-windowing:master Jun 7, 2022

chrisduerr deleted the no_device_events branch June 8, 2022 02:04

maroider mentioned this pull request Jun 13, 2022

Duplicate device events on X11 #2332

Closed

wusyong reviewed Jul 6, 2022

View reviewed changes

ajtribick mentioned this pull request Aug 5, 2022

Add Windows opt-in function for device events #2409

Merged

10 tasks

maroider mentioned this pull request Dec 15, 2022

DeviceEventFilter works differently on X11 and Windows #2591

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add X11 opt-in function for device events #2195

Add X11 opt-in function for device events #2195

chrisduerr commented Feb 16, 2022

kchibisov left a comment •

edited

Loading

chrisduerr commented Feb 17, 2022

madsmtm left a comment

madsmtm Feb 17, 2022

chrisduerr Feb 17, 2022

madsmtm Feb 17, 2022

chrisduerr Feb 17, 2022

kchibisov Feb 17, 2022

maroider Feb 24, 2022

chrisduerr commented Feb 17, 2022

maroider Feb 24, 2022

kchibisov commented Feb 24, 2022

chrisduerr commented Apr 9, 2022

kchibisov Apr 9, 2022

chrisduerr Apr 9, 2022

madsmtm Apr 17, 2022 •

edited

Loading

madsmtm left a comment

kchibisov commented May 17, 2022

wusyong Jul 6, 2022

maroider Jul 6, 2022

	/// will ignore them by default on Linux/BSD. This method allows changing this filter at
	/// will ignore them by default on X11. This method allows changing this filter at

Add X11 opt-in function for device events #2195

Add X11 opt-in function for device events #2195

Conversation

chrisduerr commented Feb 16, 2022

kchibisov left a comment • edited Loading

Choose a reason for hiding this comment

chrisduerr commented Feb 17, 2022

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisduerr commented Feb 17, 2022

Choose a reason for hiding this comment

kchibisov commented Feb 24, 2022

chrisduerr commented Apr 9, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsmtm Apr 17, 2022 • edited Loading

Choose a reason for hiding this comment

madsmtm left a comment

Choose a reason for hiding this comment

kchibisov commented May 17, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment •

edited

Loading

madsmtm Apr 17, 2022 •

edited

Loading