Refactor and cleanup score tree functions #30899
Conversation
mike-spa
force-pushed
the
cleanupScanParentAndScanChildren
branch
from
bd9297a to
84175c0
Compare
| --level; | ||
| } | ||
|
|
||
| void EngravingElementsProvider::dumpTreeTree(const mu::engraving::EngravingObject* obj, int& level) |
There was a problem hiding this comment.
Let's either delete these methods or fix them
There was a problem hiding this comment.
checkTree and dumpTree above also look like they're unused
|
|
||
| if (r) { | ||
| assert(mode != Asyncable::Mode::SetOnce && "callback is already set"); | ||
| //assert(mode != Asyncable::Mode::SetOnce && "callback is already set"); |
There was a problem hiding this comment.
This should perhaps not have been committed
There was a problem hiding this comment.
Oops, yes it shouldn't have. But I will point out that I was getting a ton of failures from it (including when runnign the vtest scripts locally)
|
|
||
| std::vector<EngravingItem*> NotationElements::filterElements(const FilterElementsOptions* elementsOptions) const | ||
| { | ||
| ElementPattern* pattern = constructElementPattern(elementsOptions); |
There was a problem hiding this comment.
Perhaps it now also doesn't have to be a pointer anymore (that's leaked at the end of the function...)
| } | ||
|
|
||
| return children; | ||
| EngravingItem::scanElements(func); |
There was a problem hiding this comment.
Maybe StaffTextBase instead of EngravingItem, just to prevent surprises in the future
| //--------------------------------------------------------- | ||
|
|
||
| void Score::scanElementsInRange(void* data, void (* func)(void*, EngravingItem*), bool all) | ||
| void Score::scanElementsInRange(std::function<void(EngravingItem*)> func, bool includeInvisible) |
There was a problem hiding this comment.
Warning about includeInvisible being unused
| #include "mscore.h" | ||
| #include "score.h" | ||
| #include "staff.h" | ||
| #include "stafflines.h" |
| } | ||
|
|
||
| static void findTextByType(void* data, mu::engraving::EngravingItem* element) | ||
| static void findTextByType(std::pair<TextStyleType, QStringList*>* typeStringsData, mu::engraving::EngravingItem* element) |
There was a problem hiding this comment.
| static void findTextByType(std::pair<TextStyleType, QStringList*>* typeStringsData, mu::engraving::EngravingItem* element) | |
| static void findTextByType(TextStyleType textStyleType, QStringList& strings, mu::engraving::EngravingItem* element) |
| const EngravingObjectList& children() const { return m_children; } | ||
|
|
||
| // Score Tree functions for scan function | ||
| friend class EngravingElementsProvider; |
There was a problem hiding this comment.
We can also remove this now
3 participants
Prompted from this consideration, I took a closer look at the functions in scoretree.cpp, namely
scanParentandscanChildren. As far as I could see, they were introduced for some score tree visualization feature back in version 3, but as far as I'm aware that feature doesn't exist in version 4. Everything about this is quite weird, because the parent and child relationships sometimes don't follow the true hierarchy of our model (ChordRests collect spanners among their children, Spanner collect SpannerSegment even though the parent of a SpannerSegment is always the System, scanParent sometimes returns an item that isn't the parent...). Most of these functions are either stale or are a copy of the correspondingscanElementsmethod. The "source of truth" is obviouslyscanElements, cause that's what we use to determine what is drawn on screen.This PR:
scanParentandscanChildrenfunctions, moving all the relevant functionality toscanElementsEngravingObject::getChildren, which collects the list of children usingscanElementsscanElementsusingstd::functioninstead of the old C-style function pointers. This makes everything nicer, especially avoiding all thevoid*pointers.scanElementsall the logic that had to do with showing or not showing the item on screen. The whole point of something likescanElementis that it doesn't know whatfuncwill do, so putting that logic inscanElementis an antipattern. Deciding whether or not to collect an element should be decided insidefuncitself.