fix: fix stuck keys on lost focus#1156
Conversation
WalkthroughThe changes introduce a debounce mechanism and explicit key state cleanup to improve input handling during and after window focus loss. Key event processing is now gated by a debounce period, and all tracked pressed keys are released upon focus loss, preventing stuck keys and errant input behavior. ImGui input state synchronization is maintained throughout. Changes
Sequence Diagram(s)sequenceDiagram
participant WindowProc
participant Menu
participant ImGui
WindowProc->>Menu: OnFocusLost()
Menu->>ImGui: Release all pressed keys (key-up events)
Menu->>ImGui: Release Tab, Escape, Space, Enter
Menu->>ImGui: Clear active widget state
Menu->>Menu: Record lastFocusLossTime
Note over Menu: During debounce window
WindowProc->>Menu: ProcessInputEventQueue()
Menu->>ImGui: Forward key events (UI only)
Menu-->>WindowProc: Skip app logic for key events
Note over Menu: After debounce window
WindowProc->>Menu: ProcessInputEventQueue()
Menu->>ImGui: Forward key events
Menu->>Menu: Process key events for app logic
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses the issue of stuck keys after a loss of focus by refining key debouncing and cleanup logic.
- Added a helper method (ShouldProcessKeyEvents) to control key event processing during the debounce period.
- Introduced state tracking for pressed keys and focus loss times in Menu.h/.cpp.
- Updated the WM_KILLFOCUS hook in Hooks.cpp to exclusively use the new OnFocusLost() method.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/Menu.h | Added key event debounce method and related member variables. |
| src/Menu.cpp | Integrated debouncing logic into key event processing and focus loss cleanup. |
| src/Hooks.cpp | Refactored WM_KILLFOCUS hook to call OnFocusLost() exclusively. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/Menu.h (3)
7-8: Avoidusing namespacein headersPlacing
using namespace std::chrono;in a public header leaks the namespace into every translation unit that includes it and may cause symbol clashes.
Prefer fully-qualified names or a localusinginside cpp files.
50-52: Document processing-window helper
ShouldProcessKeyEvents()is public but only meaningful internally.
Mark itprivate(or at mostprotected) to avoid accidental external use.
250-254: Potential false sharing on static chrono constant
FOCUS_LOSS_DEBOUNCE_MSis fine, but it could beinline constexprto avoid multiple definitions across TUs (pre-C++17 ODR issue).
Minor, but keeps the header self-contained.src/Menu.cpp (3)
2132-2134: Race-free access to debounce timestamp
ShouldProcessKeyEvents()assumes the caller already holds_inputEventMutex.
If it is ever invoked elsewhere without the lock, a data race onlastFocusLossTimeappears.
Either:bool ShouldProcessKeyEvents() const { std::shared_lock lock(_inputEventMutex); return (steady_clock::now() - lastFocusLossTime) >= FOCUS_LOSS_DEBOUNCE_MS; }or mark
lastFocusLossTimeasstd::atomic<...>to guard future callers.
2158-2164: pressedKeys may bloat on key-repeat spamWhen a key is auto-repeated,
IsPressed()stays true, so each repeat still executesinsert(no harm) but also allocates if the set is currently empty and you quickly press many distinct keys (e.g. typing).
If memory churn becomes measurable, consider pre-allocating:pressedKeys.reserve(256);Not critical but cheap to do.
2285-2289: Inline specifier keeps ODR simple
ShouldProcessKeyEvents()is tiny; marking itinlinewill prevent multiple-definition errors if the compiler inlines it into multiple TUs.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Hooks.cpp(1 hunks)src/Menu.cpp(4 hunks)src/Menu.h(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build plugin and addons
🔇 Additional comments (2)
src/Hooks.cpp (1)
616-621: Consider covering WM_ACTIVATEAPP as wellWM_KILLFOCUS is not the only message signalling lost focus (e.g. fullscreen → Alt-Tab sends WM_ACTIVATEAPP/WA_INACTIVE without WM_KILLFOCUS).
To avoid missing cases, dispatchMenu::OnFocusLost()for WM_ACTIVATEAPP whenwParam == FALSE.src/Menu.cpp (1)
2166-2171: Debounce still forwards events to ImGui – double-check expectationDuring the 100 ms debounce window you skip application hot-key logic but still pass key events to ImGui.
If the intent is to suppress all side-effects, forwardless would be safer; if UI consistency is desired, keep it.
Just confirm this matches design.
| // Reset all key states to prevent stuck keys after Alt+Tab or focus loss | ||
| ImGuiIO& io = ImGui::GetIO(); | ||
|
|
||
| // Log the number of tracked keys before clearing | ||
| size_t keyCount = pressedKeys.size(); | ||
|
|
||
| // Reset all tracked pressed keys by sending key-up events | ||
| for (uint32_t key : pressedKeys) { | ||
| ImGuiKey imguiKey = VirtualKeyToImGuiKey(key); | ||
| if (imguiKey != ImGuiKey_None) { | ||
| io.AddKeyEvent(imguiKey, false); // Send key-up event | ||
| } | ||
| } | ||
| pressedKeys.clear(); | ||
|
|
||
| // Reset modifier keys specifically (common culprits for sticking) | ||
| io.AddKeyEvent(ImGuiMod_Ctrl, false); | ||
| io.AddKeyEvent(ImGuiMod_Shift, false); | ||
| io.AddKeyEvent(ImGuiMod_Alt, false); | ||
|
|
||
| // Reset common problematic keys | ||
| io.AddKeyEvent(ImGuiKey_Tab, false); | ||
| io.AddKeyEvent(ImGuiKey_Escape, false); | ||
| io.AddKeyEvent(ImGuiKey_Space, false); | ||
| io.AddKeyEvent(ImGuiKey_Enter, false); | ||
|
|
||
| // Clear ImGui's internal navigation/active state to prevent infinite iteration | ||
| // This ensures that any active widget (input fields, dropdowns, etc.) loses focus | ||
| // and prevents navigation keys from getting stuck in infinite loops | ||
| ImGui::ClearActiveID(); | ||
|
|
||
| // Record the focus loss time for debouncing | ||
| lastFocusLossTime = std::chrono::steady_clock::now(); | ||
|
|
||
| logger::trace("Focus lost - cleared all key states ({} tracked keys), ImGui active ID, and event queue", keyCount); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing mouse button / wheel reset
OnFocusLost() releases keyboard keys but does not clear mouse buttons/wheel states.
If the user Alt-Tabs while holding a mouse button, ImGui may think it remains pressed.
Add:
for (int btn = 0; btn < ImGuiMouseButton_COUNT; ++btn)
io.AddMouseButtonEvent(btn, false);
io.AddMouseWheelEvent(0,0);🤖 Prompt for AI Agents
In src/Menu.cpp around lines 2248 to 2283, the OnFocusLost() function resets
keyboard keys but does not clear mouse button or wheel states, which can cause
ImGui to think mouse buttons remain pressed after focus loss. To fix this, add a
loop to send mouse button release events for all mouse buttons using
io.AddMouseButtonEvent with false, and reset the mouse wheel state by calling
io.AddMouseWheelEvent with zero values. Place this code after resetting keyboard
keys and before clearing ImGui's active ID.
|
✅ A pre-release build is available for this PR: |
|
Testers are saying this doesn't work. Need to rethink this. |
the issue might be occuring on either of the Alt+tab presses, both before and after. If you're only adding a delay when re-tabbing back to the game, that first alt+tab to get out of the game might be causing the issue, preventing the fix here from helping. Potentially some form of auto-discarding of inputs if a repeated input happens very fast over and over may help? This could bolster the resetting system you have already |
|
That's what debouncers do. I'll probably leave this here for others to work on but I'll move on. |
closes #902
closes #862
Summary by CodeRabbit