Skip to content
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

Handle winit's WindowEvent::ModifiersChanged #13357

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

matthunz
Copy link

@matthunz matthunz commented May 13, 2024

Objective

Solution

  • This moves handling modifiers from WindowEvent::KeyboardInput to ModifiersChanged. Currently these are the only events triggered when MacOS native overlays interrupt the app.

Testing

Works for me on MacOS sonoma 😄 no breaking changes that I can see

Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@matthunz matthunz changed the title Handle WindowEvent::ModifiersChanged Handle winit's WindowEvent::ModifiersChanged May 13, 2024
@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Input Player input via keyboard, mouse, gamepad, and more A-Windowing Platform-agnostic interface layer to run your app in O-MacOS Specific to the MacOS (Apple) desktop operating system S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 13, 2024
// Send a `KeyboardInput` event if this key is not a modifier.
let keyboard_event = converters::convert_keyboard_input(event, window);
let mod_keys = [Key::Alt, Key::Control, Key::Shift, Key::Super];
if !mod_keys.contains(&keyboard_event.logical_key) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to make this re-sending conditional? Don't we want to retain the old behavior on all other platforms (which don't send ModifiersChanged)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Huh sorry I didn't realize ModifiersChanged isn't triggered on other platforms. Windows looks to be working again in
c25df8f

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I left the conditional re-sending on MacOS to avoid duplicating events - WindowEvent::KeyboardInput and ModifiersChanged seem to be triggered at the same time except with the native overlay scenario

@mockersf
Copy link
Member

mockersf commented May 13, 2024

this feels like it should be fixed in Winit rather than in Bevy

in the issue you mention this works correctly with winit example, so I would prefer more investigation done before doing that many platform-specific things there

@matthunz
Copy link
Author

matthunz commented May 13, 2024

this feels like it should be fixed in Winit rather than in Bevy

I totally agree 👍 (unless winit does have a reason)

What I saw in that example is winit does provide the input events, just in this alternative way. It feels weird for them to provide a KeyboardInput event that works on every platform, but a specific ModifiersChanged for MacOS

@matthunz
Copy link
Author

I got some more information from kchibisov on the winit Matrix

Not 100% sure I understand but it does sound like we'd still want to handle WindowEvent::ModifiersChanged to guarantee ordering and sticky key support

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior O-MacOS Specific to the MacOS (Apple) desktop operating system S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

KeyboardInput events not being triggered by MacOS native overlays
3 participants