GUACAMOLE-2161: Fix Ctrl/Shift scancode issues for Unicode (RU/EN) layouts in RDP #621
Conversation
There was a problem hiding this comment.
A few initial comments from me, mainly from a style perspective. In general, your changes need to make sure to maintain the style used throughout the rest of the code. At a high level:
- Please make sure indentations follow established guidelines - 4 space tabs, and that they match the expected tab stops.
- It's okay to use white space and newlines in the code - you need not put something like
case 0x043c: case 0x041c: sc=0x2F; breakall on one line. That should be four lines.
Beyond that, I'm not entirely sure about having the Cyrilic-specific keymapping done directly in the code like this - I think that's what we have the individual keymaps for. I'm curious to see if @mike-jumper or @corentin-soriano have any thoughts on this, though?
Also, your commit message needs the correct Jira issue in it (GUACAMOLE-2161: rather than GUACAMOLE-XXXX:).
| #include <guacamole/client.h> | ||
| #include <guacamole/mem.h> | ||
| #include <guacamole/rwlock.h> | ||
|
|
src/protocols/rdp/keyboard.c
Outdated
| switch (keysym) { | ||
| /* Lowercase Cyrillic */ | ||
| case 0x430: keysym = 'f'; break; // а | ||
| case 0x431: keysym = ','; break; // б | ||
| case 0x432: keysym = 'd'; break; // в | ||
| case 0x433: keysym = 'u'; break; // г | ||
| case 0x434: keysym = 'l'; break; // д | ||
| case 0x435: keysym = 't'; break; // е | ||
| case 0x436: keysym = ';'; break; // ж | ||
| case 0x437: keysym = 'p'; break; // з | ||
| case 0x438: keysym = 'b'; break; // и | ||
| case 0x439: keysym = 'q'; break; // й | ||
| case 0x43A: keysym = 'r'; break; // к | ||
| case 0x43B: keysym = 'k'; break; // л | ||
| case 0x43C: keysym = 'v'; break; // м | ||
| case 0x43D: keysym = 'y'; break; // н | ||
| case 0x43E: keysym = 'j'; break; // о | ||
| case 0x43F: keysym = 'g'; break; // п | ||
| case 0x440: keysym = 'h'; break; // р | ||
| case 0x441: keysym = 'c'; break; // с | ||
| case 0x442: keysym = 'n'; break; // т | ||
| case 0x443: keysym = 'e'; break; // у | ||
| case 0x444: keysym = 'a'; break; // ф | ||
| case 0x445: keysym = '['; break; // х | ||
| case 0x446: keysym = 'w'; break; // ц | ||
| case 0x447: keysym = 'x'; break; // ч | ||
| case 0x448: keysym = 'i'; break; // ш | ||
| case 0x449: keysym = 'o'; break; // щ | ||
| case 0x44A: keysym = ']'; break; // ъ | ||
| case 0x44B: keysym = 's'; break; // ы | ||
| case 0x44C: keysym = 'm'; break; // ь | ||
| case 0x44D: keysym = '\''; break; // э | ||
| case 0x44E: keysym = '.'; break; // ю | ||
| case 0x44F: keysym = 'z'; break; // я | ||
|
|
||
| /* Uppercase Cyrillic */ | ||
| case 0x410: keysym = 'F'; break; // А | ||
| case 0x411: keysym = '<'; break; // Б | ||
| case 0x412: keysym = 'D'; break; // В | ||
| case 0x413: keysym = 'U'; break; // Г | ||
| case 0x414: keysym = 'L'; break; // Д | ||
| case 0x415: keysym = 'T'; break; // Е | ||
| case 0x416: keysym = ':'; break; // Ж | ||
| case 0x417: keysym = 'P'; break; // З | ||
| case 0x418: keysym = 'B'; break; // И | ||
| case 0x419: keysym = 'Q'; break; // Й | ||
| case 0x41A: keysym = 'R'; break; // К | ||
| case 0x41B: keysym = 'K'; break; // Л | ||
| case 0x41C: keysym = 'V'; break; // М | ||
| case 0x41D: keysym = 'Y'; break; // Н | ||
| case 0x41E: keysym = 'J'; break; // О | ||
| case 0x41F: keysym = 'G'; break; // П | ||
| case 0x420: keysym = 'H'; break; // Р | ||
| case 0x421: keysym = 'C'; break; // С | ||
| case 0x422: keysym = 'N'; break; // Т | ||
| case 0x423: keysym = 'E'; break; // У | ||
| case 0x424: keysym = 'A'; break; // Ф | ||
| case 0x425: keysym = '{'; break; // Х | ||
| case 0x426: keysym = 'W'; break; // Ц | ||
| case 0x427: keysym = 'X'; break; // Ч | ||
| case 0x428: keysym = 'I'; break; // Ш | ||
| case 0x429: keysym = 'O'; break; // Щ | ||
| case 0x42A: keysym = '}'; break; // Ъ | ||
| case 0x42B: keysym = 'S'; break; // Ы | ||
| case 0x42C: keysym = 'M'; break; // Ь | ||
| case 0x42D: keysym = '"'; break; // Э | ||
| case 0x42E: keysym = '>'; break; // Ю | ||
| case 0x42F: keysym = 'Z'; break; // Я | ||
| } | ||
| } |
There was a problem hiding this comment.
A few issues, here:
- I'm not sure about maintaining keymap translations directly in the code like this - this is one of the things that I believe the individual keymaps are supposed to handle. Is there some reason this has to be done in the code?
- Is there any way to do this without the
switchstatement, like maybe some math that takes in the values for thecasestatements and automatically converts it to the expected keysym? - Please match style as maintained throughout the rest of the code - putting the entirety of each
casestatement on a single line makes for more difficult readability.
There was a problem hiding this comment.
To clarify — this logic intentionally lives in keyboard.c because it only executes when Guacamole is operating in Unicode input mode. In that mode, the normal keymap layer (keymaps/*.h) is completely bypassed, and Guacamole sends Unicode events directly to the RDP server. Without an explicit mapping here, modifier combinations like Ctrl+C / Ctrl+V (and Cyrillic characters typed via Unicode) fail to produce the expected scancodes on the RDP side.
So this section effectively acts as a bridge between Unicode input and scancode-based shortcuts, ensuring consistent behavior across both English and Cyrillic layouts.
Regarding arithmetic mapping — unfortunately, Cyrillic Unicode codepoints (0x0410–0x044F) are not linearly aligned with their Latin counterparts, so a formulaic conversion would produce incorrect results for several keys. A switch remains the safest and most predictable approach here.
I’ve updated the formatting to match Guacamole’s style guidelines (4-space indentation, each case on its own line) and added comments to clarify why this logic is inline rather than in the keymap.
There was a problem hiding this comment.
Thanks for the clarification, this is helpful. The thing I'm still not sure about - and I'd like to have some of the other developers' opinions on - is whether we should be placing language-specific code (in this case, the Cyrillic characters) into the main body of code like this. I understand why you're doing it, I'm just not sure whether there's a better place for it or we should create some sort of other Unicode translation layer/system to handle this that could eventually be expanded to include any other languages/keymaps where this type of translation needs to take place.
DevOps9-debug
force-pushed
the
fix-keyboard-unicode-layout
branch
from
2dca4f6 to
2207f11
Compare
|
Thanks for the review, @necouchman. Just to clarify the logic — this isn’t limited to Cyrillic layouts. The same issue occurs even in English when using Unicode input mode, because Guacamole sends printable characters as Unicode events and only modifier keys (Ctrl, Alt, Shift) as scancodes. The RDP side, however, expects both parts of a shortcut (like Ctrl+C) to arrive as scancodes. Without that, it interprets the “C” as plain text, not a control command. This patch bridges that behavior by pairing Unicode key events with the correct US scancode when Ctrl or Alt are pressed, ensuring shortcuts like Ctrl+C / Ctrl+V work across all Unicode-based layouts — both English and Russian. In this case, doing the mapping directly in code is necessary because Unicode layout input bypasses the regular keymap layer entirely. Guacamole’s keymaps (keymaps/en-us.h, keymaps/ru-ru.h, etc.) only apply when Guacamole is translating X11-style keysyms → RDP scancodes. But when using Unicode input mode (which happens automatically if the remote RDP server advertises Unicode keyboard support), Guacamole no longer uses those keymap tables — it sends Unicode events directly. That’s the core of the issue: Unicode events contain only the final character (e.g., “с” or “м”), not the underlying key position. RDP shortcuts like Ctrl+C require scancode-based events (Ctrl down + 0x2E down + up). Without an explicit mapping in the Unicode path, those shortcuts fail for all Unicode layouts — even en-us — because only the character, not the key, reaches RDP. So the logic here explicitly bridges that gap by injecting the corresponding scancodes when modifiers are pressed (Ctrl/Alt), ensuring that shortcuts work while preserving Unicode input for normal typing. As for “doing this without a switch” — unfortunately not safely. The Cyrillic block (0x0410–0x044F) is not linearly aligned with Latin scancodes, so arithmetic mapping would misalign multiple characters. A switch is the cleanest and most predictable way to normalize those codepoints for now. If desired, we could move these mappings into a small static table or helper function later, but doing it inline keeps the Unicode path self-contained and easy to reason about. I’ll reformat the switch blocks to match the project’s indentation and case-per-line style. |
corentin-soriano
left a comment
corentin-soriano
left a comment
There was a problem hiding this comment.
There are many style changes in this pull request (moving braces, pointer asterisks, changes to indentation, and line breaks) but I don't see any reason to change the existing coding style here.
I tend to agree with Nick: the code appears to be specific to a particular language/keymap and could perhaps be isolated so it can eventually be extended to other languages.
Also, I think the documentation for the guac_rdp_keyboard_send_missing_key function should be updated.
| } | ||
|
|
||
| if (ctrl_or_alt_pressed) | ||
| { |
There was a problem hiding this comment.
Could we remove this if which seems to be the same as the block just above?
There was a problem hiding this comment.
Good question — the two if (ctrl_or_alt_pressed) blocks serve different purposes.
The first handles Unicode-layout-specific corrections (Cyrillic → Latin normalization and Ctrl + C/V fixes), while this second block maps regular ASCII letters/digits to US scancodes so that generic shortcuts like Ctrl + A, Ctrl + Z, etc. function correctly under Unicode mode.
I’ve added a clarifying comment in the code to make that separation clear.
Agreed, the code style should match existing conventions and should not be re-formatting the way we've laid out the code. The style has 1) been chosen specifically by the creators of the project, and 2) is relatively consistent throughout the code. |
3 participants
This PR addresses GUACAMOLE-2161
When using Guacamole with the Unicode keyboard layout (ru-ru or en-us), text input works fine but modifier keys (Ctrl, Shift, Alt) do not trigger correctly inside RDP sessions. This especially breaks shortcuts like Ctrl+C, Ctrl+V, and Ctrl+Z, as well as Shift-based capital input in Cyrillic mode.
Root cause:
Fix:
Verified results:
Ctrl+C / Ctrl+V / Ctrl+Z now work in both English and Russian layouts.
Shift-based capital input functions correctly.
Layout switching (Alt+Shift) does not break input handling.
This patch makes Guacamole fully usable in mixed-language (RU/EN) environments using Unicode keyboard input.