Implemented Initial letter Navigation in Dropdowns

[shubham-shinde-442]

Resolves: #16508 for Dropdowns

Key points -

Single Key Press: When you press a letter like S, the dropdown should highlight the next item that comes lexicographically after S, such as "Sans Serif Collection."

Multiple Key Presses in Quick Succession: If you press S followed by E quickly, it should highlight the first item after "Se," like "Segoe Fluent Icons." Similarly, S then G highlights "Showcard Gothic."

Repeated Key Presses: If the same key is pressed repeatedly (e.g., S followed by S), the selection cycles through items that begin with S.

• I signed the CLA
• The title of the PR describes the problem it addresses
• Each commit's message describes its purpose and effects, and references the issue it resolves
• If changes are extensive, there is a sequence of easily reviewable commits
• The code in the PR follows the coding rules
• There are no unnecessary changes
• The code compiles and runs on my machine, preferably after each commit individually
• I created a unit test or vtest to verify the changes I made (if applicable)

[shoogle]

For me, this PR moves the blue highlight but not the black focus rectangle. It should be the other way around.

Also, if I'm on Pop/Rock and I press C it should go to Choral rather than Common. It should always go to the next matching item, not the first one unless you've reached the end and there are no more matches.

[shubham-shinde-442]

@shoogle Updated, probably findNextItemIndex loop should start from 0

[shoogle]

probably findNextItemIndex loop should start from 0

Yes, but I'd call that function findLexicographicIndex and only use it when the dropdown values are in alphabetical order. See review comments for details.

For me, this PR moves the blue highlight but not the black focus rectangle. They should always stay together.

I misspoke before. Only the black rectangle should move. The blue highlight should stay fixed until the user presses Space or Enter because that's when we load the new value.

[shubham-shinde-442]

@shoogle, I’ve made the required changes. However, the black rectangle is only visible when navigating to the dropdown using the Tab or arrow keys. When the dropdown is opened with the mouse, it correctly shows the matched item at the top of the visible dropdown (within the dropdown’s height) if the requested item isn’t already visible, but the black rectangle doesn’t appear.

Let me know if further changes are required, or I’ll proceed to squash the commits.

[shoogle]

@shubham-shinde-442, thanks, but we need to see the black focus rectangle. The policy is to hide it for mouse users, but by pressing a key the user has initiated keyboard navigation. Also, if the dropdown fits in on the screen then scrolling won't happen, so we need another way to indicate position, i.e. the rectangle.

[shubham-shinde-442]

@shoogle I believe that navigationcontroller.cpp is handling all key press events related to navigation (about black focus rectangle). doActivePanel and setIsHighlight methods are probably responsible for this behavior. I think we need to implement similar logic in dropdownview.cpp to activate navigation (black focus) rectangle on the item that matches the initial letter typed, as it might not be entirely possible in QML.

Can you just guide me shortly on how to implement this in dropdownview.cpp when the event occurs in QML?

Links of this files -
navigationcontroller.cpp, navigationcontroller.h, dropdownview.cpp, dropdownview.h

[shoogle]

@shubham-shinde-442, methods in dropdownview.h that are prefixed by Q_INVOKABLE or mentioned inside a Q_PROPERTY() macro will be available in StyledDropdownView.qml.

For example, you could implement something like this:

// dropdownview.h
public:
    Q_INVOKABLE void requestHighlight(bool isHighlight);

// dropdownview.cpp
void DropdownView::requestHighlight(bool isHighlight)
{
    navigationController()->setIsHighlight(isHighlight);
}

// StyledDropdownView.qml
root.requestHighlight(true)

Another option is to do everything in NavigationController::eventFilter:

#include <QKeyEvent>

if (event->type() == QEvent::KeyPress) {
    auto keyEvent = static_cast<QKeyEvent*>(event);
    QString typedChar = keyEvent->text();
    LOGI() << typedChar;
    typeAheadFind(typedChar); // for you to implement in C++
    setIsHighlight(true);
    event->accept();
}

This latter method could enable initial letter navigation everywhere, not just in dropdowns. If you want to try it, do it in a new PR so we don't lose the code in this PR.

[shubham-shinde-442]

Why code style check failing? I believe code style is correct

[Jojo-Schmitz]

Seems a final linefeed is missing

[shoogle]

I found a better way to enable the highlight. Simply call:

item.navigation.requestActive(true) // 'true' enables the highlight

If this didn't work for you before, perhaps it's because you called one of these by mistake:

navigation.requestActive(true)
root.navigation.requestActive(true) // same thing

It needs to be done on the item rather than the dropdown as a whole.

---

This means your changes to dropdownview.h and .cpp are no longer needed, but for future reference, you've added one newline too many to those files. Each file should end with a single linefeed character (\n) but right now they end in two linefeeds (\n\n).

When viewing a file on GitHub, the final line of the file:

• Shouldn't be empty (empty line = extra linefeed)
• Shouldn't have a 🚫 icon near the line number (🚫 = missing linefeed)

Note: Many text editors (e.g. nano, VS Code, Qt Creator) will show an empty line when there's a single linefeed at the end of the file. In those editors, you do want to see an empty line there (but not two empty lines).

[shoogle]

Here you test typeAheadSameChar twice on consecutive lines. It would be better to only test it once:

if (typeAheadSameChar) {
    focusNextMatchingItem(typedChar, currentNavIndex + 1)
} else {
    focusNextMatchingItem(typeAheadStr, currentNavIndex)
}

This is clearer too, in my opinion.

[shoogle]

It would be better to restart the typeAheadTimer within the typeAheadFind() function.

focusNextMatchingItem() is a useful function in general, which we might want to call at other times besides during initial letter navigation.

[shoogle]

It's better to fail fast:

if (nextMatchIndex === -1) {
    return false
}

It gets the edge case out of the way and reduces the indentation of the interesting part:

highlightItem(nextMatchIndex)
currentNavIndex = nextMatchIndex
return true

[shoogle]

You should iterate using let i again. Reserve let j for a nested loop:

for (let i = 0; i < maxI; ++i) {
    for (let j = 0; j < maxJ; ++j) {
        doSomething(i, j)
    }
}

[shoogle]

This would read better: if (itemTextMatches(i, str))

[shoogle]

This function is unbalanced because itemText is normalized within the function but str is assumed to be already normalized. Your options are:

• Normalize str within the function (but we don't want to do this for performance reasons)
• Rename str to normalizedStr.
• Rewrite the function as:

  function itemSearchText(i) {
      let itemText = Utils.getItemValue(root.model, i, root.textRole)
      return normalizeForSearch(itemText.replace(/^\s+/, ""))
  }

  And do if (itemSearchText(i).startsWith(str)) { return i } inside findNextMatchingIndex().

You can choose between the latter two options.

[shoogle]

Isn't this function basically doing the same thing as the existing positionViewAtIndex(index) function?

[shoogle]

This isn't working with the denominator dropdown in the New Score dialog > Time signature popup.

You can fix it here, with .toString() being the crucial change:

let resultList = []

for (let d of root.availableDenominators) {
    resultList.push({"text" : d.toString(), "value" : d})
}

return resultList

[shoogle]

@shubham-shinde-442, your QML implementation is working well for dropdowns, but I think you should switch to the generalized C++ solution in NavigationController::eventFilter. I've tried it and it works well, not just for dropdowns, but also for toolbars and tab bars (except the score & parts tabs in the score view, which will require special handling).

If you want to take this up, please do so in a new PR.

Example code to get you started

Note: This code matches against source IDs like pad-note-4. Ideally it should match against translated names like "Quarter note", which are displayed in the UI or used in tooltips and screen reader text. This has to be fixed before it can be merged.

#include <QKeyEvent>
#include <QRegularExpression>
#include <QTimer>

bool NavigationController::eventFilter(QObject* watched, QEvent* event)
{
    switch (event->type()) {
    case QEvent::MouseButtonPress:
        resetIfNeed(watched);
    case QEvent::ShortcutOverride: {
        if (event->isAccepted()) {
            break;
        }

        INavigationPanel* activePanel = this->activePanel();

        if (!activePanel || activePanel->name() == "ScoreView") {
            break;
        }

        auto keyEvent = static_cast<QKeyEvent*>(event);
        QString typedChar = keyEvent->text();

        // TODO: Make these variables private members of NavigationController class
        static QTimer* m_typeAheadTimer = new QTimer(this);
        static QString m_typeAheadStr("");
        static QString m_typeAheadPreviousChar("");
        static bool m_typeAheadSameChar = true;

        // TODO: Convert lambda into member function of NavigationController class
        auto typeAheadReset = []() {
                m_typeAheadStr = "";
                m_typeAheadPreviousChar = "";
                m_typeAheadSameChar = true;
            };

        // TODO: Move next 4 lines into NavigationController constructor
        m_typeAheadTimer->setSingleShot(true);
        m_typeAheadTimer->setInterval(1000);
        connect(m_typeAheadTimer, &QTimer::timeout, this, typeAheadReset); // TODO: &NavigationController::typeAheadReset
        // typeAheadReset(); // TODO: Uncomment in constructor

        if (typedChar.isEmpty() || (typedChar == " " && m_typeAheadStr.isEmpty())) {
            break; // ignore shortcuts and space if it's the only character typed
        }

        // TODO: Also ignore space if it's the last character typed and there are no matches.
        // This is so users can quickly type 'Y[Space]' to press the Yes button in a dialog
        // without waiting for the typeAheadTimer to run out.

        static const QRegularExpression controlCharsExceptSpace("[\\x00-\\x1F\\x7F]");
        if (typedChar.contains(controlCharsExceptSpace)) {
            break; // ignore text containing ASCII control characters
        }

        // User typed a non-control character.
        LOGI() << typedChar;
        //typeAheadFind(typedChar); // Implemented below.
        event->accept();

        // TODO: Move everything below to new member function NavigationController::typeAheadFind(typedChar)

        if (m_typeAheadSameChar && !m_typeAheadStr.isEmpty() && typedChar != m_typeAheadPreviousChar) {
            m_typeAheadSameChar = false;
        }
        m_typeAheadStr.append(typedChar);
        m_typeAheadPreviousChar = typedChar;
        m_typeAheadTimer->start();
        LOGI() << m_typeAheadStr;

        auto normalizedForSearch = [](const QString& s) {
                return s.normalized(QString::NormalizationForm_KD);
            };

        QString searchStr = normalizedForSearch(m_typeAheadSameChar ? typedChar : m_typeAheadStr);

        auto isMatch = [normalizedForSearch](const INavigationControl& control, const QString& normalizedText) {
                static const QRegularExpression leadingWhitespace("^\\s+");
                // TODO: Don't use control.name() because it's the name (or ID) in the source code
                // rather than the translated accessible name shown in UI tooltips, etc. For example,
                // control.name() is "pad-note-4" for the quarter note button in the note input toolbar.
                QString normalizedName = normalizedForSearch(control.name().remove(leadingWhitespace));
                return normalizedName.startsWith(normalizedText, Qt::CaseInsensitive);
            };

        INavigationControl* activeControl = this->activeControl();
        if (activeControl) {
            LOGI() << "Panel: " << activePanel->name() << "; Old control:" << activeControl->name();
        }
        if (!m_typeAheadSameChar && activeControl && isMatch(*activeControl, searchStr)) {
            break;
        }

        INavigation::Index index = activeControl ? activeControl->index() : INavigation::Index();

        MoveDirection direction = activePanel->direction() == INavigationPanel::Direction::Horizontal
                                  ? MoveDirection::Right
                                  : MoveDirection::Down;

        INavigationControl* toControl;

        while ((toControl = nextEnabled(activePanel->controls(), index, direction))) {
            index = toControl->index();
            if (isMatch(*toControl, searchStr)) {
                break;
            }
        }

        if (activeControl && !toControl) {
            index = INavigation::Index();
            while ((toControl = nextEnabled(activePanel->controls(), index, direction))) {
                index = toControl->index();
                if (toControl == activeControl) {
                    break;
                }
                if (isMatch(*toControl, searchStr)) {
                    break;
                }
            }
        }

        if (!toControl || toControl == activeControl) {
            break;
        }

        if (activeControl) {
            doDeactivateControl(activeControl);
        }

        doActivateControl(toControl);
        m_navigationChanged.notify();
        setIsHighlight(true);
        LOGI() << "Panel: " << activePanel->name() << "; New control:" << toControl->name();
    }
    default:
        break;
    }

    return QObject::eventFilter(watched, event);
}

[shoogle]

@shubham-shinde-442 tells me he tried the general solution but ran into problems with Qt deleting controls when they go out of view. This isn't just a problem for initial letter navigation: it also affects Page Up & Page Down, Home & End and even the arrow keys if held for a while.

I think the problem is specific to item view, like ListView or GridView. For example, it can occur in Preferences > Shortcuts because that's a big list, but not in Preferences > Appearance (even though its scrollable) because that's just ordinary controls (not a list).

To solve it, the controlling code will need direct access to the view's underlying model, as well as the focussedRow and focussedColumn (which are different to the currentRow and currentColumn in our views). For Page Up and Page Down, the controlling code also needs to know whether a particular row is currently in view, so that it can navigate up or down by one viewport.

That means either doing all item-view navigation in QML, or exposing the necessary information to the C++, possibly via the existing NavigationPanel class or a new NavigationView class. This class would either do the navigation directly by setting focussedRow and focussedColumn to new values, or it could emit signals like navigateDown(), navigateLast(), or navigateToItemMatching(string) that would be handled in the QML.

@shubham-shinde-442, you can try to attempt that if you like, but I think it would involve a lot of back-and-forth where you try something and then I ask you to try something else. It might be easier if you just leave this one to me, along with anything mentioned in this comment (except numbers 6, 7, and 8, which I think could probably be tackled in isolation).