X11: Fix modifiers being reported after release (#1262)

* X11: Fix modifiers being reported after release

* Moves `ModifiersChanged` variant from `WindowEvent` to `DeviceEvent`

* Add CHANGELOG entry

CHANGELOG.md

@@ -1,6 +1,7 @@
 # Unreleased
 
 - On macOS, fix application termination on `ControlFlow::Exit`
+- On X11, fix key modifiers being incorrectly reported.
 
 # 0.20.0 Alpha 4 (2019-10-18)
 

src/event.rs

@@ -134,10 +134,6 @@ pub enum WindowEvent {
         input: KeyboardInput,
     },
 
-    /// Keyboard modifiers have changed
-    #[doc(hidden)]
-    ModifiersChanged { modifiers: ModifiersState },
-
     /// The cursor has moved on the window.
     CursorMoved {
         device_id: DeviceId,
@@ -266,7 +262,15 @@ pub enum DeviceEvent {
         button: ButtonId,
         state: ElementState,
     },
+
     Key(KeyboardInput),
+
+    /// Keyboard modifiers have changed
+    #[doc(hidden)]
+    ModifiersChanged {
+        modifiers: ModifiersState,
+    },
+
     Text {
         codepoint: char,
     },

src/platform_impl/linux/wayland/event_loop.rs

@@ -54,6 +54,10 @@ impl<T> WindowEventsSink<T> {
         }
     }
 
+    pub fn send_event(&mut self, evt: crate::event::Event<T>) {
+        self.buffer.push_back(evt);
+    }
+
     pub fn send_window_event(&mut self, evt: crate::event::WindowEvent, wid: WindowId) {
         self.buffer.push_back(crate::event::Event::WindowEvent {
             event: evt,
@@ -240,9 +244,7 @@ pub struct EventLoop<T: 'static> {
     // Utility for grabbing the cursor and changing visibility
     _user_source: ::calloop::Source<::calloop::channel::Channel<T>>,
     user_sender: ::calloop::channel::Sender<T>,
-    _kbd_source: ::calloop::Source<
+    _kbd_source: ::calloop::Source<::calloop::channel::Channel<crate::event::Event<()>>>,
-        ::calloop::channel::Channel<(crate::event::WindowEvent, super::WindowId)>,
-    >,
     window_target: RootELW<T>,
 }
 
@@ -296,13 +298,14 @@ impl<T: 'static> EventLoop<T> {
 
         let inner_loop = ::calloop::EventLoop::new().unwrap();
 
-        let (kbd_sender, kbd_channel) = ::calloop::channel::channel();
+        let (kbd_sender, kbd_channel) = ::calloop::channel::channel::<crate::event::Event<()>>();
         let kbd_sink = sink.clone();
         let kbd_source = inner_loop
             .handle()
             .insert_source(kbd_channel, move |evt, &mut ()| {
-                if let ::calloop::channel::Event::Msg((evt, wid)) = evt {
+                if let ::calloop::channel::Event::Msg(evt) = evt {
-                    kbd_sink.lock().unwrap().send_window_event(evt, wid);
+                    let evt = evt.map_nonuser_event().ok().unwrap();
+                    kbd_sink.lock().unwrap().send_event(evt);
                 }
             })
             .unwrap();
@@ -726,7 +729,7 @@ struct SeatManager<T: 'static> {
     sink: Arc<Mutex<WindowEventsSink<T>>>,
     store: Arc<Mutex<WindowStore>>,
     seats: Arc<Mutex<Vec<(u32, wl_seat::WlSeat)>>>,
-    kbd_sender: ::calloop::channel::Sender<(crate::event::WindowEvent, super::WindowId)>,
+    kbd_sender: ::calloop::channel::Sender<crate::event::Event<()>>,
     relative_pointer_manager_proxy: Rc<RefCell<Option<ZwpRelativePointerManagerV1>>>,
     pointer_constraints_proxy: Arc<Mutex<Option<ZwpPointerConstraintsV1>>>,
     cursor_manager: Arc<Mutex<CursorManager>>,
@@ -771,7 +774,7 @@ impl<T: 'static> SeatManager<T> {
 struct SeatData<T> {
     sink: Arc<Mutex<WindowEventsSink<T>>>,
     store: Arc<Mutex<WindowStore>>,
-    kbd_sender: ::calloop::channel::Sender<(crate::event::WindowEvent, super::WindowId)>,
+    kbd_sender: ::calloop::channel::Sender<crate::event::Event<()>>,
     pointer: Option<wl_pointer::WlPointer>,
     relative_pointer: Option<ZwpRelativePointerV1>,
     relative_pointer_manager_proxy: Rc<RefCell<Option<ZwpRelativePointerManagerV1>>>,

src/platform_impl/linux/wayland/keyboard.rs

@@ -8,11 +8,13 @@ use smithay_client_toolkit::{
     reexports::client::protocol::{wl_keyboard, wl_seat},
 };
 
-use crate::event::{ElementState, KeyboardInput, ModifiersState, VirtualKeyCode, WindowEvent};
+use crate::event::{
+    DeviceEvent, ElementState, Event, KeyboardInput, ModifiersState, VirtualKeyCode, WindowEvent,
+};
 
 pub fn init_keyboard(
     seat: &wl_seat::WlSeat,
-    sink: ::calloop::channel::Sender<(crate::event::WindowEvent, super::WindowId)>,
+    sink: ::calloop::channel::Sender<crate::event::Event<()>>,
     modifiers_tracker: Arc<Mutex<ModifiersState>>,
 ) -> wl_keyboard::WlKeyboard {
     // { variables to be captured by the closures
@@ -29,12 +31,22 @@ pub fn init_keyboard(
             match evt {
                 KbEvent::Enter { surface, .. } => {
                     let wid = make_wid(&surface);
-                    my_sink.send((WindowEvent::Focused(true), wid)).unwrap();
+                    my_sink
+                        .send(Event::WindowEvent {
+                            window_id: mk_root_wid(wid),
+                            event: WindowEvent::Focused(true),
+                        })
+                        .unwrap();
                     *target.lock().unwrap() = Some(wid);
                 }
                 KbEvent::Leave { surface, .. } => {
                     let wid = make_wid(&surface);
-                    my_sink.send((WindowEvent::Focused(false), wid)).unwrap();
+                    my_sink
+                        .send(Event::WindowEvent {
+                            window_id: mk_root_wid(wid),
+                            event: WindowEvent::Focused(false),
+                        })
+                        .unwrap();
                     *target.lock().unwrap() = None;
                 }
                 KbEvent::Key {
@@ -52,11 +64,10 @@ pub fn init_keyboard(
                         };
                         let vkcode = key_to_vkey(rawkey, keysym);
                         my_sink
-                            .send((
+                            .send(Event::WindowEvent {
-                                WindowEvent::KeyboardInput {
+                                window_id: mk_root_wid(wid),
-                                    device_id: crate::event::DeviceId(
+                                event: WindowEvent::KeyboardInput {
-                                        crate::platform_impl::DeviceId::Wayland(DeviceId),
+                                    device_id: device_id(),
-                                    ),
                                     input: KeyboardInput {
                                         state,
                                         scancode: rawkey,
@@ -64,8 +75,7 @@ pub fn init_keyboard(
                                         modifiers: modifiers_tracker.lock().unwrap().clone(),
                                     },
                                 },
-                                wid,
+                            })
-                            ))
                             .unwrap();
                         // send char event only on key press, not release
                         if let ElementState::Released = state {
@@ -74,7 +84,10 @@ pub fn init_keyboard(
                         if let Some(txt) = utf8 {
                             for chr in txt.chars() {
                                 my_sink
-                                    .send((WindowEvent::ReceivedCharacter(chr), wid))
+                                    .send(Event::WindowEvent {
+                                        window_id: mk_root_wid(wid),
+                                        event: WindowEvent::ReceivedCharacter(chr),
+                                    })
                                     .unwrap();
                             }
                         }
@@ -88,11 +101,12 @@ pub fn init_keyboard(
 
                     *modifiers_tracker.lock().unwrap() = modifiers;
 
-                    if let Some(wid) = *target.lock().unwrap() {
+                    my_sink
-                        my_sink
+                        .send(Event::DeviceEvent {
-                            .send((WindowEvent::ModifiersChanged { modifiers }, wid))
+                            device_id: device_id(),
-                            .unwrap();
+                            event: DeviceEvent::ModifiersChanged { modifiers },
-                    }
+                        })
+                        .unwrap();
                 }
             }
         },
@@ -101,11 +115,10 @@ pub fn init_keyboard(
                 let state = ElementState::Pressed;
                 let vkcode = key_to_vkey(repeat_event.rawkey, repeat_event.keysym);
                 repeat_sink
-                    .send((
+                    .send(Event::WindowEvent {
-                        WindowEvent::KeyboardInput {
+                        window_id: mk_root_wid(wid),
-                            device_id: crate::event::DeviceId(
+                        event: WindowEvent::KeyboardInput {
-                                crate::platform_impl::DeviceId::Wayland(DeviceId),
+                            device_id: device_id(),
-                            ),
                             input: KeyboardInput {
                                 state,
                                 scancode: repeat_event.rawkey,
@@ -113,13 +126,15 @@ pub fn init_keyboard(
                                 modifiers: my_modifiers.lock().unwrap().clone(),
                             },
                         },
-                        wid,
+                    })
-                    ))
                     .unwrap();
                 if let Some(txt) = repeat_event.utf8 {
                     for chr in txt.chars() {
                         repeat_sink
-                            .send((WindowEvent::ReceivedCharacter(chr), wid))
+                            .send(Event::WindowEvent {
+                                window_id: mk_root_wid(wid),
+                                event: WindowEvent::ReceivedCharacter(chr),
+                            })
                             .unwrap();
                     }
                 }
@@ -147,12 +162,22 @@ pub fn init_keyboard(
                     move |evt, _| match evt {
                         wl_keyboard::Event::Enter { surface, .. } => {
                             let wid = make_wid(&surface);
-                            my_sink.send((WindowEvent::Focused(true), wid)).unwrap();
+                            my_sink
+                                .send(Event::WindowEvent {
+                                    window_id: mk_root_wid(wid),
+                                    event: WindowEvent::Focused(true),
+                                })
+                                .unwrap();
                             target = Some(wid);
                         }
                         wl_keyboard::Event::Leave { surface, .. } => {
                             let wid = make_wid(&surface);
-                            my_sink.send((WindowEvent::Focused(false), wid)).unwrap();
+                            my_sink
+                                .send(Event::WindowEvent {
+                                    window_id: mk_root_wid(wid),
+                                    event: WindowEvent::Focused(false),
+                                })
+                                .unwrap();
                             target = None;
                         }
                         wl_keyboard::Event::Key { key, state, .. } => {
@@ -163,11 +188,10 @@ pub fn init_keyboard(
                                     _ => unreachable!(),
                                 };
                                 my_sink
-                                    .send((
+                                    .send(Event::WindowEvent {
-                                        WindowEvent::KeyboardInput {
+                                        window_id: mk_root_wid(wid),
-                                            device_id: crate::event::DeviceId(
+                                        event: WindowEvent::KeyboardInput {
-                                                crate::platform_impl::DeviceId::Wayland(DeviceId),
+                                            device_id: device_id(),
-                                            ),
                                             input: KeyboardInput {
                                                 state,
                                                 scancode: key,
@@ -175,8 +199,7 @@ pub fn init_keyboard(
                                                 modifiers: ModifiersState::default(),
                                             },
                                         },
-                                        wid,
+                                    })
-                                    ))
                                     .unwrap();
                             }
                         }
@@ -386,3 +409,11 @@ impl From<keyboard::ModifiersState> for ModifiersState {
         }
     }
 }
+
+fn device_id() -> crate::event::DeviceId {
+    crate::event::DeviceId(crate::platform_impl::DeviceId::Wayland(DeviceId))
+}
+
+fn mk_root_wid(wid: crate::platform_impl::wayland::WindowId) -> crate::window::WindowId {
+    crate::window::WindowId(crate::platform_impl::WindowId::Wayland(wid))
+}

src/platform_impl/linux/x11/event_processor.rs

@@ -25,7 +25,6 @@ pub(super) struct EventProcessor<T: 'static> {
     pub(super) target: Rc<RootELW<T>>,
     pub(super) mod_keymap: ModifierKeymap,
     pub(super) device_mod_state: ModifierKeyState,
-    pub(super) window_mod_state: ModifierKeyState,
 }
 
 impl<T: 'static> EventProcessor<T> {
@@ -131,7 +130,6 @@ impl<T: 'static> EventProcessor<T> {
 
                     self.mod_keymap.reset_from_x_connection(&wt.xconn);
                     self.device_mod_state.update(&self.mod_keymap);
-                    self.window_mod_state.update(&self.mod_keymap);
                 }
             }
 
@@ -547,7 +545,7 @@ impl<T: 'static> EventProcessor<T> {
                     };
                     let virtual_keycode = events::keysym_to_element(keysym as c_uint);
 
-                    let modifiers = self.window_mod_state.modifiers();
+                    let modifiers = self.device_mod_state.modifiers();
 
                     callback(Event::WindowEvent {
                         window_id,
@@ -561,27 +559,6 @@ impl<T: 'static> EventProcessor<T> {
                             },
                         },
                     });
-
-                    if let Some(modifier) =
-                        self.mod_keymap.get_modifier(xkev.keycode as ffi::KeyCode)
-                    {
-                        self.window_mod_state.key_event(
-                            state,
-                            xkev.keycode as ffi::KeyCode,
-                            modifier,
-                        );
-
-                        let new_modifiers = self.window_mod_state.modifiers();
-
-                        if modifiers != new_modifiers {
-                            callback(Event::WindowEvent {
-                                window_id,
-                                event: WindowEvent::ModifiersChanged {
-                                    modifiers: new_modifiers,
-                                },
-                            });
-                        }
-                    }
                 }
 
                 if state == Pressed {
@@ -894,21 +871,6 @@ impl<T: 'static> EventProcessor<T> {
                             event: Focused(true),
                         });
 
-                        // When focus is gained, send any existing modifiers
-                        // to the window in a ModifiersChanged event. This is
-                        // done to compensate for modifier keys that may be
-                        // changed while a window is out of focus.
-                        if !self.device_mod_state.is_empty() {
-                            self.window_mod_state = self.device_mod_state.clone();
-
-                            let modifiers = self.window_mod_state.modifiers();
-
-                            callback(Event::WindowEvent {
-                                window_id,
-                                event: WindowEvent::ModifiersChanged { modifiers },
-                            });
-                        }
-
                         // The deviceid for this event is for a keyboard instead of a pointer,
                         // so we have to do a little extra work.
                         let pointer_id = self
@@ -941,21 +903,6 @@ impl<T: 'static> EventProcessor<T> {
                             .unfocus(xev.event)
                             .expect("Failed to unfocus input context");
 
-                        // When focus is lost, send a ModifiersChanged event
-                        // containing no modifiers set. This is done to compensate
-                        // for modifier keys that may be changed while a window
-                        // is out of focus.
-                        if !self.window_mod_state.is_empty() {
-                            self.window_mod_state.clear();
-
-                            callback(Event::WindowEvent {
-                                window_id: mkwid(xev.event),
-                                event: WindowEvent::ModifiersChanged {
-                                    modifiers: ModifiersState::default(),
-                                },
-                            });
-                        }
-
                         callback(Event::WindowEvent {
                             window_id: mkwid(xev.event),
                             event: Focused(false),
@@ -1068,7 +1015,7 @@ impl<T: 'static> EventProcessor<T> {
                             _ => unreachable!(),
                         };
 
-                        let device_id = xev.sourceid;
+                        let device_id = mkdid(xev.sourceid);
                         let keycode = xev.detail;
                         if keycode < 8 {
                             return;
@@ -1088,6 +1035,18 @@ impl<T: 'static> EventProcessor<T> {
 
                         let virtual_keycode = events::keysym_to_element(keysym as c_uint);
 
+                        let modifiers = self.device_mod_state.modifiers();
+
+                        callback(Event::DeviceEvent {
+                            device_id,
+                            event: DeviceEvent::Key(KeyboardInput {
+                                scancode,
+                                virtual_keycode,
+                                state,
+                                modifiers,
+                            }),
+                        });
+
                         if let Some(modifier) =
                             self.mod_keymap.get_modifier(keycode as ffi::KeyCode)
                         {
@@ -1096,19 +1055,18 @@ impl<T: 'static> EventProcessor<T> {
                                 keycode as ffi::KeyCode,
                                 modifier,
                             );
+
+                            let new_modifiers = self.device_mod_state.modifiers();
+
+                            if modifiers != new_modifiers {
+                                callback(Event::DeviceEvent {
+                                    device_id,
+                                    event: DeviceEvent::ModifiersChanged {
+                                        modifiers: new_modifiers,
+                                    },
+                                });
+                            }
                         }
-
-                        let modifiers = self.device_mod_state.modifiers();
-
-                        callback(Event::DeviceEvent {
-                            device_id: mkdid(device_id),
-                            event: DeviceEvent::Key(KeyboardInput {
-                                scancode,
-                                virtual_keycode,
-                                state,
-                                modifiers,
-                            }),
-                        });
                     }
 
                     ffi::XI_HierarchyChanged => {

src/platform_impl/linux/x11/mod.rs

@@ -192,7 +192,6 @@ impl<T: 'static> EventLoop<T> {
             xi2ext,
             mod_keymap,
             device_mod_state: Default::default(),
-            window_mod_state: Default::default(),
         };
 
         // Register for device hotplug events