linux: Fix spacebar not working with multiple keyboard layouts (#34514)

Smit Barmase created 4 months ago

Closes #26468 #16667

This PR fixes the spacebar not working with multiple keyboard layouts on
Linux X11. I have tested this with Czech, Russian, German, German Neo 2,
etc. It seems to work correctly.

`XkbStateNotify` events correctly update XKB state with complete
modifier info (depressed/latched/locked), but `KeyPress/KeyRelease`
events immediately overwrite that state using `update_mask()` with only
raw X11 modifier bits. This breaks xkb state as we reset `latched_mods`
and `locked_mods` to 0, as well as we might not correctly handle cases
where this new xkb state needs to change.

Previous logic is flawed because `KeyPress/KeyRelease` event only gives
you depressed modifiers (`event.state`) and not others, which we try to
fill in from `previous_xkb_state`. This patch was introduced to fix
capitalization issue with Neo 2
(https://github.com/zed-industries/zed/pull/14466) and later to fix
wrong keys with German layout
(https://github.com/zed-industries/zed/pull/31193), both of which I have
tested this PR with.

Now, instead of manually managing XKB state, we use the `update_key`
method, which internally handles modifier states and other cases we
might have missed.
  
From `update_key` docs:

> Update the keyboard state to reflect a given key being pressed or
released.
>
> This entry point is intended for programs which track the keyboard
state explictly (like an evdev client). If the state is serialized to
you by a master process (like a Wayland compositor) using functions like
`xkb_state_serialize_mods()`, you should use `xkb_state_update_mask()`
instead. **_The two functins should not generally be used together._**
>                
> A series of calls to this function should be consistent; that is, a
call with `xkb::KEY_DOWN` for a key should be matched by an
`xkb::KEY_UP`; if a key is pressed twice, it should be released twice;
etc. Otherwise (e.g. due to missed input events), situations like "stuck
modifiers" may occur.
>              
> This function is often used in conjunction with the function
`xkb_state_key_get_syms()` (or `xkb_state_key_get_one_sym()`), for
example, when handling a key event. In this case, you should prefer to
get the keysyms *before* updating the key, such that the keysyms
reported for the key event are not affected by the event itself. This is
the conventional behavior.

  
Release Notes:

- Fix the issue where the spacebar doesn’t work with multiple keyboard
layouts on Linux X11.

Change summary

crates/gpui/src/platform/linux/x11/client.rs | 70 +++++----------------
1 file changed, 18 insertions(+), 52 deletions(-)

Detailed changes

crates/gpui/src/platform/linux/x11/client.rs

@@ -1,23 +1,22 @@
 use crate::{Capslock, xcb_flush};
-use core::str;
-use std::{
-    cell::RefCell,
-    collections::{BTreeMap, HashSet},
-    ops::Deref,
-    path::PathBuf,
-    rc::{Rc, Weak},
-    time::{Duration, Instant},
-};
-
 use anyhow::{Context as _, anyhow};
 use calloop::{
     EventLoop, LoopHandle, RegistrationToken,
     generic::{FdWrapper, Generic},
 };
 use collections::HashMap;
+use core::str;
 use http_client::Url;
 use log::Level;
 use smallvec::SmallVec;
+use std::{
+    cell::RefCell,
+    collections::{BTreeMap, HashSet},
+    ops::Deref,
+    path::PathBuf,
+    rc::{Rc, Weak},
+    time::{Duration, Instant},
+};
 use util::ResultExt;
 
 use x11rb::{
@@ -38,7 +37,7 @@ use x11rb::{
 };
 use xim::{AttributeName, Client, InputStyle, x11rb::X11rbClient};
 use xkbc::x11::ffi::{XKB_X11_MIN_MAJOR_XKB_VERSION, XKB_X11_MIN_MINOR_XKB_VERSION};
-use xkbcommon::xkb::{self as xkbc, LayoutIndex, ModMask, STATE_LAYOUT_EFFECTIVE};
+use xkbcommon::xkb::{self as xkbc, STATE_LAYOUT_EFFECTIVE};
 
 use super::{
     ButtonOrScroll, ScrollDirection, X11Display, X11WindowStatePtr, XcbAtoms, XimCallbackEvent,
@@ -141,13 +140,6 @@ impl From<xim::ClientError> for EventHandlerError {
     }
 }
 
-#[derive(Debug, Default, Clone)]
-struct XKBStateNotiy {
-    depressed_layout: LayoutIndex,
-    latched_layout: LayoutIndex,
-    locked_layout: LayoutIndex,
-}
-
 #[derive(Debug, Default)]
 pub struct Xdnd {
     other_window: xproto::Window,
@@ -200,7 +192,6 @@ pub struct X11ClientState {
     pub(crate) mouse_focused_window: Option<xproto::Window>,
     pub(crate) keyboard_focused_window: Option<xproto::Window>,
     pub(crate) xkb: xkbc::State,
-    previous_xkb_state: XKBStateNotiy,
     keyboard_layout: LinuxKeyboardLayout,
     pub(crate) ximc: Option<X11rbClient<Rc<XCBConnection>>>,
     pub(crate) xim_handler: Option<XimHandler>,
@@ -507,7 +498,6 @@ impl X11Client {
             mouse_focused_window: None,
             keyboard_focused_window: None,
             xkb: xkb_state,
-            previous_xkb_state: XKBStateNotiy::default(),
             keyboard_layout,
             ximc,
             xim_handler,
@@ -959,14 +949,6 @@ impl X11Client {
                         state.xkb_device_id,
                     )
                 };
-                let depressed_layout = xkb_state.serialize_layout(xkbc::STATE_LAYOUT_DEPRESSED);
-                let latched_layout = xkb_state.serialize_layout(xkbc::STATE_LAYOUT_LATCHED);
-                let locked_layout = xkb_state.serialize_layout(xkbc::ffi::XKB_STATE_LAYOUT_LOCKED);
-                state.previous_xkb_state = XKBStateNotiy {
-                    depressed_layout,
-                    latched_layout,
-                    locked_layout,
-                };
                 state.xkb = xkb_state;
                 drop(state);
                 self.handle_keyboard_layout_change();
@@ -983,12 +965,6 @@ impl X11Client {
                     event.latched_group as u32,
                     event.locked_group.into(),
                 );
-                state.previous_xkb_state = XKBStateNotiy {
-                    depressed_layout: event.base_group as u32,
-                    latched_layout: event.latched_group as u32,
-                    locked_layout: event.locked_group.into(),
-                };
-
                 let modifiers = Modifiers::from_xkb(&state.xkb);
                 let capslock = Capslock::from_xkb(&state.xkb);
                 if state.last_modifiers_changed_event == modifiers
@@ -1025,17 +1001,12 @@ impl X11Client {
                 state.pre_key_char_down.take();
                 let keystroke = {
                     let code = event.detail.into();
-                    let xkb_state = state.previous_xkb_state.clone();
-                    state.xkb.update_mask(
-                        event.state.bits() as ModMask,
-                        0,
-                        0,
-                        xkb_state.depressed_layout,
-                        xkb_state.latched_layout,
-                        xkb_state.locked_layout,
-                    );
                     let mut keystroke = crate::Keystroke::from_xkb(&state.xkb, modifiers, code);
                     let keysym = state.xkb.key_get_one_sym(code);
+
+                    // should be called after key_get_one_sym
+                    state.xkb.update_key(code, xkbc::KeyDirection::Down);
+
                     if keysym.is_modifier_key() {
                         return Some(());
                     }
@@ -1093,17 +1064,12 @@ impl X11Client {
 
                 let keystroke = {
                     let code = event.detail.into();
-                    let xkb_state = state.previous_xkb_state.clone();
-                    state.xkb.update_mask(
-                        event.state.bits() as ModMask,
-                        0,
-                        0,
-                        xkb_state.depressed_layout,
-                        xkb_state.latched_layout,
-                        xkb_state.locked_layout,
-                    );
                     let keystroke = crate::Keystroke::from_xkb(&state.xkb, modifiers, code);
                     let keysym = state.xkb.key_get_one_sym(code);
+
+                    // should be called after key_get_one_sym
+                    state.xkb.update_key(code, xkbc::KeyDirection::Up);
+
                     if keysym.is_modifier_key() {
                         return Some(());
                     }