Allow more item types and list selections to be nudged#32998
Open
miiizen wants to merge 8 commits intomusescore:masterfrom
Open
Allow more item types and list selections to be nudged#32998miiizen wants to merge 8 commits intomusescore:masterfrom
miiizen wants to merge 8 commits intomusescore:masterfrom
Conversation
TODO: Allow nudging of multiple anchors at once
📝 WalkthroughWalkthroughInterface methods 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 the current code and only fix it if needed.
Inline comments:
In `@src/engraving/dom/image.h`:
- Around line 100-101: Replace the hard-coded numeric Grip(2) usages in the
methods initialEditModeGrip() and defaultGrip() with the named enum Grip::MIDDLE
to make the intent explicit and avoid ordinal coupling; edit the return
expressions in the member functions initialEditModeGrip() const override and
defaultGrip() const override so they return Grip::MIDDLE instead of Grip(2),
matching the usage in image.cpp.
In `@src/notation/internal/notationinteraction.cpp`:
- Around line 4250-4256: The loop uses the global editor staff group
(noteInput()->state().staffGroup()) to decide whether to skip TAB rests, which
can incorrectly skip non-TAB rests; change the guard to inspect each selected
rest's own staff group instead: for each EngravingItem* selected that passes
isRest(), cast to Rest* via toRest(selected) and check that rest->staffGroup()
== mu::engraving::StaffGroup::TAB before continuing, then call
score()->cmdMoveRest(rest, toDirection(d)); preserve the existing null/isRest
checks and use the concrete rest's staffGroup() to determine the skip.
- Around line 4282-4294: The code opens an undo transaction with
startEdit("Nudge element") before validating the MoveDirection d, so if d ==
MoveDirection::Undefined the function returns without calling apply() or
rollback() and leaves a dangling transaction; fix by validating d before calling
startEdit (i.e., check if d == MoveDirection::Undefined and return early) or
ensure you call rollback() before returning when the IF_ASSERT_FAILED branch is
hit—modify the logic around startEdit / MoveDirection::Undefined to guarantee
either apply() or rollback() is always invoked (references: startEdit,
MoveDirection::Undefined, apply(), rollback()).
In `@src/notationscene/internal/notationactioncontroller.cpp`:
- Around line 1137-1152: The loop currently filters out lyrics into a separate
lyrics vector before filling nudgeable, causing lyrics to never reach
interaction->nudge(); modify the selection logic so lyrics are treated as
nudgeable for horizontal and quick moves by either adding lyrics to nudgeable
(e.g., push_back(el) when el->isLyrics()) or testing isLyrics() in the nudgeable
branch (use el->isLyrics() || shouldNudge(el) || el->hasGrips()), and ensure
interaction->nudge() will see lyrics; apply the same fix to the other analogous
selection loops that build nudgeable (the blocks around the same pattern that
create lyrics/nudgeable/notes).
🪄 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: 6f48b2aa-5481-48e0-84ba-1dbed4ffe40f
📒 Files selected for processing (9)
src/braille/internal/notationbraille.cppsrc/engraving/dom/hairpin.cppsrc/engraving/dom/image.cppsrc/engraving/dom/image.hsrc/notation/inotationinteraction.hsrc/notation/internal/notationinteraction.cppsrc/notation/internal/notationinteraction.hsrc/notation/tests/mocks/notationinteractionmock.hsrc/notationscene/internal/notationactioncontroller.cpp
Comment on lines
+1137
to
+1152
| std::vector<EngravingItem*> lyrics; | ||
| std::vector<EngravingItem*> nudgeable; | ||
| bool gripEditable = false; // TODO: Refactor grip editing to allow multiple selection and nudging (without rebasing anchors) | ||
| std::vector<EngravingItem*> notes; | ||
|
|
||
| for (EngravingItem* el : selectedElements) { | ||
| if (el->isLyrics()) { | ||
| lyrics.push_back(el); | ||
| } else if (el->hasGrips() && interaction->isGripEditStarted()) { | ||
| gripEditable = true; | ||
| } else if (shouldNudge(el) || el->hasGrips()) { | ||
| nudgeable.push_back(el); | ||
| } else if (el->isNote() || el->isRest()) { | ||
| notes.push_back(el); | ||
| } | ||
| } |
There was a problem hiding this comment.
Keep lyrics in the nudge path for horizontal and quick moves.
Because lyrics are siphoned off before nudgeable, selected lyrics never reach interaction->nudge(). That makes Left/Right arrows fall back to moveSelection() instead of nudging, and quickly == true on Up/Down becomes a no-op for lyrics. This reintroduces the lyrics regression the PR is meant to fix.
Suggested fix
for (EngravingItem* el : selectedElements) {
if (el->isLyrics()) {
lyrics.push_back(el);
+ if (direction == MoveDirection::Left || direction == MoveDirection::Right || quickly) {
+ nudgeable.push_back(el);
+ }
} else if (el->hasGrips() && interaction->isGripEditStarted()) {
gripEditable = true;
} else if (shouldNudge(el) || el->hasGrips()) {
nudgeable.push_back(el);
} else if (el->isNote() || el->isRest()) {Also applies to: 1168-1173, 1226-1237
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notationscene/internal/notationactioncontroller.cpp` around lines 1137 -
1152, The loop currently filters out lyrics into a separate lyrics vector before
filling nudgeable, causing lyrics to never reach interaction->nudge(); modify
the selection logic so lyrics are treated as nudgeable for horizontal and quick
moves by either adding lyrics to nudgeable (e.g., push_back(el) when
el->isLyrics()) or testing isLyrics() in the nudgeable branch (use
el->isLyrics() || shouldNudge(el) || el->hasGrips()), and ensure
interaction->nudge() will see lyrics; apply the same fix to the other analogous
selection loops that build nudgeable (the blocks around the same pattern that
create lyrics/nudgeable/notes).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notation/internal/notationinteraction.cpp`:
- Around line 4262-4271: The moveLyrics handler accepts any MoveDirection but
currently calls toDirection(d) which collapses non-Up values into
DirectionV::DOWN; update NotationInteraction::moveLyrics to validate/reject
unsupported directions the same way movePitch does: check the incoming
MoveDirection and only proceed for Up/Down (or explicitly handle
MoveDirection::Up/MoveDirection::Down), otherwise abort before startEdit and do
not call score()->cmdMoveLyrics(toLyrics(el), toDirection(d)); reference
toDirection, movePitch, NotationInteraction::moveLyrics, score()->cmdMoveLyrics
and MoveDirection to locate the change.
In `@src/notationscene/internal/notationactioncontroller.cpp`:
- Around line 1170-1182: The sequence that may call interaction->moveLyrics,
interaction->nudge, interaction->nudgeAnchors and interaction->movePitch must be
executed inside a single interaction-level edit transaction so all vertical
moves become one undoable action; modify the block so you begin one
edit/transaction on the interaction before calling any of those methods and
commit/close that single transaction after them (use the interaction's existing
transaction API used elsewhere in the codebase), then call playChord as before.
🪄 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: 6d3a6926-0f52-4674-bc7f-38ad44a79dc0
📒 Files selected for processing (4)
src/engraving/dom/image.hsrc/notation/internal/notationinteraction.cppsrc/notationscene/internal/notationactioncontroller.cppsrc/notationscene/qml/MuseScore/NotationScene/editpercussionshortcutmodel.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/notation/internal/notationinteraction.cpp`:
- Around line 4240-4257: movePitch currently applies rests from the passed
selectedElements but transposes notes via EditNote::upDown(score(), ...) which
ignores selectedElements and uses score()->selection().uniqueNotes(); update the
code so note transposition honors the provided element list: either (A) change
EditNote::upDown signature (and internal logic) to accept a container of
EngravingItem* or a NoteList and have it operate on that list instead of
score()->selection().uniqueNotes(), then call EditNote::upDown(score(),
MoveDirection::Up == d, mode, selectedElements) from
NotationInteraction::movePitch, or (B) keep EditNote::upDown API and revert
callers (notationactioncontroller.cpp) to pass the global selection so both
rests and notes use the same scope; pick one approach and ensure references to
EditNote::upDown, NotationInteraction::movePitch,
score()->selection().uniqueNotes(), and selectedElements are updated
consistently.
In `@src/notationscene/internal/notationactioncontroller.cpp`:
- Line 1296: The call to interaction->movePitch(direction, PitchMode::DIATONIC,
interaction->selection()->elements()) forwards the entire selection (including
lyrics, symbols, images) into movePitch; instead replicate the filtering
behavior used in move() and pass only note/rest items: obtain the selection via
interaction->selection()->elements(), filter that list to include only note and
rest element types (same predicate logic used by move()), and call
interaction->movePitch(direction, PitchMode::DIATONIC, filteredElements) so
non-pitchable items are excluded.
🪄 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: 23c0a531-15b1-4243-932b-344042fae1b2
📒 Files selected for processing (2)
src/notation/internal/notationinteraction.cppsrc/notationscene/internal/notationactioncontroller.cpp
Comment on lines
+4240
to
+4257
| void NotationInteraction::movePitch(MoveDirection d, PitchMode mode, const std::vector<EngravingItem*>& selectedElements) | ||
| { | ||
| IF_ASSERT_FAILED(MoveDirection::Up == d || MoveDirection::Down == d) { | ||
| return; | ||
| } | ||
| EngravingItem* selected = score()->selection().element(); | ||
| if (selected && selected->isRest()) { | ||
| if (noteInput()->state().staffGroup() == mu::engraving::StaffGroup::TAB) { | ||
| // The rest won't be visible - don't try to move it... | ||
| return; | ||
| } | ||
| startEdit(TranslatableString("undoableAction", "Change vertical position")); | ||
| score()->cmdMoveRest(toRest(selected), toDirection(d)); | ||
| } else { | ||
| startEdit(TranslatableString("undoableAction", "Change pitch")); | ||
| EditNote::upDown(score(), MoveDirection::Up == d, mode); | ||
| if (selectedElements.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| apply(); | ||
| for (EngravingItem* selected : selectedElements) { | ||
| if (selected && selected->isRest()) { | ||
| if (selected->staff()->isTabStaff(selected->tick())) { | ||
| continue; | ||
| } | ||
| score()->cmdMoveRest(toRest(selected), toDirection(d)); | ||
| } | ||
| } | ||
| EditNote::upDown(score(), MoveDirection::Up == d, mode); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== movePitch call sites =="
rg -n -C3 '\bmovePitch\s*\(' src
echo
echo "== EditNote::upDown declarations / definitions =="
fd -i 'editnote*' src -t f -x rg -n -C4 '\bupDown\s*\(' {}
echo
echo "== Score-selection usage inside EditNote =="
fd -i 'editnote*' src -t f -x rg -n -C3 'selection\(\)|score\(\)->selection|score->selection' {}Repository: musescore/MuseScore
Length of output: 9240
Incomplete refactor: movePitch() note path ignores the selectedElements parameter.
The function signature now requires an explicit element list, but note transposition still delegates to EditNote::upDown(score(), ...) which unconditionally uses score->selection().uniqueNotes() (line 479 of editnote.cpp). Meanwhile, rest moves honor the selectedElements list. This creates inconsistent behavior: callers at notationactioncontroller.cpp:1165 and 1186 pass a filtered notes subset, expecting both rests and notes to move within that subset, but only rests will actually move as scoped.
Either pass selectedElements to EditNote::upDown() (requiring its signature to change) or revert to using score()->selection() at all call sites.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notation/internal/notationinteraction.cpp` around lines 4240 - 4257,
movePitch currently applies rests from the passed selectedElements but
transposes notes via EditNote::upDown(score(), ...) which ignores
selectedElements and uses score()->selection().uniqueNotes(); update the code so
note transposition honors the provided element list: either (A) change
EditNote::upDown signature (and internal logic) to accept a container of
EngravingItem* or a NoteList and have it operate on that list instead of
score()->selection().uniqueNotes(), then call EditNote::upDown(score(),
MoveDirection::Up == d, mode, selectedElements) from
NotationInteraction::movePitch, or (B) keep EditNote::upDown API and revert
callers (notationactioncontroller.cpp) to pass the global selection so both
rests and notes use the same scope; pick one approach and ensure references to
EditNote::upDown, NotationInteraction::movePitch,
score()->selection().uniqueNotes(), and selectedElements are updated
consistently.
| } | ||
|
|
||
| interaction->movePitch(direction, PitchMode::DIATONIC); | ||
| interaction->movePitch(direction, PitchMode::DIATONIC, interaction->selection()->elements()); |
There was a problem hiding this comment.
Filter diatonic pitch moves to note/rest items only.
Line 1296 now forwards the full selection into interaction->movePitch(), but move() explicitly builds a note/rest-only subset before calling the same API. For mixed list selections, pitch-up/down-diatonic can now send lyrics, symbols, or images down the pitch path and regress note transposition for those cases.
Proposed fix
- interaction->movePitch(direction, PitchMode::DIATONIC, interaction->selection()->elements());
+ std::vector<EngravingItem*> notes;
+ for (EngravingItem* el : interaction->selection()->elements()) {
+ if (el && (el->isNote() || el->isRest())) {
+ notes.push_back(el);
+ }
+ }
+
+ if (notes.empty()) {
+ return;
+ }
+
+ interaction->movePitch(direction, PitchMode::DIATONIC, notes);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/notationscene/internal/notationactioncontroller.cpp` at line 1296, The
call to interaction->movePitch(direction, PitchMode::DIATONIC,
interaction->selection()->elements()) forwards the entire selection (including
lyrics, symbols, images) into movePitch; instead replicate the filtering
behavior used in move() and pass only note/rest items: obtain the selection via
interaction->selection()->elements(), filter that list to include only note and
rest element types (same predicate logic used by move()), and call
interaction->movePitch(direction, PitchMode::DIATONIC, filteredElements) so
non-pitchable items are excluded.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves: #30678
Items which can now be nudged with the arrow keys: