fix(input): track/release client keycode for modifiers - #5558
Conversation
Fixed, tested and working on my branch. Added tests to prevent regressions. shortcutFlags is compared against packet->modifiers, a client-side bitmask, but was updated from the host keycode from map_keycode(). - A keybindings entry moving Alt off VKEY_*MENU left the ALT bit clear, so send_key_and_modifiers() wrapped every following key in a real VKEY_MENU press - Alt->Meta on a macOS host turned Cmd+Space into Cmd+Option+Space - apply_shortcut() already gets the unmapped keycode, so both halves of the combo check now agree - Generalizes the key_rightalt_to_key_win case from LizardByte#5318 fix(input): release the remapped key when the client disconnects reset_keyboard_keys() emitted an unmapped virtual-key code. Keypresses go are sent via map_keycode(). Remapped keys stay pressed on the host after a disconnect. - With keybindings 0xA4 -> 0x5B, Option was released while Command was left held down - Also affected key_rightalt_to_key_win option test(input): cover keyboard passthrough with a recording sink platf::keyboard_update is a per-platform function and lvh's FakeKeyboard discards submit(), so ordered keyboard output could not be observed from a test. - Route the nine emit sites through emit_keyboard_update(), whose sink check sits inside #ifdef SUNSHINE_TESTS - Cover 45 held-modifier combinations, all 7 synthetic-injection subsets, and every printable ASCII character plain and shifted - Pin the spurious VKEY_MENU injection fixed in the previous commit - Native VK translation stays in libvirtualhid, which covers each backend table in its own platform's CI job
- Return the test keyboard recorder from an accessor, so the file adds no non-const variable at namespace scope - Build assertion tokens and the stream name with std::format instead of output manipulators and manual concatenation - Hold modifier masks in unsigned, and cast once where the packet helper needs a byte - Move the modifier selection loop into select_modifiers(), which drops the combination test to three levels of nesting - Make the fixture stream private, add using enum for side_e, and use std::ranges::sort with class template argument deduction
|
I'll test this as I've seen modifiers getting stuck sometimes myself on Linux host + Linux client. Will report back. |
|
So far I've not seen a single stuck modifier with this PR applied. Neither have there been any other problems related to keyboard input so LGTM from a practical standpoint. |
|
Thanks for working on this. There might be an edge case in the modifier tracking:
Releasing Right Alt currently clears the shared |
Left, right, and side-less Alt all share the same shortcutFlags::ALT bit. update_shortcutFlags() cleared that bit on any Alt release, so releasing Right Alt while Left Alt was still down (e.g. Right Alt mapped to Meta via key_rightalt_to_key_win) dropped the bit even though a real Alt key remained pressed. The next key then got wrapped in an unnecessary synthetic Alt press/release. - Track a side-less Alt key (VKEY_MENU) the same way Left/Right Alt are already tracked - Clear the aggregate ALT bit on release only when none of left/right/generic Alt remain pressed Extends KeepsRealAltWhileRightAltMapsToKeyWin to press a key after releasing Right Alt but before releasing Left Alt, asserting no synthetic Alt events are emitted. Addresses review feedback from @ReenigneArcher on LizardByte#5558.
|
I've rebased and added another commit+tests that should (hopefully) address your concerns @ReenigneArcher . |
|
I probably didn't describe the issue properly, but it applied to more modifiers than just ALT. I pushed another commit to fully address it. |
Bundle ReportBundle size has no change ✅ |
ed92e4a to
422bddb
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5558 +/- ##
==========================================
+ Coverage 34.55% 35.18% +0.62%
==========================================
Files 104 104
Lines 25314 25409 +95
Branches 11194 11211 +17
==========================================
+ Hits 8748 8939 +191
+ Misses 14460 14366 -94
+ Partials 2106 2104 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes Continue to review full report in Codecov by Harness.
|
Screenshot ComparisonPR #5558 screenshots vs Matrix:
|
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
Matrix: Windows-AMD64
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
Matrix: Windows-ARM64
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
Matrix: macOS-arm64
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
Matrix: macOS-x86_64
| Image | Baseline | PR |
|---|---|---|
sunshine_tray_initial.png |
 |
 |
sunshine_tray_menu.png |
 |
 |
sunshine_tray_pairing_request.png |
 |
 |
sunshine_tray_paused.png |
 |
 |
sunshine_tray_stopped.png |
 |
 |
sunshine_tray_streaming.png |
 |
 |
sunshine_tray_tooltip.png |
 |
 |
|
Oh thank you so much for merging this @ReenigneArcher!. Sorry you had to fix the PR on top of everything else :( . Sunshine is an awesome piece of technology that I use all throughout my household, so I appreciate your time and attention. |
Description
Fix two related bugs in the
keybindingspath. Also add tests.Please let me know if any of the changes are unacceptable. This change set fixes bugs I
encountered while streaming OSX Taho with Sunshine to Linux (via Moonlight).
Tested and working after these patches. Wayland Arch client (moonlight) -> OSX host (sunshine).
Both bugs stem from the same issue:
key_pressandshortcutFlagswere stored onthe client's unmapped virtual-key code. Key presses were emitted through
map_keycode(), so the host holds a different key than Sunshine's perspective.1. Synthetic modifier fix (
src/input.cpp,passthrough)shortcutFlagsis compared againstpacket->modifiers, a client-side bitmask. It wasupdated from the remapped host keycode.
Any
keybindingsentry that moves Alt offVKEY_MENU/VKEY_LMENU/VKEY_RMENUcaused the ALT bit to clear while the client stillreported
MODIFIER_ALT.send_key_and_modifiers()wrapped every following key in a realVKEY_MENUpress/release.Before the fix, Alt+Space from a client (linux) arrived as Cmd+Option+Space on Sunshine host (OSX).
This would cause annoyances like: "Search This Mac" appearing when attempting to spawn Spotlight.
It also made simple things like such as option-as-alt in terminals to be unusable.
The bug affected every key pressed while the remapped Alt was held, not just Space.
The synthetic-modifier path exists for clients that report modifiers in the bitmask without
sending discrete modifier key events, so both sides of that comparison should be client-side.
apply_shortcut()in the combo check already receives the unmapped keycode, so this alsomakes the two halves of that check agree.
2. Remapped key left latched on disconnect (
src/input.cpp,reset_keyboard_keys)The reset path pulled the unmapped VK back out of
key_pressand released those unmapped keys.The remapped key that was never released. With the
keybindingsconfig above, a disconnectreleased Option and left Command held.
This bug also affects the shipped
key_rightalt_to_key_winoption, which follows the same remap / unmap flow.3. Tests
platf::keyboard_updateis a per-platform free function, and libvirtualhid'sFakeKeyboarddiscards
submit(), so ordered keyboard output was not observable. The nine emit sites now gothrough one
emit_keyboard_update()forwarder whose sink check is inside#ifdef SUNSHINE_TESTS; non-test builds compile to a plain forwarder.18 tests: 45 held-modifier combinations (3 side variants x 15 non-empty subsets of
Shift/Ctrl/Alt/Meta), all 7 synthetic-injection subsets, 91 keys forwarded verbatim, a
self-verifying table proving all 95 printable ASCII characters are reachable and exercised
plain and shifted, flags propagation, press/release dedup, the shortcut-swallow path, and
regression coverage for both fixes. One test clears the recorder and asserts through
lvh::Keyboard::submit_count()so a broken delivery path can't hide behind the sink.Both fixes were verified by reverting them individually and confirming the tests fail:
fix 1 -> 3 failures (expected
{+0x20, -0x20}, got{+0x12, +0x20, -0x12, -0x20});fix 2 -> 2 failures (released
-0xA4instead of-0x5B). A no-keybindings control testpasses either way, showing fix 2 doesn't over-map.
Native VK -> native keycode translation is deliberately not tested here. It moved to
libvirtualhid, whose backend tables live in anonymous namespaces in translation units that
only compile on their own platform, and which are already covered by
TranslatesKeyboardKeysin that project's per-platform test jobs. Nothing here is platform-gated, so every CI job runs
all 18.
Screenshot
N/A - not a UI change.
Issues Fixed or Closed
None open that I could find. Fix 1 is the same root cause as #4531, which #5318 fixed for the
key_rightalt_to_key_winpath only. That special is likely redundant. Unmappedtracking
VKEY_RMENUapplies the ALT bit on its own.Roadmap Issues
None.
Type of Change
Checklist
AI Usage
See our AI usage policy.