musescore / musescore/MuseScore

Eliminate `m_isParentExplicitlySet`

Open
#25,519 1 comment 0 reactions 3 assignees View on GitHub

Nobody has claimed this yet.

P2 tech debt
Dominant language
C++
Stars
15.1k
Forks
3.3k
Avg merge
2d 2h
Merged PRs (30d)
91

Description

This was introduced in https://github.com/musescore/MuseScore/pull/8996 as an intermediate solution, but has not yet been removed since then.

In the old situation before that PR, not every EngravingItem (fka Element) had a parent. Unless set via an explicit call to setParent, elements had nullptr as their parent.

This caused some problems:

  • all parent/child relationships and ownerships were quite "ad hoc", there was no generality
  • because of this, there were many problems during destruction, namely memory leaks and use-after-free crashes, because every relationship required specialised handling, and the order of destruction mattered a lot
  • it was possible that an element's score pointed to a different Score than the element's parent's score. This may sound like something that wouldn't happen in practice, but during the generation of part scores from the full score, it is quite easy to make mistakes with this kind of things.

https://github.com/musescore/MuseScore/pull/8996 solves this problem, by generalising the parent/child relationship. Basically, every EngravingObject (fka ScoreElement) now has a parent. The parent has to be specified to the item's constructor. The ownership model is simple now: when the parent is deleted, it deletes its children, and when a child is deleted, it removes its parent's reference to it from its parent.

However, the problem is that the codebase wasn't, and still isn't, ready for this simple model:

  • many layout- and interaction logic depended on whether an element had a parent or not (i.e. nullptr) . But now, every element has a parent, changing the behaviour of that code.
  • not every alive element is currently part of a score hierarchy. For example, if an element is removed, it is semantically no longer part of the score tree, but continues to live inside the UndoStack. (And it still needs to reference its former parent, because otherwise it can't be put back on undo that's being taken care of by the UndoCommand.)

These problems, especially the former, are mitigated by the m_isParentExplicitlySet member. It basically simulates the old situation. By default it is false, so that explicitParent and parentItem return nullptr. Only after an explicit call to setParent, these methods return an actual parent.
The latter problem is solved using DummyItem: when an item is removed from the score tree, or has not yet been placed into the score view, its parent is set to the dummy item.

The m_isParentExplicitlySet member has caused confusion from time to time, and intuitively it seems like it should not be necessary. We should eliminate it, but that is quite a bit of work: for all code that depends on explicitParent or parentItem, we need to check whether it really matters whether the parent is explicitly set, or that it just needs the parent, no matter whether it's explicit or not (or that at that place the parent will always be explicit anyway so no check is necessary). In places where it does matter, a better solution needs to be found. For example, for the Palettes, in most cases we already introduced a check for score()->isPaletteScore() instead of calling explicitParent(). Similar should be done in other cases.

Maybe the DummyItem should disappear too; the fact that it was placed in a compat folder suggests that this was originally the idea. Probably, when an item is semantically not part of the score tree because it has been removed, its parent should be set to nullptr.

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.