Repository navigation
fix(wayland): derive Key from the level-0 keysym - #430
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The newly added Wayland XKB test fixture performs FFI calls without asserting non-null xkb_context_new/xkb_state_new results, which can lead to undefined behavior if allocation fails.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes Wayland key handling by deriving Key from shift-level 0 of the active layout (matching the X11 approach) to avoid layout-specific shifted-keysym mismatches that can cause stuck keys, and it removes the now-unneeded shifted-keysym translation table. It also corrects Wayland set_key_repeat_rate forwarding and adds focused unit tests using an embedded XKB keymap fixture to cover rollover and numpad/NumLock cases.
Changes:
- Compute
Keyfrom level-0 keysyms (shift-invariant) while keeping character input based on the state-resolved keysym. - Fix
Window::set_key_repeat_ratein the Wayland backend to callset_key_repeat_rate(notset_key_repeat_delay). - Add unit tests that exercise shift rollover, layout-specific shifted symbols, numpad with NumLock, and undeclared keycodes.
File summaries
| File | Description |
|---|---|
| src/os/posix/xkb_keysyms.rs | Removes shifted-keysym constants that are no longer needed after switching to level-0 mapping. |
| src/os/posix/xkb_ffi.rs | Extends xkbcommon FFI to support querying keysyms by layout/level and detecting invalid layouts. |
| src/os/posix/wayland.rs | Implements level-0 keysym resolution for Key, fixes repeat-rate forwarding, and adds XKB-fixture-driven tests. |
| src/key_handler.rs | Annotates KeyHandler::update as intentionally unused in Wayland (Wayland uses the split phases directly). |
Review details
Suppressed comments (1)
src/os/posix/wayland.rs:1485
- In the test fixture,
xkb_state_newcan return null. If that happens, later FFI calls withself.state(and theDropimpl) can invoke libxkbcommon with a null pointer, which is UB; assertstateis non-null after creation.
assert!(!keymap.is_null(), "test keymap failed to compile");
let state = ffi_dispatch!(XKBH, xkb_state_new, keymap);
Keymap {
context,
keymap,
state,
}
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let context = ffi_dispatch!( | ||
| XKBH, | ||
| xkb_context_new, | ||
| xkb_ffi::xkb_context_flags::XKB_CONTEXT_NO_FLAGS | ||
| ); | ||
| let keymap = ffi_dispatch!( | ||
| XKBH, | ||
| xkb_keymap_new_from_string, | ||
| context, | ||
| text.as_ptr(), | ||
| xkb_ffi::xkb_keymap_format::XKB_KEYMAP_FORMAT_TEXT_V1, | ||
| xkb_ffi::xkb_keymap_compile_flags::XKB_KEYMAP_COMPILE_NO_FLAGS | ||
| ); |
There was a problem hiding this comment.
Added in 7cd68a6 — asserts on context after xkb_context_new and on state after xkb_state_new, matching the existing keymap assert.
fa00aec to
7cd68a6
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Layout-group rollover can still leave keys stuck, and level-0 lookup drops the keypad decimal key.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Balanced
| keymap_state: *mut xkb_ffi::xkb_state, | ||
| key: u32, | ||
| ) -> Option<u32> { | ||
| let layout = unsafe { ffi_dispatch!(XKBH, xkb_state_key_get_layout, keymap_state, key) }; |
There was a problem hiding this comment.
Confirmed against a real us,ru keymap — level 0 of group 0 is q, of group 1 is Cyrillic_shorti, so the release did resolve to a different Key.
Fixed in c4e80e2 by recording the Key each keycode resolved to on press and clearing that on release, rather than pinning the layout — pinning to group 0 would break users whose primary layout is non-Latin. Added group_change_while_held_leaves_no_key_held, with a second group in the fixture; it fails without the fix.
| } | ||
| } | ||
|
|
||
| if let Some(key_xkb) = Self::keysym_at_level_0(keymap, keymap_state, key) { |
There was a problem hiding this comment.
Correct, and it was the only keypad key affected — I probed all 17 against a us keymap, and every other level-0 name (KP_Home, KP_End, KP_Divide, …) was already in the table. KPDL is the one whose level-0 is KP_Delete while the table only had KP_Decimal at level 1.
Fixed in c4e80e2: added the KP_Delete keysym mapped to Key::NumPadDot, plus <KPDL> in the fixture and a keypad_decimal_registers test that fails without it.
7cd68a6 to
c4e80e2
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The central German Shift+2 mapping needs a direct regression assertion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| let mut unshifted = KeyHandler::new(); | ||
| km.set_mods(0, 0); | ||
| km.press(&mut unshifted, AD01); | ||
|
|
||
| let mut shifted = KeyHandler::new(); | ||
| km.set_mods(SHIFT, 0); | ||
| km.press(&mut shifted, AD01); | ||
|
|
||
| assert_eq!(unshifted.get_keys(), vec![Key::Q]); | ||
| assert_eq!(unshifted.get_keys(), shifted.get_keys()); |
There was a problem hiding this comment.
Right — q/Q passes under the old table too, so that test wasn't discriminating. Fixed in 401ebde: it now runs both <AE02> (asserting Key::Key2 shifted and unshifted) and <AD01>.
Checked it by simulating #429's behaviour — active-level keysym plus the US shifted arms for these two keys — and the strengthened test fails where the old one passed.
That also surfaced something worth noting: with the press/release cache added for the group-rollover fix, shift_rollover_leaves_no_key_held now passes under the old mapping too, since the cache clears the release regardless. So this test is now the only one guarding the level-0 mapping itself. Good catch.
The shifted-keysym table from #429 hardcodes US pairings. On a German layout Shift+2 is quotedbl, so the press sets Key::Apostrophe while the release after Shift is up arrives as 2 and clears Key::Key2, leaving Apostrophe held. Take Key from level 0 of the active layout, as x11.rs does with XLookupKeysym(&ev.key, 0). Character input keeps the state-resolved keysym. Level 0 alone is not enough: the compositor can switch layout group while a key is held, resolving the release through a different group. Record the Key each keycode resolved to on press and clear that on release. Also fixes the numpad with NumLock on, where the active keysym is KP_1..KP_0 and was dropped; the keypad decimal key, whose level-0 name is KP_Delete rather than KP_Decimal; and set_key_repeat_rate, which forwarded to set_key_repeat_delay. Tests drive handle_key through an embedded keymap fixture; each case fails without its fix.
c4e80e2 to
401ebde
Compare
Follow-up to #429.
The shifted-keysym table hardcodes US pairings. On a German layout Shift+2 is
quotedbl, so the press setsKey::Apostrophewhile the release after Shift is up arrives as2and clearsKey::Key2, leavingApostropheheld.Takes
Keyfrom level 0 of the active layout instead, asx11.rsdoes withXLookupKeysym(&ev.key, 0), and drops the table. Character input keeps the state-resolved keysym.Also fixes two others:
KP_1..KP_0, which aren't in the match table, so those presses were dropped.set_key_repeat_rateforwarded toset_key_repeat_delay.Tests drive
handle_keythrough an embedded keymap fixture; the rollover and numpad cases fail on 7724f43.