fix(input): Prevent modified key releases from firing plain hotkeys - #3233
CryoTheRenegade wants to merge 4 commits into
Conversation
PR Summary by QodoPrevent stuck modifier combos on out-of-order key releases
AI Description
Diagram
High-Level Assessment
Files changed (6)
|
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require reviewTip of the day💡 Did you know, you can reply 'qodo' on any finding to push back, ask questions, or dig deeper |
 |
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
I assume this was entirely generated by LLM? It looks like slop the way the new logic is laid out. It is incomprehensible and unmaintainable. It needs to be unsloppified.
|
How do we know the new generated revision is no slop? |
|
Fair criticism. I used LLM assistance on the first pass, and I should have reviewed and simplified the result before asking you to review it. I own that. I’ve since rewritten the fix. The hotkey translator is stateless now. Modifier press state lives in |
|
I tried to understand this change but I was unable to. It is lacking context. What was the issue and how was it reproduced, how was it fixed and why is it fixed the way it was fixed. |
CryoTheRenegade
changed the title
|
I've reworded the PR description with a simple example and an explanation of the cause, the fix, and why the state is stored in |
| @@ -74,33 +73,15 @@ GameMessageDisposition HotKeyTranslator::translateGameMessage(const GameMessage | |||
|
|
|||
| if ( t == GameMessage::MSG_RAW_KEY_UP) | |||
There was a problem hiding this comment.
Would MSG_RAW_KEY_DOWN perhaps be an option for hotkeys? All the MetaEvents are key down events. Or is there a good reason why hotkey needs to be posted on down?
There was a problem hiding this comment.
Key-down could work. I found no comment explaining why hotkeys use key-up. I kept the current timing for now.
There was a problem hiding this comment.
I suggest test MSG_RAW_KEY_DOWN. It would be the simpler option and get rid of the new stuff you added to accomodate this event. Players will also be happier if their key presses register faster.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. WalkthroughThe changes add a ChangesKeyboard input state and message handling
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant Keyboard
participant HotKeyTranslator
participant MetaEventTranslator
Keyboard->>HotKeyTranslator: Send raw key-down with key state
HotKeyTranslator->>HotKeyTranslator: Filter modifier and autorepeat states
HotKeyTranslator->>MetaEventTranslator: Continue message translation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change filters modified and repeated key presses and gives GUI hotkeys precedence. No actionable merge-blocking issue was identified; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Keyboard commands now fire on press, and reset clears additional modifier states. Existing input restrictions and enabled-button checks remain in place. Risk is low, with remaining uncertainty about customized key-release bindings. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
Ok I lost a oversight on this over time. If i am not mistaken this change now does 3 or more things:
- a KeyState type refactor (which is nice)
- Hotkey press fix
- Alt Tab key fix
And maybe something else.
Can you split all of these things into clean individual commits in an order that makes sense?
67b80bd to
b4ed7f3
Compare
|
I've split the changes into four commits, in this order:
|
xezon
left a comment
xezon
left a comment
There was a problem hiding this comment.
Looks promising. The split makes not entirely clear what a refactor is and what not, as it seems some refactors are mingled with non-refactor commits.
|
|
||
| // since we only allocate one of each, don't bother pooling 'em | ||
| m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") WindowTranslator, 10 ); | ||
| m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") HotKeyTranslator, 15 ); |
There was a problem hiding this comment.
Can you give an example what concrete button conflict this solves now or would solve on a conflict?
Introduce KeyState and the combined modifier mask. Update the keyboard, meta-event, and hotkey types. Simplify the hotkey modifier check and name the buffered key and physical-modifier helper without changing event timing or modifier capture.
Capture modifiers for each buffered event instead of copying the final frame state to all events. For example, Ctrl-down, F-down, Ctrl-up in one frame must still mark F-down as modified. Include current modifiers on repeats and recognize the configured secondary shift key.
Run plain hotkeys on press and ignore repeats. Give active GUI buttons priority over overlapping meta commands after window input. Document why the event timing and translator order matter.
Extend Alt focus-reset handling to Ctrl, Shift, and a distinct secondary shift key. Emit releases before clearing the keyboard state.
b4ed7f3 to
e52d4a4
Compare
|
|
||
| // since we only allocate one of each, don't bother pooling 'em | ||
| m_translators[ m_numTranslators++ ] = TheMessageStream->attachTranslator( MSGNEW("GameClientSubsystem") WindowTranslator, 10 ); | ||
| // TheSuperHackers @fix Let active GUI hotkeys handle keys before MetaEventTranslator consumes overlapping bindings. |
There was a problem hiding this comment.
How did you come to the conclusion that this is the right thing to do?
If a unit has a button mapped on key "E", then pressing "E" will no longer select matching units for the selection: a deviation of the original, possibly confusing the player.
| GameMessage::Type t = msg->getType(); | ||
|
|
||
| if ( t == GameMessage::MSG_RAW_KEY_UP) | ||
| // TheSuperHackers @fix Run hotkeys on press so releasing a modifier first cannot trigger a plain hotkey. |
There was a problem hiding this comment.
"Run hotkeys on press instead of release ..."
| GameMessage::Type t = msg->getType(); | ||
|
|
||
| if ( t == GameMessage::MSG_RAW_KEY_UP) | ||
| // TheSuperHackers @fix Run hotkeys on press so releasing a modifier first cannot trigger a plain hotkey. |
There was a problem hiding this comment.
Can also say that the press is consistent with the typical meta event key mappings.
2 participants
Thanks to DrGoldFish, who reported this issue and tested the fix.
The bug can happen in this order using Legi's keybinds:
The game can treat the last step as a normal F key press and run the F command. It should remember that F was pressed with Ctrl.
There is also a focus problem. If Ctrl or Shift is released while the game is not focused, the game may miss the release. This can leave force-attack or selection mode active.
The keyboard code reads several events at once. It previously gave every event the modifier state from the end of that group. This could give an event the wrong Ctrl, Shift, or Alt state.
The keyboard code now saves the modifier state when each event is handled. It remembers which modifier was held when a key was pressed. It passes that information to the matching key release.
HotKeyTranslatorthen knows that the release is part of a modified key press and does not run the normal hotkey.This state is stored in
Keyboardbecause another message handler may remove the key-down event beforeHotKeyTranslatorreceives it.The reset code now sends key-up events for Ctrl, Shift, and Alt. The existing
MetaEventcode is unchanged.