Skip to content

Conversation

@murarth
Copy link
Contributor

@murarth murarth commented Aug 28, 2019

  • Tested on all platforms changed
  • Compilation warnings were addressed
  • cargo fmt has been run on this branch
  • Added an entry to CHANGELOG.md if knowledge of this change could be valuable to users
  • Updated documentation to reflect any user-facing changes, including notes of platform-specific behavior
  • Created or updated an example program if it would help users understand this functionality
  • Updated feature matrix, if new features were added or implemented

Copy link
Contributor

@elinorbgr elinorbgr left a comment

Choose a reason for hiding this comment

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

LGTM for the wayland part, cannot tell for the x11 one.

@goddessfreya
Copy link
Contributor

goddessfreya commented Sep 5, 2019

ModifiersState seams to be functional on X11, but I don't think I ever saw the ModifierChanged event. but ModifierChanged didn't seam to be consistently showing up. but ModifierChanged didn't update when you moved your mouse off the window, so you couldn't know that the keys were released/held.

$ cargo run --example window | grep "EventsCleared\|NewEvents\|Keyboard\|Modifier"
<SNIP>
NewEvents(WaitCancelled { start: Instant { tv_sec: 167749, tv_nsec: 641587350 }, requested_resume: None })
DeviceEvent { device_id: DeviceId(X(DeviceId(10))), event: Key(KeyboardInput { scancode: 42, state: Pressed, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } }) }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 42, state: Pressed, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: true } } } }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: ModifiersChanged { device_id: DeviceId(X(DeviceId(3))), modifiers: ModifiersState { shift: true, ctrl: false, alt: false, logo: true } } }
EventsCleared
NewEvents(WaitCancelled { start: Instant { tv_sec: 167749, tv_nsec: 817716417 }, requested_resume: None })
DeviceEvent { device_id: DeviceId(X(DeviceId(10))), event: Key(KeyboardInput { scancode: 42, state: Released, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } }) }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 42, state: Released, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: true, ctrl: false, alt: false, logo: true } } } }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: ModifiersChanged { device_id: DeviceId(X(DeviceId(3))), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: true } } }
EventsCleared
NewEvents(WaitCancelled { start: Instant { tv_sec: 167750, tv_nsec: 737497331 }, requested_resume: None })
DeviceEvent { device_id: DeviceId(X(DeviceId(10))), event: Key(KeyboardInput { scancode: 125, state: Pressed, virtual_keycode: None, modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } }) }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 125, state: Pressed, virtual_keycode: None, modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: true } } } }
EventsCleared
NewEvents(WaitCancelled { start: Instant { tv_sec: 167750, tv_nsec: 969649256 }, requested_resume: None })
DeviceEvent { device_id: DeviceId(X(DeviceId(10))), event: Key(KeyboardInput { scancode: 42, state: Pressed, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } }) }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 42, state: Pressed, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: true } } } }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: ModifiersChanged { device_id: DeviceId(X(DeviceId(3))), modifiers: ModifiersState { shift: true, ctrl: false, alt: false, logo: true } } }
EventsCleared
NewEvents(WaitCancelled { start: Instant { tv_sec: 167754, tv_nsec: 873709305 }, requested_resume: None })
DeviceEvent { device_id: DeviceId(X(DeviceId(10))), event: Key(KeyboardInput { scancode: 16, state: Pressed, virtual_keycode: Some(Q), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } }) }
EventsCleared
NewEvents(WaitCancelled { start: Instant { tv_sec: 167754, tv_nsec: 876675192 }, requested_resume: None })
EventsCleared

Moving your mouse onto the window, pressing the keys, moving it off, removing the keys made ModifiersState permanently loose sync.

$ cargo run --example window | grep -v "Cursor" | grep "Keyboard\|Modifier\|ModifiersChanged"
<SNIP>
DeviceEvent { device_id: DeviceId(X(DeviceId(10))), event: Key(KeyboardInput { scancode: 42, state: Pressed, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } }) }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 42, state: Pressed, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: true } } } }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: ModifiersChanged { device_id: DeviceId(X(DeviceId(3))), modifiers: ModifiersState { shift: true, ctrl: false, alt: false, logo: true } } }
DeviceEvent { device_id: DeviceId(X(DeviceId(10))), event: Key(KeyboardInput { scancode: 42, state: Released, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: false } }) }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: KeyboardInput { device_id: DeviceId(X(DeviceId(3))), input: KeyboardInput { scancode: 42, state: Released, virtual_keycode: Some(LShift), modifiers: ModifiersState { shift: true, ctrl: false, alt: false, logo: true } } } }
WindowEvent { window_id: WindowId(X(WindowId(58720257))), event: ModifiersChanged { device_id: DeviceId(X(DeviceId(3))), modifiers: ModifiersState { shift: false, ctrl: false, alt: false, logo: true } } }

Also, shouldn't we distinguish between Left and Right shift/alt? Seams a shame we don't track them separately.

@murarth
Copy link
Contributor Author

murarth commented Sep 6, 2019

I've addressed the issue with modifier desync when switching windows in what I believe is the most reasonable way: When a window goes out of focus, a ModifiersChanged event is reported to the window indicating that all modifier keys are released. When a window comes into focus, a ModifiersChanged event is reported indicating any modifiers currently held. (Of course, in each of these cases, if there are no modifiers set, no event is reported.)

While this may not reflect the "true" modifier state for the user's window manager environment, it simplifies the logic required by winit to keep modifier state correct. A more "accurate" solution would require tracking last known modifier state for each open window -- increasing complexity and the potential for bugs. The most "accurate" solution would even require updating out-of-focus windows for every DeviceEvent, which would be far more complex and would create the potential of a panic due to internal RefCell borrowing.

I think the solution I've implemented will be sufficient for most applications.

@murarth
Copy link
Contributor Author

murarth commented Sep 8, 2019

Fixed the CI errors by running cargo fmt. Is there anything else holding this back?

@goddessfreya
Copy link
Contributor

When ControlFlow is Wait, keyboard events are only reported when the mouse is moved, which leaves ModifiersChanged desynced until the mouse goes back onto the window. This keyboard delay was also reproduced on master, however, so it is not caused by this PR. LGTM.

@goddessfreya goddessfreya merged commit 206c3c2 into rust-windowing:master Sep 8, 2019
@murarth murarth deleted the modifiers-changed-event branch September 8, 2019 18:57
@goddessfreya
Copy link
Contributor

goddessfreya commented Sep 9, 2019

The keyboard delay was being "caused" by stdout being buffered... What a false positive. (Took a good half hour to figure out...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants