musescore / musescore/MuseScore

Rewrite Alt+Right next-element functions as a tree-traversal

Open
#23,890 7 comments 0 reactions 1 assignee View on GitHub

Nobody has claimed this yet.

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

Description

The EngravingItem::nextElement() and EngravingItem::previousElement() virtual functions define the order in which elements are visited by the Alt+Right and Alt+Left navigation shortcuts.

The way these functions are currently written, they must be overridden by every EngravingItem subclass to determine what comes next in the sequence. However, there is no guarantee that the sequence is consistent, correct, or complete. Ideally, all interactive elements should be included, while all non-interactive elements (e.g. Page, System, etc.) should be excluded.

These functions could be greatly simplified by rewriting them as a tree-traversal, which could look something like this:

EngravingItem* EngravingItem::nextElement()
{
    // Return first child if there is one.
    for (EngravingItem* child : children()) {
        if (child) {
            return child;
        }
    }
    // Otherwise return next sibling in ancestor element.
    EngravingItem* current = this;
    while (EngravingItem* parent = current->parent()) {
        EngravingItemList siblings = parent->children();
        auto it = siblings.cbegin();
        for (;; ++it) {
            IF_ASSERT_FAILED(it != siblings.cend()) {
                break;
            }
            if (*it == current) {
                ++it;
                break;
            }
        }
        for (; it != siblings.cend(); ++it) {
            if (EngravingItem* sibling = *it) {
                return sibling;
            }
        }
        current = parent;
    }
    return nullptr;
}

EngravingItem* EngravingItem::nextNavigableElement()
{
    for (EngravingItem* next = nextElement(); next; next = next->nextElement()) {
        if (!next->flag(ElementFlag::NOT_SELECTABLE)) {
            return next;
        }
    }
    return nullptr;
}

But which tree should we traverse? There are several possible candidates for the parent() and children() functions.

Navigation Parent Navigation Children
EngravingObject::parent()
EngravingObject::explicitParent()
EngravingObject::children()
EngravingObject::scanParent() EngravingObject::scanChildren()
EngravingItem::parentItem(false /*explicit*/)
EngravingItem::parentItem(true /*explicit*/)
EngravingItem::childrenItems()
AccessibleItem::accessibleParent() AccessibleItem::accessibleChild(i)
AccessibleItem::accessibleChildCount()

We'll need to use the one that gives the closest match to the current next-element sequence.

We may still need to override nextElement in a few subclasses, for example, to ensure that navigation stays in the current staff rather than going up and down the segments.

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.