transform notation actions to commands#34150
Conversation
📝 WalkthroughWalkthroughThe change adds articulation, voice-selection, and flip commands, along with controller queries for rest, articulation, and voice state. Command registration and state evaluation now use these queries to update availability. Note input UI configuration references the new command identifiers, while 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Public OSS repositories can only analyze public repositories installed in this organization. No linked repositories were analyzed; skipped 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: 2
🧹 Nitpick comments (2)
src/notationscene/internal/notationcommandsstate.cpp (1)
273-279: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer removing commented-out logging over keeping it.
Consider dropping the commented
LOGDA()block rather than leaving it as dead code; if per-command tracing is still useful, guarding it behind a trace level would be cleaner.🤖 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 `@src/notationscene/internal/notationcommandsstate.cpp` around lines 273 - 279, Remove the commented-out LOGDA() block from NotationCommandsState::commandState; leave the method returning the result of doCommandState(command), or implement active trace-level logging if per-command diagnostics are still required.src/notationscene/internal/notationactioncontroller.cpp (1)
1110-1112: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePrefer
std::setmember lookup overstd::find.
std::findperforms a linear O(n) scan oncurrentNoteArticulations. Using the set's membercount()orfind()would be O(log n). Impact is negligible for small articulation sets, but it's the idiomatic approach for ordered associative containers.♻️ Proposed refactor
for (auto it = result.begin(); it != result.end();) { - if (std::find(currentNoteArticulations.begin(), currentNoteArticulations.end(), - *it) == currentNoteArticulations.end()) { + if (currentNoteArticulations.count(*it) == 0) { it = result.erase(it); } else { ++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 `@src/notationscene/internal/notationactioncontroller.cpp` around lines 1110 - 1112, Replace the linear std::find check in the result iteration within the notation action controller with currentNoteArticulations.count(*it) or currentNoteArticulations.find(*it), preserving the existing condition and behavior while using the set’s logarithmic member lookup.
🤖 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 `@src/notationscene/internal/notationactioncontroller.cpp`:
- Around line 1018-1050: currentIsRest() incorrectly returns true for an empty
non-range selection. After obtaining the selection elements in currentIsRest(),
add an early return false when the collection is empty, before iterating and
checking isRest(), matching the guard used by currentVoice().
In `@src/notationscene/internal/notationcommandsstate.cpp`:
- Around line 146-151: Add ADD_ARTICULATION_COMMANDS to the selectionChanged()
notification handler alongside the existing updateCommandStates calls, so
articulation command states refresh whenever the selection changes.
---
Nitpick comments:
In `@src/notationscene/internal/notationactioncontroller.cpp`:
- Around line 1110-1112: Replace the linear std::find check in the result
iteration within the notation action controller with
currentNoteArticulations.count(*it) or currentNoteArticulations.find(*it),
preserving the existing condition and behavior while using the set’s logarithmic
member lookup.
In `@src/notationscene/internal/notationcommandsstate.cpp`:
- Around line 273-279: Remove the commented-out LOGDA() block from
NotationCommandsState::commandState; leave the method returning the result of
doCommandState(command), or implement active trace-level logging if per-command
diagnostics are still required.
🪄 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: CHILL
Plan: Pro
Run ID: 9af83cc4-7030-41e0-abeb-84f8922e8d9d
📒 Files selected for processing (10)
src/notationscene/inotationcommandscontroller.hsrc/notationscene/internal/notationactioncontroller.cppsrc/notationscene/internal/notationactioncontroller.hsrc/notationscene/internal/notationcommandsregister.cppsrc/notationscene/internal/notationcommandsstate.cppsrc/notationscene/internal/notationuiactions.cppsrc/notationscene/internal/notationuiactions.hsrc/notationscene/notationcommands.hsrc/notationscene/qml/MuseScore/NotationScene/noteinputbarmodel.cppsrc/notationscene/qml/MuseScore/NotationScene/noteinputbarmodel.h
💤 Files with no reviewable changes (1)
- src/notationscene/internal/notationuiactions.h
| bool NotationActionController::currentIsRest() const | ||
| { | ||
| INotationInteractionPtr interaction = currentNotationInteraction(); | ||
| if (!interaction) { | ||
| return false; | ||
| } | ||
|
|
||
| INotationNoteInputPtr noteInput = interaction->noteInput(); | ||
| if (!noteInput) { | ||
| return false; | ||
| } | ||
|
|
||
| if (noteInput->isNoteInputMode()) { | ||
| return noteInput->state().rest(); | ||
| } | ||
|
|
||
| INotationSelectionPtr selection = interaction->selection(); | ||
| if (!selection) { | ||
| return false; | ||
| } | ||
|
|
||
| if (selection->isNone() || selection->isRange()) { | ||
| return false; | ||
| } | ||
|
|
||
| for (const EngravingItem* element: selection->elements()) { | ||
| if (!element->isRest()) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Guard against empty selection in currentIsRest().
If selection->isNone() is false and selection->isRange() is false but selection->elements() is empty, the loop body never executes and the function returns true (vacuous truth). currentVoice() at Line 1152 explicitly guards this case with selectedElements.empty(), but currentIsRest() does not. Add an early return for consistency and correctness.
🛡️ Proposed fix
if (selection->isNone() || selection->isRange()) {
return false;
}
+ if (selection->elements().empty()) {
+ return false;
+ }
+
for (const EngravingItem* element: selection->elements()) {📝 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.
| bool NotationActionController::currentIsRest() const | |
| { | |
| INotationInteractionPtr interaction = currentNotationInteraction(); | |
| if (!interaction) { | |
| return false; | |
| } | |
| INotationNoteInputPtr noteInput = interaction->noteInput(); | |
| if (!noteInput) { | |
| return false; | |
| } | |
| if (noteInput->isNoteInputMode()) { | |
| return noteInput->state().rest(); | |
| } | |
| INotationSelectionPtr selection = interaction->selection(); | |
| if (!selection) { | |
| return false; | |
| } | |
| if (selection->isNone() || selection->isRange()) { | |
| return false; | |
| } | |
| for (const EngravingItem* element: selection->elements()) { | |
| if (!element->isRest()) { | |
| return false; | |
| } | |
| } | |
| return true; | |
| } | |
| bool NotationActionController::currentIsRest() const | |
| { | |
| INotationInteractionPtr interaction = currentNotationInteraction(); | |
| if (!interaction) { | |
| return false; | |
| } | |
| INotationNoteInputPtr noteInput = interaction->noteInput(); | |
| if (!noteInput) { | |
| return false; | |
| } | |
| if (noteInput->isNoteInputMode()) { | |
| return noteInput->state().rest(); | |
| } | |
| INotationSelectionPtr selection = interaction->selection(); | |
| if (!selection) { | |
| return false; | |
| } | |
| if (selection->isNone() || selection->isRange()) { | |
| return false; | |
| } | |
| if (selection->elements().empty()) { | |
| return false; | |
| } | |
| for (const EngravingItem* element: selection->elements()) { | |
| if (!element->isRest()) { | |
| return false; | |
| } | |
| } | |
| return 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 `@src/notationscene/internal/notationactioncontroller.cpp` around lines 1018 -
1050, currentIsRest() incorrectly returns true for an empty non-range selection.
After obtaining the selection elements in currentIsRest(), add an early return
false when the collection is empty, before iterating and checking isRest(),
matching the guard used by currentVoice().
| controller()->selectionChanged().onNotify(this, [this]() { | ||
| updateCommandStates(HAS_SELECTION_REQUIRED_COMMANDS); | ||
| updateCommandStates(ADD_COMMANDS); | ||
| updateCommandStates({ PAD_REST_COMMAND }); | ||
| updateCommandStates(commands(VOICE_COMMANDS)); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file around the reported lines
sed -n '1,340p' src/notationscene/internal/notationcommandsstate.cpp
printf '\n--- SEARCH: ADD_ARTICULATION_COMMANDS ---\n'
rg -n "ADD_ARTICULATION_COMMANDS|HAS_SELECTION_REQUIRED_COMMANDS|PAD_REST_COMMAND|VOICE_COMMANDS|currentArticulations|selectionChanged\(|noteInputStateChanged\(" src/notationscene/internal/notationcommandsstate.cpp src -g'*.cpp' -g'*.h'Repository: musescore/MuseScore
Length of output: 20531
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect articulation source and selection-change flow in the controller
sed -n '1020,1095p' src/notationscene/internal/notationactioncontroller.cpp
printf '\n---\n'
sed -n '1160,1205p' src/notationscene/internal/notationactioncontroller.cpp
printf '\n---\n'
sed -n '1948,2005p' src/notationscene/internal/notationactioncontroller.cpp
printf '\n--- SEARCH: articulation notifications/usages ---\n'
rg -n "currentArticulations\(|articulation|selectionChanged\(\)\.notify|selectionChanged\(\).onNotify|noteInputStateChanged\(\)\.notify" src/notationscene/internal/notationactioncontroller.cpp src/notationscene/internal/notationcommandsstate.cpp src/notationscene -g'*.cpp' -g'*.h'Repository: musescore/MuseScore
Length of output: 11563
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the controller's selection-change notification path around the line reported by search
sed -n '520,590p' src/notationscene/internal/notationactioncontroller.cpp
printf '\n---\n'
sed -n '1295,1355p' src/notationscene/internal/notationactioncontroller.cpp
printf '\n---\n'
sed -n '1,140p' src/notationscene/internal/notationactioncontroller.hRepository: musescore/MuseScore
Length of output: 11273
Refresh articulation command state on selection changes. selectionChanged() updates voice/rest, but not ADD_ARTICULATION_COMMANDS, so marcato/sforzato/tenuto/staccato can stay stale after a plain selection change. Add the articulation commands here too.
🤖 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 `@src/notationscene/internal/notationcommandsstate.cpp` around lines 146 - 151,
Add ADD_ARTICULATION_COMMANDS to the selectionChanged() notification handler
alongside the existing updateCommandStates calls, so articulation command states
refresh whenever the selection changes.
2 participants
No description provided.