Allow grace notes to be attached to rests#34213
Open
manolo wants to merge 25 commits into
Open
Conversation
Contributor
|
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 (24)
🚧 Files skipped from review as they are similar to previous changes (22)
📝 WalkthroughWalkthroughGrace notes are generalized from chord-only attachments to 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engraving/dom/chord.cpp (1)
2017-2018: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMove grace note clearance to
ChordRest::removeMarkings.Now that
ChordRestassumes ownership of grace notes, clearing them duringremoveMarkingsshould be elevated toChordRest::removeMarkings(bool). Leaving it here inChord::removeMarkingsmeans that if aRestis cloned and its markings are requested to be removed (e.g., when splitting a rest over a barline or subdividing its duration), the grace notes will not be cleared and will instead be improperly duplicated onto the new rest segments.🤖 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/engraving/dom/chord.cpp` around lines 2017 - 2018, Move the grace-note deletion and clearing from Chord::removeMarkings into ChordRest::removeMarkings(bool), ensuring the base implementation clears graceNotes() for both Chord and Rest. Remove the now-redundant cleanup from Chord::removeMarkings while preserving the existing marking-removal behavior.src/engraving/editing/cmd.cpp (1)
971-977: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTransfer grace notes to replacement rests.
Currently, the logic to transfer grace notes from a replaced
ChordRest(cr) is placed inside theelseblock (which handles creating a newChord). If the replacement element is aRest(isRestis true), this block is skipped, causing any existing grace notes on the original chord or rest to be orphaned or lost during the replacement.Since the goal of this PR is to enable grace notes on rests, replacing a chord with a rest should preserve its attached grace notes. Move this transfer logic outside the
elseblock so it applies to both new chords and new rests via thencrpointer.💡 Proposed fix
chord->setStemDirection(stemDirection); chord->add(note); - if (cr) { - // transfer grace notes from the replaced chord or rest to the new chord - std::vector<Chord*> graceNotes = cr->graceNotes(); - for (Chord* grace : graceNotes) { - undoChangeParent(grace, chord, chord->staffIdx()); - } - } note->setNval(nval, tick); if (forceAccidental) { int tpc = style().styleB(Sid::concertPitch) ? nval.tpc1 : nval.tpc2; AccidentalVal alter = tpc2alter(tpc); AccidentalType at = Accidental::value2subtype(alter); Accidental* a = Factory::createAccidental(note); a->setAccidentalType(at); a->setRole(AccidentalRole::USER); note->add(a); } ncr = chord; if (i + 1 < n) { tie = Factory::createTie(this->dummy()); tie->setStartNote(note); tie->setTick(tie->startNote()->tick()); tie->setTrack(track); note->setTieFor(tie); } } + if (cr) { + // transfer grace notes from the replaced chord or rest to the new chord or rest + std::vector<Chord*> graceNotes = cr->graceNotes(); + for (Chord* grace : graceNotes) { + undoChangeParent(grace, ncr, ncr->staffIdx()); + } + } if (tuplet) { ncr->setTuplet(tuplet);🤖 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/engraving/editing/cmd.cpp` around lines 971 - 977, Move the grace-note transfer logic currently guarded by the new-chord else block to the shared replacement path after both chord and rest creation, using the replacement `ncr` pointer as the parent. Ensure grace notes from `cr` are transferred for both chord and rest replacements, while preserving the existing `undoChangeParent` behavior.
🧹 Nitpick comments (2)
src/engraving/rendering/score/chordlayout.cpp (1)
632-632: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid unnecessary vector copy.
item->graceNotesBefore()returns a reference toGraceNotesGroup(which inherits fromstd::vector<Chord*>). Assigning it tostd::vector<Chord*>by value triggers the vector's copy constructor. Useconst auto&to bind it by reference and avoid the unnecessary heap allocation.Consider applying the same optimization to
item->graceNotesAfter()at line 646.♻️ Proposed refactor
- std::vector<Chord*> graceNotesBefore = item->graceNotesBefore(); + const auto& graceNotesBefore = item->graceNotesBefore();🤖 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/engraving/rendering/score/chordlayout.cpp` at line 632, Update the local declarations for item->graceNotesBefore() and item->graceNotesAfter() in the chord layout flow to bind the returned groups by const reference using const auto&, avoiding vector copies while preserving read-only behavior.src/importexport/musicxml/internal/import/importmusicxmlpass2.cpp (1)
2773-2773: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
nullptrinstead of0for pointer initialization.For consistency with modern C++ standards and the rest of the codebase, prefer
nullptr.♻️ Proposed refactor
- ChordRest* prevChord = 0; // previous chord or rest + ChordRest* prevChord = nullptr; // previous chord or rest🤖 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/importexport/musicxml/internal/import/importmusicxmlpass2.cpp` at line 2773, Update the prevChord pointer initialization to use nullptr instead of 0, preserving its existing role as the previous chord-or-rest sentinel.
🤖 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/braille/internal/braille.cpp`:
- Line 2157: Update the condition surrounding chord->isGrace() to verify
chord->explicitParent() is non-null before calling isChordRest(), while
preserving the existing behavior for attached grace notes.
In `@src/engraving/compat/midi/compatmidirenderinternal.cpp`:
- Around line 1230-1246: Update collectRestGraces in the rest-handling block to
calculate per-grace timing offsets instead of using default CollectNoteParams.
Stagger each grace note with progressively negative graceOffsetOn values and set
graceOffsetOff to shorten its duration, matching the timing behavior established
by collectGraceBeforeChordEvents and GraceChordCtx::buildRestGraceCtxList for
both graceNotesBefore and graceNotesAfter.
In `@src/engraving/dom/chordrest.cpp`:
- Around line 583-592: In the CHORD case, validate gc->graceIndex() before
inserting into m_graceNotes. Clamp the index to m_graceNotes.size() so malformed
or stale state appends safely, then preserve the existing setFlag, insert, and
added flow.
In `@src/engraving/editing/edit.cpp`:
- Line 3783: Update the linked grace-chord handling around the destination
`ChordRest` and `pc` resolution so rest-hosted grace chords are not rejected by
the non-grace chord check. Accept a destination ChordRest, select its indexed
grace chord, and require the destination to be a Chord only for non-grace
inputs.
In `@src/engraving/editing/edittie.cpp`:
- Around line 140-145: Update the tie-target selection around searchTieNote and
the host check so a grace-to-grace tie is resolved first, including when the
current grace note’s host is a rest. Only skip the operation when no valid next
grace-note target exists; preserve the existing chord-host behavior for ties to
a main note.
In `@src/engraving/rw/read500/measureread.cpp`:
- Around line 318-323: Update the MMRest handling branch around the graceNotes
loop to clear the pending graceNotes collection after attaching all notes to the
rest, matching the normal Rest path and preventing them from carrying into the
next element.
In `@src/importexport/capella/internal/capella.cpp`:
- Around line 533-551: Update attachPendingGraceNotes in
src/importexport/capella/internal/capella.cpp (lines 533-551) and
src/importexport/ove/internal/importove.cpp (lines 1507-1521) to remove matching
grace notes from graceNotes with removeAt(ii), attach them, and retain
voice-mismatched notes without deleting or clearing them. Add
qDeleteAll(graceNotes) at the end of readCapVoice before the Pass II comment and
at the end of convertNotes to clean up any notes still unattached when each
import scope ends.
In `@src/importexport/musicxml/internal/import/importmusicxmlpass2.cpp`:
- Around line 7320-7323: The grace-chord state must be cleared whenever
addGraceChordsBefore consumes gcl. In
src/importexport/musicxml/internal/import/importmusicxmlpass2.cpp lines
7320-7323 and 7279-7286, reset gac to 0 immediately before the respective
addGraceChordsBefore call, preserving the existing grace-chord processing.
---
Outside diff comments:
In `@src/engraving/dom/chord.cpp`:
- Around line 2017-2018: Move the grace-note deletion and clearing from
Chord::removeMarkings into ChordRest::removeMarkings(bool), ensuring the base
implementation clears graceNotes() for both Chord and Rest. Remove the
now-redundant cleanup from Chord::removeMarkings while preserving the existing
marking-removal behavior.
In `@src/engraving/editing/cmd.cpp`:
- Around line 971-977: Move the grace-note transfer logic currently guarded by
the new-chord else block to the shared replacement path after both chord and
rest creation, using the replacement `ncr` pointer as the parent. Ensure grace
notes from `cr` are transferred for both chord and rest replacements, while
preserving the existing `undoChangeParent` behavior.
---
Nitpick comments:
In `@src/engraving/rendering/score/chordlayout.cpp`:
- Line 632: Update the local declarations for item->graceNotesBefore() and
item->graceNotesAfter() in the chord layout flow to bind the returned groups by
const reference using const auto&, avoiding vector copies while preserving
read-only behavior.
In `@src/importexport/musicxml/internal/import/importmusicxmlpass2.cpp`:
- Line 2773: Update the prevChord pointer initialization to use nullptr instead
of 0, preserving its existing role as the previous chord-or-rest sentinel.
🪄 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: 7a11cf03-9dd7-4ed9-aef1-370283ca2c26
📒 Files selected for processing (74)
src/braille/internal/braille.cppsrc/braille/tests/braille_tests.cppsrc/braille/tests/data/testGraceOnRest.mscxsrc/braille/tests/data/testGraceOnRest_ref.brfsrc/engraving/compat/midi/compatmidirender.cppsrc/engraving/compat/midi/compatmidirenderinternal.cppsrc/engraving/dom/chord.cppsrc/engraving/dom/chord.hsrc/engraving/dom/chordrest.cppsrc/engraving/dom/chordrest.hsrc/engraving/dom/excerpt.cppsrc/engraving/dom/rest.cppsrc/engraving/dom/score.hsrc/engraving/dom/select.cppsrc/engraving/dom/slur.cppsrc/engraving/dom/utils.cppsrc/engraving/editing/clonevoice.cppsrc/engraving/editing/cmd.cppsrc/engraving/editing/edit.cppsrc/engraving/editing/editclef.cppsrc/engraving/editing/editenharmonicspelling.cppsrc/engraving/editing/edittie.cppsrc/engraving/editing/navigation.cppsrc/engraving/editing/transpose.cppsrc/engraving/playback/playbackeventsrenderer.cppsrc/engraving/playback/playbackeventsrenderer.hsrc/engraving/playback/playbackmodel.cppsrc/engraving/playback/renderers/chordarticulationsrenderer.cppsrc/engraving/playback/renderers/chordarticulationsrenderer.hsrc/engraving/playback/renderers/gracechordcontext.cppsrc/engraving/playback/renderers/gracechordcontext.hsrc/engraving/playback/renderingcontext.hsrc/engraving/rendering/score/beamlayout.cppsrc/engraving/rendering/score/beamlayout.hsrc/engraving/rendering/score/chordlayout.cppsrc/engraving/rendering/score/chordlayout.hsrc/engraving/rendering/score/masklayout.cppsrc/engraving/rendering/score/measurelayout.cppsrc/engraving/rendering/score/modifydom.cppsrc/engraving/rendering/score/modifydom.hsrc/engraving/rendering/score/scorehorizontalviewlayout.cppsrc/engraving/rendering/score/segmentlayout.cppsrc/engraving/rendering/score/slurtielayout.cppsrc/engraving/rendering/score/systemlayout.cppsrc/engraving/rendering/score/tlayout.cppsrc/engraving/rw/read500/measureread.cppsrc/engraving/rw/read500/read500.cppsrc/engraving/rw/write/twrite.cppsrc/engraving/tests/midi/midirenderer_data/grace_after_rest.mscxsrc/engraving/tests/midi/midirenderer_tests.cppsrc/engraving/tests/note_data/grace-on-rest-ref.mscxsrc/engraving/tests/note_tests.cppsrc/engraving/tests/playback/playbackeventsrenderer_data/rest_with_grace_notes/rest_with_grace_notes.mscxsrc/engraving/tests/playback/playbackeventsrendering_tests.cppsrc/importexport/capella/internal/capella.cppsrc/importexport/mei/internal/meiexporter.cppsrc/importexport/mei/internal/meiexporter.hsrc/importexport/mei/internal/meiimporter.cppsrc/importexport/mei/internal/meiimporter.hsrc/importexport/mei/tests/data/gracenote-03.meisrc/importexport/mei/tests/data/gracenote-03.mscxsrc/importexport/mei/tests/mei_tests.cppsrc/importexport/mnx/internal/export/mnxexportsequences.cppsrc/importexport/mnx/internal/import/mnximportpartmeasures.cppsrc/importexport/musicxml/internal/export/exportmusicxml.cppsrc/importexport/musicxml/internal/import/importmusicxmlpass2.cppsrc/importexport/musicxml/internal/import/importmusicxmlpass2.hsrc/importexport/musicxml/tests/data/testGraceAfterOnRest.xmlsrc/importexport/musicxml/tests/data/testGraceBeforeRest.xmlsrc/importexport/musicxml/tests/data/testGraceBeforeRest_ref.mscxsrc/importexport/musicxml/tests/data/testGraceRestSkipped.xmlsrc/importexport/musicxml/tests/data/testGraceRestSkipped_ref.mscxsrc/importexport/musicxml/tests/musicxml_tests.cppsrc/importexport/ove/internal/importove.cpp
💤 Files with no reviewable changes (1)
- src/engraving/dom/chord.h
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/importexport/capella/internal/capella.cpp`:
- Around line 533-541: Add qDeleteAll(graceNotes) at the end of the first-pass
loop in readCapVoice, after all Capella note-object cases are processed and
before calculating endTick, so any remaining unattached grace notes are deleted.
🪄 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: 8c090080-5d9b-41f1-b873-28dc8a221078
📒 Files selected for processing (5)
src/braille/internal/braille.cppsrc/engraving/dom/chordrest.cppsrc/engraving/editing/edit.cppsrc/engraving/editing/edittie.cppsrc/importexport/capella/internal/capella.cpp
🚧 Files skipped from review as they are similar to previous changes (4)
- src/braille/internal/braille.cpp
- src/engraving/editing/edittie.cpp
- src/engraving/editing/edit.cpp
- src/engraving/dom/chordrest.cpp
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Repair the grace-note handling that the review suggestions left malformed and extend it for rest hosts: - findLinkedChord accepts a ChordRest destination and resolves the linked grace chord by index, so grace notes on a rest are cloned into parts. - cmdAddTie ties a grace-before note to the next grace note when one follows, falling back to the host main note only for chord hosts, so consecutive grace notes on a rest can be tied. - the read500 MMRest branch attaches and clears any pending grace notes, mirroring the normal rest path so they do not carry into the next element.
Rest-hosted grace notes were emitted with default timing, so they all sounded at the rest onset for the full rest length. Stagger them in sequence, mirroring the placement of GraceChordCtx::buildRestGraceCtxList: grace-before packs from the rest start, grace-after ends at the rest end, and the group is capped at half the rest. Each grace keeps its own play-event length and is shifted later as a whole so onTime stays below offTime.
Fix the handling of grace chords that are queued until their host note or rest is read: - Capella and OVE keep voice-mismatched grace chords queued instead of deleting them, and free any that remain unattached at the end of the voice/measure to avoid both data loss and leaks. - MusicXML resets the grace-after count before appending grace-before chords, so a bypassed grace-after step cannot leak a stale count into the next note.
manolo
force-pushed
the
19701-grace-notes-on-rests
branch
from
July 16, 2026 09:49
6480525 to
3831888
Compare
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
3 participants
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: #19701
MuseScore stores grace notes on
Chord, so they can only be attached to notes, never to rests. This makes it impossible to notate common figures such as a grace note (or a "grace note after") that occurs after a rest and before a barline, as in the final measures of Liza Lehmann's "You mustn't swim till you're six weeks old" cited in the issue. The only workaround so far was to attach the grace to an invisible non playing note in a higher voice.This PR moves grace note storage and the grace note API from
Chordup to the sharedChordRestbase, so a rest can host grace notes just like a note. The change is threaded through the whole pipeline: native read/write, layout, selection, navigation, editing, playback, part generation, and the MusicXML, MEI, MNX, Capella, OVE and Braille file formats.Prior art
Grace notes attached to rests or placed before barlines have long been supported in other notation software like Encore and Dorico, while Sibelius and Finale rely on workarounds. MuseScore previously lacked this capability, which limited accurate import of existing scores. Adding support aligns MuseScore with established tools and improves compatibility with other formats.
Summary of changes
ChordtoChordRest.Results
Here 2 screenshots of the xml score provided in the forum article imported in musescore before and after this change.
Before:
After:
