Remove dead playlistDirty methods from Score/MasterScore#33784
Merged
Conversation
cbjeukendrup
force-pushed
the
remove-playlist-dirty
branch
4 times, most recently
from
f48f9a9 to
638d3b1
Compare
cbjeukendrup
force-pushed
the
remove-playlist-dirty
branch
from
638d3b1 to
5326a3f
Compare
It was already kind of, but not completely.
This might involve small behaviour changes, but those should be fine.
They were supposed to undo some transactions that are commented out
This `undo` parameter probably meant “whether this UndoableCommand currently lives in the undo stack or in the redo stack, but that’s difficult to understand. Rename it to `wasDone`, as in, “was not undone”.
The UndoStack constructor pushes one state entry onto `m_states`, so the state entry corresponding to the reopened UndoableCommand is at one index higher than the UndoableCommand. Reported at musescore#33658 (comment) and musescore#33658 (review).
CodeRabbit was [concerned](musescore#33658 (comment)) about the fact that we never reset this flag. That’s half true: it is reset indirectly in `m_editData.clear()` in `doEndEditElement()`, which is called as soon as the drag-created hairpin is deselected; and that’s pretty soon, because just after dragging, the hairpin is deselected and a new (empty) dynamic is created. So in practice there is no reproducible bug. However, it’s a small effort to reset the flag just where a reset would be expected. Just slightly more robust.
During score load, some operations *invalidate* the repeat list, and some operations *request* it, causing it to be updated and marked as valid, even while the score has not been fully loaded yet and thus will change. Because apparently not all operations that should invalidate the repeat list during score load *do* invalidate the repeat list, it is a bit hit-or-miss whether the repeat list is up to date (either invalidated, or rightfully updated) at the time of the last `connectTies` call, depending on whether the repeat list was by chance last invalidated or validated. Apparently the previous commit changed something about that, leading to a failure in partialtie_tests.cpp. Solution: make sure `connectTies` is called after the repeat list has been certainly invalidated after score load. Do this inside score read, so that it is always done, regardless of whether it’s via ScoreRW or EngravingProject. Disclaimer: despite somewhat thorough investigation, I’m still not *completely* confident about this diagnosis of the problem, given how complex the jump points resolution is during score load (repeatedly resolving things based on incomplete apparently states). It feels like this is more complicated than it should be, and therefore so bug-prone.
Generated by Claude Opus 4.8: Measure-anchored spanners (voltas, ...) in the location-based file format are split across two connectors: a start connector that carries the spanner and a <next> anchor, and an end connector with a <prev> anchor. In ConnectorInfoReader::readAddConnector, the start branch called addSpanner(), which immediately computes the cached start/end elements. But at that point tick2/track2 are not set yet, so the end element was cached one measure too short (tick2 == tick). The end branch then set the real tick2/track2 via the direct setters, which bypass property-system invalidation, and never recomputed the end element. Nothing else recomputed it until layout. For voltas confined to a single staff this was masked, because StaffRead::readStaff calls Score::checkSpanner() per measure, which recomputes every spanner's start/end. But voltas duplicated onto a non-top staff use a cross-staff connector resolved by reconnectBrokenConnectors() only after all staves are read, so checkSpanner never runs for them again and the stale (too-short) end measure survived. As a result, RepeatList::collectRepeatListElements() read a wrong Volta::endMeasure() and produced an incorrect playback order until a layout pass recomputed the spanner end element. Compute each anchor's element exactly when that anchor is finalized: defer the end computation in the start branch (addSpanner with computeStartEnd = false, then computeStartElement), and call computeEndElement in the end branch once tick2/track2 are set. Both branches run back-to-back within a single addToScore pass, so each element is computed once, with correct values, and the previous wasted (wrong) end computation is removed. Applied identically to read400, read410, read460 and read500. (end of AI-generated message) (I wonder if “cross staff voltas” are a valid concept, but apparently this file contains one, so…)
miiizen
approved these changes
Jun 12, 2026
cbjeukendrup
force-pushed
the
remove-playlist-dirty
branch
from
5326a3f to
14efa02
Compare
mike-spa
reviewed
Jun 15, 2026
| // matching end connector is processed (below). Computing it here would cache a wrong value | ||
| // (tick2 == tick) that nothing recomputes until layout. See readAddConnector end branch. | ||
| item->score()->addSpanner(sp, /*computeStartEnd=*/ false); | ||
| sp->computeStartElement(); |
Contributor
There was a problem hiding this comment.
There is no such thing as cross-staff voltas. But the AI generated message is probably correct about the fact that tick2 is only known later, which is yet another instalment of this saga and the completely insane way that we write spanners in our files.
mike-spa
approved these changes
Jun 15, 2026
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
5 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.
Builds on #33658