Engraving: simplify dummy parent and eliminate m_isParentExplicitlySet - #34698
Draft
cbjeukendrup wants to merge 12 commits into
Draft
Engraving: simplify dummy parent and eliminate m_isParentExplicitlySet#34698cbjeukendrup wants to merge 12 commits into
m_isParentExplicitlySet#34698cbjeukendrup wants to merge 12 commits into
Conversation
This was referenced Aug 25, 2026
cbjeukendrup
force-pushed
the
engraving-dummy-parent
branch
from
August 25, 2026 13:46
a7a587c to
59106a5
Compare
The dummy hung off the root item, which is only an accessibility concern, while the dummy is where every unattached object is parked - a matter for the score itself. Worse, the two kept constructing each other: RootItem's constructor created a DummyElement, whose init() created another RootItem, which created a third DummyElement that was never initialised. The score now creates and owns the dummy, and the root item is only the head of the accessibility tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It is not an element of the score in any sense; it is the parent an object has while it is not attached to anything. Moved out of compat/ into dom/ with the rest of the object model. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A bracket only renders its bracket item, and is rebuilt from it on every layout - it does not survive one, which is why Bracket::undoChangeProperty delegates to the item. So a bracket whose item is destroyed has nothing left to represent, and the score kept it alive by pointing it at a placeholder BracketItem parked on the dummy. Delete the brackets instead; the dummy no longer has to carry a stand-in bracket item. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A constructor that takes a specific parent type has no way to say "no parent yet": the dummy is not a Segment, so call sites reach for a stand-in Segment that the dummy carries for exactly that purpose. DummyParentOr<Segment> accepts either, so that not being attached to anything can be said outright. It is a best-effort compile-time narrowing, nothing more: it holds only an EngravingObject* and makes no difference at runtime. Applied to the parent types that are reached with the dummy - Segment, Chord, ChordRest, Note, Measure, MeasureBase and System - in the constructors, in Factory, and in the type-narrowing setOwnershipParent overloads. Constructors that only forward their parent to a base take EngravingObject* now, since a DummyParentOr cannot convert to EngravingItem*. That includes a few leaf classes whose Factory function is the more specific of the two (Fermata, NoteLine, InstrumentChange, GuitarBend, Tie), so the check moves to Factory for those. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The dummy carried a fake page/system/measure/segment/chord/note chain that existed only so that call sites had something of the right static type to pass to a constructor: score->dummy()->segment() meant "no segment". Now that the constructors accept the dummy, that chain can go, and the 364 call sites say score->dummy() instead. parentOrDummy() covers the places that pick between a real parent and none; readTremoloCompat() takes the score it reads into, rather than reaching it through a stand-in chord. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing is laid out in it, and the objects parked on it are not all items - staves and parts end up there too. Making it a plain EngravingObject also takes it out of the score's accessibility tree, where it used to appear as a group node listing everything unattached, one level below the root the same objects reported as their accessible root. Those objects now name the dummy's root item as their accessible parent, and it lists them back; the two trees no longer cross. RootItem says which of the two it heads instead of comparing itself against the score's. Every Factory function can now be handed the dummy, so those that take a plain EngravingItem* take a DummyParentOr<EngravingItem>. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The parent an object is constructed with was only context: it went into that parent's child list without being reported as its parent, an in-between state that ownershipParent() needed a flag of its own to describe. Now it simply is the parent, and an object that has none yet is constructed with the dummy. The call sites that were relying on the parent being context only now say so: MEI and MNX built spanners on the segment they start at, the 1.x reader built a slur on its measure and a chord on the segment it turned out not to belong to, braille built a slur on a system, and Guitar Pro built a tempo change on a segment. A spanner is not attached to anything - the score manages it - so handing one a segment made Score::addElement() try to add it there. An item embedded in another without being owned by it no longer exists, so accessibleParentItem() no longer has to look for one. A spanner segment that is not placed on a system yet says that the spanner lists it, which is what Spanner::accessibleChildren() does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Since construction no longer leaves objects half-attached, "attached" is simply "the parent is not the dummy", and the m_isParentExplicitlySet flag can go. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An object is attached to the parent it is constructed with, so telling it again right afterwards says nothing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The dummy's child list is long-lived and large: it holds every object that has no owner, which on a large score means all the beams and spanners. Laying out goldberg.mscx (1052 measures) makes 24.6k removeChild calls that walk 59M list entries between them, an average of 2405 per call, and effectively all of it is the dummy. The item being removed has almost always just been added, so it sits at the end of the list: searching from the back brings the same run down to 65k entries walked, 2.6 per call. Order is preserved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The fallback in accessibleParentItem() sent every item without a layout parent to the head of the dummy's tree, but that tree lists only the objects actually parked on the dummy. An item attached to something that does not place it - a measure taken off its system by a multi-measure rest - therefore named a parent that could not list it back: reachable from below, unreachable from above. Only unattached objects fall back to the dummy's root now. The rest name no parent, which keeps them out of the accessibility tree entirely, as they are out of view too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
cbjeukendrup
force-pushed
the
engraving-dummy-parent
branch
from
August 25, 2026 14:24
59106a5 to
109f783
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
2 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: #25519
Builds on #34606
This PR promotes the dummy parent element from a hack into a piece of anti-memory-leak infrastructure.
The first commit (disregarding the commits inherited from #34606) untangles the complicated relationship between DummyElement and RootItem; the RootItem used to own the dummy, which in turn owned another RootItem which created another DummyElement but broke the recursion by leaving that one uninitialised. Now, the dummy is owned directly by the score.
Then we rename DummyElement to DummyParent, to better reflect the purpose.
The third commit starts removing the dummy real element members from DummyParent, since "dummy real elements" is as contradictory as it sounds. We start with the BracketItem.
To unlock removing the other ones too, the fourth commit introduces a convenience type (or maybe rather an inconvenience type) for correctness, ensuring that constructors and
setOwnershipParentoverloads can take either the DummyParent or one of the specified real types.As a result, the fifth commit is able to parent objects directly to the dummy instead of a "dummy real element", removing those members of DummyParent.
The sixth commit makes the DummyParent an EngravingObject rather than EngravingItem, since that feels more correct. It does mean introducing
DummyParentOr<EngravingItem>in a number of places, but that's fine, because then we can easily track down usages of that and replace theEngravingItemwith more specific types.The seventh commit lets the EngravingObject constructor set the passed parent as the real parent unless it's the dummy. This finally removed the "has a parent but not explicitly set" state, allowing to eliminate
m_isParentExplicitlySetin the eighth commit.What then follows, is cleanups that have now become possible; most notably, removing
setOwnershipParentcalls that repeat what was just passed to the constructor.Another one worth mentioning is an empirically motivated optimisation in
EngravingObject::removeChild; it now searches from the back of the list, which in practice appears to be faster overall.Finally there is a commit that should improve consistency of the accessibility tree. However, I must say that this whole accessibility tree and how it handles items with no layoutParent (and whether it should handle such items at all) could use a closer look from someone who has an idea of how it should work and who is able to test it (accessibility is completely broken on macOS currently).
Reviewing commit-by-commit is again recommended.