Move ModifiersChanged variant to WindowEvent
#1381
Conversation
|
@vberger It was discovered that this is not the case for GNOME + Wayland, and you can't partially revert this on it... So I guess we should live with 2 events on sway. |
|
@kchibisov Actually, given what you describe I suspect there might be some bug in SCTK. Could you share a
|
|
@vberger Here are 2 logs from glutin window example, one from gnome and one from sway. I was doing the thing you've described here and using |
goddessfreya
added
S - api
|
@kchibisov Ok, thanks. The main difference seems to be that Gnome sends the |
I haven't look too deep at what this does internally, but at least cargo-check is fully happy now. :) Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
|
So, after digging at macOS a bit, I have a working reproduction that I can work with: 🔎 trace of |
From debugging, I determined that macOS' emission of a flagsChanged around window switching is inconsistent. It is fair to assume, I think, that when the user switches windows, they do not expect their former modifiers state to remain effective; so I think it's best to clear that state by sending a ModifiersChanged(ModifiersState::empty()). Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
I don't know enough about the code to implement the fix as it is done on this branch, but this commit at least fixes the build. Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
macOS: Add fix for ModifiersChanged relocation
windows: Fix build
|
@rye That sounds right. Thank you for contributing the Mac and Windows implementations. |
Very similar to the changes made in [1], as focus is lost, send an event to the window indicating that the modifiers have been released. It's unclear to me (without a Windows device to test this on) whether this is necessary, but it certainly ensures that unfocused windows will have at least received this event, which is an improvement. [1]: f79f216 Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
…-focus windows: Send ModifiersChanged(ModifiersState::empty) on KILLFOCUS
| @@ -319,6 +319,7 @@ extern "C" fn window_did_become_key(this: &Object, _: Sel, _: id) { | |||
| extern "C" fn window_did_resign_key(this: &Object, _: Sel, _: id) { | |||
| trace!("Triggered `windowDidResignKey:`"); | |||
| with_state(this, |state| { | |||
| state.emit_event(WindowEvent::ModifiersChanged(ModifiersState::empty())); | |||
There was a problem hiding this comment.
I've played around with things on macOS too, but haven't gotten a chance to send a PR yet. But based on my understanding, this would break when focus is lost with a modifier pressed and the modifier is still held when the window is focused again, right?
Based on my testing, there is no flags changed emitted when the window is focused or unfocused, so sending an empty on unfocus only resets things to none, but it doesn't correctly make sure that the modifier is reapplied on enter?
There was a problem hiding this comment.
That is correct—the changes I made did not re-apply the former pre-unfocus modifiers once focus is regained. The Wayland implementation does have and use a modifiers_tracker, but I didn't find something that was already built that I could use, besides the modifiers field on ViewState. I also wasn't sure if we even needed it (modifiers tracking), since in most of the cases I tested I had to release all modifiers to regain focus anyhow. (The only exception is mousing out of the window, but personally I'd still expect to have to freshly hold down a modifier if I want to use it, I guess.)
Is there a way we can figure out (e.g. by querying or looking at a field) what the current modifiers are when focus is regained? I'm not sure what the best approach here is as I have absolutely zero experience with macOS window development. What is "correct" doesn't seem to be clear to me, besides clearing modifiers when the window is unfocused.
I was primarily targeting the ⌘-TAB use case here, so I'd definitely be open to additional ideas if you have them.
There was a problem hiding this comment.
since in most of the cases I tested I had to release all modifiers to regain focus anyhow
This is definitely not the case. In my testing it was quite trivial to do this.
Is there a way we can figure out (e.g. by querying or looking at a field) what the current modifiers are when focus is regained?
When I looked at it, it didn't seem like the state we have on the focus callback would give us access to it. I have no experience with macOS either, though I was able to write a reliably working solution that uses key events in addition to flag changes. You can check if state.modifiers != event_mods and then emit an event and set state.modifiers to event_mods.
That seemed to work pretty well, though I have not tested it sufficiently to determine if this solution would work reliably in all scenarios. But based on my initial estimate, it at least seemed to give good feedback for the key events.
The problem of course is that we'd have to wait until a key event to update the modifiers, which means that they are not correct until another key is pressed.
There was a problem hiding this comment.
This is definitely not the case.
Yeah, I realized this after I wrote it. The mouse is a thing, after all. :)
As I thought about it more, I realized it'd work to do this check within mouse_click, key_down, and key_up within the view module. That check would be fairly straightforward to do, and since these all come from NSEvent the modifierFlags should definitely be available, and checkable via event_mods as you suggested.
I'll go ahead and PR that as well.
There was a problem hiding this comment.
As I thought about it more, I realized it'd work to do this check within mouse_click, key_down, and key_up within the view module. That check would be fairly straightforward to do, and since these all come from NSEvent the modifierFlags should definitely be available, and checkable via event_mods as you suggested.
That's not entirely accurate though. If a modifier is released while the window is not in focus, you won't accurately be able to tell pressed modifiers until one of these actions occurs, even though your window already has full focus. So that's definitely a bit strange.
I feel like there should be a solution without caveats like this, but I haven't had the time to look more into that yet.
There was a problem hiding this comment.
@chrisduerr, yeah, that's right. The fundamental problem is that we don't seem to be able to handle a NSEvent when focus is regained, because such an event isn't emitted. If such an event were emitted, we could just grab the modifiers out of it.
I've done quite a bit of digging now and I'm still getting more and more confident that our answer will be to just check modifiers and queue up an event to update state.modifiers if necessary.
While the docs I linked above do say that
You can retrieve the window object in question by sending
objecttonotification,
the "window object" one would retrieve would be a NSWindow, which does not expose any modifiers information. As such, in the context of handling a didResignKey or didBecomeKey event, we simply don't have access to the modifier information.
So I think this is a protocol-level issue: our notification of gaining or losing focus gets fired on the NSWindowDelegate, not on the NSView, where keyboard, mouse, and "flags" events get fired, and indeed no event is consistently fired after regaining focus on our NSView that would let us get updated modifiers. Unless I'm missing something, that'd mean we don't really have a way, besides periodically checking the provided modifiers with an event that fires and sending a fresh event before sending the event that we got.
I've got another PR into this branch that will add this check in a couple of key spots where I think user interaction is most likely. And of course, if ModifiersChanged gets fired, then state.modifiers will be consistent and events will not get doubly fired.
There was a problem hiding this comment.
How does this code interact with the new changes? I'd imagine it might not be necessary anymore?
What happens if a modifier is held, the window is unfocused and the modifier is still held when entering again? Does this cause an empty event without another modifier event because the internal window state never let it go?
There was a problem hiding this comment.
I'd imagine it might not be necessary anymore?
That's an interesting question, and I think you're right that it's probably not be necessary to keep it. Anecdotally, commenting out this line did not change the fact that Alacritty is fixed by this PR.
One point of contention is that the Wayland impl is emitting an empty event and then another to restore it when focus is restored. But the Wayland event lifecycle is not the same as on macOS.
What happens if a modifier is held, the window is unfocused and the modifier is still held when entering again? Does this cause an empty event without another modifier event because the internal window state never let it go?
In this case, the window losing focus triggers an emptying event, and regaining focus does not trigger an event until selected further user interaction happens. (Either keypress, mouse click, mouse enter, mouse exit, scrolling, or pressure change.) You're right that we'd be firing an empty event. I suppose since we now have a check before actually sending out events for user interaction, we no longer need to send out empty modifiers state, since windows might not want to get a synthetic event like that.
I should also do this check (potentially updating stale modifiers) on mouse_motion, I think, since it's pretty lightweight.
Sometimes, `ViewState` and `event` might have different values for their stored `modifiers` flags. These are internally stored as a bitmask in the latter and an enum in the former. We can check to see if they differ, and if they do, automatically dispatch an event to update consumers of modifier state as well as the stored `state.modifiers`. That's what the hook does. This hook is then called in the key_down, mouse_entered, mouse_exited, mouse_click, scroll_wheel, and pressure_change_with_event callbacks, which each will contain updated modifiers. Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
This is so that we can interact with the ViewState directly from the WindowDelegate. Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
This logic is implemented similarly on other platforms, so we wish to regain parity here. Originally this behavior was implemented to always fire an event with ModifiersState::empty(), but that was not the best as it was not necessarily correct and could be a duplicate event. This solution is perhaps the most elegant possible to implement the desired behavior of sending a synthetic empty modifiers event when a window loses focus, trading some safety for interoperation between the NSWindowDelegate and the NSView (as the objc runtime must now be consulted in order to acquire access to the ViewState which is "owned" by the NSView). Signed-off-by: Kristofer Rye <kristofer.rye@gmail.com>
macOS: Add synthetic empty on resignKey
|
@rye I haven't looked at the code, but if we emit an event on focus loss, wouldn't it be possible to do the same on focus? That should make it possible to get rid of the events that are setup everywhere and instead only use the flags changed, focus and unfocus hooks. |
|
@chrisduerr I seem to remember trying that and I think I concluded that
More explicitly, the type of the event It'd be possible to possibly stash the stale modifiers state and re-emit it, but then again, we don't have up-to-date modifiers information until another event is fired on our view. |
|
@rye Gotcha, so your unsafety shenanigans are just used to send the event and update the modifier state. I thought they might have allowed you to access the current modifier state of the window. |
|
@chrisduerr exactly, yep; unsafety here is used to get (and update) the current view's |
|
I've taken a look at the Windows code and swallowing the modifiers when the window isn't focused and sending the modifier state on focus is trivial. The only problem is checking if the window is focused in the I also found out that this hasn't even been handled correctly in the current version of the code, which means that the |
Check for modifiers change in window events
|
@murarth Seems like the github runner failure was caused by rustwasm/wasm-bindgen#2009, which should be fixed in the latest nightly. Might be worth restarting to get all greens? |
|
@ryanisaacg Could you take another look at my latest Windows changes, just to verify it does actually work? I think this is still missing a Windows review. The previous comments by @murarth should help understand the intended behavior, if that is still uncertain. |
|
Sure, I'll take a look this weekend. |
|
Just for information, I've got some patches that should further improve macOS handling and one that should fix an outstanding bug with mouse reports. But it might take a bit until I can get up the PRs for that. It'll definitely be sometime this weekend though. |
|
Windows looks good to me; matches expected behavior. |
Since the `mouse_entered` function was generating a mouse motion, which updates the modifier state, a modifiers changed event was incorrectly generated. The updating of the modifier state has also been changed to make sure it consistently happens before events that have a modifier state attached to it, without happening on any other event. This of course means that no `CursorMoved` event is generated anymore when the user enters the window without it being focused, however I'd say that is consistent with how winit should behave.
Fix modifier changed event on macOS
|
@vbogaevsky could you give it another test on macOS? I've made some changes that should address the remaining differences between Linux/Windows and macOS. |
|
@rye You've implemented the initial macOS version, could you maybe help with testing the latest changes if @vbogaevsky isn't around to help out? I'd really like to get this tested asap, so outstanding issues can be resolved quickly and this can finally be merged. |
|
@chrisduerr, I certainly will. I'll double check macOS in the next hour or so. |
|
Thanks a ton for the quick response. I think the main thing are the issues mentioned in @vbogaevsky last review and the expected platform behavior mentioned by @murarth. |
|
|
The |
Fixes #1365
I've only implemented the change for X11 and Wayland. Others will need to add Mac and Windows implementations to this PR.
cargo fmthas been run on this branchcargo docbuilds successfullyCHANGELOG.mdif knowledge of this change could be valuable to users