diff options
| -rw-r--r-- | src/keymap.cpp | 36 | ||||
| -rw-r--r-- | src/keymap.h | 9 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 45 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 130 |
4 files changed, 203 insertions, 17 deletions
diff --git a/src/keymap.cpp b/src/keymap.cpp index 29a5f71..cdd26a1 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -18,6 +18,8 @@ #include "keymap.h" +#include <algorithm> + #include <QSettings> QStringList KeyMap::knownActions() @@ -64,6 +66,18 @@ QList<QPair<QString, QString>> KeyMap::defaultBindings() return { { QStringLiteral("Ctrl+J"), QStringLiteral("next_thread") }, { QStringLiteral("Ctrl+K"), QStringLiteral("prev_thread") }, + // Alt, because Shift+Up/Down is QTreeView's built-in extend-selection, + // which multi-row tagging depends on, and plain Up/Down is the view's + // own navigation, which already steps INTO an expanded thread's + // replies and is what gives message-to-message movement for free. + // + // These must stay chords. Every action is a QAction with + // WindowShortcut, dispatched before the focused widget sees the key, + // and Qt withholds only plain LETTERS from editable widgets: a bare + // Up bound here would break the arrow keys in the query bar, the tag + // dialog and the web view at once, exactly as Return did. + { QStringLiteral("Alt+Down"), QStringLiteral("next_thread") }, + { QStringLiteral("Alt+Up"), QStringLiteral("prev_thread") }, { QStringLiteral("Return"), QStringLiteral("open_thread") }, { QStringLiteral("Ctrl+E"), QStringLiteral("archive") }, { QStringLiteral("Ctrl+D"), QStringLiteral("delete") }, @@ -153,6 +167,28 @@ void KeyMap::loadDefaults() m_bindings.insert(normalizeSequence(binding.first), binding.second); } +QList<QKeySequence> KeyMap::sequencesFor(const QString &action) const +{ + const QKeySequence primary = sequenceFor(action); + if (primary.isEmpty()) + return {}; + + QList<QKeySequence> all{ primary }; + QList<QKeySequence> rest; + for (auto it = m_bindings.cbegin(); it != m_bindings.cend(); ++it) { + if (it.value() == action && it.key() != primary) + rest.append(it.key()); + } + // QHash iteration order is unspecified, so the tail is sorted rather than + // left to chance: an action's shortcut list must not reorder between runs. + std::sort(rest.begin(), rest.end(), + [](const QKeySequence &a, const QKeySequence &b) { + return a.toString() < b.toString(); + }); + all += rest; + return all; +} + QKeySequence KeyMap::sequenceFor(const QString &action) const { // Several sequences can reach one action: the built-in default, which diff --git a/src/keymap.h b/src/keymap.h index 1c7df5f..f0addf5 100644 --- a/src/keymap.h +++ b/src/keymap.h @@ -55,6 +55,15 @@ public: /// text, so the menu shows a stable choice rather than a hash-order one. QKeySequence sequenceFor(const QString &action) const; + /// EVERY sequence bound to an action, with sequenceFor()'s choice first. + /// + /// An action can have more than one binding, and setShortcut() keeps only + /// the last: next_thread ships with both Ctrl+J and Alt+Down, and with the + /// singular setter whichever arrived second was silently unreachable. + /// Ordered rather than hash-ordered, so the menu still advertises the same + /// binding sequenceFor() chose. + QList<QKeySequence> sequencesFor(const QString &action) const; + /// The built-in sequence for an action, ignoring any user override. static QKeySequence defaultSequenceFor(const QString &action); diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 30148d4..57a6988 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -629,9 +629,11 @@ QAction *MainWindow::addAction(const QString &name, const QString &text, // The binding comes from KeyMap, so a [keys] override reaches the menus // and the shortcut reference as well as the keyboard. - const QKeySequence sequence = m_keyMap.sequenceFor(name); - if (!sequence.isEmpty()) - action->setShortcut(sequence); + // Plural: an action can carry more than one binding, and setShortcut() + // keeps only the last one given. next_thread has both Ctrl+J and Alt+Down. + const QList<QKeySequence> sequences = m_keyMap.sequencesFor(name); + if (!sequences.isEmpty()) + action->setShortcuts(sequences); // Shortcuts must work while focus is in the thread list or the message // view, not only on the window itself. @@ -655,23 +657,32 @@ void MainWindow::registerActions() }); addAction(QStringLiteral("next_thread"), tr("&Next thread"), tr("Select the next thread"), [this]() { - // The THREAD after this one, which is not "the next row" once replies - // are expanded: from a thread row the next row may be its own first - // reply, and from a reply row the row number counts siblings, not - // threads. Both are resolved by walking up to the containing thread - // first. - const QModelIndex current = m_threadView->currentIndex(); - const QModelIndex thread = threadRowOf(current); - const int row = thread.isValid() ? thread.row() + 1 : 0; - if (row < m_model->rowCount()) - selectThreadRow(row); + // Walked by INDEX, never by row number. A tree numbers rows per + // parent, so current.row() + 1 names a SIBLING: from the last reply of + // an expanded thread it asks for a row that does not exist, and from a + // thread row it counts top-level threads only by accident (item 60). + // + // The skip loop is what keeps this meaning thread-to-thread while the + // view's own Up/Down still steps message-to-message. + QModelIndex index = m_threadView->indexBelow( + m_threadView->currentIndex()); + while (index.isValid() + && index.data(ThreadListModel::IsMessageRole).toBool()) { + index = m_threadView->indexBelow(index); + } + if (index.isValid()) + selectRowAt(index); }); addAction(QStringLiteral("prev_thread"), tr("&Previous thread"), tr("Select the previous thread"), [this]() { - const QModelIndex current = m_threadView->currentIndex(); - const QModelIndex thread = threadRowOf(current); - if (thread.isValid() && thread.row() > 0) - selectThreadRow(thread.row() - 1); + QModelIndex index = m_threadView->indexAbove( + m_threadView->currentIndex()); + while (index.isValid() + && index.data(ThreadListModel::IsMessageRole).toBool()) { + index = m_threadView->indexAbove(index); + } + if (index.isValid()) + selectRowAt(index); }); addAction(QStringLiteral("open_thread"), tr("&Open thread"), tr("Focus the thread list"), [this]() { 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; |