Repository navigation
feat: match shortcuts in non-Latin keyboard layouts - #771
Conversation
Shortcuts did not work in a Cyrillic, Greek or other non-Latin layout, unless the first configured layout was Latin. The lookup translated each key in the first layout (group 0), so it also matched wrong keys: - with "ru,us", no shortcut worked, also when "us" was active - with "de,us" and "us" active, Ctrl+Z did a redo and "y" selected the arrow tool - with "us,dvorak" and Dvorak active, the keys at the QWERTY positions of "b", "c" and "e" selected the brush, crop and ellipse tools Now the lookup uses the unshifted key in the active layout. When this key is a letter of a non-Latin script, the binding also matches the same key in the first configured Latin layout. If there is no Latin layout, it matches the key at the same position in US QWERTY. A Latin key never falls back, so QWERTZ and Dvorak get no extra bindings. A binding for the non-Latin key itself has priority. The text tool uses the same fallback for Ctrl+A, Ctrl+C, Ctrl+V and Ctrl+X. See Satty-org#585.
The tests compile real XKB keymaps with xkbcommon and resolve key presses from evdev keycodes to commands. They cover German alone (Satty-org#508), German with US and with Russian, Russian with US in both orders, Russian alone, Dvorak alone and as a second layout, and Russian phonetic Dvorak with English Dvorak. The lookup over the keymap moves out of shortcut_keys, so that the tests use the same code with xkbcommon in place of GDK. xkbcommon is a dev-dependency only, and the dev shell gets libxkbcommon to link it.
A new test checks the table against the real "us" layout in both directions: each key of the table is correct, and no printable key of the main block is missing. The test found one missing key: the extra key of ISO keyboards between the left Shift and Z, which types "<".
rokokol
changed the title
The keycode crate came with the first layout-independent shortcuts in 9a01f05. Satty-org#508 and Satty-org#555 removed the code that used it, but not the dependency. The shortcut lookup in this branch does not need it.
|
Thank you for this PR. Looks sane to me and in line with what I found out when looking into this. It's a bit tricky to test :) There's one concern I have, although it is not a new one, we already have it as it is. Correct me if I'm wrong, but basically, we can only ship default bindings for latin, number keypad and function keys. Attempts to e.g. bind It'd be kinda nice if binding symbolic names was a bit more permissive. If this is not feasible, or perhaps long term feasible but not in scope of this PR, it'd be nice if we could prevent doing this by accident :-) |
|
LGTM - so I merge it now. Thank you so much for this - it is even more tricky than I hoped ... Regarding:
We should also bind "equal" - browsers and editors also do it - it even works here in my German layout by Ctrl+Shift+0 (= in on shifted zero).
Have a PR in progress - but hope @rokokol can test it... |
| im_context.focus_in(); | ||
| if !im_context.filter_keypress(controller.current_event().unwrap()) { | ||
| sender.input(SketchBoardInput::new_key_event(KeyEventMsg::new(key, code, modifier))); | ||
| sender.input(SketchBoardInput::new_key_event(KeyEventMsg::new(key, code, modifier, controller.group()))); |
There was a problem hiding this comment.
Ahh, this is what I missed back then when I wrote the code - the active layout...
|
Thank you both for the review! @robertwidfen Sure, I will be glad to test it, please ping me when it is up One note: with the current lookup, |
Yes this is exactly what #772 does then - IMHO it should not break anything but give more freedom in bindings, e.g. "equal" will bind in all layouts. |
Description
Closes #585
The problem
Since #608, the shortcut lookup translates each key in the first configured layout (group 0), not in the active one. That
us,ruworks today is a side effect ofusbeing first. The same rule breaks other setups:mainru,us, any layout activeb/иruonlyи, Ctrl+яde,us,usactivezde,us,usactiveyus,dvorak, Dvorak activex,j,.us,ru, text tool,ruactiveф, Ctrl+сThe
de,usrow is the #508 regression again, and the Dvorak row is the accidental binding discussed in #585The lookup
[keybinds]The gate in step 2 differs a little from my proposal in #585. I proposed "the keyval is not an ASCII letter". That gate lets punctuation through, and in Dvorak
.is on the QWERTYekey, so it would select the ellipse tool. "A letter of a non-Latin script" excludes this. It also excludesöand other non-ASCII Latin lettersStep 3 runs only without a Latin layout. With
de,ru, the Cyrillic key on the Germanykey must givey, not the USzTwo independent rules keep #508 fixed. First, the key of the active layout is looked up before any fallback, so Ctrl+Y on a German keyboard finds redo before anything else. Second, a Latin key never falls back, so the German Y key has no physical "z" to match. The German test fails only when both rules are broken
The text tool checks Ctrl+A, Ctrl+C, Ctrl+V and Ctrl+X by itself, not through the keybinding registry. In a Russian layout, Ctrl+
с(the Russian letter on the C key, which looks like the Latin C) was not recognized there and fell through to the global Ctrl+C, which copies the image instead of the selected text. The text tool now uses the same fallback, but only while Ctrl is held, so typing letters works as beforeThe case with only one non-Latin layout
With only
ruconfigured, the keymap has no Latin group. GDK translates a keycode only in the groups of the keymap, so steps 1 and 2 cannot give a Latin key. Without step 3, no letter shortcut works for these usersStep 3 uses the hardware keycode of the event. GDK gives the XKB keycode, which is the evdev keycode plus 8. Evdev keycodes are sequential along the rows of the keyboard, so
US_QWERTY_ROWSholds four rows, each with the evdev keycode of its first key. Example:иgives XKB keycode 56, evdev 48, which is the 6th key of the row that starts at 43, sobI also looked at compiling a
uskeymap with xkbcommon and translating the keycode in it. That uses the same engine as the system and needs no hand-written table. I did not use it at runtime, because it adds a runtime dependency (thexkbcommoncrate and a link to libxkbcommon) and compiles a keymap only for 48 keys that do not change. The evdev positions of US QWERTY have been the same for decades. The tests use xkbcommon only as a dev-dependency: they run the lookup on real layouts and check the table against the realuslayoutAlso the keycode crate is removed: it came with the first layout-independent shortcuts, and nothing uses it since #555
Testing
keybindings.rsdealone (the fix: Ctrl+Y for DE by removing layout-independent keys #508 case),de,us,de,ru,us,ruandru,us,rualone, Dvorak alone and as a second layout, andus(dvorak),ru(phonetic_dvorak). With Dvorak alone, nothing changes compared tomain: no key falls back, because Dvorak has no non-Latin letters. For this,xkbcommonis a dev-dependency (not in the release build), and the Nix dev shell getslibxkbcommon. CI runs only clippy, which does not link the testsUS_QWERTY_ROWSagainst the realuslayout in both directions: each key of the table is correct, and no printable key of the main block is missing. It found one missing key, the extra key of ISO keyboards between the left Shift and ZChecklist