Refactor Implode/Explode and Exchange voices#32850
Refactor Implode/Explode and Exchange voices#32850cbjeukendrup wants to merge 5 commits intomusescore:masterfrom
Conversation
All the things that it does in its `redo` method are already undoable by themselves
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThe changes refactor voice editing operations from Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 9
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9dcabea7-5c1e-444f-84ac-4c20edbbf09c
📒 Files selected for processing (19)
src/braille/internal/braille.cppsrc/engraving/CMakeLists.txtsrc/engraving/dom/measure.cppsrc/engraving/dom/measure.hsrc/engraving/dom/score.hsrc/engraving/editing/clonevoice.cppsrc/engraving/editing/clonevoice.hsrc/engraving/editing/cmd.cppsrc/engraving/editing/edit.cppsrc/engraving/editing/editvoicing.cppsrc/engraving/editing/editvoicing.hsrc/engraving/editing/exchangevoices.cppsrc/engraving/editing/exchangevoices.hsrc/engraving/editing/implodeexplode.cppsrc/engraving/editing/implodeexplode.hsrc/engraving/editing/undo.hsrc/engraving/tests/exchangevoices_tests.cppsrc/engraving/tests/implodeexplode_tests.cppsrc/notation/internal/notationinteraction.cpp
💤 Files with no reviewable changes (8)
- src/engraving/dom/measure.h
- src/engraving/editing/undo.h
- src/engraving/dom/measure.cpp
- src/engraving/editing/cmd.cpp
- src/engraving/editing/editvoicing.h
- src/engraving/dom/score.h
- src/engraving/editing/editvoicing.cpp
- src/engraving/editing/edit.cpp
| if (oe && !oe->generated() && oe->isChordRest()) { | ||
| ChordRest* ocr = toChordRest(oe); | ||
| ChordRest* ncr = toChordRest(maybeLinkedClone(ocr)); | ||
| ncr->setScore(destScore); | ||
| ncr->setTrack(dstTrack); | ||
|
|
||
| //Don't clone gaps to a first voice | ||
| if (!(ncr->track() % VOICES) && ncr->isRest()) { | ||
| toRest(ncr)->setGap(false); | ||
| } | ||
|
|
||
| //Handle beams | ||
| if (ocr->beam() && !ocr->beam()->empty() && ocr->beam()->elements().front() == ocr) { | ||
| Beam* nb = ocr->beam()->clone(); | ||
| nb->clear(); | ||
| nb->setTrack(dstTrack); | ||
| nb->setScore(destScore); | ||
| nb->add(ncr); | ||
| ncr->setBeam(nb); | ||
| } | ||
|
|
||
| // clone Tuplets | ||
| Tuplet* ot = ocr->tuplet(); | ||
| if (ot) { | ||
| ot->setTrack(srcTrack); | ||
| Tuplet* nt = tupletMap.findNew(ot); | ||
| if (nt == 0) { | ||
| nt = toTuplet(maybeLinkedClone(ot)); | ||
| nt->setTrack(dstTrack); | ||
| nt->setParent(dm); | ||
| tupletMap.add(ot, nt); | ||
|
|
||
| Tuplet* nt1 = nt; | ||
| while (ot->tuplet()) { | ||
| Tuplet* nt2 = tupletMap.findNew(ot->tuplet()); | ||
| if (nt2 == 0) { | ||
| nt2 = toTuplet(maybeLinkedClone(ot->tuplet())); | ||
| nt2->setTrack(dstTrack); | ||
| nt2->setParent(dm); | ||
| tupletMap.add(ot->tuplet(), nt2); | ||
| } | ||
| nt2->add(nt1); | ||
| nt1->setTuplet(nt2); | ||
| ot = ot->tuplet(); | ||
| nt1 = nt2; | ||
| } | ||
| } | ||
| nt->add(ncr); | ||
| ncr->setTuplet(nt); | ||
| } | ||
|
|
||
| // clone additional settings | ||
| if (oe->isRest()) { | ||
| Rest* ore = toRest(ocr); | ||
| // If we would clone a full measure rest just don't clone this rest | ||
| if (ore->isFullMeasureRest() && (dstTrack % VOICES)) { | ||
| continue; |
There was a problem hiding this comment.
Skip full-measure rests before cloning.
The continue on Line 126 runs after maybeLinkedClone(ocr), so every skipped whole-measure rest in a secondary voice leaves an unattached clone behind. Move that guard ahead of the clone, or delete ncr before continuing.
Suggested fix
if (oe && !oe->generated() && oe->isChordRest()) {
ChordRest* ocr = toChordRest(oe);
+ if (ocr->isRest() && toRest(ocr)->isFullMeasureRest() && (dstTrack % VOICES)) {
+ continue;
+ }
+
ChordRest* ncr = toChordRest(maybeLinkedClone(ocr));
ncr->setScore(destScore);
ncr->setTrack(dstTrack);
@@
- // clone additional settings
- if (oe->isRest()) {
- Rest* ore = toRest(ocr);
- // If we would clone a full measure rest just don't clone this rest
- if (ore->isFullMeasureRest() && (dstTrack % VOICES)) {
- continue;
- }
- }📝 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.
| if (oe && !oe->generated() && oe->isChordRest()) { | |
| ChordRest* ocr = toChordRest(oe); | |
| ChordRest* ncr = toChordRest(maybeLinkedClone(ocr)); | |
| ncr->setScore(destScore); | |
| ncr->setTrack(dstTrack); | |
| //Don't clone gaps to a first voice | |
| if (!(ncr->track() % VOICES) && ncr->isRest()) { | |
| toRest(ncr)->setGap(false); | |
| } | |
| //Handle beams | |
| if (ocr->beam() && !ocr->beam()->empty() && ocr->beam()->elements().front() == ocr) { | |
| Beam* nb = ocr->beam()->clone(); | |
| nb->clear(); | |
| nb->setTrack(dstTrack); | |
| nb->setScore(destScore); | |
| nb->add(ncr); | |
| ncr->setBeam(nb); | |
| } | |
| // clone Tuplets | |
| Tuplet* ot = ocr->tuplet(); | |
| if (ot) { | |
| ot->setTrack(srcTrack); | |
| Tuplet* nt = tupletMap.findNew(ot); | |
| if (nt == 0) { | |
| nt = toTuplet(maybeLinkedClone(ot)); | |
| nt->setTrack(dstTrack); | |
| nt->setParent(dm); | |
| tupletMap.add(ot, nt); | |
| Tuplet* nt1 = nt; | |
| while (ot->tuplet()) { | |
| Tuplet* nt2 = tupletMap.findNew(ot->tuplet()); | |
| if (nt2 == 0) { | |
| nt2 = toTuplet(maybeLinkedClone(ot->tuplet())); | |
| nt2->setTrack(dstTrack); | |
| nt2->setParent(dm); | |
| tupletMap.add(ot->tuplet(), nt2); | |
| } | |
| nt2->add(nt1); | |
| nt1->setTuplet(nt2); | |
| ot = ot->tuplet(); | |
| nt1 = nt2; | |
| } | |
| } | |
| nt->add(ncr); | |
| ncr->setTuplet(nt); | |
| } | |
| // clone additional settings | |
| if (oe->isRest()) { | |
| Rest* ore = toRest(ocr); | |
| // If we would clone a full measure rest just don't clone this rest | |
| if (ore->isFullMeasureRest() && (dstTrack % VOICES)) { | |
| continue; | |
| if (oe && !oe->generated() && oe->isChordRest()) { | |
| ChordRest* ocr = toChordRest(oe); | |
| if (ocr->isRest() && toRest(ocr)->isFullMeasureRest() && (dstTrack % VOICES)) { | |
| continue; | |
| } | |
| ChordRest* ncr = toChordRest(maybeLinkedClone(ocr)); | |
| ncr->setScore(destScore); | |
| ncr->setTrack(dstTrack); | |
| //Don't clone gaps to a first voice | |
| if (!(ncr->track() % VOICES) && ncr->isRest()) { | |
| toRest(ncr)->setGap(false); | |
| } | |
| //Handle beams | |
| if (ocr->beam() && !ocr->beam()->empty() && ocr->beam()->elements().front() == ocr) { | |
| Beam* nb = ocr->beam()->clone(); | |
| nb->clear(); | |
| nb->setTrack(dstTrack); | |
| nb->setScore(destScore); | |
| nb->add(ncr); | |
| ncr->setBeam(nb); | |
| } | |
| // clone Tuplets | |
| Tuplet* ot = ocr->tuplet(); | |
| if (ot) { | |
| ot->setTrack(srcTrack); | |
| Tuplet* nt = tupletMap.findNew(ot); | |
| if (nt == 0) { | |
| nt = toTuplet(maybeLinkedClone(ot)); | |
| nt->setTrack(dstTrack); | |
| nt->setParent(dm); | |
| tupletMap.add(ot, nt); | |
| Tuplet* nt1 = nt; | |
| while (ot->tuplet()) { | |
| Tuplet* nt2 = tupletMap.findNew(ot->tuplet()); | |
| if (nt2 == 0) { | |
| nt2 = toTuplet(maybeLinkedClone(ot->tuplet())); | |
| nt2->setTrack(dstTrack); | |
| nt2->setParent(dm); | |
| tupletMap.add(ot->tuplet(), nt2); | |
| } | |
| nt2->add(nt1); | |
| nt1->setTuplet(nt2); | |
| ot = ot->tuplet(); | |
| nt1 = nt2; | |
| } | |
| } | |
| nt->add(ncr); | |
| ncr->setTuplet(nt); | |
| } |
| for (Spanner* oldSp : on->spannerBack()) { | ||
| Note* newStart = Spanner::startElementFromSpanner(oldSp, nn); | ||
| if (newStart) { | ||
| Spanner* newSp = toSpanner(maybeLinkedClone(oldSp)); | ||
| newSp->setNoteSpan(newStart, nn); | ||
| destScore->doUndoAddElement(newSp); |
There was a problem hiding this comment.
Respect link == false when recreating spanners.
The unlinked path is broken in two ways: cr1->links()/cr2->links() only finds anchors for linkedClone(), and both spanner insertion sites still call doUndoAddElement() unconditionally. In implode/explode that can leave slurs, hairpins, and HPO spanners orphaned in the current score instead of recreating them on the cloned destination content across linked staves. Track the cloned anchors explicitly and use undoAddElement() when link is false.
Also applies to: 277-314
| // Find and add corresponding slurs and hairpins | ||
| static const std::set<ElementType> SPANNERS_TO_COPY { ElementType::SLUR, ElementType::HAMMER_ON_PULL_OFF, ElementType::HAIRPIN }; | ||
| auto spanners = sourceScore->spannerMap().findOverlapping(start.ticks(), lTick.ticks()); |
There was a problem hiding this comment.
deleteSource should clean up the same spanner types that were copied.
doCloneVoice() duplicates SLUR, HAMMER_ON_PULL_OFF, and HAIRPIN, but the source cleanup only removes slurs. With deleteSource == true, the move leaves the original hairpins/HPOs behind on the source track, so the edit is no longer move-only.
Also applies to: 343-349
| if (score->excerpt()) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C8 'ExchangeVoices::exchangeVoicesInSelection\s*\(' srcRepository: musescore/MuseScore
Length of output: 10399
🏁 Script executed:
# Find the NotationInteraction class definition and score() method
rg -n 'class NotationInteraction' srcRepository: musescore/MuseScore
Length of output: 474
🏁 Script executed:
# Look at the score() method in NotationInteraction
fd -e 'h' -e 'cpp' | xargs rg -l 'NotationInteraction' | head -5Repository: musescore/MuseScore
Length of output: 245
🏁 Script executed:
# Check the context around the call at line 6143
sed -n '6100,6150p' src/notation/internal/notationinteraction.cppRepository: musescore/MuseScore
Length of output: 1686
🏁 Script executed:
# Check test fixture to understand what score is being used
head -80 src/engraving/tests/exchangevoices_tests.cppRepository: musescore/MuseScore
Length of output: 2545
🏁 Script executed:
# Look at NotationInteraction header to find the score() method
sed -n '60,200p' src/notation/internal/notationinteraction.hRepository: musescore/MuseScore
Length of output: 7072
🏁 Script executed:
# Search for score() method definition in NotationInteraction
rg -n 'Score\*\s+score\(\)' src/notation/internal/notationinteraction.hRepository: musescore/MuseScore
Length of output: 107
🏁 Script executed:
# Check if there's a base class definition in Notation or INotationInteraction
rg -A 5 'Score\*\s+score\(\)' src/notationRepository: musescore/MuseScore
Length of output: 6565
🏁 Script executed:
# Find the implementation of NotationInteraction::score()
rg -A 10 'Score\* NotationInteraction::score\(\)' src/notationRepository: musescore/MuseScore
Length of output: 749
🏁 Script executed:
# Check the Notation class to understand what score it holds
rg -n 'class Notation' src/notation/internal/notation.h | head -5Repository: musescore/MuseScore
Length of output: 320
🏁 Script executed:
# Look at Notation class definition to see how score is stored
sed -n '1,150p' src/notation/internal/notation.hRepository: musescore/MuseScore
Length of output: 4027
🏁 Script executed:
# Look for how Notation objects are created, particularly for excerpts/parts
rg -n 'new Notation' src/notationRepository: musescore/MuseScore
Length of output: 283
🏁 Script executed:
# Search for MasterNotation and how it handles excerpts/parts
rg -n 'class MasterNotation' srcRepository: musescore/MuseScore
Length of output: 454
🏁 Script executed:
# Check if there's a way to get Notation objects for excerpts
rg -n 'excerpt' src/notation/internal/notation.h -iRepository: musescore/MuseScore
Length of output: 45
🏁 Script executed:
# Search for how parts/excerpts are handled in the notation layer
rg -n 'Notation.*excerpt' src/notation -iRepository: musescore/MuseScore
Length of output: 11918
🏁 Script executed:
# Look at ExcerptNotation reinit and initialization
sed -n '30,80p' src/notation/internal/excerptnotation.cppRepository: musescore/MuseScore
Length of output: 1081
🏁 Script executed:
# Check Notation constructor implementation
rg -A 15 'Notation::Notation' src/notation/internal/notation.cpp | head -40Repository: musescore/MuseScore
Length of output: 1043
🏁 Script executed:
# Search for where swapVoices is called to understand user interaction context
rg -B 5 -A 5 'swapVoices' srcRepository: musescore/MuseScore
Length of output: 6097
🏁 Script executed:
# Check if there's any validation in swapVoices to prevent calling on excerpts
sed -n '6130,6150p' src/notation/internal/notationinteraction.cppRepository: musescore/MuseScore
Length of output: 574
🏁 Script executed:
# Look for any guards in the UI action that might prevent calling on excerpts
rg -B 10 'voice-x12' src/notationscene/internal/notationactioncontroller.cpp | head -30Repository: musescore/MuseScore
Length of output: 750
🏁 Script executed:
# Check if there's any selection filtering based on excerpt state
rg -n 'excerpt' src/notation/internal/notationinteraction.cppRepository: musescore/MuseScore
Length of output: 45
Excerpt scores silently bypass voice exchange with no error.
Line 115 returns before voice exchange can occur if an excerpt score is passed. Users can invoke swapVoices action on a part notation, which calls this function with an excerpt score, resulting in a silent no-op with no error message. Either add an excerpt guard at the NotationInteraction level or report an error when an excerpt score reaches this function.
| for (track_idx_t srcTrack2 : srcTrackList) { | ||
| // don't care about other linked staves | ||
| if (!(staffTrack <= srcTrack2) || !(srcTrack2 < staffTrack + VOICES)) { | ||
| continue; | ||
| } | ||
|
|
||
| track_idx_t tempTrack = srcTrack; | ||
| std::vector<track_idx_t> testTracks = muse::values(tracks, tempTrack + trackDiff); | ||
| bool hasVoice = false; | ||
| for (track_idx_t testTrack : testTracks) { | ||
| if (staffTrack <= testTrack && testTrack < staffTrack + VOICES && muse::contains(dstTrackList, testTrack)) { | ||
| hasVoice = true; | ||
| // voice is simply exchangeable now (deal directly) | ||
| score->undo(new ExchangeVoicesInMeasure(measure2, srcTrack2, testTrack, staffTrack / 4)); | ||
| } | ||
| } | ||
|
|
||
| // only source voice is in this staff | ||
| if (!hasVoice) { | ||
| CloneVoice::cloneVoice(measure->first(), measure2->endTick(), measure2->first(), tempTrack, srcTrack2, false, true); | ||
| muse::remove(srcTrackList, srcTrack2); | ||
| } |
There was a problem hiding this comment.
Don’t remove from these vectors while range-iterating them.
Lines 175 and 197 mutate srcTrackList / dstTrackList inside range-for loops. That invalidates the iterators backing the loops and can skip or duplicate excerpt remaps nondeterministically.
🐛 Proposed fix
- for (track_idx_t srcTrack2 : srcTrackList) {
+ for (auto it = srcTrackList.begin(); it != srcTrackList.end(); ) {
+ track_idx_t srcTrack2 = *it;
// don't care about other linked staves
if (!(staffTrack <= srcTrack2) || !(srcTrack2 < staffTrack + VOICES)) {
- continue;
+ ++it;
+ continue;
}
track_idx_t tempTrack = srcTrack;
std::vector<track_idx_t> testTracks = muse::values(tracks, tempTrack + trackDiff);
bool hasVoice = false;
@@
// only source voice is in this staff
if (!hasVoice) {
CloneVoice::cloneVoice(measure->first(), measure2->endTick(), measure2->first(), tempTrack, srcTrack2, false, true);
- muse::remove(srcTrackList, srcTrack2);
+ it = srcTrackList.erase(it);
+ continue;
}
+ ++it;
}
- for (track_idx_t dstTrack2 : dstTrackList) {
+ for (auto it = dstTrackList.begin(); it != dstTrackList.end(); ) {
+ track_idx_t dstTrack2 = *it;
// don't care about other linked staves
if (!(staffTrack <= dstTrack2) || !(dstTrack2 < staffTrack + VOICES)) {
- continue;
+ ++it;
+ continue;
}
track_idx_t tempTrack = dstTrack;
std::vector<track_idx_t> testTracks = muse::values(tracks, tempTrack - trackDiff);
bool hasVoice = false;
@@
// only destination voice is in this staff
if (!hasVoice) {
CloneVoice::cloneVoice(measure->first(), measure2->endTick(), measure2->first(), tempTrack, dstTrack2, false, true);
- muse::remove(dstTrackList, dstTrack2);
+ it = dstTrackList.erase(it);
+ continue;
}
+ ++it;
}Also applies to: 179-197
| score->deselectAll(); | ||
| score->select(startMeasure, SelectType::RANGE, srcStaff); | ||
| score->select(endMeasure, SelectType::RANGE, srcStaff); | ||
| startSegment = score->selection().startSegment(); | ||
| endSegment = score->selection().endSegment(); | ||
| if (srcStaff == lastStaff - 1) { | ||
| // only one staff was selected up front - determine number of staves | ||
| // loop through all chords looking for maximum number of notes | ||
| int n = 0; | ||
| for (Segment* s = startSegment; s && s != endSegment; s = s->next1()) { | ||
| EngravingItem* e = s->element(srcTrack); | ||
| if (e && e->isChord()) { | ||
| Chord* c = toChord(e); | ||
| n = std::max(n, int(c->notes().size())); | ||
| for (Chord* graceChord : c->graceNotes()) { | ||
| n = std::max(n, int(graceChord->notes().size())); | ||
| } | ||
| } | ||
| } | ||
| lastStaff = std::min(score->nstaves(), srcStaff + n); | ||
| } | ||
|
|
||
| // Check that all source and dest measures have the same time stretch - allows explode within a local time signature, | ||
| // but don't yet support it between differing local time signatures. | ||
| Fraction timeStretch(1, 0); | ||
| for (Measure* m = startMeasure; m && m->tick() <= endMeasure->tick(); m = m->nextMeasure()) { | ||
| for (size_t staffIdx = srcStaff; staffIdx < lastStaff; ++staffIdx) { | ||
| Fraction mTimeStretch = score->staff(staffIdx)->timeStretch(m->tick()); | ||
| if (!timeStretch.isValid()) { | ||
| timeStretch = mTimeStretch; | ||
| } else if (timeStretch != mTimeStretch) { | ||
| MScore::setError(MsError::CANNOT_EXPLODE_IMPLODE_LOCAL_TIMESIG); | ||
| return false; |
There was a problem hiding this comment.
Guard the no-note path before lastStaff becomes invalid.
This branch narrows the selection to the top staff before the local-time-signature check, so a failed explode leaves the user's original selection changed even though the edit is rolled back. It also lets n stay 0 on rest-only input, which makes lastStaff == srcStaff and the final lastStaff - 1 reselection underflow.
| sTracks[full] = i; | ||
|
|
||
| for (size_t j = srcTrack + full * VOICES; j < lastStaff * VOICES; j++) { | ||
| if (i == j) { | ||
| dTracks[full] = j; | ||
| break; | ||
| } | ||
| for (Measure* m = seg->measure(); m && m->tick() < lTick; m = m->nextMeasure()) { | ||
| if (!m->hasVoice(j) || (m->hasVoice(j) && m->isOnlyRests(j))) { | ||
| dTracks[full] = j; | ||
| } else { | ||
| dTracks[full] = muse::nidx; | ||
| break; | ||
| } | ||
| } | ||
| if (dTracks[full] != muse::nidx) { | ||
| break; | ||
| } | ||
| } | ||
| full++; | ||
| } | ||
| } | ||
|
|
||
| IF_ASSERT_FAILED(full > 0) { | ||
| return false; | ||
| } | ||
| lastStaff = track2staff(dTracks[full - 1]) + 1; | ||
|
|
||
| // Check that all source and dest measures have the same time stretch - allows explode within a local time signature, | ||
| // but don't yet support it between differing local time signatures. | ||
| Fraction timeStretch(1, 0); | ||
| for (Measure* m = startMeasure; m && m->tick() <= endMeasure->tick(); m = m->nextMeasure()) { | ||
| for (size_t staffIdx = srcStaff; staffIdx < lastStaff; ++staffIdx) { | ||
| Fraction mTimeStretch = score->staff(staffIdx)->timeStretch(m->tick()); | ||
| if (!timeStretch.isValid()) { | ||
| timeStretch = mTimeStretch; | ||
| } else if (timeStretch != mTimeStretch) { | ||
| MScore::setError(MsError::CANNOT_EXPLODE_IMPLODE_LOCAL_TIMESIG); | ||
| return false; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| for (track_idx_t i = srcTrack, j = 0; i < lastStaff * VOICES && j < VOICES; i += VOICES, j++) { | ||
| track_idx_t strack = sTracks[j % VOICES]; | ||
| track_idx_t dtrack = dTracks[j % VOICES]; | ||
| if (strack != muse::nidx && strack != dtrack && dtrack != muse::nidx) { | ||
| CloneVoice::cloneVoice(startSegment, lTick, startSegment, strack, dtrack, true, false); |
There was a problem hiding this comment.
Only advance a voice slot after you've found a destination track for the whole clone range.
The vacancy probe starts at seg->measure(), even though the later clone starts at startSegment, so a destination track with earlier material in the selected range can still be picked. And if no candidate is found, full is still incremented, which leaves dTracks[full - 1] == muse::nidx when lastStaff is derived.
| Segment* startSegment = score->selection().startSegment(); | ||
| Segment* endSegment = score->selection().endSegment(); | ||
| Measure* startMeasure = startSegment->measure(); | ||
| Measure* endMeasure = endSegment ? endSegment->measure() : score->lastMeasure(); | ||
| Fraction startTick = startSegment->tick(); | ||
| Fraction endTick = endSegment ? endSegment->tick() : score->lastMeasure()->endTick(); |
There was a problem hiding this comment.
Treat measure-boundary end segments as exclusive here too.
When endSegment is the first segment of the next measure, this code includes that next measure in endMeasure. The mutation loop stops at endSegment, but the time-stretch validation, source-track discovery, and final reselection still operate one measure too far.
Patch sketch
- Measure* endMeasure = endSegment ? endSegment->measure() : score->lastMeasure();
+ Measure* endMeasure = nullptr;
+ if (!endSegment) {
+ endMeasure = score->lastMeasure();
+ } else if (endSegment->tick() == endSegment->measure()->tick()) {
+ endMeasure = endSegment->measure()->prevMeasure()
+ ? endSegment->measure()->prevMeasure()
+ : score->firstMeasure();
+ } else {
+ endMeasure = endSegment->measure();
+ }📝 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.
| Segment* startSegment = score->selection().startSegment(); | |
| Segment* endSegment = score->selection().endSegment(); | |
| Measure* startMeasure = startSegment->measure(); | |
| Measure* endMeasure = endSegment ? endSegment->measure() : score->lastMeasure(); | |
| Fraction startTick = startSegment->tick(); | |
| Fraction endTick = endSegment ? endSegment->tick() : score->lastMeasure()->endTick(); | |
| Segment* startSegment = score->selection().startSegment(); | |
| Segment* endSegment = score->selection().endSegment(); | |
| Measure* startMeasure = startSegment->measure(); | |
| Measure* endMeasure = nullptr; | |
| if (!endSegment) { | |
| endMeasure = score->lastMeasure(); | |
| } else if (endSegment->tick() == endSegment->measure()->tick()) { | |
| endMeasure = endSegment->measure()->prevMeasure() | |
| ? endSegment->measure()->prevMeasure() | |
| : score->firstMeasure(); | |
| } else { | |
| endMeasure = endSegment->measure(); | |
| } | |
| Fraction startTick = startSegment->tick(); | |
| Fraction endTick = endSegment ? endSegment->tick() : score->lastMeasure()->endTick(); |
| score->startCmd(TranslatableString::untranslatable("Implode/explode tests")); | ||
| score->cmdExplode(); | ||
| ImplodeExplode::explode(score); | ||
| score->endCmd(); |
There was a problem hiding this comment.
Mirror the production rollback path in these tests.
Lines 62 and 93 ignore the new bool return and still commit the surrounding command. ImplodeExplode::{explode, implode} now signal validation failures this way, and src/notation/internal/notationinteraction.cpp:6451-6481 rolls back instead of applying when they return false. These tests should do the same, otherwise they can exercise a state the app would never keep.
Also applies to: 92-94
|
All those "review comments" are in code that was moved, not new code, so they can better be fixed by others in next PRs. |
1 participant
See the commit messages for details.
There should be no changes in behaviour, except that some undo bugs might be resolved.
Summary by CodeRabbit
Release Notes