feat(ui): step by index, and give thread stepping a second binding

next_thread and prev_thread now walk with indexBelow/indexAbove, skipping
message rows, so they keep meaning thread-to-thread whatever is expanded.
Stepping message-to-message needs no code: QTreeView's own Up/Down walk VISIBLE
rows and already enter an expanded thread, and being the view's key handling
rather than a shortcut they stay inert when the message pane, a menu or an entry
bar has focus.

Item 60 turns out to have been fixed already, in 5487d58 on this branch, by
threadRowOf() walking up to the containing thread before doing the arithmetic.
The backlog entry was written against master, where that helper does not exist,
so it described a defect this branch had resolved a commit earlier. Verified by
writing both failing tests first and watching them pass: from the last reply of
an expanded thread, and from a thread root with its replies showing. They are
kept, because the property they assert is the one this change must not lose.
What the rewrite buys is that nothing is keyed on a row number any more, which
is the rule a deeper tree would break next.

Alt+Up/Down added alongside Ctrl+J/K. That required KeyMap::sequencesFor and a
move from setShortcut to setShortcuts, because the singular setter keeps only
the last binding and the second one was silently unreachable. Alt because
Shift+arrows is the built-in extend-selection that multi-row tagging depends on,
and because a bare arrow cannot be a window shortcut without breaking every text
field in the window, as Return already demonstrated. sequencesFor puts
sequenceFor's own choice first so the menus advertise an unchanged binding, and
sorts the tail, since QHash order is unspecified.

1 files changed, 130 insertions, 0 deletions

diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp
index d77cee3..b3511ec 100644
--- a/tests/test_mainwindow.cpp
+++ b/tests/test_mainwindow.cpp
@@ -104,6 +104,9 @@ private slots:
     void childRowsAreIndentedUnderTheirThread();
     void aThreadWithRepliesDrawsAVisibleExpander();
     void cardsNeverScrollSideways();
+    void nextThreadLeavesTheLastReply();
+    void altDownSkipsReplies();
+    void bothThreadStepBindingsReachTheAction();
     void replyRowsKeepTheirTextUnderTheThreadLine();
     void clickingTheExpanderTogglesTheThread();
     void selectingAMessageRowTargetsThatMessageNotItsThread();
@@ -643,6 +646,133 @@ void TestMainWindow::childRowsAreIndentedUnderTheirThread()
     QCOMPARE(replyCard.spines.size(), replyIn.depth);
 }
 
+namespace {
+
+/// Two threads, the first with one reply, expanded. The shared fixture for the
+/// two navigation tests below.
+struct NavFixture
+{
+    QTreeView *view = nullptr;
+    ThreadListModel *model = nullptr;
+    QModelIndex root;
+    QModelIndex reply;
+};
+
+NavFixture buildNavFixture(MainWindow &window)
+{
+    NavFixture f;
+    f.view = window.findChild<QTreeView *>();
+    f.model = window.findChild<ThreadListModel *>();
+
+    ThreadSummary first = makeThread(QStringLiteral("T1"),
+                                     QStringList{ QStringLiteral("inbox") });
+    first.totalCount = 2;
+    ThreadSummary second = makeThread(QStringLiteral("T2"),
+                                      QStringList{ QStringLiteral("inbox") });
+    second.totalCount = 1;
+    f.model->appendBatch({ first, second });
+
+    MessageNode rootNode;
+    rootNode.messageId = QStringLiteral("M1");
+    rootNode.threadId = QStringLiteral("T1");
+    rootNode.depth = 0;
+    MessageNode replyNode;
+    replyNode.messageId = QStringLiteral("M2");
+    replyNode.threadId = QStringLiteral("T1");
+    replyNode.depth = 1;
+    f.model->setThreadMessages(QStringLiteral("T1"), { rootNode, replyNode });
+
+    f.root = f.model->index(0, 0);
+    f.view->expand(f.root);
+    f.reply = f.model->index(0, 0, f.root);
+    return f;
+}
+
+}  // namespace
+
+void TestMainWindow::nextThreadLeavesTheLastReply()
+{
+    const Config config;
+    MainWindow window(config);
+    window.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&window));
+
+    const NavFixture f = buildNavFixture(window);
+    QVERIFY(f.reply.isValid());
+    QVERIFY2(f.view->isExpanded(f.root),
+             "the thread is collapsed, so this test would arrow down a flat "
+             "list and pass against the bug it exists to catch");
+
+    f.view->setCurrentIndex(f.reply);
+
+    // The defect (item 60): selectRow(current.row() + 1) asked for row 1 UNDER
+    // T1, which does not exist, so the action did nothing at all.
+    window.findChild<QAction *>(QStringLiteral("next_thread"))->trigger();
+
+    QCOMPARE(f.view->currentIndex().data(ThreadListModel::ThreadIdRole)
+                 .toString(),
+             QStringLiteral("T2"));
+}
+
+void TestMainWindow::altDownSkipsReplies()
+{
+    const Config config;
+    MainWindow window(config);
+    window.show();
+    QVERIFY(QTest::qWaitForWindowExposed(&window));
+
+    const NavFixture f = buildNavFixture(window);
+    QVERIFY(f.view->isExpanded(f.root));
+
+    // From the thread ROOT with its replies showing: one step must land on the
+    // next THREAD, not on the first reply. That is what makes the action mean
+    // thread-to-thread while plain Up/Down still steps message-to-message.
+    f.view->setCurrentIndex(f.root);
+    window.findChild<QAction *>(QStringLiteral("next_thread"))->trigger();
+
+    QCOMPARE(f.view->currentIndex().data(ThreadListModel::ThreadIdRole)
+                 .toString(),
+             QStringLiteral("T2"));
+    QVERIFY(!f.view->currentIndex().data(ThreadListModel::IsMessageRole)
+                 .toBool());
+
+    // And back, which is the mirror case the old arithmetic also failed.
+    window.findChild<QAction *>(QStringLiteral("prev_thread"))->trigger();
+    QCOMPARE(f.view->currentIndex().data(ThreadListModel::ThreadIdRole)
+                 .toString(),
+             QStringLiteral("T1"));
+    QVERIFY(!f.view->currentIndex().data(ThreadListModel::IsMessageRole)
+                 .toBool());
+}
+
+void TestMainWindow::bothThreadStepBindingsReachTheAction()
+{
+    const Config config;
+    MainWindow window(config);
+
+    // Two bindings per action, which needs setShortcuts rather than
+    // setShortcut: Ctrl+J/K for a neomutt hand, Alt+Up/Down for a mouse one.
+    // Alt because Shift+arrows is QTreeView's built-in extend-selection that
+    // multi-row tagging depends on, and a bare arrow cannot be a window
+    // shortcut without breaking every text field in the window.
+    for (const auto &pair : { std::pair<const char *, const char *>{
+                                  "next_thread", "Alt+Down" },
+                              { "prev_thread", "Alt+Up" } }) {
+        auto *action =
+            window.findChild<QAction *>(QString::fromLatin1(pair.first));
+        QVERIFY2(action, pair.first);
+        const QList<QKeySequence> shortcuts = action->shortcuts();
+        QVERIFY2(shortcuts.size() >= 2,
+                 qPrintable(QStringLiteral("%1 carries %2 shortcut(s), so the "
+                                           "second binding is unreachable")
+                                .arg(QString::fromLatin1(pair.first))
+                                .arg(shortcuts.size())));
+        QVERIFY2(shortcuts.contains(
+                     QKeySequence(QString::fromLatin1(pair.second))),
+                 pair.second);
+    }
+}
+
 void TestMainWindow::cardsNeverScrollSideways()
 {
     const Config config;