fix(UI): prevent hotkeys from triggering ImGui navigation#2068
Conversation
📝 WalkthroughWalkthroughUpdated keyboard hotkey handling: added InputCombo::MatchesKeyboardCombo, switched Menu input processing to use it and to conditionally forward key events to ImGui, adjusted skip-compilation hotkey logic, and fixed Upscaling.ini EOF newline. (≈36 words) Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant OS as OS
participant Menu as Menu
participant InputUtil as InputUtil
participant ImGui as ImGui
participant ShaderCache as ShaderCache
User->>Menu: key event (vkKey + modifiers)
Menu->>InputUtil: MatchesKeyboardCombo(combo, vkKey)?
InputUtil->>OS: GetAsyncKeyState(Ctrl/Shift/Alt)
OS-->>InputUtil: current modifier states
InputUtil-->>Menu: match true/false
alt combo matched
Menu->>ShaderCache: call SkipCompilationKey lambda (check IsCompiling / toggle backgroundCompilation)
else not matched
alt ShouldSwallowInput() true AND key not a menu hotkey
Menu->>ImGui: forward key event (update ImGuiIO)
else
Note right of Menu: Do not forward event to ImGui
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
No actionable suggestions for changed features. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
features/Upscaling/Shaders/Features/Upscaling.ini (1)
1-2: Line ending normalization appears unrelated to PR objectives.This change only normalizes line endings (CRLF → LF) without modifying the actual configuration content. While line ending consistency is beneficial, this Upscaling configuration file appears unrelated to the PR's stated purpose of preventing hotkeys from triggering ImGui navigation.
If this was an incidental formatting cleanup, consider whether it should be in a separate housekeeping commit.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@features/Upscaling/Shaders/Features/Upscaling.ini` around lines 1 - 2, The change in Features/Upscaling/Shaders/Features/Upscaling.ini only normalizes line endings (CRLF → LF) without content changes (see [Info] and Version = 1-3-1); either revert this whitespace-only change from the current PR or move it into a separate housekeeping commit so the functional PR stays focused on preventing hotkeys from triggering ImGui navigation, ensuring the Upscaling.ini file content remains identical while preserving the original CRLF if you choose to revert.src/Menu.cpp (1)
1009-1009: Pre-existing thread safety note (not blocking).The guard logic is correct, but
shaderCache->backgroundCompilationis a plainbool(perShaderCache.h:554) that's written here and read from compilation worker threads (ShaderCache.cpp:3138-3141) without synchronization. This is a pre-existing concern, not introduced by this PR.Consider tracking this for a future cleanup:
-bool backgroundCompilation = false; +std::atomic<bool> backgroundCompilation = false;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Menu.cpp` at line 1009, The write to shaderCache->backgroundCompilation (tied to settings.SkipCompilationKey handler) is unsynchronized while worker threads read it, so make the access thread-safe: change ShaderCache::backgroundCompilation to std::atomic<bool> or protect it with a mutex and provide an explicit setter/getter (e.g., ShaderCache::SetBackgroundCompilation(bool) / ShaderCache::GetBackgroundCompilation()) and update all reads/writes (including the compilation worker code and the settings handler) to use those atomic/mutex-protected accessors to ensure safe cross-thread reads/writes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@features/Upscaling/Shaders/Features/Upscaling.ini`:
- Around line 1-2: The change in
Features/Upscaling/Shaders/Features/Upscaling.ini only normalizes line endings
(CRLF → LF) without content changes (see [Info] and Version = 1-3-1); either
revert this whitespace-only change from the current PR or move it into a
separate housekeeping commit so the functional PR stays focused on preventing
hotkeys from triggering ImGui navigation, ensuring the Upscaling.ini file
content remains identical while preserving the original CRLF if you choose to
revert.
In `@src/Menu.cpp`:
- Line 1009: The write to shaderCache->backgroundCompilation (tied to
settings.SkipCompilationKey handler) is unsynchronized while worker threads read
it, so make the access thread-safe: change ShaderCache::backgroundCompilation to
std::atomic<bool> or protect it with a mutex and provide an explicit
setter/getter (e.g., ShaderCache::SetBackgroundCompilation(bool) /
ShaderCache::GetBackgroundCompilation()) and update all reads/writes (including
the compilation worker code and the settings handler) to use those
atomic/mutex-protected accessors to ensure safe cross-thread reads/writes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ad94c4de-2332-48b4-a9c7-d5e215834d7e
📒 Files selected for processing (3)
features/Upscaling/Shaders/Features/Upscaling.inisrc/Menu.cppsrc/Utils/Input.h
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
|
pre commit CI bot keeps adding upscaling.ini to this PR for no reason |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/Menu.cpp (1)
1050-1058: Comment assumes default binding; SkipCompilationKey is user-configurable.The comment states "SkipCompilationKey (ESC) is excluded — ESC must reach ImGui for menu/dialog close" but
SkipCompilationKeyis user-configurable. If a user binds it to a navigation key (e.g., End, Home), that key will still be forwarded to ImGui when the menu is open, potentially causing unwanted scrolling.The current behavior is acceptable since the action is already blocked on line 1009, but the comment could be more accurate.
📝 Suggested comment clarification
- // Don't forward hotkey events to ImGui when input is captured (prevents e.g. End key scrolling the feature list) - // SkipCompilationKey (ESC) is excluded — ESC must reach ImGui for menu/dialog close. + // Don't forward hotkey events to ImGui when input is captured (prevents e.g. End key scrolling the feature list). + // SkipCompilationKey is excluded because its action is already blocked when UI is active (see line 1009), + // and the key may need to reach ImGui (e.g., ESC for closing dialogs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Menu.cpp` around lines 1050 - 1058, Update the misleading comment around the hotkey swallowing logic to reflect that SkipCompilationKey is user-configurable rather than always ESC: clarify that the exclusion of SkipCompilationKey (previously described as ESC) only applies to the configured SkipCompilationKey binding and that if a user binds it to navigation keys those keys may be forwarded to ImGui; reference the surrounding logic using ShouldSwallowInput, the hotkeys array, InputCombo::MatchesKeyboardCombo and the isHotkey variable so the reviewer can find and adjust the comment accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Menu.cpp`:
- Around line 1050-1058: Update the misleading comment around the hotkey
swallowing logic to reflect that SkipCompilationKey is user-configurable rather
than always ESC: clarify that the exclusion of SkipCompilationKey (previously
described as ESC) only applies to the configured SkipCompilationKey binding and
that if a user binds it to navigation keys those keys may be forwarded to ImGui;
reference the surrounding logic using ShouldSwallowInput, the hotkeys array,
InputCombo::MatchesKeyboardCombo and the isHotkey variable so the reviewer can
find and adjust the comment accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a076c91-be15-4e6e-8aba-37fc1963543a
📒 Files selected for processing (3)
features/Upscaling/Shaders/Features/Upscaling.inisrc/Menu.cppsrc/Utils/Input.h
✅ Files skipped from review due to trivial changes (1)
- features/Upscaling/Shaders/Features/Upscaling.ini
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Utils/Input.h
|
✅ A pre-release build is available for this PR: |
alandtse
left a comment
alandtse
left a comment
There was a problem hiding this comment.
You tested that combos still work after this change correct?
Yes combos still work |
…shaders#2068) Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> (cherry picked from commit 41d486c)
2 participants
Summary by CodeRabbit
Refactor
Style