fix: Properly determine keysym for non-printable keys sent to X11#1149
fix: Properly determine keysym for non-printable keys sent to X11#1149Unnvaldr wants to merge 1 commit intoutkarshdalal:masterfrom
Conversation
📝 WalkthroughWalkthroughRefactors Keyboard event handling to treat a broad set of Android key codes as non-printable, sending a zero keysym for them to the X server; adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/src/main/java/com/winlator/xserver/Keyboard.java (2)
104-143: Consider addingKEYCODE_INSERTand numpad keys to the non-printable list.
KEYCODE_INSERTis mapped increateKeycodeMap()(line 184) but missing fromNON_PRINTABLE_KEYCODES. WhilegetUnicodeChar()likely returns 0 for it anyway, including it would be more consistent and explicit.Similarly, the numpad keys (
KEYCODE_NUMPAD_0throughKEYCODE_NUMPAD_9,KEYCODE_NUMPAD_DOT, etc.) are mapped but not listed. Depending on Num Lock state, these may return varying unicode values fromgetUnicodeChar(), which could cause inconsistent behavior.💡 Suggested additions
KeyEvent.KEYCODE_F11, KeyEvent.KEYCODE_F12, + + // Other non-printable keys + KeyEvent.KEYCODE_INSERT, + + // Numpad keys (behavior depends on Num Lock state) + KeyEvent.KEYCODE_NUMPAD_0, + KeyEvent.KEYCODE_NUMPAD_1, + KeyEvent.KEYCODE_NUMPAD_2, + KeyEvent.KEYCODE_NUMPAD_3, + KeyEvent.KEYCODE_NUMPAD_4, + KeyEvent.KEYCODE_NUMPAD_5, + KeyEvent.KEYCODE_NUMPAD_6, + KeyEvent.KEYCODE_NUMPAD_7, + KeyEvent.KEYCODE_NUMPAD_8, + KeyEvent.KEYCODE_NUMPAD_9, + KeyEvent.KEYCODE_NUMPAD_DOT, + KeyEvent.KEYCODE_NUMPAD_DIVIDE, + KeyEvent.KEYCODE_NUMPAD_MULTIPLY, + KeyEvent.KEYCODE_NUMPAD_SUBTRACT, + KeyEvent.KEYCODE_NUMPAD_ADD, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/xserver/Keyboard.java` around lines 104 - 143, Add missing insert and numpad key constants to the NON_PRINTABLE_KEYCODES array so they are treated explicitly as non-printable; update the array that currently lists modifier, control, navigation and function keys (symbol: NON_PRINTABLE_KEYCODES) to include KEYCODE_INSERT and the numpad keys used in createKeycodeMap() (e.g., KEYCODE_NUMPAD_0 through KEYCODE_NUMPAD_9, KEYCODE_NUMPAD_DOT and any other KEYCODE_NUMPAD_* entries present) to ensure consistent handling regardless of getUnicodeChar()/NumLock state.
145-150: Optional: Consider using aSetfor O(1) lookup.The linear search is fine for ~30 elements, but if the list grows or for consistency with other patterns, a
HashSetwould provide constant-time lookups.♻️ Alternative using HashSet
+ private static final Set<Integer> NON_PRINTABLE_KEYCODES_SET; + static { + NON_PRINTABLE_KEYCODES_SET = new HashSet<>(); + for (int code : NON_PRINTABLE_KEYCODES) { + NON_PRINTABLE_KEYCODES_SET.add(code); + } + } + public static boolean isNonPrintable(int keyCode) { - for (int code : NON_PRINTABLE_KEYCODES) { - if (code == keyCode) return true; - } - return false; + return NON_PRINTABLE_KEYCODES_SET.contains(keyCode); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/xserver/Keyboard.java` around lines 145 - 150, Replace the linear scan in isNonPrintable with a constant-time lookup by switching from the int[] NON_PRINTABLE_KEYCODES to a Set<Integer> (e.g., NON_PRINTABLE_KEYCODES_SET) initialized once (static, unmodifiable) and change isNonPrintable(int keyCode) to return NON_PRINTABLE_KEYCODES_SET.contains(keyCode); update any references to the old array accordingly to use the new Set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@app/src/main/java/com/winlator/xserver/Keyboard.java`:
- Around line 104-143: Add missing insert and numpad key constants to the
NON_PRINTABLE_KEYCODES array so they are treated explicitly as non-printable;
update the array that currently lists modifier, control, navigation and function
keys (symbol: NON_PRINTABLE_KEYCODES) to include KEYCODE_INSERT and the numpad
keys used in createKeycodeMap() (e.g., KEYCODE_NUMPAD_0 through
KEYCODE_NUMPAD_9, KEYCODE_NUMPAD_DOT and any other KEYCODE_NUMPAD_* entries
present) to ensure consistent handling regardless of getUnicodeChar()/NumLock
state.
- Around line 145-150: Replace the linear scan in isNonPrintable with a
constant-time lookup by switching from the int[] NON_PRINTABLE_KEYCODES to a
Set<Integer> (e.g., NON_PRINTABLE_KEYCODES_SET) initialized once (static,
unmodifiable) and change isNonPrintable(int keyCode) to return
NON_PRINTABLE_KEYCODES_SET.contains(keyCode); update any references to the old
array accordingly to use the new Set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba672dad-46dd-4550-81d7-860228ad3953
📒 Files selected for processing (1)
app/src/main/java/com/winlator/xserver/Keyboard.java
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="app/src/main/java/com/winlator/xserver/Keyboard.java">
<violation number="1" location="app/src/main/java/com/winlator/xserver/Keyboard.java:104">
P2: Do not expose a mutable array as `public`; external mutation can change keyboard classification logic at runtime.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Unnvaldr
force-pushed
the
fix-non-printable-keyboard-keys
branch
from
414a3b1 to
0ad553c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/src/main/java/com/winlator/xserver/Keyboard.java (1)
25-64: Avoid a second source of truth for special-key classification.
NON_PRINTABLE_KEYCODESnow has to stay aligned with bothcreateKeycodeMap()andcreateKeyboard(), and it has already drifted:KEYCODE_INSERTis mapped at Line 194 but not classified here. Consider deriving this from the X key mapping layer instead of maintaining a separate Android-key list.Also applies to: 93-98
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/main/java/com/winlator/xserver/Keyboard.java` around lines 25 - 64, NON_PRINTABLE_KEYCODES is a fragile second source of truth that has drifted from createKeycodeMap() / createKeyboard() (e.g., KEYCODE_INSERT is mapped but not listed); remove the hard-coded NON_PRINTABLE_KEYCODES array and instead derive the non-printable set at runtime from the existing X key mapping layer: iterate the map returned by createKeycodeMap() (or the keyboard entries from createKeyboard()) and treat entries whose mapping represents a non-printable/special X key (e.g., no printable Unicode, maps to a special KeySym/constant) as non-printable, then replace uses of NON_PRINTABLE_KEYCODES with that computed set; also update any related logic around the previously duplicated list (areas around the previous lines 93-98) to use the derived set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/src/main/java/com/winlator/xserver/Keyboard.java`:
- Around line 388-389: The Num Lock and Caps Lock keys are wired with keysyms
(keyboard.setKeysyms for XKeycode.KEY_NUM_LOCK and KEY_CAPS_LOCK) but their
state is handled like momentary modifiers by setKeyPress() and setKeyRelease(),
so the locks clear on key release; change the key handling so that in the
key-down path (setKeyPress or wherever KEY_* press events are processed) you
toggle the corresponding modifier bit for Num Lock and Caps Lock instead of
simply setting it, and make the key-up path (setKeyRelease) a no-op for those
two keycodes (or at least avoid clearing their modifier bit). Locate the logic
that checks keycodes in setKeyPress()/setKeyRelease() and implement
toggle-on-down + ignore-on-up specifically for XKeycode.KEY_NUM_LOCK and
XKeycode.KEY_CAPS_LOCK so the locks latch correctly.
---
Nitpick comments:
In `@app/src/main/java/com/winlator/xserver/Keyboard.java`:
- Around line 25-64: NON_PRINTABLE_KEYCODES is a fragile second source of truth
that has drifted from createKeycodeMap() / createKeyboard() (e.g.,
KEYCODE_INSERT is mapped but not listed); remove the hard-coded
NON_PRINTABLE_KEYCODES array and instead derive the non-printable set at runtime
from the existing X key mapping layer: iterate the map returned by
createKeycodeMap() (or the keyboard entries from createKeyboard()) and treat
entries whose mapping represents a non-printable/special X key (e.g., no
printable Unicode, maps to a special KeySym/constant) as non-printable, then
replace uses of NON_PRINTABLE_KEYCODES with that computed set; also update any
related logic around the previously duplicated list (areas around the previous
lines 93-98) to use the derived set.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fb473678-970b-485a-b7c8-7cc95af22746
📒 Files selected for processing (1)
app/src/main/java/com/winlator/xserver/Keyboard.java
|
I'm not exactly sure what this change fixes it (other than the num lock and caps lock) |
|
These games I have listed already worked with keyboard, but non-printable keys were not properly recognized by it, because of the erroneous gatekeeping for Enter key only. |
|
what games does keyboard key presses currently not work? |
|
Could you read the PR's content please? I will provide the recordings later this week for the sake of the contrib reqs. |
|
@Unnvaldr - keep this in draft until you have the recording please |
|
@utkarshdalal Done, added the recordings. |
|
@Unnvaldr can you upload on youtube or directly on github? the link is not working for me |
|
same |
|
Uploaded to GH @utkarshdalal, @AndreVto. Although very strange; could you write to me on Discord in regards what you encounter with Proton Drive's videos? |
|
@Unnvaldr alright, thanks.
I still would like to know which games you observed this issue, you said earlier the games from your PR description worked fine before so I'm not exactly sure what to look from the recordings |
TES IV: Oblivion, Hidden & Dangerous 2 amongst the prominent ones, like mentioned in the PR. Keys like modifiers, Tab etc. were not properly passed to the game's process. |
|
@Unnvaldr What physical keyboard do you have? which language layout? and which keyboard app do you use? Gboard? Samsung Keyboard?
Physical keyboard keys presses should be going through onKeyEvent which has no "gatekeeping" |
3 participants
Description
Windows expects some of the keys to send keyboard symbols for the keys that should have it and no symbol for non-standard keys.
With Android to X11 conversion, it is also required to properly remap Android keys to X11 keys.
What was changed, is that non-printable keys were predefined and are ultimately used, to determine whether an equivalent symbol should be sent to xServer or not.
This way, some of the games using DirectX's DirectInput, properly recognizes non-standard keys like TAB, Space etc.
In addition, missing key symbols were added for Num Lock and Caps Lock.
Recording
Hard to showcase, keys now work in games like TES IV: Oblivion, Hidden & Dangerous 2 etc.
The Elder Scrolls IV: Oblivion
https://drive.proton.me/urls/XJ91Y5FBTW#lR1DKvrjJ4H7
Screen_Recording_20260412_013631_2.mp4
Half-Life 2
https://drive.proton.me/urls/TKJPSHPCSM#OmfoqTYmeBu-
Screen_Recording_20260412_014240_2.mp4
Hidden & Dangerous 2:
https://drive.proton.me/urls/JB5GA4G27W#8AjvijWz7IVS
Screen_Recording_20260412_020320_2.mp4
Checklist
#code-changes, I have discussed this change there and it has been green-lighted. If I do not have access, I have still provided clear context in this PR. If I skip both, I accept that this change may face delays in review, may not be reviewed at all, or may be closed.CONTRIBUTING.md.Summary by cubic
Fixes Android→X11 keyboard mapping by sending keysym 0 for non-printable keys and adding missing Num Lock/Caps Lock keysyms. DirectInput apps now recognize control and navigation keys correctly.
Written for commit 0ad553c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes