returned shortcut module v1#148
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (56)
💤 Files with no reviewable changes (9)
📝 WalkthroughWalkthroughAdds a selectable Shortcuts v2 module with new CMake configuration, interfaces, storage, controller, API, and module lifecycle wiring. The existing shortcuts implementation is decoupled from command shortcut registration and simplified around unified shortcut data. New QML models and components provide shortcut listing, editing, conflict handling, import/export, reset, and runtime activation. macOS-specific code translates shortcut sequences through the active keyboard layout. 🚥 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: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/shortcuts/shortcutstypes.h (1)
54-60: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the
contextcomparison inoperator==.During the decoupling of the legacy shortcuts module, the comparison for the
contextfield was accidentally removed fromoperator==, despitecontextstill being an active member of this struct.Omitting it means two shortcuts with identical actions and sequences but strictly different contexts will evaluate to
truewhen compared. Based on upstream usages (likeShortcutsModel::resetToDefaultSelectedShortcuts), this false positive will incorrectly trigger loopcontinuestatements and silently skip critical shortcut conflict resolutions.🐛 Proposed fix to restore struct equality logic
bool operator ==(const Shortcut& sc) const { return action == sc.action + && context == sc.context && sequences == sc.sequences && standardKey == sc.standardKey && autoRepeat == sc.autoRepeat; }🤖 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/shortcutstypes.h` around lines 54 - 60, Update Shortcut::operator== to include a comparison of the active context member alongside action, sequences, standardKey, and autoRepeat, so shortcuts with different contexts are not considered equal.
🤖 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 344-358: Replace the ReadResourceLockGuard with
WriteResourceLockGuard in both importFromFile paths:
framework/shortcuts_v2/internal/commandshortcutsregister.cpp lines 344-358 and
framework/shortcuts_v2/internal/shortcutsregister.cpp lines 519-533. Keep the
existing file-copy, error handling, and reload behavior unchanged.
- Around line 159-174: Update
CommandShortcutsRegister::filterAndUpdateAdditionalShortcuts so the
additional-shortcut lookup uses shortcut.command rather than shortcut.action
when searching shortcuts. Preserve the existing refresh and removal behavior,
ensuring matching continues to use the command/scope identity expected by the
register.
In `@framework/shortcuts_v2/internal/shortcutscontroller.cpp`:
- Around line 27-33: Disable unconditional debug logging by changing the
SHORTCUTS_DEBUG configuration used by SC_LOG() in shortcutscontroller.cpp so
production builds resolve to LOGN() rather than LOGDA(). Preserve the existing
conditional macro structure and allow debug logging only through an intentional
build-time configuration.
In `@framework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.cpp`:
- Around line 47-61: Update the shortcut identity handling in the self-skip loop
and trySave() so entries are matched by a stable unique identifier rather than
full QVariantMap equality. Use the identifier introduced by the shortcutToObject
root-cause fix for the comparisons currently using shortcut == originShortcut
and m_allShortcuts.indexOf(m_conflictShortcut), preserving the existing
conflict-selection behavior for distinct shortcuts with identical fields.
- Around line 183-200: Update checkNewSequenceForConflicts() and trySave() to
use newSequenceInNativeFormat() whenever comparing the edited sequence with
stored shortcut sequences. Preserve the existing conflict detection and
no-change guard logic while ensuring both comparisons use native-text
representations consistently.
In
`@framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm`:
- Around line 376-406: Document the rationale for the untranslated-sequence
condition in doLoadShortcuts, specifically explaining the low-byte key check and
the special handling of Qt::Key_A before the recordMapping call. Keep the
existing condition and behavior unchanged, and place the explanation alongside
the condition using the surrounding method’s established documentation style.
- Around line 250-286: Update keyCodeToString so the resolved LMGetKbdType
function pointer is checked for null before invocation, matching the guard
already used in keyboardLayout. Return the existing empty-string fallback when
the symbol cannot be resolved, and preserve the current UCKeyTranslate flow
otherwise.
- Around line 89-111: Update keyboardLayout() to release the TISInputSourceRef
returned by TISCopyCurrentKeyboardLayoutInputSource() after obtaining the
Unicode layout data, while keeping that data valid for the translation scope.
Prefer retaining the source/data ownership through the caller’s translation
operation or change keyboardLayout() to return an owning representation instead
of a raw UCKeyboardLayout*.
- Around line 288-311: Update translateToCurrentKeyboardLayout to validate
sequence is non-empty before accessing sequence[0], and validate
keyStr/fromString produces a non-empty sequence before accessing its first
element. Return {} for either empty case while preserving the existing
keyboard-layout and key-lookup handling.
In `@framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp`:
- Around line 373-382: Update ShortcutsModel::shortcutToObject to include a
stable unique identifier for each shortcut, then update EditShortcutModel::load
to compare and resolve shortcuts using that identifier rather than full
QVariantMap equality, ensuring distinct shortcuts with identical title,
sequence, context, and autoRepeat remain distinguishable.
- Around line 182-216: Make ShortcutsModel::apply() update
commandShortcutsRegister() and shortcutsRegister() atomically: if the second
setShortcuts() fails, restore the command register’s previous shortcuts (or use
an existing transaction mechanism) before returning false. Preserve the current
error logging and success behavior while ensuring neither register remains
partially updated.
- Around line 259-286: Update ShortcutsModel::importShortcutsFromFile(),
exportShortcutsToFile(), and resetToDefaultSelectedShortcuts() to process
command shortcuts through commandShortcutsRegister() alongside
shortcutsRegister(). Ensure command shortcut entries are included in
import/export round-trips and reset using the command register’s defaults
instead of being cleared.
In `@framework/shortcuts_v2/shortcutstypes.h`:
- Around line 95-110: Replace the static std::set lookup in needIgnoreKey with a
switch over the ignored Qt::Key values, returning true for each listed key and
false by default. Preserve the existing ignored-key list and function behavior
while removing the set allocation and lookup.
- Around line 74-84: Update sequencesToString and sequencesFromString to use a
serialization delimiter that cannot occur in QKeySequence text, preserving each
multi-stroke shortcut as one sequence during round-tripping. Apply the same
delimiter consistently for joining and splitting, without changing the
surrounding conversion behavior.
In `@framework/shortcuts/internal/shortcutscontroller.cpp`:
- Around line 72-74: Replace the IF_ASSERT_FAILED guard in the shortcut
activation flow with a standard emptiness check for shortcutsForSequence,
returning ActionCode() without asserting when the sequence is unknown. Add a
warning log if consistent with the surrounding logging conventions, while
preserving normal activation for matched sequences.
---
Outside diff comments:
In `@framework/shortcuts/shortcutstypes.h`:
- Around line 54-60: Update Shortcut::operator== to include a comparison of the
active context member alongside action, sequences, standardKey, and autoRepeat,
so shortcuts with different contexts are not considered equal.
🪄 Autofix (Beta)
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
Run ID: 96554f4e-cf1f-4a53-a2e2-c5dbe57dd726
📒 Files selected for processing (56)
framework/CMakeLists.txtframework/cmake/MuseDeclareOptions.cmakeframework/cmake/muse_framework_config.h.inframework/shortcuts/CMakeLists.txtframework/shortcuts/internal/shortcutsconfiguration.cppframework/shortcuts/internal/shortcutsconfiguration.hframework/shortcuts/internal/shortcutscontroller.cppframework/shortcuts/internal/shortcutscontroller.hframework/shortcuts/internal/shortcutsregister.cppframework/shortcuts/ishortcutsconfiguration.hframework/shortcuts/qml/Muse/Shortcuts/editshortcutmodel.cppframework/shortcuts/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts/qml/Muse/Shortcuts/shortcutsinstancemodel.cppframework/shortcuts/qml/Muse/Shortcuts/shortcutsinstancemodel.hframework/shortcuts/qml/Muse/Shortcuts/shortcutsmodel.cppframework/shortcuts/qml/Muse/Shortcuts/shortcutsmodel.hframework/shortcuts/shortcutsmodule.cppframework/shortcuts/shortcutsmodule.hframework/shortcuts/shortcutstypes.hframework/shortcuts_v2/CMakeLists.txtframework/shortcuts_v2/README.mdframework/shortcuts_v2/api/shortcutsapi.cppframework/shortcuts_v2/api/shortcutsapi.hframework/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/internal/shortcutscontroller.cppframework/shortcuts_v2/internal/shortcutscontroller.hframework/shortcuts_v2/internal/shortcutsregister.cppframework/shortcuts_v2/internal/shortcutsregister.hframework/shortcuts_v2/ishortcutsconfiguration.hframework/shortcuts_v2/ishortcutscontroller.hframework/shortcuts_v2/ishortcutsregister.hframework/shortcuts_v2/ishortcutsresolver.hframework/shortcuts_v2/qml/Muse/Shortcuts/CMakeLists.txtframework/shortcuts_v2/qml/Muse/Shortcuts/EditShortcutDialogContent.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/Shortcuts.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/ShortcutsPage.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/StandardEditShortcutDialog.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.hframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsBottomPanel.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsTopPanel.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.hframework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mmframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsinstancemodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsinstancemodel.hframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.hframework/shortcuts_v2/shortcutcontext.hframework/shortcuts_v2/shortcutsmodule.cppframework/shortcuts_v2/shortcutsmodule.hframework/shortcuts_v2/shortcutstypes.h
💤 Files with no reviewable changes (9)
- framework/shortcuts/qml/Muse/Shortcuts/internal/ShortcutsList.qml
- framework/shortcuts/internal/shortcutscontroller.h
- framework/shortcuts/ishortcutsconfiguration.h
- framework/shortcuts/CMakeLists.txt
- framework/shortcuts/shortcutsmodule.cpp
- framework/shortcuts/internal/shortcutsconfiguration.h
- framework/shortcuts/shortcutsmodule.h
- framework/shortcuts/qml/Muse/Shortcuts/shortcutsinstancemodel.h
- framework/shortcuts/internal/shortcutsregister.cpp
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
framework/shortcuts/shortcutstypes.h (1)
54-60: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the
contextcomparison inoperator==.During the decoupling of the legacy shortcuts module, the comparison for the
contextfield was accidentally removed fromoperator==, despitecontextstill being an active member of this struct.Omitting it means two shortcuts with identical actions and sequences but strictly different contexts will evaluate to
truewhen compared. Based on upstream usages (likeShortcutsModel::resetToDefaultSelectedShortcuts), this false positive will incorrectly trigger loopcontinuestatements and silently skip critical shortcut conflict resolutions.🐛 Proposed fix to restore struct equality logic
bool operator ==(const Shortcut& sc) const { return action == sc.action + && context == sc.context && sequences == sc.sequences && standardKey == sc.standardKey && autoRepeat == sc.autoRepeat; }🤖 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/shortcutstypes.h` around lines 54 - 60, Update Shortcut::operator== to include a comparison of the active context member alongside action, sequences, standardKey, and autoRepeat, so shortcuts with different contexts are not considered equal.
🤖 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 344-358: Replace the ReadResourceLockGuard with
WriteResourceLockGuard in both importFromFile paths:
framework/shortcuts_v2/internal/commandshortcutsregister.cpp lines 344-358 and
framework/shortcuts_v2/internal/shortcutsregister.cpp lines 519-533. Keep the
existing file-copy, error handling, and reload behavior unchanged.
- Around line 159-174: Update
CommandShortcutsRegister::filterAndUpdateAdditionalShortcuts so the
additional-shortcut lookup uses shortcut.command rather than shortcut.action
when searching shortcuts. Preserve the existing refresh and removal behavior,
ensuring matching continues to use the command/scope identity expected by the
register.
In `@framework/shortcuts_v2/internal/shortcutscontroller.cpp`:
- Around line 27-33: Disable unconditional debug logging by changing the
SHORTCUTS_DEBUG configuration used by SC_LOG() in shortcutscontroller.cpp so
production builds resolve to LOGN() rather than LOGDA(). Preserve the existing
conditional macro structure and allow debug logging only through an intentional
build-time configuration.
In `@framework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.cpp`:
- Around line 47-61: Update the shortcut identity handling in the self-skip loop
and trySave() so entries are matched by a stable unique identifier rather than
full QVariantMap equality. Use the identifier introduced by the shortcutToObject
root-cause fix for the comparisons currently using shortcut == originShortcut
and m_allShortcuts.indexOf(m_conflictShortcut), preserving the existing
conflict-selection behavior for distinct shortcuts with identical fields.
- Around line 183-200: Update checkNewSequenceForConflicts() and trySave() to
use newSequenceInNativeFormat() whenever comparing the edited sequence with
stored shortcut sequences. Preserve the existing conflict detection and
no-change guard logic while ensuring both comparisons use native-text
representations consistently.
In
`@framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm`:
- Around line 376-406: Document the rationale for the untranslated-sequence
condition in doLoadShortcuts, specifically explaining the low-byte key check and
the special handling of Qt::Key_A before the recordMapping call. Keep the
existing condition and behavior unchanged, and place the explanation alongside
the condition using the surrounding method’s established documentation style.
- Around line 250-286: Update keyCodeToString so the resolved LMGetKbdType
function pointer is checked for null before invocation, matching the guard
already used in keyboardLayout. Return the existing empty-string fallback when
the symbol cannot be resolved, and preserve the current UCKeyTranslate flow
otherwise.
- Around line 89-111: Update keyboardLayout() to release the TISInputSourceRef
returned by TISCopyCurrentKeyboardLayoutInputSource() after obtaining the
Unicode layout data, while keeping that data valid for the translation scope.
Prefer retaining the source/data ownership through the caller’s translation
operation or change keyboardLayout() to return an owning representation instead
of a raw UCKeyboardLayout*.
- Around line 288-311: Update translateToCurrentKeyboardLayout to validate
sequence is non-empty before accessing sequence[0], and validate
keyStr/fromString produces a non-empty sequence before accessing its first
element. Return {} for either empty case while preserving the existing
keyboard-layout and key-lookup handling.
In `@framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp`:
- Around line 373-382: Update ShortcutsModel::shortcutToObject to include a
stable unique identifier for each shortcut, then update EditShortcutModel::load
to compare and resolve shortcuts using that identifier rather than full
QVariantMap equality, ensuring distinct shortcuts with identical title,
sequence, context, and autoRepeat remain distinguishable.
- Around line 182-216: Make ShortcutsModel::apply() update
commandShortcutsRegister() and shortcutsRegister() atomically: if the second
setShortcuts() fails, restore the command register’s previous shortcuts (or use
an existing transaction mechanism) before returning false. Preserve the current
error logging and success behavior while ensuring neither register remains
partially updated.
- Around line 259-286: Update ShortcutsModel::importShortcutsFromFile(),
exportShortcutsToFile(), and resetToDefaultSelectedShortcuts() to process
command shortcuts through commandShortcutsRegister() alongside
shortcutsRegister(). Ensure command shortcut entries are included in
import/export round-trips and reset using the command register’s defaults
instead of being cleared.
In `@framework/shortcuts_v2/shortcutstypes.h`:
- Around line 95-110: Replace the static std::set lookup in needIgnoreKey with a
switch over the ignored Qt::Key values, returning true for each listed key and
false by default. Preserve the existing ignored-key list and function behavior
while removing the set allocation and lookup.
- Around line 74-84: Update sequencesToString and sequencesFromString to use a
serialization delimiter that cannot occur in QKeySequence text, preserving each
multi-stroke shortcut as one sequence during round-tripping. Apply the same
delimiter consistently for joining and splitting, without changing the
surrounding conversion behavior.
In `@framework/shortcuts/internal/shortcutscontroller.cpp`:
- Around line 72-74: Replace the IF_ASSERT_FAILED guard in the shortcut
activation flow with a standard emptiness check for shortcutsForSequence,
returning ActionCode() without asserting when the sequence is unknown. Add a
warning log if consistent with the surrounding logging conventions, while
preserving normal activation for matched sequences.
---
Outside diff comments:
In `@framework/shortcuts/shortcutstypes.h`:
- Around line 54-60: Update Shortcut::operator== to include a comparison of the
active context member alongside action, sequences, standardKey, and autoRepeat,
so shortcuts with different contexts are not considered equal.
🪄 Autofix (Beta)
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
Run ID: 96554f4e-cf1f-4a53-a2e2-c5dbe57dd726
📒 Files selected for processing (56)
framework/CMakeLists.txtframework/cmake/MuseDeclareOptions.cmakeframework/cmake/muse_framework_config.h.inframework/shortcuts/CMakeLists.txtframework/shortcuts/internal/shortcutsconfiguration.cppframework/shortcuts/internal/shortcutsconfiguration.hframework/shortcuts/internal/shortcutscontroller.cppframework/shortcuts/internal/shortcutscontroller.hframework/shortcuts/internal/shortcutsregister.cppframework/shortcuts/ishortcutsconfiguration.hframework/shortcuts/qml/Muse/Shortcuts/editshortcutmodel.cppframework/shortcuts/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts/qml/Muse/Shortcuts/shortcutsinstancemodel.cppframework/shortcuts/qml/Muse/Shortcuts/shortcutsinstancemodel.hframework/shortcuts/qml/Muse/Shortcuts/shortcutsmodel.cppframework/shortcuts/qml/Muse/Shortcuts/shortcutsmodel.hframework/shortcuts/shortcutsmodule.cppframework/shortcuts/shortcutsmodule.hframework/shortcuts/shortcutstypes.hframework/shortcuts_v2/CMakeLists.txtframework/shortcuts_v2/README.mdframework/shortcuts_v2/api/shortcutsapi.cppframework/shortcuts_v2/api/shortcutsapi.hframework/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/internal/shortcutscontroller.cppframework/shortcuts_v2/internal/shortcutscontroller.hframework/shortcuts_v2/internal/shortcutsregister.cppframework/shortcuts_v2/internal/shortcutsregister.hframework/shortcuts_v2/ishortcutsconfiguration.hframework/shortcuts_v2/ishortcutscontroller.hframework/shortcuts_v2/ishortcutsregister.hframework/shortcuts_v2/ishortcutsresolver.hframework/shortcuts_v2/qml/Muse/Shortcuts/CMakeLists.txtframework/shortcuts_v2/qml/Muse/Shortcuts/EditShortcutDialogContent.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/Shortcuts.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/ShortcutsPage.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/StandardEditShortcutDialog.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.hframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsBottomPanel.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsList.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/internal/ShortcutsTopPanel.qmlframework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.hframework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mmframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsinstancemodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsinstancemodel.hframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cppframework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.hframework/shortcuts_v2/shortcutcontext.hframework/shortcuts_v2/shortcutsmodule.cppframework/shortcuts_v2/shortcutsmodule.hframework/shortcuts_v2/shortcutstypes.h
💤 Files with no reviewable changes (9)
- framework/shortcuts/qml/Muse/Shortcuts/internal/ShortcutsList.qml
- framework/shortcuts/internal/shortcutscontroller.h
- framework/shortcuts/ishortcutsconfiguration.h
- framework/shortcuts/CMakeLists.txt
- framework/shortcuts/shortcutsmodule.cpp
- framework/shortcuts/internal/shortcutsconfiguration.h
- framework/shortcuts/shortcutsmodule.h
- framework/shortcuts/qml/Muse/Shortcuts/shortcutsinstancemodel.h
- framework/shortcuts/internal/shortcutsregister.cpp
🛑 Comments failed to post (15)
framework/shortcuts_v2/internal/commandshortcutsregister.cpp (2)
159-174: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash fd shortcutstypes.h cat -n framework/shortcuts_v2/shortcutstypes.h 2>/dev/null echo "---" rg -n 'operator==' framework/shortcuts_v2 framework/shortcuts 2>/dev/null echo "---" rg -n 'struct Shortcut\b' -A 40 framework/shortcuts_v2/shortcutstypes.h 2>/dev/nullRepository: musescore/muse_framework
Length of output: 6276
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== commandshortcutsregister.cpp ==" cat -n framework/shortcuts_v2/internal/commandshortcutsregister.cpp | sed -n '145,190p' echo echo "== action-based counterpart ==" cat -n framework/shortcuts_v2/internal/shortcutsregister.cpp | sed -n '270,315p' echo echo "== string comparison overloads / find usage ==" rg -n 'operator==.*std::string|std::string.*operator==|std::find\\(' framework/shortcuts_v2 framework/shortcuts echo echo "== command shortcut persistence / loading ==" rg -n 'm_additionalShortcutsMap|additionalShortcuts|filterAndUpdateAdditionalShortcuts|mergeShortcuts|makeUnique|writeToFile|readFromFile' framework/shortcuts_v2/internalRepository: musescore/muse_framework
Length of output: 3827
🏁 Script executed:
#!/bin/bash set -euo pipefail cat -n framework/shortcuts_v2/internal/commandshortcutsregister.cpp | sed -n '145,190p' echo cat -n framework/shortcuts_v2/internal/shortcutsregister.cpp | sed -n '270,315p' echo rg -n 'operator==.*std::string|std::string.*operator==|std::find\(' framework/shortcuts_v2 framework/shortcutsRepository: musescore/muse_framework
Length of output: 4685
Use
shortcut.commandin the additional-shortcut lookup.std::find(..., shortcut.action)matches onactionvia theShortcut(std::string)constructor, but this register is keyed bycommand/scope, so additional shortcuts won’t be refreshed or removed correctly.🧰 Tools
🪛 Clang (14.0.6)
[warning] 159-159: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 161-161: variable 'noAdditionalShortcuts' is not initialized
(cppcoreguidelines-init-variables)
🤖 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 159 - 174, Update CommandShortcutsRegister::filterAndUpdateAdditionalShortcuts so the additional-shortcut lookup uses shortcut.command rather than shortcut.action when searching shortcuts. Preserve the existing refresh and removal behavior, ensuring matching continues to use the command/scope identity expected by the register.
344-358: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a write lock in both
importFromFilepaths. They copy into the shared user-data file while holdingmi::ReadResourceLockGuard; this should match the other write paths and usemi::WriteResourceLockGuardinstead.🧰 Tools
🪛 Clang (14.0.6)
[warning] 344-344: use a trailing return type for this function
(modernize-use-trailing-return-type)
📍 Affects 2 files
framework/shortcuts_v2/internal/commandshortcutsregister.cpp#L344-L358(this comment)framework/shortcuts_v2/internal/shortcutsregister.cpp#L519-L533🤖 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 344 - 358, Replace the ReadResourceLockGuard with WriteResourceLockGuard in both importFromFile paths: framework/shortcuts_v2/internal/commandshortcutsregister.cpp lines 344-358 and framework/shortcuts_v2/internal/shortcutsregister.cpp lines 519-533. Keep the existing file-copy, error handling, and reload behavior unchanged.framework/shortcuts_v2/internal/shortcutscontroller.cpp (1)
27-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Debug logging hardcoded on for all builds.
SHORTCUTS_DEBUGis hardcoded to1, soSC_LOG()always resolves toLOGDA()(always-visible debug log) rather than the presumably-quieterLOGN(). This means everyactivate()call logs sequence/allowed-shortcut counts (lines 57, 64, 76) in production builds. This looks like a debug leftover.♻️ Proposed fix
-#define SHORTCUTS_DEBUG 1 +#define SHORTCUTS_DEBUG 0📝 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.`#define` SHORTCUTS_DEBUG 0 `#if` SHORTCUTS_DEBUG `#define` SC_LOG() LOGDA() << "[SC] " `#else` `#define` SC_LOG() LOGN() `#endif`🧰 Tools
🪛 Clang (14.0.6)
[warning] 27-27: macro 'SHORTCUTS_DEBUG' used to declare a constant; consider using a 'constexpr' constant
(cppcoreguidelines-macro-usage)
[warning] 30-30: function-like macro 'SC_LOG' used; consider a 'constexpr' template function
(cppcoreguidelines-macro-usage)
🤖 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/shortcutscontroller.cpp` around lines 27 - 33, Disable unconditional debug logging by changing the SHORTCUTS_DEBUG configuration used by SC_LOG() in shortcutscontroller.cpp so production builds resolve to LOGN() rather than LOGDA(). Preserve the existing conditional macro structure and allow debug logging only through an intentional build-time configuration.framework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.cpp (2)
47-61: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Self/conflict matching relies on full-map value equality with no unique identifier.
shortcut == originShortcut(Line 51) and laterm_allShortcuts.indexOf(m_conflictShortcut)intrySave()(Line 253) rely onQVariantMapequality across title/sequence/context/autoRepeat. The map built byShortcutsModel::shortcutToObjectcarries no unique id, so two distinct shortcuts that happen to share those fields could be indistinguishable, causing the wrong entry to be skipped or cleared. See consolidated comment onshortcutsmodel.cpp'sshortcutToObjectfor the root-cause fix.🧰 Tools
🪛 Clang (14.0.6)
[warning] 47-47: variable 'originShortcutMap' is not initialized
(cppcoreguidelines-init-variables)
[warning] 48-48: variable 'originCtx' is not initialized
(cppcoreguidelines-init-variables)
🤖 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/editshortcutmodel.cpp` around lines 47 - 61, Update the shortcut identity handling in the self-skip loop and trySave() so entries are matched by a stable unique identifier rather than full QVariantMap equality. Use the identifier introduced by the shortcutToObject root-cause fix for the comparisons currently using shortcut == originShortcut and m_allShortcuts.indexOf(m_conflictShortcut), preserving the existing conflict-selection behavior for distinct shortcuts with identical fields.
183-200: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail # Locate the relevant files and inspect the exact implementations. git ls-files framework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.cpp \ framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp \ framework/shortcuts_v2/qml/Muse/Shortcuts/*.h \ framework/shortcuts/shortcutstypes.h \ | sed 's#^`#FILE`: #' echo echo "=== editshortcutmodel.cpp (selected lines) ===" sed -n '150,280p' framework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.cpp echo echo "=== shortcutsmodel.cpp (selected sequence serialization) ===" sed -n '1,240p' framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp echo echo "=== shortcutstypes.h (sequence helpers) ===" sed -n '1,260p' framework/shortcuts/shortcutstypes.hRepository: musescore/muse_framework
Length of output: 14458
🏁 Script executed:
python3 - <<'PY' from pathlib import Path p = Path('framework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.cpp') text = p.read_text() for needle in [ 'newSequence()', 'newSequenceInNativeFormat()', 'checkNewSequenceForConflicts()', 'trySave()', 'm_originSequence', ]: print(f'=== {needle} ===') for i, line in enumerate(text.splitlines(), 1): if needle in line: for j in range(max(1, i-4), min(len(text.splitlines()), i+12)+1): print(f'{j}: {text.splitlines()[j-1]}') break PYRepository: musescore/muse_framework
Length of output: 2929
🏁 Script executed:
python3 - <<'PY' from pathlib import Path p = Path('framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp') text = p.read_text().splitlines() for i, line in enumerate(text, 1): if 'sequencesToNativeText' in line or 'shortcutToObject' in line or 'sequence' in line and 'NativeText' in line: start = max(1, i-6) end = min(len(text), i+12) print(f'--- around line {i} ---') for j in range(start, end+1): print(f'{j}: {text[j-1]}') PYRepository: musescore/muse_framework
Length of output: 4353
🌐 Web query:
Qt QKeySequence toString default format NativeText PortableText documentation💡 Result:
In the Qt framework, the QKeySequence::toString function is used to convert a key sequence into a human-readable string [1][2]. The function signature is: QString toString(QKeySequence::SequenceFormat format = PortableText) const [1][3] The format parameter is an enum of type QKeySequence::SequenceFormat, which accepts two primary values: 1. PortableText: This is the default format [1][4]. It is designed to be platform-independent, using standard key names (e.g., "Ctrl", "Shift") that remain consistent across different operating systems [4]. It is recommended for serializing key sequences for storage (e.g., in configuration files) to ensure they can be reliably loaded on any platform [4]. 2. NativeText: This format uses native key names and symbols specific to the current operating system [4]. For example, on macOS, it may use platform-specific symbols like the Command key (⌘) icon rather than text like "Ctrl" or "Cmd" [2][4]. This format is typically used for displaying shortcuts to the user in elements like menus or tooltips to ensure the application feels integrated into the user's OS [2][4]. Summary: Use PortableText when you need a consistent, portable string representation for storage or cross-platform data exchange, and use NativeText when you need to display the shortcut in a manner that matches the user's native environment [4].
Citations:
- 1: https://doc.qt.io/qt-6.11/qkeysequence.html
- 2: https://doc.qt.io/qt-6.5/qkeysequence.html
- 3: https://doc.qt.io/qt-6.10/qkeysequence.html
- 4: https://runebook.dev/en/docs/qt/qkeysequence/SequenceFormat-enum
Use native-text for these comparisons.
checkNewSequenceForConflicts()andtrySave()comparenewSequence()(PortableText) against values stored as NativeText, so the duplicate-shortcut check and the no-change guard can diverge on platforms like macOS. Switch both paths tonewSequenceInNativeFormat().🤖 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/editshortcutmodel.cpp` around lines 183 - 200, Update checkNewSequenceForConflicts() and trySave() to use newSequenceInNativeFormat() whenever comparing the edited sequence with stored shortcut sequences. Preserve the existing conflict detection and no-change guard logic while ensuring both comparisons use native-text representations consistently.framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm (4)
89-111: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail file="framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm" echo "== line count ==" wc -l "$file" echo echo "== relevant excerpt ==" sed -n '80,130p' "$file" echo echo "== call sites of keyboardLayout ==" rg -n "keyboardLayout\(" "$file" echo echo "== translateToCurrentKeyboardLayout excerpt ==" sed -n '280,320p' "$file" echo echo "== constructor / locale change excerpt ==" sed -n '1,80p' "$file"Repository: musescore/muse_framework
Length of output: 5832
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '89,111p' framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mmRepository: musescore/muse_framework
Length of output: 1225
keyboardLayout()leaks theTISInputSourceRefreturned byTISCopyCurrentKeyboardLayoutInputSource(). It’s aCopyAPI, butcurrentKeyboardis never released, so every shortcut translation leaks another input-source reference. Keep the source/data owned for the translation scope or change the helper to return an owning object instead of a rawUCKeyboardLayout*.🤖 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/platform/macos/macosshortcutsinstancemodel.mm` around lines 89 - 111, Update keyboardLayout() to release the TISInputSourceRef returned by TISCopyCurrentKeyboardLayoutInputSource() after obtaining the Unicode layout data, while keeping that data valid for the translation scope. Prefer retaining the source/data ownership through the caller’s translation operation or change keyboardLayout() to return an owning representation instead of a raw UCKeyboardLayout*.
250-286: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -e file="framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm" # Show the relevant area around keyCodeToString and keyboardLayout. nl -ba "$file" | sed -n '80,140p;240,310p' # Find all references to LMGetKbdType and related Carbon pointer resolution. rg -n "LMGetKbdType|CFBundleGetFunctionPointerForName|keyboardLayout\(" "$file"Repository: musescore/muse_framework
Length of output: 750
🏁 Script executed:
set -e file="framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm" # Read the relevant ranges with line numbers using awk (since nl is unavailable). awk 'NR>=85 && NR<=115 { printf "%4d %s\n", NR, $0 } NR>=248 && NR<=300 { printf "%4d %s\n", NR, $0 }' "$file"Repository: musescore/muse_framework
Length of output: 3649
Guard
LMGetKbdType()before use.
keyCodeToString()can still crash here if Carbon fails to resolve the symbol. Add the same null-check used inkeyboardLayout()before calling it.🤖 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/platform/macos/macosshortcutsinstancemodel.mm` around lines 250 - 286, Update keyCodeToString so the resolved LMGetKbdType function pointer is checked for null before invocation, matching the guard already used in keyboardLayout. Return the existing empty-string fallback when the symbol cannot be resolved, and preserve the current UCKeyTranslate flow otherwise.
288-311: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail file='framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm' echo '--- file outline ---' ast-grep outline "$file" --view expanded || true echo '--- relevant lines 250-340 ---' sed -n '250,340p' "$file" | cat -n echo '--- doActivate vicinity 400-430 ---' sed -n '400,430p' "$file" | cat -n echo '--- search translateToCurrentKeyboardLayout references ---' rg -n "translateToCurrentKeyboardLayout|m_macSequenceMap|QKeySequence::fromString|keyCodeToString|specialKeysMap|sequence\[0\]" "$file"Repository: musescore/muse_framework
Length of output: 6336
🏁 Script executed:
#!/bin/bash set -euo pipefail file='framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm' echo '--- specialKeysMap and mapping loop 1-120, 360-390 ---' sed -n '1,120p' "$file" | cat -n echo '---' sed -n '360,390p' "$file" | cat -nRepository: musescore/muse_framework
Length of output: 6983
🏁 Script executed:
#!/bin/bash set -euo pipefail file='framework/shortcuts_v2/qml/Muse/Shortcuts/platform/macos/macosshortcutsinstancemodel.mm' sed -n '388,420p' "$file" | cat -nRepository: musescore/muse_framework
Length of output: 1802
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' # Probe general Qt semantics from available local knowledge? not possible. print("Need output from repository read only inspection only.") PYRepository: musescore/muse_framework
Length of output: 217
🌐 Web query:
Qt QKeySequence operator[] empty sequence count assert index < 4 semantics💡 Result:
In Qt, the QKeySequence::operator[] provides read-only access to the individual key combinations (represented as QKeyCombination objects in Qt 6 and later, or integers in earlier versions) that comprise the sequence [1][2]. A QKeySequence can store up to four such elements [2]. Regarding the semantics of accessing an index: 1. Valid Range: The index parameter must be in the range [3] [2][4]. 2. Assertions: Historically and in standard practice, the implementation uses Q_ASSERT (or similar diagnostic mechanisms in debug builds) to enforce the index < 4 constraint [2][4]. Accessing an index outside this range results in undefined behavior, which often manifests as a crash or memory corruption in optimized/release builds where assertions may be disabled [2]. 3. Empty Sequences: Calling operator[] on an empty QKeySequence (where count returns 0) does not fundamentally change the index requirement [1][5]. If you access index 0 on an empty sequence, you are still accessing an invalid position, and the same bounds-checking logic (or lack thereof in release builds) applies. It is considered best practice to check the sequence's size using the count method before attempting to access elements via operator[] to ensure the index is within the valid range [1][2]. For robust application logic, consider using higher-level methods like matches when comparing sequences, as direct indexing is generally intended for serialization or low-level inspection of the sequence contents [2].
Citations:
- 1: https://doc.qt.io/qt-6/qkeysequence.html
- 2: https://runebook.dev/en/docs/qt/qkeysequence/operator-5b-5d
- 3: https://codebrowser.dev/qt6/qtbase/src/gui/kernel/qkeysequence.h.html
- 4: https://dreamswork.github.io/qt4/classQKeySequence.html
- 5: https://doc.qt.io/qt-6.8/qkeysequence.html
Guard both
QKeySequence::operator[]accesses heresequence[0]andQKeySequence::fromString(...)[0]both assume a non-empty sequence.keyCodeToString()can return""(forkVK_Helpor onUCKeyTranslatefailure), so the translated sequence can be empty and the second index is undefined behavior. Return{}before indexing in either case.🤖 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/platform/macos/macosshortcutsinstancemodel.mm` around lines 288 - 311, Update translateToCurrentKeyboardLayout to validate sequence is non-empty before accessing sequence[0], and validate keyStr/fromString produces a non-empty sequence before accessing its first element. Return {} for either empty case while preserving the existing keyboard-layout and key-lookup handling.
376-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Undocumented magic condition mixing bit-masking and a literal key comparison.
Line 386's
!(untranslatedSequence[0].key() & 0xff) || untranslatedSequence[0].key() == Qt::Key_Ahas no explanation, unlike the rest ofdoLoadShortcuts()which is unusually well-documented (lines 325-364). It's not obvious why the low byte of the key value or specificallyKey_Adetermine whether the untranslated sequence should also be self-mapped, which makes this fragile to maintain or extend for other special keys.🤖 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/platform/macos/macosshortcutsinstancemodel.mm` around lines 376 - 406, Document the rationale for the untranslated-sequence condition in doLoadShortcuts, specifically explaining the low-byte key check and the special handling of Qt::Key_A before the recordMapping call. Keep the existing condition and behavior unchanged, and place the explanation alongside the condition using the surrounding method’s established documentation style.framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp (3)
182-216: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the target file around the reported lines sed -n '150,260p' framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp # Locate the register interfaces and implementations used by apply() rg -n "class .*ShortcutsRegister|setShortcuts\(|commandShortcutsRegister\(|shortcutsRegister\(" framework/shortcuts_v2 -S # Map candidate files for the register types fd -a ".*shortcuts.*(register|model).*" framework/shortcuts_v2Repository: musescore/muse_framework
Length of output: 8924
🏁 Script executed:
#!/bin/bash set -euo pipefail # Read the surrounding declarations for the model and the register abstractions ast-grep outline framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp --view expanded || true # Show only the files that define the register classes/interfaces rg -n "setShortcuts\\(" framework/shortcuts_v2 -g'*.h' -g'*.cpp' -g'*.qml' -SRepository: musescore/muse_framework
Length of output: 3285
🏁 Script executed:
#!/bin/bash set -euo pipefail # If available, inspect the register implementations to see whether partial writes can be rolled back for f in $(fd -a "shortcuts.*(register|model).*" framework/shortcuts_v2 2>/dev/null); do echo "===== $f =====" sed -n '1,260p' "$f" | sed -n '/setShortcuts\|rollback\|transaction\|apply()/p' doneRepository: musescore/muse_framework
Length of output: 1977
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect repository tree for the relevant implementation files git ls-files framework/shortcuts_v2 | sed -n '1,200p' # Find references to the two registers and their persistence behavior rg -n "commandShortcutsRegister|shortcutsRegister|class .*Register|setShortcuts" framework/shortcuts_v2 -SRepository: musescore/muse_framework
Length of output: 8857
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the actual persistence logic in both register implementations sed -n '285,360p' framework/shortcuts_v2/internal/commandshortcutsregister.cpp echo '---' sed -n '380,460p' framework/shortcuts_v2/internal/shortcutsregister.cppRepository: musescore/muse_framework
Length of output: 4248
Make
apply()atomic across both shortcut registers. If the secondsetShortcuts()fails, the command shortcuts stay persisted and the two stores end up out of sync; add rollback or commit both updates as one unit.🧰 Tools
🪛 Clang (14.0.6)
[warning] 182-182: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 182-182: method 'apply' can be made static
(readability-convert-member-functions-to-static)
[warning] 186-186: variable 'shortcuts' is not initialized
(cppcoreguidelines-init-variables)
[warning] 201-201: variable 'shortcuts' is not initialized
(cppcoreguidelines-init-variables)
🤖 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 182 - 216, Make ShortcutsModel::apply() update commandShortcutsRegister() and shortcutsRegister() atomically: if the second setShortcuts() fails, restore the command register’s previous shortcuts (or use an existing transaction mechanism) before returning false. Preserve the current error logging and success behavior while ensuring neither register remains partially updated.
259-286: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the relevant file and related symbols first. printf '\n== shortcutsmodel.cpp outline ==\n' ast-grep outline framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp --view expanded || true printf '\n== search for commandShortcutsRegister usages ==\n' rg -n "commandShortcutsRegister|importFromFile|exportToFile|defaultShortcut|resetToDefaultSelectedShortcuts|apply\(|reset\(" framework/shortcuts_v2 -S printf '\n== file list for shortcuts_v2 ==\n' git ls-files framework/shortcuts_v2 | sed -n '1,200p'Repository: musescore/muse_framework
Length of output: 11842
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,260p' framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp printf '\n---\n' sed -n '260,420p' framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp printf '\n---\n' rg -n "commandShortcutsRegister|defaultShortcut|importFromFile|exportToFile|apply\(|reset\(" framework/shortcuts_v2 -SRepository: musescore/muse_framework
Length of output: 18506
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '\n== commandshortcutsregister.h ==\n' sed -n '1,220p' framework/shortcuts_v2/internal/commandshortcutsregister.h printf '\n== commandshortcutsregister.cpp (relevant sections) ==\n' sed -n '1,220p' framework/shortcuts_v2/internal/commandshortcutsregister.cpp printf '\n---\n' sed -n '220,420p' framework/shortcuts_v2/internal/commandshortcutsregister.cpp printf '\n== shortcutsregister.cpp default lookup and import/export ==\n' sed -n '470,560p' framework/shortcuts_v2/internal/shortcutsregister.cppRepository: musescore/muse_framework
Length of output: 16019
Handle command shortcuts in import/export and reset-to-default
importShortcutsFromFile()/exportShortcutsToFile()only go throughshortcutsRegister(), so command shortcuts never round-trip.resetToDefaultSelectedShortcuts()also usesshortcutsRegister()->defaultShortcut(shortcut.action), which leaves command items without a default and clears their sequences instead of restoring them. Wire these paths throughcommandShortcutsRegister()as well.🧰 Tools
🪛 Clang (14.0.6)
[warning] 259-259: method 'importShortcutsFromFile' can be made static
(readability-convert-member-functions-to-static)
[warning] 271-271: method 'exportShortcutsToFile' can be made static
(readability-convert-member-functions-to-static)
🤖 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 259 - 286, Update ShortcutsModel::importShortcutsFromFile(), exportShortcutsToFile(), and resetToDefaultSelectedShortcuts() to process command shortcuts through commandShortcutsRegister() alongside shortcutsRegister(). Ensure command shortcut entries are included in import/export round-trips and reset using the command register’s defaults instead of being cleared.
373-382: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Shortcut identity objects lack a unique key.
shortcutToObjectexposes only title/sequence/context/autoRepeat.EditShortcutModel(ineditshortcutmodel.cpp) uses full-map equality on these objects to detect "is this the origin shortcut" and to resolvem_allShortcuts.indexOf(m_conflictShortcut)for clearing the conflicting entry. Two distinct shortcuts sharing these four fields are indistinguishable, risking the wrong entry being treated as self or cleared.🛠️ Suggested fix: add a stable identifier
QVariant ShortcutsModel::shortcutToObject(const Item& item) const { QVariantMap obj; + obj["actionCode"] = QString::fromStdString(item.shortcut.action); + obj["command"] = QString::fromStdString(item.shortcut.command); obj["title"] = item.title; obj["sequence"] = item.sequence; obj["context"] = QString::fromStdString(item.shortcut.context); obj["autoRepeat"] = item.shortcut.autoRepeat; return obj; }Then compare on that identifier in
EditShortcutModel::load()instead of full-map equality.📝 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.QVariant ShortcutsModel::shortcutToObject(const Item& item) const { QVariantMap obj; obj["actionCode"] = QString::fromStdString(item.shortcut.action); obj["command"] = QString::fromStdString(item.shortcut.command); obj["title"] = item.title; obj["sequence"] = item.sequence; obj["context"] = QString::fromStdString(item.shortcut.context); obj["autoRepeat"] = item.shortcut.autoRepeat; return obj; }🧰 Tools
🪛 Clang (14.0.6)
[warning] 373-373: use a trailing return type for this function
(modernize-use-trailing-return-type)
[warning] 375-375: variable 'obj' is not initialized
(cppcoreguidelines-init-variables)
🤖 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 373 - 382, Update ShortcutsModel::shortcutToObject to include a stable unique identifier for each shortcut, then update EditShortcutModel::load to compare and resolve shortcuts using that identifier rather than full QVariantMap equality, ensuring distinct shortcuts with identical title, sequence, context, and autoRepeat remain distinguishable.framework/shortcuts_v2/shortcutstypes.h (2)
74-84: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,180p' framework/shortcuts_v2/shortcutstypes.h printf '\n--- usages ---\n' rg -n "sequencesToString|sequencesFromString" -S .Repository: musescore/muse_framework
Length of output: 5951
🌐 Web query:
Qt QKeySequence string format comma space multi-key chord listToString NativeText documentation💡 Result:
In Qt, QKeySequence supports multi-key chords (sequences of up to four key codes) represented as a string by separating individual key codes with commas [1][2]. When using the toString method, these sequences are joined by a comma followed by a space (e.g., "Alt+X, Ctrl+Y, Z") [2]. The static method QKeySequence::listToString is used to convert a QList of QKeySequence objects into a single string representation based on a specified SequenceFormat [3][4]. Regarding the SequenceFormat enum: 1. NativeText (value 0): Represents the key sequence as a platform-specific string [5][1]. This format is translated and, on macOS, is designed to resemble the key sequences shown in system menu bars [5][1]. It is recommended for displaying shortcuts to the user [5][1]. 2. PortableText (value 1): Represents the key sequence in a "portable" format suitable for reading from and writing to files [5][1]. In many environments, such as Windows and X11, this format often appears similar to the native text [5][1]. For parsing strings back into sequences, QKeySequence provides the fromString method [3][6], and for parsing lists of sequences, QKeySequence::listFromString is available [3][6]. The strings recognized by these methods include standard modifier names like "Ctrl", "Shift", "Alt", and "Meta", along with their translated equivalents [1][2].
Citations:
- 1: https://github.com/qt/qtbase/blob/9db7cc79a26ced4997277b5c206ca15949133240/src/gui/kernel/qkeysequence.cpp
- 2: https://doc.qt.io/archives/qt-6.4/qkeysequence.html
- 3: https://doc.qt.io/qt-6.11/qkeysequence.html
- 4: https://doc.qt.io/qt-6.8/qkeysequence.html
- 5: https://doc.qt.io/qt-6.5/qkeysequence.html
- 6: https://doc.qt.io/qt-6.10/qkeysequence.html
🏁 Script executed:
sed -n '1,140p' framework/shortcuts/shortcutstypes.h printf '\n--- v2 editshortcutmodel ---\n' sed -n '170,220p' framework/shortcuts_v2/qml/Muse/Shortcuts/editshortcutmodel.cpp printf '\n--- v2 shortcutsmodel ---\n' sed -n '270,320p' framework/shortcuts_v2/qml/Muse/Shortcuts/shortcutsmodel.cpp printf '\n--- legacy editshortcutmodel ---\n' sed -n '180,220p' framework/shortcuts/qml/Muse/Shortcuts/editshortcutmodel.cpp printf '\n--- legacy shortcutsmodel ---\n' sed -n '210,240p' framework/shortcuts/qml/Muse/Shortcuts/shortcutsmodel.cppRepository: musescore/muse_framework
Length of output: 8980
🏁 Script executed:
python3 - <<'PY' samples = [ "Ctrl+K, Ctrl+S", "Ctrl+Alt+K", "Ctrl+K, Ctrl+S; Alt+X", ] for s in samples: print(s, "->", s.split(", ")) PYRepository: musescore/muse_framework
Length of output: 283
🏁 Script executed:
python3 - <<'PY' samples = [ "Ctrl+K, Ctrl+S", "Ctrl+Alt+K", "Ctrl+K, Ctrl+S; Alt+X", ] for s in samples: print(repr(s), "->", s.split(", ")) PYRepository: musescore/muse_framework
Length of output: 289
Use a delimiter that doesn’t conflict with
QKeySequencetext", "breaks round-tripping for multi-stroke shortcuts likeCtrl+K, Ctrl+S, because loading will split a single chord into separate entries.Proposed fix
static std::string sequencesToString(const std::vector<std::string>& seqs) { - return muse::strings::join(seqs, ", "); + return muse::strings::join(seqs, "; "); } static std::vector<std::string> sequencesFromString(const std::string& str) { std::vector<std::string> seqs; - muse::strings::split(str, seqs, ", "); + muse::strings::split(str, seqs, "; "); return seqs; }📝 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.static std::string sequencesToString(const std::vector<std::string>& seqs) { return muse::strings::join(seqs, "; "); } static std::vector<std::string> sequencesFromString(const std::string& str) { std::vector<std::string> seqs; muse::strings::split(str, seqs, "; "); return seqs; }🤖 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/shortcutstypes.h` around lines 74 - 84, Update sequencesToString and sequencesFromString to use a serialization delimiter that cannot occur in QKeySequence text, preserving each multi-stroke shortcut as one sequence during round-tripping. Apply the same delimiter consistently for joining and splitting, without changing the surrounding conversion behavior.
95-110: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Use a
switchstatement instead ofstd::setfor performance.Creating and searching a
static const std::setrequires heap allocations upon initialization and tree traversal on lookup. SinceQt::Keyis an enum, aswitchstatement evaluates inO(1)time, is more cache-friendly, and completely avoids the static initialization overhead on the hot path.⚡ Proposed refactor
inline bool needIgnoreKey(Qt::Key key) { - static const std::set<Qt::Key> ignoredKeys { - Qt::Key_Shift, - Qt::Key_Control, - Qt::Key_Meta, - Qt::Key_Alt, - Qt::Key_AltGr, - Qt::Key_CapsLock, - Qt::Key_NumLock, - Qt::Key_ScrollLock, - Qt::Key_unknown - }; - - return ignoredKeys.find(key) != ignoredKeys.end(); + switch (key) { + case Qt::Key_Shift: + case Qt::Key_Control: + case Qt::Key_Meta: + case Qt::Key_Alt: + case Qt::Key_AltGr: + case Qt::Key_CapsLock: + case Qt::Key_NumLock: + case Qt::Key_ScrollLock: + case Qt::Key_unknown: + return true; + default: + return false; + } }📝 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.inline bool needIgnoreKey(Qt::Key key) { switch (key) { case Qt::Key_Shift: case Qt::Key_Control: case Qt::Key_Meta: case Qt::Key_Alt: case Qt::Key_AltGr: case Qt::Key_CapsLock: case Qt::Key_NumLock: case Qt::Key_ScrollLock: case Qt::Key_unknown: return true; default: return false; } }🤖 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/shortcutstypes.h` around lines 95 - 110, Replace the static std::set lookup in needIgnoreKey with a switch over the ignored Qt::Key values, returning true for each listed key and false by default. Preserve the existing ignored-key list and function behavior while removing the set allocation and lookup.framework/shortcuts/internal/shortcutscontroller.cpp (1)
72-74: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid asserting on unknown shortcut sequences.
Using
IF_ASSERT_FAILEDelevates an empty sequence match to an assertion failure. Sinceactivatecan potentially be invoked externally (e.g., via the script API or edge cases in QML input handling), passing an unknown sequence will now trigger an assert in debug builds.Consider restoring a standard
ifcheck and a warning log if runtime invocation with arbitrary strings is expected.🛡️ Proposed fix
- IF_ASSERT_FAILED(!shortcutsForSequence.empty()) { + if (shortcutsForSequence.empty()) { + LOGW() << "No shortcuts found for sequence" << sequence; return ActionCode(); }📝 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.if (shortcutsForSequence.empty()) { LOGW() << "No shortcuts found for sequence" << sequence; return ActionCode(); }🧰 Tools
🪛 Clang (14.0.6)
[warning] 73-73: avoid repeating the return type from the declaration; use a braced initializer list instead
(modernize-return-braced-init-list)
🤖 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/internal/shortcutscontroller.cpp` around lines 72 - 74, Replace the IF_ASSERT_FAILED guard in the shortcut activation flow with a standard emptiness check for shortcutsForSequence, returning ActionCode() without asserting when the sequence is unknown. Add a warning log if consistent with the surrounding logging conventions, while preserving normal activation for matched sequences.
2 participants
No description provided.