refactor: hotkey handling#1118
Conversation
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Menu
participant Settings
User->>Menu: Press/Release Key
Menu->>Menu: ProcessInputEventQueue()
alt Pending Hotkey Assignment
Menu->>Settings: Assign new hotkey via HotkeyAction lambda
Menu->>Menu: Reset assignment flag
else Normal Key Action
Menu->>Menu: Execute action via KeyAction lambda
end
Menu->>Menu: Continue with remaining input processing
Assessment against linked issues
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
CodeRabbit Configuration File (
|
Automated formatting by clang-format, prettier, and other hooks. See https://pre-commit.ci for details.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Menu.cpp (1)
1927-1940: Consider moving arrays to static/member variables for better performance.The
HotkeyActionarray is recreated on every key release event, which could impact performance if this method is called frequently. Consider moving these arrays to static variables or class members to avoid repeated allocations.+ // Move to class members or static variables + static const HotkeyAction hotkeyActions[] = { + { &settings.ToggleKey, &settingToggleKey, [this](uint32_t key) { settings.ToggleKey = key; settingToggleKey = false; } }, + { &settings.SkipCompilationKey, &settingSkipCompilationKey, [this](uint32_t key) { settings.SkipCompilationKey = key; settingSkipCompilationKey = false; } }, + { &settings.EffectToggleKey, &settingsEffectsToggle, [this](uint32_t key) { settings.EffectToggleKey = key; settingsEffectsToggle = false; } }, + { &settings.PerfOverlay.OverlayToggleKey, &settingOverlayToggleKey, [this](uint32_t key) { settings.PerfOverlay.OverlayToggleKey = key; settingOverlayToggleKey = false; } }, + }; - struct HotkeyAction - { - uint32_t* settingKey; - bool* settingFlag; - std::function<void(uint32_t)> action; - }; - auto shaderCache = globals::shaderCache; - auto devMode = globals::state->IsDeveloperMode(); - HotkeyAction hotkeyActions[] = { - { &settings.ToggleKey, &settingToggleKey, [this](uint32_t key) { settings.ToggleKey = key; settingToggleKey = false; } }, - { &settings.SkipCompilationKey, &settingSkipCompilationKey, [this](uint32_t key) { settings.SkipCompilationKey = key; settingSkipCompilationKey = false; } }, - { &settings.EffectToggleKey, &settingsEffectsToggle, [this](uint32_t key) { settings.EffectToggleKey = key; settingsEffectsToggle = false; } }, - { &settings.PerfOverlay.OverlayToggleKey, &settingOverlayToggleKey, [this](uint32_t key) { settings.PerfOverlay.OverlayToggleKey = key; settingOverlayToggleKey = false; } }, - };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Menu.cpp(1 hunks)
🔇 Additional comments (2)
src/Menu.cpp (2)
1927-1969: Excellent refactoring approach for maintainability.The data-driven approach using structured arrays and lambda functions significantly improves code maintainability compared to repetitive if-else chains. The logic correctly handles both hotkey setting and regular key actions with proper precedence.
1955-1962:Details
✅ Verification successful
Verify undefined variables and consider performance optimization.
Two concerns with this code segment:
priorShaderKeyandnextShaderKeyare referenced but not defined in the visible scope. Please ensure these variables are properly declared.Similar to the
HotkeyActionarray, consider movingKeyActionarray to avoid repeated allocations.
🏁 Script executed:
#!/bin/bash # Search for priorShaderKey and nextShaderKey definitions echo "Searching for priorShaderKey and nextShaderKey definitions..." rg -A 2 -B 2 "priorShaderKey|nextShaderKey" --type cppLength of output: 1107
priorShaderKey/nextShaderKey are properly declared
Both keys are defined in src/Menu.h:
uint32_t priorShaderKey = VK_PRIOR;uint32_t nextShaderKey = VK_NEXT;The
KeyActionarray in Menu.cpp is a fixed‐size local array allocated on the stack; its construction (including lambdas) has minimal overhead. If this code path is executed every frame and you observe a performance hotspot, you could hoist it to a static scope or cache it between calls. Otherwise, no changes are required here.
|
@coderabbitai Does the refactor still allow the setting of the key to something else? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 65555 @alandtse Yes, the refactor absolutely still allows setting keys to something else! Looking at the refactored code, the hotkey assignment functionality has been preserved and actually improved with the new structure. Here's how it works: The new HotkeyAction hotkeyActions[] = {
{ &settings.ToggleKey, &settingToggleKey, [this](uint32_t key) { settings.ToggleKey = key; settingToggleKey = false; } },
{ &settings.SkipCompilationKey, &settingSkipCompilationKey, [this](uint32_t key) { settings.SkipCompilationKey = key; settingSkipCompilationKey = false; } },
{ &settings.EffectToggleKey, &settingsEffectsToggle, [this](uint32_t key) { settings.EffectToggleKey = key; settingsEffectsToggle = false; } },
{ &settings.PerfOverlay.OverlayToggleKey, &settingOverlayToggleKey, [this](uint32_t key) { settings.PerfOverlay.OverlayToggleKey = key; settingOverlayToggleKey = false; } },
};Each entry has:
The UI in So the refactor successfully modernized the code structure while maintaining full hotkey customization capabilities. |
|
Does this affect behaviour |
|
Everything works for me. |
3 participants
closes #1108
Summary by CodeRabbit