feat: add select all notes in chord action#33762
Conversation
JohannJEG
force-pushed
the
feature/select-all-chord-notes
branch
from
5984409 to
bd26e5f
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds a new notation UI action "select-notes-in-chord" and its labels, declares and implements NotationActionController::selectAllNotesInChord() which finds chords referenced by selected note elements and selects all notes from those chords, registers the action handler gated on an active selection, and binds the action to Ctrl+Alt+A in standard, AZERTY, and Mac shortcut config files. 🚥 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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)src/notationscene/internal/notationactioncontroller.cppIn file included from src/notationscene/internal/notationactioncontroller.cpp:22: ... [truncated 2200 characters] ... Called from ClangFrontend__CTrans.CTrans_funct.instruction_insert_cxx_temporary_markers in file "src/clang/cTrans.ml", line 4866, characters 22-60 src/notationscene/internal/notationuiactions.cppIn file included from src/notationscene/internal/notationuiactions.cpp:22: ... [truncated 2200 characters] ... r in file "src/istd/IExn.ml" (inlined), line 13, characters 2-50 Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. 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
🤖 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`:
- Line 257: The action "notation-select-all-chord" is currently enabled by
Controller::hasSelection but only operates on notes; change the enabler to check
for a selected note instead. Replace the predicate in registerAction with a
Controller method that returns true only if the current selection contains at
least one note (e.g. &Controller::hasSelectedNote or implement
hasSelectedNote/hasNoteSelected on Controller to inspect the selection for note
elements) so registerAction("notation-select-all-chord",
&Controller::selectAllChordNotes, &Controller::hasSelectedNote) is used.
- Around line 1630-1663: The file uses std::set inside
NotationActionController::selectAllChordNotes() but doesn't include <set>,
relying on transitive includes; add a direct `#include` <set> to
src/notationscene/internal/notationactioncontroller.cpp (or the corresponding
header) alongside the other includes so std::set is declared for the function
and compilation no longer depends on transitive headers.
🪄 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: 659bc6f5-eddb-4750-b6c7-d8cbd941af31
📒 Files selected for processing (6)
src/app/configs/data/shortcuts.xmlsrc/app/configs/data/shortcuts_azerty.xmlsrc/app/configs/data/shortcuts_mac.xmlsrc/notationscene/internal/notationactioncontroller.cppsrc/notationscene/internal/notationactioncontroller.hsrc/notationscene/internal/notationuiactions.cpp
| registerAction("select-similar-staff", &Controller::selectAllSimilarElementsInStaff, &Controller::hasSelection); | ||
| registerAction("select-similar-range", &Controller::selectAllSimilarElementsInRange, &Controller::hasSelection); | ||
| registerAction("select-dialog", &Controller::openSelectionMoreOptions, &Controller::hasSelection); | ||
| registerAction("notation-select-all-chord", &Controller::selectAllChordNotes, &Controller::hasSelection); |
There was a problem hiding this comment.
Narrow the enabler to “has selected note” instead of generic selection.
The action is currently enabled for any selection, but it only acts on notes; non-note selections trigger a no-op. Gate this action on note presence to match its behavior and avoid misleading enabled state.
🤖 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` at line 257, The
action "notation-select-all-chord" is currently enabled by
Controller::hasSelection but only operates on notes; change the enabler to check
for a selected note instead. Replace the predicate in registerAction with a
Controller method that returns true only if the current selection contains at
least one note (e.g. &Controller::hasSelectedNote or implement
hasSelectedNote/hasNoteSelected on Controller to inspect the selection for note
elements) so registerAction("notation-select-all-chord",
&Controller::selectAllChordNotes, &Controller::hasSelectedNote) is used.
| void NotationActionController::selectAllChordNotes() | ||
| { | ||
| TRACEFUNC; | ||
| auto interaction = currentNotationInteraction(); | ||
| if (!interaction) { | ||
| return; | ||
| } | ||
|
|
||
| const std::vector<EngravingItem*>& selectedElements = interaction->selection()->elements(); | ||
| if (selectedElements.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| std::set<const Chord*> chords; | ||
| for (const EngravingItem* item : selectedElements) { | ||
| if (item->isNote()) { | ||
| chords.insert(toNote(item)->chord()); | ||
| } | ||
| } | ||
|
|
||
| if (chords.empty()) { | ||
| return; | ||
| } | ||
|
|
||
| std::vector<EngravingItem*> allNotes; | ||
| for (const Chord* chord : chords) { | ||
| for (Note* note : chord->notes()) { | ||
| allNotes.push_back(note); | ||
| } | ||
| } | ||
|
|
||
| interaction->clearSelection(); | ||
| interaction->select(allNotes, SelectType::ADD); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify direct std::set usage and local include presence in notationactioncontroller.cpp
fd -p src/notationscene/internal/notationactioncontroller.cpp -x sh -c '
f="$1"
echo "== $f =="
rg -n "std::set<|`#include` <set>" "$f"
' sh {}Repository: musescore/MuseScore
Length of output: 167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="src/notationscene/internal/notationactioncontroller.cpp"
echo "== includes near top =="
sed -n '1,120p' "$f" | nl -ba | sed -n '1,120p'
echo
echo "== any explicit <set> include in this file =="
rg -n "#\s*include\s*<set>|#\s*include\s*\"[^\"\n]*set[^\"\n]*\"" "$f" || true
echo
echo "== std::set usage (context) =="
rg -n "std::set<" "$f" -n -C 2Repository: musescore/MuseScore
Length of output: 148
🏁 Script executed:
#!/bin/bash
set -euo pipefail
f="src/notationscene/internal/notationactioncontroller.cpp"
echo "== first 140 lines (includes area) =="
cat -n "$f" | sed -n '1,140p'
echo
echo "== explicit std::set include in this file =="
rg -n "^\s*#\s*include\s*<set>\s*$" "$f" || true
echo
echo "== any <set> substring includes =="
rg -n "^\s*#\s*include\s*<[^>]*set[^>]*>\s*$|^\s*#\s*include\s*\"[^\"]*set[^\"]*\"\s*$" "$f" || true
echo
echo "== std::set usage (with context) =="
rg -n "std::set<" "$f" -C 2Repository: musescore/MuseScore
Length of output: 7630
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the header that is included as "notationactioncontroller.h"
hdr="$(fd -t f -i 'notationactioncontroller\.h$' src | head -n 1)"
echo "== header candidate =="
echo "$hdr"
echo
echo "== explicit <set> include in header =="
rg -n "^\s*#\s*include\s*<set>\s*$" "$hdr" || true
echo
echo "== any <...set...> include lines in header =="
rg -n "^\s*#\s*include\s*<[^>]*set[^>]*>\s*$" "$hdr" || trueRepository: musescore/MuseScore
Length of output: 227
Add missing direct #include <set> for std::set
selectAllChordNotes() uses std::set, but neither src/notationscene/internal/notationactioncontroller.cpp nor src/notationscene/internal/notationactioncontroller.h includes <set>, so the build currently relies on transitive includes.
Suggested patch
`#include` "io/file.h"
+#include <set>
`#include` "engraving/dom/masterscore.h"
`#include` "engraving/dom/note.h"
`#include` "engraving/dom/chord.h"🤖 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 1630 -
1663, The file uses std::set inside
NotationActionController::selectAllChordNotes() but doesn't include <set>,
relying on transitive includes; add a direct `#include` <set> to
src/notationscene/internal/notationactioncontroller.cpp (or the corresponding
header) alongside the other includes so std::set is declared for the function
and compilation no longer depends on transitive headers.
mathesoncalum
left a comment
mathesoncalum
left a comment
There was a problem hiding this comment.
Take a look at NotationViewInputController::mouseDoubleClickEvent - this method already has some logic for selecting an entire chord. I think we should avoid repeating this logic in multiple places if possible.
I reckon it'll just be a case of carefully checking the logic we already have, ensuring that the new NotationActionController::selectAllChordNotes doesn't miss any cases covered by the original method, and then simply calling dispatcher()->dispatch("notation-select-all-chord"... from mouseDoubleClickEvent.
Also I'm not crazy about notation-select-all-chord as a name - IMO that could be confused for "selecting all chords". Maybe select-notes-in-chord or something like that.
Cheers!
notation-select-all-chord -> select-notes-in-chord selectAllChordNotes -> selectAllNotesInChord Reorganize shortcut entry under select-* actions
|
Thanks for the feedback. I agree that Regarding the Due to those behavioral differences, I wasn't sure a direct replacement through I do think there may be value in refactoring If you think the dispatch-based approach is still preferable, I'm happy to take another look. |
|
Ah that is an interesting point - I just assumed that deselection would be part-and-parcel of this, but maybe not. @avvvvve what do you reckon? For example if I have an entire chord selected already and I press |
3 participants
Resolves: #33670
Summary
Implements a new action to expand the current note selection to all notes within their chord(s).
When one or more notes are selected:
Shortcut