GSoC 2020: scanElements refactoring based on tree model [vtests]

[krkartikay]

This PR refactors the scanElements function to scan the elements using the score tree to traverse the elements instead of handling each element case by case.

This builds up on the tree model that I created in my previous PR, #6174 .

Latest GSoC blog link: https://musescore.org/en/user/1743616/blog/2020/07/20/gsoc-2020-tree-model-week-7-finished-scanelements-refactoring

[shoogle]

Looks pretty good, but I don't like how Element::scanElements forces you to check the treeChildCount() multiple times (e.g. in barline.cpp). As mentioned on Telegram, a cleaner approach might be this:

void ScoreElement::scanElements(data, func, all)
{
    // Apply to all children recursively
    for (ScoreElement* el : (*this)) {
        el->scanElements(data, func, all);
    }
}

void Element::scanElements(data, func, all)
{
    // Apply to self
    if (all || visible() || score()->showInvisible()) {
        func(data, this);
    }
}

void BarLine::scanElements(data, func, all)
{
    Element::scanElements(data, func, all) // Apply to self
    ScoreElement::scanElements(data, func, all) // Apply to children
}

Or you could create non-virtual functions scanSelf and scanChildren to do the same thing.

However, these methods might mean that you have to override scanElements in more places than you do currently, so it's up to you.

[shoogle]

No need to keep a list here since it will just go out of date. Better to say something generic like "override this if you need to apply to apply to this element as well as its children".

But why have the treeChildCount() check anyway? What problems occur if you just apply the function to all elements regardless of whether they are leaf nodes?

[krkartikay]

I applying the function to all nodes, surprisingly it seemed to work with no obvious bugs. But I couldn't select measures as I could earlier, by clicking on the staff, the system got selected instead. There might be other subtle bugs like this, but I could only find one right now.

[shoogle]

Interesting! Normally systems cannot be selected. The same goes for pages, but they have special handling here. Perhaps if you handle systems in a similar way then the bug will disappear?

Note that the bug has nothing to do with drawing. Element::draw is a virtual function that does nothing by default, and it is not overridden by System. The bug actually occurs here, where scanElements is used to build a BSP Tree of elements that will be searched to find the element at the given position. System is added to that tree when it wasn't previously.

If bugs occur with other classes then we should aim to fix them in the same way as the drawing case: Rather than skipping elements, scanElements should visit every element, but the effect of calling func on certain elements should be no-op.

[shoogle]

There are some places where an element is scanned before its children, and in other places it is scanned after its children. I realize that the old scanElements was also inconsistent in this regard, but I think you should pick one method to use exclusively and see if the tests still pass. I recommend children first because I think children are more likely to affect the layout of parents than the other way around, but I could be wrong.

[shoogle]

Nearly there! There's also a few comments left from previous reviews that need to be addressed (either to fix the problem or to explain why it is not a problem).

[shoogle]

It's code like this that makes me think MStaff should be a proper element and part of the tree model. Then this could be simply:

if (all || mstaff()->isVisible()) {
    func(data, this);
}

Anyway, keep it as it is for now. We can add this to our list of things to consider later.

[krkartikay]

Note: vtest faliure is intended, because this PR causes ties to be drawn only once (as opposed to twice earlier which was a bug) so the ties appear thinner.

[shoogle]

A few more suggestions.

[shoogle]

Looks good to me! Not sure why the vtests failed given that you updated the PR title, but as far as I am concerned this is ready to merge. (The vests change is due to fixing a bug in the old scanElements that caused ties to be scanned twice.)

[shoogle]

Rebase needed. Also, .cpp and .h files will need to be passed through Uncrustify because there is now a test to enforce coding style. On the bright side, it is now possible to enable the pre-commit hook without causing unwanted changes.

[krkartikay]

Rebased again and resolved conflict in note.cpp.