Shortcuts v2. Added presets - #213
Conversation
📝 WalkthroughWalkthroughThe shortcuts build now selects the V2 or legacy module conditionally. Shortcuts V2 adds named preset configuration, selection, persistence, diff application, reset, and deletion. The QML model and panels expose preset state and track unsaved changes. Tests cover preset loading, persistence, reset, editing, and deletion. Compatibility stubs implement the expanded configuration interface. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@framework/shortcuts_v2/internal/commandshortcutsregister.cpp`:
- Around line 361-369: Update setShortcuts() so the empty-diff branch assigns ok
from removeUserFile() instead of assuming success, and change removeUserFile()
to return whether removal succeeded. Preserve m_shortcuts updates and success
signaling only when ok is true, and propagate the same failure handling through
reset and deletion flows, including appropriate logging or handling.
- Around line 413-415: Replace mi::ReadResourceLockGuard with
mi::WriteResourceLockGuard around the io::File::copy operation in the preset
import block, preserving the existing multiwindowsProvider() and
COMMAND_SHORTCUTS_TAG lock scope.
In `@framework/shortcuts_v2/internal/shortcutsconfiguration.cpp`:
- Around line 116-119: Validate shortcutsName before
commandShortcutsUserAppDataPath constructs the path: reject empty names or
segments, path separators, and traversal components such as “.” or “..”. Apply
the same validation to persisted preset names before setShortcuts(),
importFromFile(), or deletePreset() resolves, writes, copies, or removes paths,
and fail safely for invalid values.
In `@framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp`:
- Around line 191-193: Update the currentPresetNameChanged callback in
ShortcutsModel so it also emits presetsChanged() after a preset switch. Preserve
the existing currentPresetNameChanged() emission, ensuring preset-dependent
properties such as isCurrentPresetEdited and canDeleteCurrentPreset refresh for
the newly selected preset.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0224aa99-2354-486d-be4e-cc87de88eb14
📒 Files selected for processing (27)
framework/CMakeLists.txtframework/shortcuts/ishortcutsconfiguration.hframework/shortcuts/ishortcutscontroller.hframework/shortcuts/ishortcutsregister.hframework/shortcuts/shortcutcontext.hframework/shortcuts/shortcutsmodule.hframework/shortcuts/shortcutstypes.hframework/shortcuts_v2/CMakeLists.txtframework/shortcuts_v2/icommandshortcutsregister.hframework/shortcuts_v2/internal/commandshortcutsregister.cppframework/shortcuts_v2/internal/commandshortcutsregister.hframework/shortcuts_v2/internal/shortcutsconfiguration.cppframework/shortcuts_v2/internal/shortcutsconfiguration.hframework/shortcuts_v2/ishortcutsconfiguration.hframework/shortcuts_v2/qml/Muse/Shortcuts/ShortcutsPage.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsTopPanel.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.hframework/shortcuts_v2/tests/CMakeLists.txtframework/shortcuts_v2/tests/commandshortcutsregister_tests.cppframework/shortcuts_v2/tests/mocks/shortcutsconfigurationmock.hframework/shortcuts_v2/tests/testdata/shortcuts.jsonframework/shortcuts_v2/tests/testdata/shortcuts_azerty.jsonframework/shortcuts_v2/tests/testdata/shortcuts_mac.jsonframework/stubs/shortcuts/shortcutsconfigurationstub.cppframework/stubs/shortcuts/shortcutsconfigurationstub.hframework/ui/uitypes.h
💤 Files with no reviewable changes (2)
- framework/ui/uitypes.h
- framework/CMakeLists.txt
| bool ok = true; | ||
| if (diff.empty()) { | ||
| removeUserFile(); | ||
| } else { | ||
| ok = writeToFile(diff, userShortcutsPath()); | ||
| } | ||
|
|
||
| if (ok) { | ||
| m_shortcuts = needToWrite; | ||
| m_shortcuts = diff; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not report success when user-file removal fails.
When diff is empty, Line 363 removes the user file but leaves ok true. If removal fails, this method updates m_shortcuts, sends m_shortcutsChanged, and returns success while the old diff remains on disk. The edit returns after the next reload.
Make removeUserFile() return the removal status. Propagate that status through setShortcuts(). Log or handle the same failure in reset and deletion flows.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 361-361: variable name 'ok' is too short, expected at least 3 characters
(readability-identifier-length)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/shortcuts_v2/internal/commandshortcutsregister.cpp` around lines
361 - 369, Update setShortcuts() so the empty-diff branch assigns ok from
removeUserFile() instead of assuming success, and change removeUserFile() to
return whether removal succeeded. Preserve m_shortcuts updates and success
signaling only when ok is true, and propagate the same failure handling through
reset and deletion flows, including appropriate logging or handling.
| { | ||
| mi::ReadResourceLockGuard guard(multiwindowsProvider(), COMMAND_SHORTCUTS_TAG); | ||
| Ret ret = io::File::copy(filePath, configuration()->commandShortcutsUserAppDataPath(), true); | ||
| Ret ret = io::File::copy(filePath, userShortcutsPath(), true); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use a write lock for preset import.
io::File::copy() writes userShortcutsPath(), but this block uses mi::ReadResourceLockGuard. A concurrent reload can read a partially copied JSON file. A concurrent writer can also overwrite the imported preset.
Replace the read guard with mi::WriteResourceLockGuard.
Proposed fix
- mi::ReadResourceLockGuard guard(multiwindowsProvider(), COMMAND_SHORTCUTS_TAG);
+ mi::WriteResourceLockGuard guard(multiwindowsProvider(), COMMAND_SHORTCUTS_TAG);
Ret ret = io::File::copy(filePath, userShortcutsPath(), true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| mi::ReadResourceLockGuard guard(multiwindowsProvider(), COMMAND_SHORTCUTS_TAG); | |
| Ret ret = io::File::copy(filePath, configuration()->commandShortcutsUserAppDataPath(), true); | |
| Ret ret = io::File::copy(filePath, userShortcutsPath(), true); | |
| { | |
| mi::WriteResourceLockGuard guard(multiwindowsProvider(), COMMAND_SHORTCUTS_TAG); | |
| Ret ret = io::File::copy(filePath, userShortcutsPath(), true); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/shortcuts_v2/internal/commandshortcutsregister.cpp` around lines
413 - 415, Replace mi::ReadResourceLockGuard with mi::WriteResourceLockGuard
around the io::File::copy operation in the preset import block, preserving the
existing multiwindowsProvider() and COMMAND_SHORTCUTS_TAG lock scope.
| io::path_t ShortcutsConfiguration::commandShortcutsUserAppDataPath(const std::string& shortcutsName) const | ||
| { | ||
| return userShortcutsDirPath() + "/" + shortcutsName + ".json"; | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Validate preset names before building file paths.
Line 118 concatenates an unchecked preset name into a writable path. A value such as "../../other-file" escapes the shortcuts directory. setShortcuts(), importFromFile(), and deletePreset() then write, copy, or remove the resolved path.
Reject path separators, traversal components, and empty segments before persisting or resolving a preset name. Apply the validation to persisted values before using them.
🧰 Tools
🪛 Clang (14.0.6)
[warning] 116-116: use a trailing return type for this function
(modernize-use-trailing-return-type)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/shortcuts_v2/internal/shortcutsconfiguration.cpp` around lines 116
- 119, Validate shortcutsName before commandShortcutsUserAppDataPath constructs
the path: reject empty names or segments, path separators, and traversal
components such as “.” or “..”. Apply the same validation to persisted preset
names before setShortcuts(), importFromFile(), or deletePreset() resolves,
writes, copies, or removes paths, and fail safely for invalid values.
| commandShortcutsRegister()->currentPresetNameChanged().onReceive(this, [this](const std::string&) { | ||
| emit currentPresetNameChanged(); | ||
| }, async::Asyncable::Mode::SetReplace); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Notify preset-dependent properties after a preset switch.
Line 191 emits currentPresetNameChanged() only. isCurrentPresetEdited and canDeleteCurrentPreset use presetsChanged as their notifier. The reset button can retain the state of the previous preset after selection. Emit presetsChanged() in this callback.
Proposed fix
commandShortcutsRegister()->currentPresetNameChanged().onReceive(this, [this](const std::string&) {
emit currentPresetNameChanged();
+ emit presetsChanged();
}, async::Asyncable::Mode::SetReplace);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| commandShortcutsRegister()->currentPresetNameChanged().onReceive(this, [this](const std::string&) { | |
| emit currentPresetNameChanged(); | |
| }, async::Asyncable::Mode::SetReplace); | |
| commandShortcutsRegister()->currentPresetNameChanged().onReceive(this, [this](const std::string&) { | |
| emit currentPresetNameChanged(); | |
| emit presetsChanged(); | |
| }, async::Asyncable::Mode::SetReplace); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp` around lines
191 - 193, Update the currentPresetNameChanged callback in ShortcutsModel so it
also emits presetsChanged() after a preset switch. Preserve the existing
currentPresetNameChanged() emission, ensuring preset-dependent properties such
as isCurrentPresetEdited and canDeleteCurrentPreset refresh for the newly
selected preset.
1 participant
Adds preset support to the command shortcuts system. A preset is a named keyboard layout scheme (e.g. AZERTY) that the user can switch to, customize, and reset — without losing customizations of other presets.
Shortcuts are resolved as a chain of layers, each layer is a diff applied on top of the previous one:
The user diff of each set lives in its own file, so switching presets never loses customizations. A set is considered edited iff its user file exists (same convention as workspaces). When the user reverts all changes, the computed diff becomes empty and the file is deleted — the set returns to the pristine state automatically.
UI isn't finished (just an example)
Screen.Recording.2026-08-11.at.5.19.27.PM.mov