-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Windows: update side-aware modifiers information on state change #4235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
| let mut state = ModifiersState::empty(); | ||
| let mut pressed_mods = ModifiersKeys::empty(); | ||
|
|
||
| pressed_mods.set(ModifiersKeys::LSHIFT, key_pressed(VK_LSHIFT)); | ||
| pressed_mods.set(ModifiersKeys::RSHIFT, key_pressed(VK_RSHIFT)); | ||
| state.set( | ||
| ModifiersState::SHIFT, | ||
| pressed_mods.contains(ModifiersKeys::LSHIFT) | ||
| || pressed_mods.contains(ModifiersKeys::RSHIFT), | ||
| ); | ||
|
|
||
| pressed_mods.set(ModifiersKeys::LCONTROL, key_pressed(VK_LCONTROL) && !filter_out_altgr); | ||
| pressed_mods.set(ModifiersKeys::RCONTROL, key_pressed(VK_RCONTROL) && !filter_out_altgr); | ||
| state.set( | ||
| ModifiersState::CONTROL, | ||
| pressed_mods.contains(ModifiersKeys::LCONTROL) | ||
| || pressed_mods.contains(ModifiersKeys::RCONTROL), | ||
| ); | ||
|
|
||
| pressed_mods.set(ModifiersKeys::LALT, key_pressed(VK_LMENU) && !filter_out_altgr); | ||
| pressed_mods.set(ModifiersKeys::RALT, key_pressed(VK_RMENU) && !filter_out_altgr); | ||
| state.set( | ||
| ModifiersState::ALT, | ||
| pressed_mods.contains(ModifiersKeys::LALT) | ||
| || pressed_mods.contains(ModifiersKeys::RALT), | ||
| ); | ||
|
|
||
| pressed_mods.set(ModifiersKeys::LMETA, key_pressed(VK_LWIN)); | ||
| pressed_mods.set(ModifiersKeys::RMETA, key_pressed(VK_RWIN)); | ||
| state.set( | ||
| ModifiersState::META, | ||
| pressed_mods.contains(ModifiersKeys::LMETA) | ||
| || pressed_mods.contains(ModifiersKeys::RMETA), | ||
| ); | ||
| Modifiers::new(state, pressed_mods) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we roll back to old behavior where the check of key_pressed(VK_CONTROL) and such was used, except for META key where this is how it's being detected anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would you waste another system API call to get information you have already requested?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure that it matches? Could you link docs saying so, so the change is equivalent and it won't regress existing code, especially considering that we have virtual mods, etc?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find docs that say this with 100% certainty, only that it seems to follow from the description.
https://learn.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-getasynckeystate
You can use the virtual-key code constants VK_SHIFT, VK_CONTROL, and VK_MENU as values for the vKey parameter. This gives the state of the SHIFT, CTRL, or ALT keys without distinguishing between left and right.
You can use the following virtual-key code constants as values for vKey to distinguish between the left and right instances of those keys.
Also checked it with on-screen keyboards, but that, of course, doesn't guarantee very much either.
Do you have any key tests in Winit that are capable of verifying that?
Or is making ~33% more calls than needed the only way to avoid this risk?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well more calls is only in a bad state, arguably, you can check for virtual, and only if it's present, check for left/right specifically, which would save calls on average, since most of the time you have only one modifier pressed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, good point
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, remember another reason for the current design - with altgr you can't use the side-agnostic state of Ctrl/Alt to then decide whether to check for the left/right sides, so the old approach only works for Shift.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then it's an example of itself that the left/right are not attached to modifier itself. Like if logical is not active, but e.g. right is active, it still means that modifier is not active.
So e.g. modifier input is not applied to compose input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, this is an example of how Windows is poorly designed that can be partially fixed within Winit, that's all.
Like if logical is not active, but e.g. right is active, it still means that modifier is not active.
No, it will be logically active since on Windows AltGr = RAlt+LCtrl, so you can't have Alt not active, but Right Alt active, both will be active
compose input
Again, this is not compose input, it's Ctrl+Alt = AltGr modifier input
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Anyway, updated for Shift where this actually saves 1 call, thanks for the tip!
|
By the way, do you know why on every single mouse event you get modifier update calls if only keyboard events change modifier state? Isn't that excessive? |
modifier can change without any key press or when key press was not visible to your application, so when you click mouse button the current modifier state will be considered, since modifiers are also carried on mouse events. IIRC. |
But not without a key/mod change event which are tracked separately? Or in what case would happen and not be caught by Winit?
Can this happen when app has focus? Otherwise there is already a modifier update on when app gains focus. Maybe similarly a cheaper way would be to update modifiers when mouse is within app's window area (if focus change isn't enough). |
What if the window was out of focus though?
I'm not sure, I know that on e.g. X11 it's utterly broken, since sometimes you have input that was send by other application to you and it relies on modifiers that were sent in those synthetic-ish events. It's just very easy to break things like that in general if you under update. I'm pro updating less if we surely can do so, but I just know that any platform that is not Wayland has issues with that. |
I noted that below - there is already code that updates mod state on regaining focus. The skipping while out-of-focus part makes sense Appreciate how overpolling can mask some bugs, just that it seems rather wasteful with all the potential high frequency of mouse input, so though whether it was done as a generic precaution or as an attempt to fix a specific Windows issue. (> since sometimes you have input that was send by other application to you and it relies on modifiers that were sent in those synthetic-ish events. and there is no way to add an update only on those events?) |
If those can be detected then it should be done that way. I'm not a windows dev, so hard for me to say on that matter. Generally, we can relax the checks on mouse input. |
c56d9de to
0603ea5
Compare
0603ea5 to
96feb00
Compare
changelogmodule if knowledge of this change could be valuable to usersAddresses #3611, but won't completely fix it since the physical state of modifier keys is sometimes unknown to Winit since it doesn't track the raw physical keys