Fix MMRest bugs#33837
Open
miiizen wants to merge 8 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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR extracts all multi-measure-rest (MMRest) layout logic from 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/engraving/rendering/score/mmrestlayout.cpp`:
- Around line 418-432: The validity scan for folding measures into an MMRest is
not rejecting measures that contain breaths in the middle of the measure body.
Add a check within the segment iteration loop (starting at line 418) to detect
Breath annotations that are not positioned at the end of the measure (i.e., not
at the last tick), and return false if such a mid-measure breath is found. This
ensures measures with internal breaths are not folded into an MMRest, since
changeMeasureElParents() only migrates breaths at lastMeasure->ticks(), leaving
internal breaths stranded on hidden measures.
- Around line 190-201: The code in the loop over staffIdx only calls setTicks()
on newly created MMRest elements (inside the if block where
chordRestSeg->element(track) == 0), but when an existing MMRest is reused after
reuseExistingMMRest() has changed mmrMeasure->ticks(), those existing elements
are not updated with the new duration. Fix this by moving the
setTicks(mmrMeasure->ticks()) call outside the if block so it applies to all
MMRest elements, both newly created and pre-existing, ensuring they remain in
sync with the measure's changed duration after splits or merges.
In `@src/engraving/tests/measure_tests.cpp`:
- Around line 728-841: The test function breathInPart creates a partScore via
TestUtils::createPart but never deallocates it, causing a resource leak. Add a
delete statement for partScore immediately before the existing delete score
statement at the end of the test to ensure both dynamically allocated Score
objects are properly freed.
🪄 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: d6df11e1-f003-4b9a-b9d3-42c5af1eadc7
📒 Files selected for processing (11)
src/engraving/dom/measure.cppsrc/engraving/editing/edit.cppsrc/engraving/rendering/score/measurelayout.cppsrc/engraving/rendering/score/measurelayout.hsrc/engraving/rendering/score/mmrestlayout.cppsrc/engraving/rendering/score/mmrestlayout.hsrc/engraving/rendering/score/rendering.cmakesrc/engraving/rendering/score/systemlayout.cppsrc/engraving/tests/measure_data/breath-parts.mscxsrc/engraving/tests/measure_data/mmrest-breath.mscxsrc/engraving/tests/measure_tests.cpp
Comment on lines
+190
to
+201
| Segment* chordRestSeg = mmrMeasure->undoGetSegmentR(SegmentType::ChordRest, Fraction(0, 1)); | ||
| for (size_t staffIdx = 0; staffIdx < ctx.dom().nstaves(); ++staffIdx) { | ||
| track_idx_t track = staffIdx * VOICES; | ||
| if (chordRestSeg->element(track) == 0) { | ||
| MMRest* mmr = Factory::createMMRest(chordRestSeg); | ||
| mmr->setDurationType(DurationType::V_MEASURE); | ||
| mmr->setTicks(mmrMeasure->ticks()); | ||
| mmr->setTrack(track); | ||
| mmr->setParent(chordRestSeg); | ||
| ctx.mutDom().doUndoAddElement(mmr); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Update existing MMRest elements when reusing a changed-length MMRest.
reuseExistingMMRest() can change mmrMeasure->ticks(), but this loop only calls setTicks() for newly created MMRest items. Existing per-staff MMRest elements keep the old duration, leaving the rest element out of sync with its covering measure after splits/merges.
Proposed fix
Segment* chordRestSeg = mmrMeasure->undoGetSegmentR(SegmentType::ChordRest, Fraction(0, 1));
for (size_t staffIdx = 0; staffIdx < ctx.dom().nstaves(); ++staffIdx) {
track_idx_t track = staffIdx * VOICES;
- if (chordRestSeg->element(track) == 0) {
+ if (EngravingItem* existing = chordRestSeg->element(track)) {
+ if (existing->isMMRest()) {
+ MMRest* mmr = toMMRest(existing);
+ mmr->setDurationType(DurationType::V_MEASURE);
+ mmr->setTicks(mmrMeasure->ticks());
+ }
+ } else {
MMRest* mmr = Factory::createMMRest(chordRestSeg);
mmr->setDurationType(DurationType::V_MEASURE);
mmr->setTicks(mmrMeasure->ticks());
mmr->setTrack(track);
mmr->setParent(chordRestSeg);🤖 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/mmrestlayout.cpp` around lines 190 - 201, The
code in the loop over staffIdx only calls setTicks() on newly created MMRest
elements (inside the if block where chordRestSeg->element(track) == 0), but when
an existing MMRest is reused after reuseExistingMMRest() has changed
mmrMeasure->ticks(), those existing elements are not updated with the new
duration. Fix this by moving the setTicks(mmrMeasure->ticks()) call outside the
if block so it applies to all MMRest elements, both newly created and
pre-existing, ensuring they remain in sync with the measure's changed duration
after splits or merges.
Comment on lines
+728
to
+841
| TEST_F(Engraving_MeasureTests, breathInPart) | ||
| { | ||
| auto findBreath = [](Score* score, int measureIdx) -> Breath* { | ||
| Measure* m = score->crMeasure(measureIdx); | ||
| if (!m) { | ||
| return nullptr; | ||
| } | ||
| Segment* seg = m->findSegmentR(SegmentType::Breath, m->ticks()); | ||
| return toBreath(seg ? seg->element(0) : nullptr); | ||
| }; | ||
|
|
||
| MasterScore* score = ScoreRW::readScore(MEASURE_DATA_DIR + u"breath-parts.mscx"); | ||
| EXPECT_TRUE(score); | ||
|
|
||
| Score* partScore = TestUtils::createPart(score); | ||
| EXPECT_TRUE(partScore); | ||
|
|
||
| // Add a breath to measure 2 | ||
| { | ||
| Measure* m = score->crMeasure(2); | ||
| EXPECT_TRUE(m); | ||
| ChordRest* cr = m->findChordRest(m->tick(), 0); | ||
| EXPECT_TRUE(cr); | ||
| Breath* b = Factory::createBreath(score->dummy()->segment()); | ||
| b->setSymId(SymId::breathMarkComma); | ||
| EditData dd(nullptr); | ||
| dd.dropElement = b; | ||
| score->startCmd(TranslatableString::untranslatable("Engraving measure tests")); | ||
| cr->drop(dd); | ||
| score->endCmd(); | ||
| } | ||
|
|
||
| // Check breath at measure 2 in score and part | ||
| { | ||
| Breath* scoreBreath = findBreath(score, 2); | ||
| EXPECT_TRUE(scoreBreath); | ||
| EXPECT_EQ(scoreBreath->symId(), SymId::breathMarkComma); | ||
|
|
||
| Breath* partBreath = findBreath(partScore, 2); | ||
| EXPECT_TRUE(partBreath); | ||
| EXPECT_EQ(partBreath->symId(), SymId::breathMarkComma); | ||
| } | ||
|
|
||
| // Add breath with different symbol to measure 4 | ||
| { | ||
| Measure* m = score->crMeasure(4); | ||
| EXPECT_TRUE(m); | ||
| ChordRest* cr = m->findChordRest(m->tick(), 0); | ||
| EXPECT_TRUE(cr); | ||
| Breath* b = Factory::createBreath(score->dummy()->segment()); | ||
| b->setSymId(SymId::breathMarkTick); | ||
| EditData dd(nullptr); | ||
| dd.dropElement = b; | ||
| score->startCmd(TranslatableString::untranslatable("Engraving measure tests")); | ||
| cr->drop(dd); | ||
| score->endCmd(); | ||
| } | ||
|
|
||
| { | ||
| // Check breath at measure 4 in score and part | ||
| Breath* scoreBreath = findBreath(score, 4); | ||
| EXPECT_TRUE(scoreBreath); | ||
| EXPECT_EQ(scoreBreath->symId(), SymId::breathMarkTick); | ||
|
|
||
| Breath* partBreath = findBreath(partScore, 4); | ||
| EXPECT_TRUE(partBreath); | ||
| EXPECT_EQ(partBreath->symId(), SymId::breathMarkTick); | ||
| } | ||
|
|
||
| // Delete breath at measure 2 | ||
| { | ||
| EngravingItem* b = findBreath(score, 2); | ||
| EXPECT_TRUE(b); | ||
| score->select(b); | ||
| score->startCmd(TranslatableString::untranslatable("Engraving measure tests")); | ||
| score->cmdDeleteSelection(); | ||
| score->endCmd(); | ||
| } | ||
|
|
||
| { | ||
| // Check breath at measure 4 still present in both master score and part | ||
| Breath* scoreBreath1 = findBreath(score, 2); | ||
| Breath* scoreBreath2 = findBreath(score, 4); | ||
| EXPECT_FALSE(scoreBreath1); | ||
| EXPECT_TRUE(scoreBreath2); | ||
| EXPECT_EQ(scoreBreath2->symId(), SymId::breathMarkTick); | ||
| Breath* partBreath1 = findBreath(partScore, 2); | ||
| Breath* partBreath2 = findBreath(partScore, 4); | ||
| EXPECT_FALSE(partBreath1); | ||
| EXPECT_TRUE(partBreath2); | ||
| EXPECT_EQ(partBreath2->symId(), SymId::breathMarkTick); | ||
| } | ||
|
|
||
| // Undo deletion | ||
| score->undoRedo(true, 0); | ||
|
|
||
| { | ||
| // Check both breaths present in master score and part | ||
| Breath* scoreBreath1 = findBreath(score, 2); | ||
| Breath* scoreBreath2 = findBreath(score, 4); | ||
| EXPECT_TRUE(scoreBreath1); | ||
| EXPECT_EQ(scoreBreath1->symId(), SymId::breathMarkComma); | ||
| EXPECT_TRUE(scoreBreath2); | ||
| EXPECT_EQ(scoreBreath2->symId(), SymId::breathMarkTick); | ||
| Breath* partBreath1 = findBreath(partScore, 2); | ||
| Breath* partBreath2 = findBreath(partScore, 4); | ||
| EXPECT_TRUE(partBreath1); | ||
| EXPECT_EQ(partBreath1->symId(), SymId::breathMarkComma); | ||
| EXPECT_TRUE(partBreath2); | ||
| EXPECT_EQ(partBreath2->symId(), SymId::breathMarkTick); | ||
| } | ||
|
|
||
| delete score; | ||
| } |
Contributor
There was a problem hiding this comment.
Resource leak: partScore is never deleted.
The partScore created on line 742 via TestUtils::createPart(score) is never deallocated. The test performs all assertions against both master score and partScore, then only deletes score on line 840.
🔧 Proposed fix
delete score;
+ delete partScore;
}📝 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.
Suggested change
| TEST_F(Engraving_MeasureTests, breathInPart) | |
| { | |
| auto findBreath = [](Score* score, int measureIdx) -> Breath* { | |
| Measure* m = score->crMeasure(measureIdx); | |
| if (!m) { | |
| return nullptr; | |
| } | |
| Segment* seg = m->findSegmentR(SegmentType::Breath, m->ticks()); | |
| return toBreath(seg ? seg->element(0) : nullptr); | |
| }; | |
| MasterScore* score = ScoreRW::readScore(MEASURE_DATA_DIR + u"breath-parts.mscx"); | |
| EXPECT_TRUE(score); | |
| Score* partScore = TestUtils::createPart(score); | |
| EXPECT_TRUE(partScore); | |
| // Add a breath to measure 2 | |
| { | |
| Measure* m = score->crMeasure(2); | |
| EXPECT_TRUE(m); | |
| ChordRest* cr = m->findChordRest(m->tick(), 0); | |
| EXPECT_TRUE(cr); | |
| Breath* b = Factory::createBreath(score->dummy()->segment()); | |
| b->setSymId(SymId::breathMarkComma); | |
| EditData dd(nullptr); | |
| dd.dropElement = b; | |
| score->startCmd(TranslatableString::untranslatable("Engraving measure tests")); | |
| cr->drop(dd); | |
| score->endCmd(); | |
| } | |
| // Check breath at measure 2 in score and part | |
| { | |
| Breath* scoreBreath = findBreath(score, 2); | |
| EXPECT_TRUE(scoreBreath); | |
| EXPECT_EQ(scoreBreath->symId(), SymId::breathMarkComma); | |
| Breath* partBreath = findBreath(partScore, 2); | |
| EXPECT_TRUE(partBreath); | |
| EXPECT_EQ(partBreath->symId(), SymId::breathMarkComma); | |
| } | |
| // Add breath with different symbol to measure 4 | |
| { | |
| Measure* m = score->crMeasure(4); | |
| EXPECT_TRUE(m); | |
| ChordRest* cr = m->findChordRest(m->tick(), 0); | |
| EXPECT_TRUE(cr); | |
| Breath* b = Factory::createBreath(score->dummy()->segment()); | |
| b->setSymId(SymId::breathMarkTick); | |
| EditData dd(nullptr); | |
| dd.dropElement = b; | |
| score->startCmd(TranslatableString::untranslatable("Engraving measure tests")); | |
| cr->drop(dd); | |
| score->endCmd(); | |
| } | |
| { | |
| // Check breath at measure 4 in score and part | |
| Breath* scoreBreath = findBreath(score, 4); | |
| EXPECT_TRUE(scoreBreath); | |
| EXPECT_EQ(scoreBreath->symId(), SymId::breathMarkTick); | |
| Breath* partBreath = findBreath(partScore, 4); | |
| EXPECT_TRUE(partBreath); | |
| EXPECT_EQ(partBreath->symId(), SymId::breathMarkTick); | |
| } | |
| // Delete breath at measure 2 | |
| { | |
| EngravingItem* b = findBreath(score, 2); | |
| EXPECT_TRUE(b); | |
| score->select(b); | |
| score->startCmd(TranslatableString::untranslatable("Engraving measure tests")); | |
| score->cmdDeleteSelection(); | |
| score->endCmd(); | |
| } | |
| { | |
| // Check breath at measure 4 still present in both master score and part | |
| Breath* scoreBreath1 = findBreath(score, 2); | |
| Breath* scoreBreath2 = findBreath(score, 4); | |
| EXPECT_FALSE(scoreBreath1); | |
| EXPECT_TRUE(scoreBreath2); | |
| EXPECT_EQ(scoreBreath2->symId(), SymId::breathMarkTick); | |
| Breath* partBreath1 = findBreath(partScore, 2); | |
| Breath* partBreath2 = findBreath(partScore, 4); | |
| EXPECT_FALSE(partBreath1); | |
| EXPECT_TRUE(partBreath2); | |
| EXPECT_EQ(partBreath2->symId(), SymId::breathMarkTick); | |
| } | |
| // Undo deletion | |
| score->undoRedo(true, 0); | |
| { | |
| // Check both breaths present in master score and part | |
| Breath* scoreBreath1 = findBreath(score, 2); | |
| Breath* scoreBreath2 = findBreath(score, 4); | |
| EXPECT_TRUE(scoreBreath1); | |
| EXPECT_EQ(scoreBreath1->symId(), SymId::breathMarkComma); | |
| EXPECT_TRUE(scoreBreath2); | |
| EXPECT_EQ(scoreBreath2->symId(), SymId::breathMarkTick); | |
| Breath* partBreath1 = findBreath(partScore, 2); | |
| Breath* partBreath2 = findBreath(partScore, 4); | |
| EXPECT_TRUE(partBreath1); | |
| EXPECT_EQ(partBreath1->symId(), SymId::breathMarkComma); | |
| EXPECT_TRUE(partBreath2); | |
| EXPECT_EQ(partBreath2->symId(), SymId::breathMarkTick); | |
| } | |
| delete score; | |
| } | |
| // Check both breaths present in master score and part | |
| Breath* scoreBreath1 = findBreath(score, 2); | |
| Breath* scoreBreath2 = findBreath(score, 4); | |
| EXPECT_TRUE(scoreBreath1); | |
| EXPECT_EQ(scoreBreath1->symId(), SymId::breathMarkComma); | |
| EXPECT_TRUE(scoreBreath2); | |
| EXPECT_EQ(scoreBreath2->symId(), SymId::breathMarkTick); | |
| Breath* partBreath1 = findBreath(partScore, 2); | |
| Breath* partBreath2 = findBreath(partScore, 4); | |
| EXPECT_TRUE(partBreath1); | |
| EXPECT_EQ(partBreath1->symId(), SymId::breathMarkComma); | |
| EXPECT_TRUE(partBreath2); | |
| EXPECT_EQ(partBreath2->symId(), SymId::breathMarkTick); | |
| } | |
| delete score; | |
| delete partScore; | |
| } |
🤖 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/tests/measure_tests.cpp` around lines 728 - 841, The test
function breathInPart creates a partScore via TestUtils::createPart but never
deallocates it, causing a resource leak. Add a delete statement for partScore
immediately before the existing delete score statement at the end of the test to
ensure both dynamically allocated Score objects are properly freed.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/engraving/rendering/score/mmrestlayout.cpp (1)
540-552: 💤 Low valueComment-code mismatch: breaks for all breaths, not just mid-measure.
The comment on line 540 says "mid-way into the previous measure" but the code at lines 550-552 returns
truefor any breath segment, including end-of-measure breaths. If this behavior is intentional (breaths should always split MMRests at measure boundaries), consider updating the comment to reflect that.Suggested clarification
- // Break for breaths and annotations found mid-way into the previous measure + // Break for breaths in the previous measure (breaths define measure boundaries for MMRests) for (Segment* s = prevMeasure->first(); s; s = s->next()) {🤖 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/mmrestlayout.cpp` around lines 540 - 552, The comment at line 540 in mmrestlayout.cpp states the function breaks for breaths found "mid-way into the previous measure," but the actual code at lines 550-552 returns true for any breath segment detected via isBreathType(), not just mid-measure breaths. Update the comment to accurately reflect that the code breaks MMRests whenever any breath is found in the measure, regardless of its position within that measure, or clarify if the behavior should be constrained to only mid-measure breaths by adding additional logic to the isBreathType() check.
🤖 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.
Nitpick comments:
In `@src/engraving/rendering/score/mmrestlayout.cpp`:
- Around line 540-552: The comment at line 540 in mmrestlayout.cpp states the
function breaks for breaths found "mid-way into the previous measure," but the
actual code at lines 550-552 returns true for any breath segment detected via
isBreathType(), not just mid-measure breaths. Update the comment to accurately
reflect that the code breaks MMRests whenever any breath is found in the
measure, regardless of its position within that measure, or clarify if the
behavior should be constrained to only mid-measure breaths by adding additional
logic to the isBreathType() check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ce62322f-26a0-444d-b2d3-dd06089119ce
📒 Files selected for processing (1)
src/engraving/rendering/score/mmrestlayout.cpp
Contributor
|
I tested commit 3a099e5 locally on Windows 11. The breath cases now work correctly up to the undo steps. Thank you for fixing them! Steps:
Cases that do not reproduce the issue:
2026-06-16_231421_light.mp4 |
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: #33774
Resolves: #33775
Resolves: Not being able to delete many elements when MMRests are enabled
Resolves: Any range selection made after an MMRest would select the previous MMRest
I've also moved code relevant to MMRests to its own file.