From 5487d581069333a64e0e0480f53f06a7b64e486d Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:35:06 +0200 Subject: refactor(view): make ThreadListView a QTreeView for message rows The strip survived the port because every geometry call it needs exists on both classes. What did not survive is anything keyed on a row NUMBER: a tree numbers rows per parent, so row 0 exists once per expanded thread and the old flat 0..N walk would paint the first thread's strip over every one of them. The walk now goes by index, and the alternating colour follows visual position rather than index.row() for the same reason. QTableView::isRowSelected(int) has no QTreeView equivalent; isSelected on the index replaces it. MainWindow loses verticalHeader and selectRow, so row height comes from uniformRowHeights and three helpers replace the row arithmetic. next_thread and prev_thread now resolve the containing thread first: in a tree current.row() + 1 is the next SIBLING, which under an expanded thread is the next reply, not the next thread. Two test defects found by mutation and worth recording, since both produced a green suite over a broken assertion: The indent test asserted on column 0. A QTreeView indents only the column holding the expander, verified against Qt 6.11: with setTreePosition(4), column 0 reports the same left edge for a thread and its reply while column 4 reports 420 against 440. It was failing against a correctly indented tree. The strip test passed with the view's skip deleted, because the real model already returns no pills for a child row, so the view's guard was never the thing under test. It now runs against a stub model that hands pills to every row, which leaves the view's skip as the only thing that can keep replies clean. That rewrite then failed for a third reason: without the delegates MainWindow installs, rows take the default height, the band is measured against SubjectDelegate::rowHeightFor and overflows into the row below, and the thread's own strip paints across the reply. Reads exactly like a missing skip and is not one. --- tests/test_mainwindow.cpp | 293 ++++++++++++++++++++++++++++++++++++++-------- 1 file changed, 243 insertions(+), 50 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 5031ead..7b6e55d 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -38,6 +38,7 @@ #include #include #include +#include #include #include "config.h" @@ -47,6 +48,7 @@ #include "notmuchworker.h" #include "tagchip.h" #include "threadlistmodel.h" +#include "threadlistview.h" /// MainWindow is mostly wiring, and the parts that need a real database are /// still verified manually. What is checked here is the action registry: the @@ -94,6 +96,8 @@ private slots: void theStatusBarFollowsTheSyncPhase(); void aSelectedReadThreadIsNotDimmedIntoTheHighlight(); void thePillRowSpansTheWholeWidthNotOneColumn(); + void childRowsAreIndentedUnderTheirThread(); + void noTagStripIsPaintedUnderAMessageRow(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); void markAllReadDoesNothingWhenNothingIsUnread(); @@ -452,7 +456,7 @@ void TestMainWindow::headerStateFromADifferentColumnLayoutIsDiscarded() const Config config; MainWindow reopened(config); - auto *view = reopened.findChild(); + auto *view = reopened.findChild(); QVERIFY(view); QCOMPARE(view->columnWidth(ThreadListModel::AttachmentColumn), 28); QCOMPARE(view->columnWidth(ThreadListModel::DateColumn), 130); @@ -503,6 +507,27 @@ void TestMainWindow::returnInTheQueryBarRunsTheQueryNotOpenThread() QVERIFY(!actionFired); } +/// Selects a top-level THREAD row, replacing QTableView::selectRow which a +/// QTreeView does not have. +/// +/// Not merely a rename: setCurrentIndex alone leaves the selection model empty, +/// and select() alone leaves current invalid, so every test asserting on either +/// would break in a different way. Both are set here, exactly as +/// QTableView::selectRow did. +static void selectThreadRow(QTreeView *view, int row) +{ + const QModelIndex index = view->model()->index(row, 0, QModelIndex()); + view->selectionModel()->select( + index, QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(index); +} + +/// The height of a top-level row, replacing QTableView::rowHeight(int). +static int threadRowHeight(QTreeView *view, int row) +{ + return view->visualRect(view->model()->index(row, 0, QModelIndex())).height(); +} + /// A thread summary carrying the tags a test needs. Enough to drive selection; /// nothing here touches a database. static ThreadSummary makeThread(const QString &id, const QStringList &tags) @@ -530,7 +555,7 @@ void TestMainWindow::thePillRowSpansTheWholeWidthNotOneColumn() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); ThreadSummary thread = makeThread(QStringLiteral("t1"), {}); @@ -576,7 +601,7 @@ void TestMainWindow::thePillRowSpansTheWholeWidthNotOneColumn() for (const QVariant &colour : colours) pillColours.insert(colour.value().rgb()); - const int rowHeight = view->rowHeight(0); + const int rowHeight = threadRowHeight(view, 0); QVERIFY(rowHeight > 0); int chipPixels = 0; @@ -592,6 +617,174 @@ void TestMainWindow::thePillRowSpansTheWholeWidthNotOneColumn() "still confined to that cell rather than spanning the row"); } +void TestMainWindow::childRowsAreIndentedUnderTheirThread() +{ + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY2(view, "the thread list is not a QTreeView, so it cannot indent"); + + model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); + + MessageNode first; + first.messageId = QStringLiteral("m0@example.org"); + first.threadId = QStringLiteral("t1"); + first.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.from = QStringLiteral("A Replier "); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { first, reply }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex root = model->index(0, 0, QModelIndex()); + view->expand(root); + QApplication::processEvents(); + + // Measured on the TREE POSITION column, not on column 0. A QTreeView + // indents only the column carrying the expander, verified against Qt 6.11: + // with setTreePosition(4), column 0 reports the same left edge for a thread + // and its reply (0 and 0) while column 4 reports 420 and 440. Asserting on + // column 0 therefore fails against a perfectly indented tree. + const int treeColumn = ThreadListModel::SubjectColumn; + const QModelIndex rootCell = model->index(0, treeColumn, QModelIndex()); + const QModelIndex child = model->index(0, treeColumn, root); + QVERIFY(child.isValid()); + + // Guards before the claim: a probe that cannot see both rows can report + // anything it likes about their relative position. + QVERIFY2(view->visualRect(rootCell).height() > 0, + "the thread row has no height, so nothing about it is measurable"); + QVERIFY2(view->visualRect(child).height() > 0, + "the reply row has no height: it is collapsed or off-screen, and " + "an indent test against it would pass without drawing anything"); + + QVERIFY2(view->visualRect(child).left() > view->visualRect(rootCell).left(), + "the reply is not indented relative to its thread"); +} + +void TestMainWindow::noTagStripIsPaintedUnderAMessageRow() +{ + // The strip is a row-wide band of the THREAD's tags. Painted under every + // reply as well it would stripe the list and repeat identical tags down the + // whole expansion. + // + // TWO independent guards stop that, and this test is aimed at the SECOND: + // the model returns no pills for a child row, and the view skips child rows + // in its walk. Asserting against the real model tests only the first, and + // the view's guard can be deleted without the test noticing: verified by + // mutation, which passed with the skip removed. So the model is replaced + // here by one that hands out pills for EVERY row, thread and reply alike, + // leaving the view's own skip as the only thing that can keep the reply + // rows clean. + /// Hands out the same pills for a message row as for a thread row, which + /// the real model never does. Without this the view's skip is unobservable. + class PillsEverywhereModel : public ThreadListModel + { + public: + QVariant data(const QModelIndex &index, int role) const override + { + if (role == PillTagsRole) { + return QStringList{ QStringLiteral("mailing-list/SBo"), + QStringLiteral("signed") }; + } + if (role == PillColoursRole) { + return QVariantList{ QVariant::fromValue(QColor(Qt::magenta)), + QVariant::fromValue(QColor(Qt::cyan)) }; + } + return ThreadListModel::data(index, role); + } + }; + + PillsEverywhereModel model; + ThreadListView view; + view.setModel(&model); + view.setTreePosition(ThreadListModel::SubjectColumn); + view.setUniformRowHeights(true); + + // The delegates MainWindow installs, and not optional here. The strip's + // band is measured against SubjectDelegate::rowHeightFor; without the + // delegate the rows take the default height, the band overflows into the + // row below, and the thread's own strip paints across the reply. That + // reads exactly like a missing skip in the walk and is not one. + view.setItemDelegate(new RowStyleDelegate(&view)); + view.setItemDelegateForColumn(ThreadListModel::SubjectColumn, + new SubjectDelegate(&view)); + view.setColumnWidth(ThreadListModel::AttachmentColumn, 28); + view.setColumnWidth(ThreadListModel::FlagColumn, 28); + view.setColumnWidth(ThreadListModel::DateColumn, 130); + view.setColumnWidth(ThreadListModel::AuthorsColumn, 180); + view.setColumnWidth(ThreadListModel::SubjectColumn, 520); + + ThreadSummary thread = makeThread(QStringLiteral("t1"), {}); + thread.tags = QStringList{ QStringLiteral("mailing-list/SBo"), + QStringLiteral("signed") }; + model.appendBatch({ thread }); + + MessageNode first; + first.messageId = QStringLiteral("m0@example.org"); + first.threadId = QStringLiteral("t1"); + first.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.depth = 1; + model.setThreadMessages(QStringLiteral("t1"), { first, reply }); + + view.resize(1400, 300); + view.show(); + QVERIFY(QTest::qWaitForWindowExposed(&view)); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + view.expand(root); + QApplication::processEvents(); + + const QModelIndex child = model.index(0, 0, root); + const QRect childRect = view.visualRect(child); + QVERIFY2(childRect.height() > 0, "the reply row is not on screen"); + + // The exact colours the stub supplies, so an antialiased edge of anything + // else cannot be counted as a pill. + QSet pillColours; + pillColours.insert(QColor(Qt::magenta).rgb()); + pillColours.insert(QColor(Qt::cyan).rgb()); + + QImage shot(view.viewport()->size(), QImage::Format_ARGB32); + shot.fill(Qt::transparent); + view.viewport()->render(&shot); + + // Guard proving the probe can see pills at all: the THREAD row must have + // them, or a zero count under the reply proves nothing about the reply. + const QRect rootRect = view.visualRect(root); + int threadPills = 0; + for (int y = rootRect.top(); y < qMin(rootRect.bottom(), shot.height()); ++y) { + for (int x = 0; x < shot.width(); ++x) { + if (pillColours.contains(shot.pixel(x, y) | 0xff000000)) + ++threadPills; + } + } + QVERIFY2(threadPills > 0, + "no pill pixels under the THREAD row either, so this probe cannot " + "tell a missing strip from a broken render"); + + int replyPills = 0; + for (int y = childRect.top(); y < qMin(childRect.bottom(), shot.height()); ++y) { + for (int x = 0; x < shot.width(); ++x) { + if (pillColours.contains(shot.pixel(x, y) | 0xff000000)) + ++replyPills; + } + } + + QCOMPARE(replyPills, 0); +} + void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() { // Read threads carry a dimmed Qt::ForegroundRole, blended against the @@ -607,7 +800,7 @@ void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); // Both rows READ, so both are dimmed and neither is bold: the only thing @@ -633,10 +826,10 @@ void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() // dimmed row switches it to the highlight's own text colour, so the two // rows MUST differ; comparing two identically-styled rows would pass // against a delegate that did nothing at all. - view->selectRow(0); + selectThreadRow(view, 0); QApplication::processEvents(); - const int rowHeight = view->rowHeight(0); + const int rowHeight = threadRowHeight(view, 0); QVERIFY(rowHeight > 0); QImage shot(view->viewport()->size(), QImage::Format_ARGB32); @@ -724,7 +917,7 @@ void TestMainWindow::markAllReadActsOnEveryRowAndUndoesInOneStep() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *action = window.findChild(QStringLiteral("mark_all_read")); QVERIFY(action); @@ -746,7 +939,7 @@ void TestMainWindow::markAllReadActsOnEveryRowAndUndoesInOneStep() // One row selected, to prove the action ignores the selection rather than // acting on it. - view->selectRow(0); + selectThreadRow(view, 0); action->trigger(); @@ -831,7 +1024,7 @@ void TestMainWindow::markReadTimerRestartsRatherThanStacking() QVERIFY(model); auto *timer = window.findChild(QStringLiteral("markReadTimer")); QVERIFY(timer); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), @@ -841,13 +1034,13 @@ void TestMainWindow::markReadTimerRestartsRatherThanStacking() makeThread(QStringLiteral("t3"), { QStringLiteral("unread") }) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(timer->isActive(), "no timer armed for an unread thread"); // Move on before it can fire. One timer stays armed, not three. - view->selectRow(1); + selectThreadRow(view, 1); QVERIFY(timer->isActive()); - view->selectRow(2); + selectThreadRow(view, 2); QVERIFY(timer->isActive()); // Exactly one timer exists at all, which is what "restarted, not stacked" @@ -867,7 +1060,7 @@ void TestMainWindow::markReadTimerIsNotArmedForAReadThread() QVERIFY(model); auto *timer = window.findChild(QStringLiteral("markReadTimer")); QVERIFY(timer); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("read"), @@ -875,16 +1068,16 @@ void TestMainWindow::markReadTimerIsNotArmedForAReadThread() makeThread(QStringLiteral("unread"), { QStringLiteral("unread") }) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(!timer->isActive(), "armed a timer for an already-read thread"); // And the unread one still arms, so this is not "never arms". - view->selectRow(1); + selectThreadRow(view, 1); QVERIFY(timer->isActive()); // Moving back to a read thread disarms it again, rather than leaving the // previous thread's timer running to fire against the wrong row. - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY(!timer->isActive()); } @@ -909,12 +1102,12 @@ void TestMainWindow::markReadCanBeDisabled() QVERIFY(model); auto *timer = window.findChild(QStringLiteral("markReadTimer")); QVERIFY(timer); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), { QStringLiteral("unread") }) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(!timer->isActive(), "a negative mark_read_delay_ms must disable the timer"); @@ -1091,7 +1284,7 @@ void TestMainWindow::selectAllIsBoundAndSelectsEveryRow() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), {}), @@ -1120,7 +1313,7 @@ void TestMainWindow::aMultiRowSelectionDoesNotArmTheMarkReadTimer() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *timer = window.findChild(QStringLiteral("markReadTimer")); QVERIFY(timer); @@ -1134,7 +1327,7 @@ void TestMainWindow::aMultiRowSelectionDoesNotArmTheMarkReadTimer() // Sweep down as Shift+arrow does: current moves onto a row while the // selection already spans more than one. - view->selectRow(0); + selectThreadRow(view, 0); view->selectionModel()->select( model->index(1, 0), QItemSelectionModel::Select | QItemSelectionModel::Rows); @@ -1159,7 +1352,7 @@ void TestMainWindow::growingASelectionCancelsAnAlreadyArmedTimer() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *timer = window.findChild(QStringLiteral("markReadTimer")); QVERIFY(timer); @@ -1169,7 +1362,7 @@ void TestMainWindow::growingASelectionCancelsAnAlreadyArmedTimer() makeThread(QStringLiteral("t2"), { QStringLiteral("unread") }) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(timer->isActive(), "no timer armed for a single unread thread"); // Extend to a second row, as Shift+click would. @@ -1191,7 +1384,7 @@ void TestMainWindow::collapsingBackToOneRowLoadsThatThreadAgain() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *timer = window.findChild(QStringLiteral("markReadTimer")); QVERIFY(timer); @@ -1205,7 +1398,7 @@ void TestMainWindow::collapsingBackToOneRowLoadsThatThreadAgain() QVERIFY(!timer->isActive()); // Back to one row, as a plain click would leave it. - view->selectRow(1); + selectThreadRow(view, 1); QVERIFY2(timer->isActive(), "collapsing back to one row did not resume mark-read"); @@ -1221,7 +1414,7 @@ void TestMainWindow::theStatusBarReportsAMultiRowSelection() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *status = window.findChild(QStringLiteral("statusMessage")); QVERIFY2(status, "no status label to report into"); @@ -1246,7 +1439,7 @@ void TestMainWindow::clearSelectionBlanksThePaneAndDeselects() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), {}), @@ -1256,7 +1449,7 @@ void TestMainWindow::clearSelectionBlanksThePaneAndDeselects() // and what CLAUDE.md requires: selectAll() on a fresh view emits no // currentRowChanged at all, so a test starting there passes against a // missing guard. - view->selectRow(0); + selectThreadRow(view, 0); QCOMPARE(view->selectionModel()->selectedRows().size(), 1); auto *action = window.findChild(QStringLiteral("clear_selection")); @@ -1302,13 +1495,13 @@ void TestMainWindow::clearPaneLeavesTheSelectionAlone() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), {}), makeThread(QStringLiteral("t2"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); QCOMPARE(view->selectionModel()->selectedRows().size(), 1); auto *action = window.findChild(QStringLiteral("clear_pane")); @@ -1421,7 +1614,7 @@ void TestMainWindow::theThreadListOffersAContextMenu() const Config config; MainWindow window(config); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); QCOMPARE(view->contextMenuPolicy(), Qt::CustomContextMenu); @@ -1464,7 +1657,7 @@ void TestMainWindow::aSecondRowBlanksThePaneNotOnlyAThird() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *timer = window.findChild(QStringLiteral("markReadTimer")); QVERIFY(timer); @@ -1477,7 +1670,7 @@ void TestMainWindow::aSecondRowBlanksThePaneNotOnlyAThird() { QStringLiteral("unread") }) }); // One row: ordinary reading, so a timer is armed and a thread is current. - view->selectRow(0); + selectThreadRow(view, 0); QCOMPARE(view->selectionModel()->selectedRows().size(), 1); QVERIFY(timer->isActive()); @@ -1829,13 +2022,13 @@ void TestMainWindow::escapeBlanksTheMessagePane() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), {}), makeThread(QStringLiteral("t2"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); QVERIFY2(!window.currentThreadId().isEmpty(), "no thread was opened to blank"); @@ -1856,14 +2049,14 @@ void TestMainWindow::deleteTogglesOnAnAlreadyDeletedThread() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *action = window.findChild(QStringLiteral("delete")); QVERIFY(action); model->appendBatch({ makeThread(QStringLiteral("t1"), { QStringLiteral("deleted") }) }); - view->selectRow(0); + selectThreadRow(view, 0); action->trigger(); @@ -1884,7 +2077,7 @@ void TestMainWindow::deleteOnAMixedSelectionDeletesRatherThanSplittingIt() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *action = window.findChild(QStringLiteral("delete")); QVERIFY(action); @@ -1935,7 +2128,7 @@ void TestMainWindow::theSelectionCountIsStateAndDoesNotExpire() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *status = window.findChild(QStringLiteral("statusMessage")); QVERIFY(status); @@ -2059,13 +2252,13 @@ void TestMainWindow::anEditDuringABackgroundSyncIsNotSentYet() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *action = window.findChild(QStringLiteral("flag")); QVERIFY2(action, "no flag action registered"); model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); // A cron sync takes the lock. QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", @@ -2090,13 +2283,13 @@ void TestMainWindow::aHeldEditIsSentWhenTheBackgroundSyncEnds() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *action = window.findChild(QStringLiteral("flag")); QVERIFY(action); model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, @@ -2125,7 +2318,7 @@ void TestMainWindow::aHeldEditCountsAsUnsynced() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *action = window.findChild(QStringLiteral("flag")); QVERIFY(action); @@ -2134,7 +2327,7 @@ void TestMainWindow::aHeldEditCountsAsUnsynced() QVERIFY2(label->isHidden(), "the indicator starts hidden at zero"); model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); - view->selectRow(0); + selectThreadRow(view, 0); QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, @@ -2162,7 +2355,7 @@ void TestMainWindow::anUnreadableLockTableStillSendsTheEdit() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *action = window.findChild(QStringLiteral("flag")); QVERIFY(action); @@ -2173,7 +2366,7 @@ void TestMainWindow::anUnreadableLockTableStillSendsTheEdit() QMetaObject::invokeMethod(&window, "onExternalSyncStateChanged", Q_ARG(SyncMonitor::State, SyncMonitor::State::Running)); - view->selectRow(0); + selectThreadRow(view, 0); action->trigger(); QVERIFY2(window.hasEditAwaitingSend(), "the edit was not held during a running sync, so this test is not " @@ -2189,7 +2382,7 @@ void TestMainWindow::anUnreadableLockTableStillSendsTheEdit() "platform without /proc/locks"); // And a NEW edit is sent rather than held. - view->selectRow(1); + selectThreadRow(view, 1); action->trigger(); QVERIFY2(!window.hasEditAwaitingSend(), "an unreadable lock table held a new edit, so writes never resume"); @@ -2206,7 +2399,7 @@ void TestMainWindow::aRejectedWriteKeepsEarlierUndoHistory() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); auto *flag = window.findChild(QStringLiteral("flag")); QVERIFY(flag); @@ -2215,7 +2408,7 @@ void TestMainWindow::aRejectedWriteKeepsEarlierUndoHistory() model->appendBatch({ makeThread(QStringLiteral("t1"), { QStringLiteral("inbox") }) }); - view->selectRow(0); + selectThreadRow(view, 0); // One edit that succeeds, so there is history worth keeping. archive->trigger(); -- cgit v1.2.3 From 10ff78629b3d60810b85110a2f194e0d1b87752a Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:56:58 +0200 Subject: fix(ui): make the expander visible and the reply indent readable Both were reported from the running application after the previous commit claimed them working, and the tests that passed could not see either fault. The expander took four attempts, each of which looked right in code: - QTreeView::drawBranches is the documented hook and does not work here. It runs BEFORE the row's cells, so with the expander on a content column the delegate's own background paints over it. A 60-pixel triangle survived as 8, indistinguishable from the theme's near-invisible dot. - Sizing the glyph from the row rather than the branch rect put most of it outside that rect. - Moving it into SubjectDelegate but calling it from only the no-chip branch left every real row without one, since every real row has an account chip and takes the other branch. It is now drawn by the delegate, which owns the cell and paints after the background, from both branches, with setRootIsDecorated(false) so the style does not draw its dot underneath. The indent was 20px and invisible for a reason the geometry could not show: a thread row draws an account chip before its subject and a reply row does not, so a reply's text already starts about a chip's width LEFT of its thread's. The indent has to beat that before any nesting reads at all, hence 72px. The indent test asserted on visualRect, which was correctly indented the whole time, and so passed against a build with no visible nesting. It now measures where the TEXT lands, accounting for the chip, and fails at 20px. The new expander test counts painted pixels of the glyph colour against a control row with no replies, and fails when the call is dropped from either branch. --- src/mainwindow.cpp | 21 ++++++-- src/tagchip.cpp | 56 +++++++++++++++++++- src/tagchip.h | 14 +++++ src/threadlistmodel.cpp | 7 +++ src/threadlistmodel.h | 7 +++ src/threadlistview.h | 1 + tests/test_mainwindow.cpp | 130 +++++++++++++++++++++++++++++++++++++++++++++- 7 files changed, 231 insertions(+), 5 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 720ec60..8bab2e8 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -559,9 +559,24 @@ void MainWindow::buildUi() // paperclip out of a 28px column entirely. m_threadView->setTreePosition(ThreadListModel::SubjectColumn); - // The root thread rows are the top level, so no decoration for them beyond - // the expander a thread with replies gets on its own. - m_threadView->setRootIsDecorated(true); + // No style-drawn branch decoration. SubjectDelegate draws the expander + // itself, because drawBranches runs BEFORE the row's cells and the + // delegate's own background paints straight over anything put there: a + // 60-pixel triangle survived as 8 pixels, indistinguishable from the + // near-invisible dot this replaces. Leaving both enabled would draw the + // theme's dot underneath the delegate's glyph. + m_threadView->setRootIsDecorated(false); + + // Wider than Qt's default 20px, and the reason is specific rather than + // aesthetic. A thread row carries an account chip in front of its subject + // and a reply row does not, so a reply's text already starts roughly a + // chip's width (~60px) to the LEFT of its thread's. At the default indent + // the 20px shift is swallowed by that difference and the replies read as + // flush with the thread, or even outdented. Verified against the running + // app, not assumed: visualRect reported a correct 20px indent while the + // rendered text showed none, because the geometry is indented and the + // delegate then lays the text out from its own left edge. + m_threadView->setIndentation(SubjectDelegate::kReplyIndent); // Two delegates, and the split is not cosmetic. RowStyleDelegate carries // only the selection fix every column needs: the read/unread dimming diff --git a/src/tagchip.cpp b/src/tagchip.cpp index 1f4ad79..b3e743e 100644 --- a/src/tagchip.cpp +++ b/src/tagchip.cpp @@ -156,6 +156,51 @@ void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &optio const QString account = index.data(ThreadListModel::AccountLabelRole).toString(); + + // The expander is drawn HERE and not in QTreeView::drawBranches, which is + // the obvious place and does not work. drawBranches runs before the row's + // cells, so with the expander column set to the subject the delegate's own + // background fills straight over it: measured at 8 surviving pixels of a + // 60-pixel triangle, which is exactly the near-invisible dot that made this + // override necessary in the first place. The delegate owns this cell and + // paints after the background, so it is the only place the glyph survives. + const auto drawExpander = [&](const QRect &cell) { + if (!index.data(ThreadListModel::HasRepliesRole).toBool()) + return; + + const int size = qMax(7, qMin(cell.height() / 3, 10)); + const QPoint centre(cell.left() + size, + cell.top() + subjectBandHeight(option) / 2 + + kRowPadding); + + QPolygon triangle; + if (option.state & QStyle::State_Open) { + triangle << QPoint(centre.x() - size / 2, centre.y() - size / 4) + << QPoint(centre.x() + size / 2, centre.y() - size / 4) + << QPoint(centre.x(), centre.y() + size / 2); + } else { + triangle << QPoint(centre.x() - size / 4, centre.y() - size / 2) + << QPoint(centre.x() + size / 2, centre.y()) + << QPoint(centre.x() - size / 4, centre.y() + size / 2); + } + + painter->save(); + painter->setRenderHint(QPainter::Antialiasing, true); + painter->setPen(Qt::NoPen); + // From the palette, so it survives a theme change, and undimmed: this + // is the only cue that a thread can be opened at all. + painter->setBrush(option.palette.color(QPalette::Text)); + painter->drawPolygon(triangle); + painter->restore(); + }; + // Room for the expander in front of whatever follows, on a thread row that + // has one. Reserved before either branch draws, so the chip and the bare + // subject are indented identically and a thread with replies does not sit + // a few pixels left of one without. + const bool hasReplies = + index.data(ThreadListModel::HasRepliesRole).toBool(); + const int expanderWidth = hasReplies ? kExpanderWidth : 0; + if (account.isEmpty()) { // No chip to draw, so the base class renders the text, confined to the // upper band: the lower one belongs to the row-wide pill strip that @@ -163,8 +208,10 @@ void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &optio QStyleOptionViewItem chrome = option; initStyleOption(&chrome, index); chrome.rect.setHeight(subjectBandHeight(option)); + chrome.rect.setLeft(chrome.rect.left() + expanderWidth); QStyledItemDelegate::paint(painter, chrome, index); + drawExpander(option.rect); return; } @@ -186,7 +233,7 @@ void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &optio const int textBandHeight = subjectBandHeight(option); const int textTop = option.rect.top() + kRowPadding; - const QRect chipRect(option.rect.left() + TagChip::kSpacing, + const QRect chipRect(option.rect.left() + expanderWidth + TagChip::kSpacing, textTop + (textBandHeight - chipSize.height()) / 2, chipSize.width(), chipSize.height()); @@ -234,6 +281,13 @@ void SubjectDelegate::paint(QPainter *painter, const QStyleOptionViewItem &optio rowMetrics.elidedText(index.data(Qt::DisplayRole).toString(), Qt::ElideRight, textRect.width())); painter->restore(); + + // Last, so the chrome fill above cannot cover it. BOTH branches of this + // function have to call it: a thread row with an account chip takes this + // one, and that is every row in the real application, so calling it only + // from the no-chip branch leaves the feature invisible in practice while + // still passing any test built on an untagged thread. + drawExpander(option.rect); } QSize SubjectDelegate::sizeHint(const QStyleOptionViewItem &option, diff --git a/src/tagchip.h b/src/tagchip.h index cc3b5de..3145358 100644 --- a/src/tagchip.h +++ b/src/tagchip.h @@ -92,6 +92,20 @@ public: /// Vertical breathing room above the subject and below the pill row. static constexpr int kRowPadding = 4; + /// How far a reply row is indented under its thread. + /// + /// Deliberately far wider than Qt's 20px default. A thread row carries an + /// account chip in front of its subject and a reply row does not, so a + /// reply's text starts about a chip's width to the LEFT of its thread's + /// before any indent is applied. 20px does not cover that, and the replies + /// come out looking flush or outdented; this has to beat a chip's width to + /// read as nesting at all. + static constexpr int kReplyIndent = 72; + + /// Horizontal room reserved in front of a thread's subject for the + /// expander glyph the delegate draws. + static constexpr int kExpanderWidth = 18; + /// The font the pill strip is drawn in: a size down from the row's own. /// /// At the same size the pills read as a second row of content competing diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index d9882dc..043e982 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -212,6 +212,10 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const return node.messageId; case MessageDepthRole: return node.depth; + case HasRepliesRole: + // A reply never has its own expander: nesting past the first level + // is drawn from depth, not from further parent-child structure. + return false; case ThreadIdRole: // A message row still belongs to a thread, and a caller that only // needs the containing thread must not have to walk up itself. @@ -274,6 +278,9 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (role == MessageDepthRole) return 0; + if (role == HasRepliesRole) + return hasChildren(index.siblingAtColumn(0)); + if (role == TagsRole) return thread.tags; diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index e39ade6..700b3a6 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -96,6 +96,13 @@ public: /// The message's reply depth, for the view's indentation. 1 for a /// direct reply, since depth 0 is the root row itself. MessageDepthRole, + + /// True when the row is a thread that has replies to show. + /// + /// Read by SubjectDelegate, which draws the expander itself: the + /// delegate cannot call hasChildren without the model, and the same + /// answer has to reach the cell that reserves room for the glyph. + HasRepliesRole, }; /// Row fill for a thread tagged `deleted`, and for one tagged `spam`. diff --git a/src/threadlistview.h b/src/threadlistview.h index 520d610..9e0da28 100644 --- a/src/threadlistview.h +++ b/src/threadlistview.h @@ -56,4 +56,5 @@ public: protected: void paintEvent(QPaintEvent *event) override; + }; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 7b6e55d..474d856 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -97,6 +97,7 @@ private slots: void aSelectedReadThreadIsNotDimmedIntoTheHighlight(); void thePillRowSpansTheWholeWidthNotOneColumn(); void childRowsAreIndentedUnderTheirThread(); + void aThreadWithRepliesDrawsAVisibleExpander(); void noTagStripIsPaintedUnderAMessageRow(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); @@ -627,7 +628,12 @@ void TestMainWindow::childRowsAreIndentedUnderTheirThread() auto *view = window.findChild(); QVERIFY2(view, "the thread list is not a QTreeView, so it cannot indent"); - model->appendBatch({ makeThread(QStringLiteral("t1"), {}) }); + // With an account tag, so the thread row draws the chip that a reply row + // does not. That asymmetry is the whole reason the indent has to be wide, + // and a test against an untagged thread never sees it. + model->appendBatch({ makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }) }); MessageNode first; first.messageId = QStringLiteral("m0@example.org"); @@ -668,6 +674,128 @@ void TestMainWindow::childRowsAreIndentedUnderTheirThread() QVERIFY2(view->visualRect(child).left() > view->visualRect(rootCell).left(), "the reply is not indented relative to its thread"); + + // The geometry being indented is NOT the same as the reply LOOKING + // indented, and asserting only the former shipped a build with no visible + // nesting at all. A thread row draws an account chip before its subject and + // a reply row does not, so the reply's text starts about a chip's width to + // the left of the thread's; at Qt's default 20px indent that difference + // swallows the shift entirely. + // + // So the real property: where the TEXT lands. The reply's subject must + // begin to the right of the thread's, which is what the eye reads as + // nesting. + const int chipWidth = + TagChip::sizeFor(QFontMetrics(view->font()), + model->data(rootCell, ThreadListModel::AccountLabelRole) + .toString()).width(); + QVERIFY2(chipWidth > 0, + "the thread row has no account chip, so this test cannot measure " + "the offset it is meant to compensate for"); + + const int threadTextLeft = view->visualRect(rootCell).left() + chipWidth; + QVERIFY2(view->visualRect(child).left() > threadTextLeft, + qPrintable(QStringLiteral("the reply's text starts at x=%1, not " + "right of the thread's text at x=%2: the " + "indent does not beat the account chip " + "and the nesting is invisible") + .arg(view->visualRect(child).left()) + .arg(threadTextLeft))); +} + +void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() +{ + // The expander is the ONLY thing saying a thread can be opened, and it took + // four wrong attempts to get on screen, each of which looked correct in + // code: + // + // - QTreeView::drawBranches, the documented hook, runs BEFORE the row's + // cells, so with the expander on a content column the delegate's own + // background paints over it. A 60-pixel triangle survived as 8. + // - Sizing it from the row rather than the branch rect put most of it + // outside that rect. + // - Moving it into the delegate but calling it from only one of the two + // branches left every real row without one, since every real row has an + // account chip and takes the other branch. + // + // None of those is visible to a test that asserts on geometry or on model + // roles, so this one counts painted pixels of the palette colour the glyph + // is drawn in. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + // Two threads: one with replies, one without. The second is the control, + // and without it a test that counts text pixels would pass on any row. + ThreadSummary withReplies = makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + withReplies.totalCount = 3; + ThreadSummary lone = makeThread( + QStringLiteral("t2"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + lone.totalCount = 1; + model->appendBatch({ withReplies, lone }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + QApplication::processEvents(); + + const QModelIndex first = + model->index(0, ThreadListModel::SubjectColumn, QModelIndex()); + const QModelIndex second = + model->index(1, ThreadListModel::SubjectColumn, QModelIndex()); + + // Guards: both rows on screen, and the model agreeing about which has + // replies. Without these a zero count could mean anything. + QVERIFY2(view->visualRect(first).height() > 0, "the first row is not drawn"); + QVERIFY2(view->visualRect(second).height() > 0, + "the control row is not drawn"); + QVERIFY(model->data(first, ThreadListModel::HasRepliesRole).toBool()); + QVERIFY(!model->data(second, ThreadListModel::HasRepliesRole).toBool()); + + QImage shot(view->viewport()->size(), QImage::Format_ARGB32); + shot.fill(Qt::transparent); + view->viewport()->render(&shot); + + // The exact colour the glyph is filled with, matched exactly rather than by + // a brightness threshold, which would count antialiased subject text. + const QRgb glyph = view->palette().color(QPalette::Text).rgb(); + + // Only the strip in front of the subject text, so the subject's own glyphs + // cannot be counted. kExpanderWidth is the room the delegate reserves. + const auto countGlyphPixels = [&](const QModelIndex &index) { + const QRect rect = view->visualRect(index); + int found = 0; + for (int y = rect.top(); y < qMin(rect.bottom(), shot.height()); ++y) { + for (int x = rect.left(); + x < qMin(rect.left() + SubjectDelegate::kExpanderWidth, + shot.width()); + ++x) { + if ((shot.pixel(x, y) | 0xff000000) == (glyph | 0xff000000)) + ++found; + } + } + return found; + }; + + const int drawn = countGlyphPixels(first); + const int control = countGlyphPixels(second); + + QVERIFY2(drawn > 12, + qPrintable(QStringLiteral("only %1 expander pixels: the glyph is " + "clipped or painted over, which is how " + "it shipped as an invisible dot") + .arg(drawn))); + + // The control must have none, or the count above is measuring something + // every row draws. + QCOMPARE(control, 0); } void TestMainWindow::noTagStripIsPaintedUnderAMessageRow() -- cgit v1.2.3 From 1304ecf7c683a7874b8f571433379caf72e0483b Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 11:07:03 +0200 Subject: feat(ui): mark replies with a thread line, a tint and dimmer text Indentation alone still read as a table, which was the user's original complaint about the whole item. Three cues now say the rows belong to the thread above them: a spine down the left of the expanded block with a stub out to each reply, a background tint, and text a size down and undimmed only when unread. Both colours are mixed from the palette rather than fixed, the same rule readColour follows: a tint that reads as grouping on a light theme is invisible or muddy on a dark one. The tint is deliberately near the threshold of noticing, since it sits beside the deleted and spam fills, which carry real meaning and must stay the loudest thing in the list. The spine is accumulated across the visible reply rows and drawn once after the loop. Drawn per row it left a gap at every row boundary and read as a column of dashes rather than as the structure holding the block together. Two bugs fixed here, both mine, both from the previous commit: Clicking the expander did nothing. setRootIsDecorated(false), needed to stop the style painting its own indicator under ours, also removed the style's hit area, so the glyph rendered perfectly and was inert. ThreadListView handles the press itself now, over the strip the delegate reserves, leaving the rest of the subject cell to select the row. Every click then expanded rather than toggling, because isExpanded and setExpanded are keyed on column 0 and were being asked about the subject-column index, which always answers false. Visible, clickable and toggling are three separate properties and a test for one passes against the other two being broken: the pixel test proved the triangle was drawn while it could not be clicked, and the first click test proved it opened while it could never close. The test now clicks twice and asserts open then closed. replyRowsKeepTheirTextUnderTheThreadLine covers the other trap. paintEvent runs AFTER the cells, so the first version of the tint filled the whole reply row and erased the sender and subject the delegate had just drawn: zero surviving text pixels, a block of blank tinted rows. The fill and the stub stay in the band below the text, where the tag strip lives on thread rows. --- src/threadlistmodel.cpp | 64 +++++++++++++++++++++++- src/threadlistmodel.h | 11 +++++ src/threadlistview.cpp | 109 ++++++++++++++++++++++++++++++++++++++-- src/threadlistview.h | 10 ++++ tests/test_mainwindow.cpp | 123 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 311 insertions(+), 6 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 043e982..f9efee7 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -75,6 +75,46 @@ QString ThreadListModel::flagGlyph() return glyph; } +QColor ThreadListModel::replyBackground() +{ + // Mixed from the palette rather than fixed, for the same reason as + // readColour: a tint that reads as "grouped" on a light theme is either + // invisible or muddy on a dark one. + // + // Toward Text rather than toward a hue, so it darkens on a light theme and + // lightens on a dark one without picking a colour that means something + // else. 0.07 is deliberately near the threshold of noticing: it is a + // grouping cue sitting beside the deleted and spam fills, which carry + // actual meaning and must stay the loudest thing in the list. + const QPalette palette = QGuiApplication::palette(); + const QColor base = palette.color(QPalette::Base); + const QColor text = palette.color(QPalette::Text); + + constexpr qreal kWeight = 0.07; + const qreal inverse = 1.0 - kWeight; + return QColor::fromRgbF( + text.redF() * kWeight + base.redF() * inverse, + text.greenF() * kWeight + base.greenF() * inverse, + text.blueF() * kWeight + base.blueF() * inverse); +} + +QColor ThreadListModel::threadLineColour() +{ + // Stronger than the tint, weaker than the text: the line is structure, so + // it has to be followable down a long expansion without competing with the + // senders beside it. + const QPalette palette = QGuiApplication::palette(); + const QColor base = palette.color(QPalette::Base); + const QColor text = palette.color(QPalette::Text); + + constexpr qreal kWeight = 0.35; + const qreal inverse = 1.0 - kWeight; + return QColor::fromRgbF( + text.redF() * kWeight + base.redF() * inverse, + text.greenF() * kWeight + base.greenF() * inverse, + text.blueF() * kWeight + base.blueF() * inverse); +} + QColor ThreadListModel::readColour() { // Derived from the palette, never hardcoded: a fixed grey that reads as @@ -248,9 +288,29 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const default: return {}; } + case Qt::BackgroundRole: + // Tinted, so an expanded thread reads as one block rather than as + // more table rows. Applied per cell here; ThreadListView fills the + // same colour across the strip's band so the row does not end up + // half tinted. + return replyBackground(); + case Qt::FontRole: { + // A size down from the thread rows, so a thread reads as the + // heading and its replies as the contents. Never bold: an unread + // reply is still subordinate to the thread it belongs to, and the + // thread row above already carries the unread cue for the whole + // conversation. + QFont font = QGuiApplication::font(); + if (font.pointSize() > 0) + font.setPointSize(qMax(6, font.pointSize() - 1)); + else if (font.pixelSize() > 0) + font.setPixelSize(qMax(8, font.pixelSize() - 2)); + return font; + } case Qt::ForegroundRole: - // Same rule as a thread row: read recedes, unread stays at the - // palette's own colour. + // Dimmed whether read or not, for the same reason as the font: a + // reply is subordinate content. An unread one is left undimmed so + // it can still be found. return node.isUnread() ? QVariant() : QVariant(readColour()); default: return {}; diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 700b3a6..c56e80a 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -120,6 +120,17 @@ public: static QColor deletedColour(); static QColor spamColour(); + /// Background for a reply row, so an expanded thread reads as one block + /// rather than as more table rows. + /// + /// Derived from the palette and deliberately subtle: it marks a grouping, + /// and a tint strong enough to notice on its own would compete with the + /// deleted and spam row colours, which carry real meaning. + static QColor replyBackground(); + + /// The line drawn down the left of an expanded thread's replies. + static QColor threadLineColour(); + /// The dimmed text colour a READ thread carries. /// /// Unread rows are left at the palette's own colour and read ones recede, diff --git a/src/threadlistview.cpp b/src/threadlistview.cpp index ebca4cc..1ffec04 100644 --- a/src/threadlistview.cpp +++ b/src/threadlistview.cpp @@ -21,10 +21,44 @@ #include "tagchip.h" #include "threadlistmodel.h" +#include #include #include #include +void ThreadListView::mousePressEvent(QMouseEvent *event) +{ + const QModelIndex index = indexAt(event->pos()); + + // Only a thread row, only the subject column, only the strip the delegate + // reserved for the glyph. Anything wider would swallow clicks meant to + // select the row, which is what the rest of the subject cell is for. + if (event->button() == Qt::LeftButton && index.isValid() + && !index.parent().isValid() + && index.column() == ThreadListModel::SubjectColumn + && index.data(ThreadListModel::HasRepliesRole).toBool()) { + + const QRect rect = visualRect(index); + if (event->pos().x() >= rect.left() + && event->pos().x() < rect.left() + SubjectDelegate::kExpanderWidth) { + // Column 0, not the clicked index. Expansion state belongs to the + // ROW, and QTreeView keys it on the first column: asking + // isExpanded() about the subject-column index always answers false, + // so every click expanded again instead of toggling. + const QModelIndex row = index.siblingAtColumn(0); + setExpanded(row, !isExpanded(row)); + + // Swallowed, so the click that opened a thread does not also load + // it into the message pane: expanding is a request to see the + // thread's shape, not to read it. + event->accept(); + return; + } + } + + QTreeView::mousePressEvent(event); +} + void ThreadListView::paintEvent(QPaintEvent *event) { QTreeView::paintEvent(event); @@ -55,16 +89,74 @@ void ThreadListView::paintEvent(QPaintEvent *event) // the same one. int visualRow = 0; + // The spine's extent, collected across the reply rows and drawn once after + // the loop. Per-row segments leave a gap at every row boundary and read as + // a column of dashes rather than as one line. + int spineX = -1; + int spineTop = std::numeric_limits::max(); + int spineBottom = std::numeric_limits::min(); + for (; walk.isValid(); walk = indexBelow(walk), ++visualRow) { const QRect rowRect = visualRect(walk); if (rowRect.top() > viewport()->height()) break; - // No strip under a message row. The strip carries the THREAD's tags, so - // one under each reply would stripe the list and repeat identical tags - // down the whole expansion. - if (walk.parent().isValid()) + // A message row: no tag strip, but it does get the band filled to its + // own tint and a thread line down its left. + // + // The band has to be filled here for the same reason a thread row's is. + // The cells paint the tint per cell, so nothing covers the width to the + // right of the last column or the lower band the strip normally + // occupies, and an untouched reply row comes out tinted across its text + // and bare underneath it. + if (walk.parent().isValid()) { + // Only the band BELOW the text, never the whole row. paintEvent + // runs after the cells, so filling the row's full height paints + // over the sender and subject the delegate just drew: measured at + // zero surviving text pixels, a block of blank tinted rows. + const int bandTop = rowRect.top() + SubjectDelegate::kRowPadding + + rowMetrics.height(); + const QRect band(columnViewportPosition(ThreadListModel::DateColumn), + bandTop, + viewport()->width() + - columnViewportPosition( + ThreadListModel::DateColumn), + rowRect.bottom() - bandTop + 1); + + if (selectionModel() + && selectionModel()->isSelected( + walk.siblingAtColumn(ThreadListModel::SubjectColumn))) { + painter.fillRect(band, palette().brush(QPalette::Highlight)); + } else { + painter.fillRect(band, ThreadListModel::replyBackground()); + } + + // The spine is NOT drawn here. Drawing it per row leaves a gap + // wherever consecutive rows do not abut exactly, which is every row + // boundary once the rows carry padding: the result reads as a + // column of dashes rather than as one line. It is drawn as a single + // continuous run after this loop, from the collected extents below. + const int subjectLeft = + columnViewportPosition(ThreadListModel::SubjectColumn); + const int lineX = subjectLeft + SubjectDelegate::kExpanderWidth / 2; + + if (spineX < 0) + spineX = lineX; + spineTop = qMin(spineTop, rowRect.top()); + spineBottom = qMax(spineBottom, rowRect.bottom() + 1); + + // A stub out to the row, so each reply is visibly attached to the + // spine rather than merely beside it. Drawn in the LOWER band, not + // at the row's midpoint: the midpoint crosses the sender text, and + // this paints after the cells. + const int stubY = bandTop + (rowRect.bottom() - bandTop) / 2; + painter.setPen(QPen(ThreadListModel::threadLineColour(), 2)); + painter.drawLine(lineX, stubY, + subjectLeft + SubjectDelegate::kReplyIndent + - TagChip::kSpacing * 2, + stubY); continue; + } const QModelIndex index = walk.siblingAtColumn( ThreadListModel::SubjectColumn); @@ -156,4 +248,13 @@ void ThreadListView::paintEvent(QPaintEvent *event) x += size.width() + TagChip::kSpacing; } } + + // One continuous spine over every visible reply row, drawn last so no + // cell fill can break it. Segments drawn per row left a dash at every row + // boundary, which read as a dotted decoration rather than as the structure + // holding the block together. + if (spineX >= 0 && spineBottom > spineTop) { + painter.setPen(QPen(ThreadListModel::threadLineColour(), 2)); + painter.drawLine(spineX, spineTop, spineX, spineBottom); + } } diff --git a/src/threadlistview.h b/src/threadlistview.h index 9e0da28..0ebe3ea 100644 --- a/src/threadlistview.h +++ b/src/threadlistview.h @@ -57,4 +57,14 @@ public: protected: void paintEvent(QPaintEvent *event) override; + /// Toggles a thread when its expander glyph is clicked. + /// + /// The view owns this because the glyph is drawn by SubjectDelegate and a + /// delegate gets no click of its own without an editor. Being VISIBLE and + /// being CLICKABLE are separate properties: setRootIsDecorated(false), + /// needed to stop the style drawing its own indicator underneath ours, also + /// removed the style's hit area, so the expander painted correctly and did + /// nothing at all. + void mousePressEvent(QMouseEvent *event) override; + }; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 474d856..cee32c6 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -99,6 +99,8 @@ private slots: void childRowsAreIndentedUnderTheirThread(); void aThreadWithRepliesDrawsAVisibleExpander(); void noTagStripIsPaintedUnderAMessageRow(); + void replyRowsKeepTheirTextUnderTheThreadLine(); + void clickingTheExpanderTogglesTheThread(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); void markAllReadDoesNothingWhenNothingIsUnread(); @@ -798,6 +800,127 @@ void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() QCOMPARE(control, 0); } +void TestMainWindow::clickingTheExpanderTogglesTheThread() +{ + // The glyph being VISIBLE and the glyph being CLICKABLE are separate + // properties, and the pixel test for the first passes happily against a + // triangle nothing can hit. Turning off rootIsDecorated to stop the style + // drawing its own dot under ours also removed the style's hit area, so the + // expander rendered perfectly and did nothing. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + ThreadSummary t = makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + t.totalCount = 3; + model->appendBatch({ t }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + QApplication::processEvents(); + + const QModelIndex root = model->index(0, 0, QModelIndex()); + const QModelIndex subject = + model->index(0, ThreadListModel::SubjectColumn, QModelIndex()); + const QRect rect = view->visualRect(subject); + + // Guards: the row is drawn, it claims to have replies, and it starts + // collapsed. Without the last one a toggle test can pass by doing nothing. + QVERIFY2(rect.height() > 0, "the thread row is not on screen"); + QVERIFY(model->data(subject, ThreadListModel::HasRepliesRole).toBool()); + QVERIFY(!view->isExpanded(root)); + + // Aimed at the glyph itself: the delegate reserves kExpanderWidth at the + // left of the subject cell and centres the triangle in it. + const QPoint hit(rect.left() + SubjectDelegate::kExpanderWidth / 2, + rect.top() + SubjectDelegate::kRowPadding + + QFontMetrics(view->font()).height() / 2); + + QTest::mouseClick(view->viewport(), Qt::LeftButton, Qt::NoModifier, hit); + QApplication::processEvents(); + QVERIFY2(view->isExpanded(root), + "clicking the expander did not open the thread"); + + QTest::mouseClick(view->viewport(), Qt::LeftButton, Qt::NoModifier, hit); + QApplication::processEvents(); + QVERIFY2(!view->isExpanded(root), + "clicking the expander again did not close the thread"); +} + +void TestMainWindow::replyRowsKeepTheirTextUnderTheThreadLine() +{ + // paintEvent runs AFTER the cells, so anything it fills across a reply row + // covers the text the delegate just drew. The tint and the thread line are + // both painted there, which makes this the obvious way to ship a block of + // blank rows. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + ThreadSummary t = makeThread( + QStringLiteral("t1"), + QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); + t.totalCount = 2; + model->appendBatch({ t }); + + MessageNode first; + first.messageId = QStringLiteral("m0@example.org"); + first.threadId = QStringLiteral("t1"); + first.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.from = QStringLiteral("A Replier "); + reply.subject = QStringLiteral("Re: a subject"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { first, reply }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex root = model->index(0, 0, QModelIndex()); + view->expand(root); + QApplication::processEvents(); + + const QModelIndex child = + model->index(0, ThreadListModel::AuthorsColumn, root); + const QRect rect = view->visualRect(child); + QVERIFY2(rect.height() > 0, "the reply row is not on screen"); + + QImage shot(view->viewport()->size(), QImage::Format_ARGB32); + shot.fill(Qt::transparent); + view->viewport()->render(&shot); + + // Count pixels in the sender cell that differ from the row's own tint. + // Text is the only thing that can produce them. + const QRgb tint = ThreadListModel::replyBackground().rgb() | 0xff000000; + int textPixels = 0; + for (int y = rect.top(); y < qMin(rect.bottom(), shot.height()); ++y) { + for (int x = rect.left(); x < qMin(rect.right(), shot.width()); ++x) { + if ((shot.pixel(x, y) | 0xff000000) != tint) + ++textPixels; + } + } + + QVERIFY2(textPixels > 20, + qPrintable(QStringLiteral("only %1 non-background pixels in the " + "reply's sender cell: the row was " + "painted over after its text was drawn") + .arg(textPixels))); +} + void TestMainWindow::noTagStripIsPaintedUnderAMessageRow() { // The strip is a row-wide band of the THREAD's tags. Painted under every -- cgit v1.2.3 From fbb60396d6e1e0f0da542c1945df6cd0ae8cb701 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 11:13:10 +0200 Subject: feat(ui): render a single message when its row is selected loadMessage queries by id: and returns one MessageRef, always matched, since the user asked for that message by clicking its row and a stub would answer the wrong question. An unknown id emits an empty vector rather than an error: a stale row after a reindex is an ordinary race, not a failure worth the status bar. The signal fires even when empty so the UI handler runs instead of waiting for a reply that never comes. The branch in onThreadSelected is placed BEFORE threadAt(), which is the whole trap. threadAt takes a top-level row number and a child's row number indexes its siblings, so handing a message row's number to it loads whichever thread happens to sit at that position. Mutation-checked: with the branch disabled the test reports thread 't1' for a reply belonging to 't2', a wrong answer plausible enough to survive review. m_currentMessageId and m_currentThreadId are mutually exclusive and each clears the other, so a queued reply can tell which kind of selection it belongs to. onMessageLoaded carries a third guard onThreadLoaded does not need: a reply landing after the selection moved to a thread row would render one message where the conversation belongs. No mark-read timer for a message row in this pass. Marking one message of a thread read is a per-message tag write and the pending-edit map is keyed by thread; item 28 is the record of what happens when that count goes wrong. --- src/mainwindow.cpp | 52 ++++++++++++++++++++++++++++++++ src/mainwindow.h | 9 ++++++ src/notmuchworker.cpp | 48 +++++++++++++++++++++++++++++ src/notmuchworker.h | 8 +++++ tests/test_mainwindow.cpp | 72 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_notmuchworker.cpp | 38 +++++++++++++++++++++++ 6 files changed, 227 insertions(+) (limited to 'tests/test_mainwindow.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 8bab2e8..d9aa57a 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -876,6 +876,7 @@ void MainWindow::registerActions() // thread straight back, which is the queued-reply race documented in // CLAUDE.md. m_currentThreadId.clear(); + m_currentMessageId.clear(); m_messageView->clear(); showPlaceholderPane(); m_markReadTimer->stop(); @@ -909,6 +910,7 @@ void MainWindow::registerActions() m_threadView->setCurrentIndex(QModelIndex()); m_currentThreadId.clear(); + m_currentMessageId.clear(); m_messageView->clear(); showPlaceholderPane(); m_markReadTimer->stop(); @@ -1321,6 +1323,8 @@ void MainWindow::wireWorker() this, &MainWindow::onQueryFinished); connect(m_worker, &NotmuchWorker::threadTreeLoaded, this, &MainWindow::onThreadTreeLoaded); + connect(m_worker, &NotmuchWorker::messageLoaded, + this, &MainWindow::onMessageLoaded); connect(m_worker, &NotmuchWorker::threadLoaded, this, &MainWindow::onThreadLoaded); connect(m_worker, &NotmuchWorker::errorOccurred, @@ -1644,6 +1648,7 @@ void MainWindow::onSelectionChanged() m_markReadTimer->stop(); m_markReadThreadId.clear(); m_currentThreadId.clear(); + m_currentMessageId.clear(); m_messageView->clear(); showPlaceholderPane(); } @@ -1675,12 +1680,39 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, m_markReadTimer->stop(); m_markReadThreadId.clear(); m_currentThreadId.clear(); + m_currentMessageId.clear(); m_messageView->clear(); showPlaceholderPane(); return; } + // A message row renders that message ALONE. Checked before threadAt(), + // which takes a top-level row number: a child's row number indexes its + // siblings, so passing it here would silently load whichever thread happens + // to sit at that position in the list. + if (m_model->isMessageRow(current)) { + const MessageNode node = m_model->messageAt(current); + if (node.messageId.isEmpty()) + return; + + // No mark-read timer for a message row in this pass. Marking one + // message of a thread read is a per-message tag write, and the + // pending-edit map is keyed by thread; item 28 is the record of what + // happens when that count goes wrong. + m_markReadTimer->stop(); + m_markReadThreadId.clear(); + + m_currentThreadId.clear(); + m_currentMessageId = node.messageId; + m_messageView->setTags(node.tags); + QMetaObject::invokeMethod(m_worker, "loadMessage", Qt::QueuedConnection, + Q_ARG(QString, node.messageId), + Q_ARG(quint64, m_generation)); + return; + } + const ThreadSummary thread = m_model->threadAt(current.row()); + m_currentMessageId.clear(); m_currentThreadId = thread.threadId; m_messageView->setTags(thread.tags); scheduleMarkRead(thread); @@ -1690,6 +1722,26 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, Q_ARG(quint64, m_generation)); } +void MainWindow::onMessageLoaded(const QVector &messages, + quint64 generation) +{ + // The same two guards onThreadLoaded carries. A stale generation means the + // query moved on, and a reply landing after the selection grew past one row + // would paint a message back over a deliberately blanked pane. + if (generation != m_generation || messages.isEmpty()) + return; + if (m_threadView->selectionModel()->selectedRows().size() > 1) + return; + + // A third guard this one needs and onThreadLoaded does not: a reply that + // lands after the selection moved to a THREAD row would render one message + // where the whole conversation belongs. + if (m_currentMessageId.isEmpty()) + return; + + onThreadLoaded(messages, generation); +} + void MainWindow::onThreadExpanded(const QModelIndex &index) { if (!index.isValid() || m_model->isMessageRow(index)) diff --git a/src/mainwindow.h b/src/mainwindow.h index 7014855..a4eca20 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -171,6 +171,10 @@ private slots: /// Fills in the expanded thread's message rows. void onThreadTreeLoaded(const QVector &nodes, quint64 generation); + + /// Renders the single message a message row asked for. + void onMessageLoaded(const QVector &messages, + quint64 generation); void onWorkerError(const QString &message); void onSyncFinished(bool success, int exitCode); @@ -514,6 +518,11 @@ private: QString m_lastQuery; QString m_currentThreadId; + /// The message a MESSAGE row is showing, empty whenever the pane holds a + /// whole thread. The two are mutually exclusive and each clears the other, + /// so a late reply can tell which kind of selection it belongs to. + QString m_currentMessageId; + /// The selection count last written to the status bar, so it can be taken /// back without clobbering a message some other action put there. QString m_selectionMessage; diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 47bbb62..7b999cf 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -335,6 +335,54 @@ void NotmuchWorker::loadThreadTree(const QString &threadId, emit threadTreeLoaded(nodes, generation); } +void NotmuchWorker::loadMessage(const QString &messageId, quint64 generation) +{ + if (!openReadOnly()) + return; + + // id: is an exact-match prefix, and the id is quoted because a message id + // can legitimately contain characters notmuch's parser would otherwise read + // as query syntax. + const QString query = QStringLiteral("id:\"%1\"").arg(messageId); + NmQuery nmQuery(notmuch_query_create(m_db, query.toUtf8().constData())); + if (!nmQuery) { + emit errorOccurred( + QStringLiteral("Cannot load message %1").arg(messageId)); + return; + } + + notmuch_messages_t *rawMessages = nullptr; + if (notmuch_query_search_messages(nmQuery.get(), &rawMessages) + != NOTMUCH_STATUS_SUCCESS) { + emit errorOccurred( + QStringLiteral("Cannot search message %1").arg(messageId)); + return; + } + NmMessages messages(rawMessages); + + QVector result; + if (notmuch_messages_valid(messages.get())) { + NmMessage message(notmuch_messages_get(messages.get())); + if (message) { + MessageRef ref; + ref.messageId = QString::fromUtf8( + notmuch_message_get_message_id(message.get())); + ref.filePath = QString::fromUtf8( + notmuch_message_get_filename(message.get())); + ref.tags = tagsOf(message.get()); + + // Always matched: the user asked for this message by clicking its + // row, so rendering it as a stub would answer the wrong question. + ref.matched = true; + result.append(ref); + } + } + + // Emitted even when empty, so the UI's handler runs and can decide what to + // do rather than waiting for a reply that never comes. + emit messageLoaded(result, generation); +} + void NotmuchWorker::applyTagsToThreads(const QStringList &threadIds, const QStringList &add, const QStringList &remove, diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 99cad04..df1799d 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -70,6 +70,13 @@ public slots: void loadThreadTree(const QString &threadId, const QString &matchQuery, quint64 generation); + /// Loads ONE message, for a message row selected in the list. + /// + /// Emits messageLoaded with an empty vector when the id is unknown, which + /// is an ordinary race after a reindex rather than an error worth + /// reporting. + void loadMessage(const QString &messageId, quint64 generation); + /// Applies tag changes. Opens the database read-write, applies, and closes /// immediately: notmuch's write lock is exclusive process-wide, so holding /// it would block the user's cron `notmuch new`. @@ -117,6 +124,7 @@ signals: void threadLoaded(const QVector &messages, quint64 generation); void threadTreeLoaded(const QVector &nodes, quint64 generation); + void messageLoaded(const QVector &messages, quint64 generation); void tagsApplied(const TagChange &change); void allTagsReady(const QStringList &tags, quint64 generation); diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index cee32c6..3a375d4 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -101,6 +101,7 @@ private slots: void noTagStripIsPaintedUnderAMessageRow(); void replyRowsKeepTheirTextUnderTheThreadLine(); void clickingTheExpanderTogglesTheThread(); + void selectingAMessageRowTargetsThatMessageNotItsThread(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); void markAllReadDoesNothingWhenNothingIsUnread(); @@ -800,6 +801,77 @@ void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() QCOMPARE(control, 0); } +void TestMainWindow::selectingAMessageRowTargetsThatMessageNotItsThread() +{ + // test_mainwindow has no worker (backlog item 36), so this cannot assert on + // what the pane renders. What it CAN assert is the decision the UI makes: + // a message row must stop tracking a current thread, or a reply arriving + // for either kind of selection cannot tell which one it belongs to. + // + // The trap this covers is specific. threadAt() takes a TOP-LEVEL row + // number, and a child's row number indexes its siblings, so handing a + // message row's number to it loads whichever thread happens to sit at that + // position in the list. Row 0 under a thread is a plausible-looking wrong + // answer, which is why the fixture puts the reply under the SECOND thread. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + ThreadSummary first = makeThread(QStringLiteral("t1"), {}); + ThreadSummary second = makeThread(QStringLiteral("t2"), {}); + second.totalCount = 2; + model->appendBatch({ first, second }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t2"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t2"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t2"), { root, reply }); + + window.resize(1400, 300); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + // Start on a thread row, so the transition to a message row is what is + // being observed rather than the initial state. + const QModelIndex threadRow = model->index(1, 0, QModelIndex()); + selectThreadRow(view, 1); + QApplication::processEvents(); + QCOMPARE(window.currentThreadId(), QStringLiteral("t2")); + + view->expand(threadRow); + QApplication::processEvents(); + + const QModelIndex messageRow = model->index(0, 0, threadRow); + QVERIFY(messageRow.isValid()); + QVERIFY2(model->isMessageRow(messageRow), + "the fixture did not produce a message row, so this test would " + "assert nothing about one"); + + view->selectionModel()->select( + messageRow, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(messageRow); + QApplication::processEvents(); + + // The thread is no longer what the pane is about. Left set, a late + // loadThread reply would repaint the whole conversation over the single + // message the user asked for. + QVERIFY2(window.currentThreadId().isEmpty(), + qPrintable(QStringLiteral("selecting a reply left the current " + "thread set to '%1': the pane is still " + "tracking the conversation") + .arg(window.currentThreadId()))); +} + void TestMainWindow::clickingTheExpanderTogglesTheThread() { // The glyph being VISIBLE and the glyph being CLICKABLE are separate diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index 0f47c55..c84e262 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -58,6 +58,8 @@ private slots: void requestAllTagsReturnsSortedTags(); void requestAllTagsOnUnreadableConfigEmitsError(); + void loadMessageReturnsOnlyThatMessage(); + void loadMessageOnAnUnknownIdReturnsNothing(); void loadThreadTreeReportsReplyDepth(); void loadThreadTreeCarriesTheFactsARowNeeds(); @@ -161,6 +163,42 @@ QStringList TestNotmuchWorker::tagsOf(const QString &messageId) return {}; } +void TestNotmuchWorker::loadMessageReturnsOnlyThatMessage() +{ + // a2 is a reply in a two-message thread. Selecting a reply row must render + // that message alone; loadThread would hand back the whole thread and the + // pane would show the conversation the user was trying to look inside. + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy loaded(&worker, &NotmuchWorker::messageLoaded); + worker.loadMessage(QStringLiteral("a2@example.org"), 1); + + QCOMPARE(loaded.count(), 1); + const auto messages = loaded.first().at(0).value>(); + + QCOMPARE(messages.size(), 1); + QCOMPARE(messages.first().messageId, QStringLiteral("a2@example.org")); + QVERIFY(!messages.first().filePath.isEmpty()); + + // matched, so the pane renders it expanded rather than as a stub. The user + // asked for this message by clicking it, which is as matched as it gets. + QVERIFY(messages.first().matched); +} + +void TestNotmuchWorker::loadMessageOnAnUnknownIdReturnsNothing() +{ + // Empty rather than an error: a stale row after a reindex is an ordinary + // race, not a failure worth a message in the status bar. + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy loaded(&worker, &NotmuchWorker::messageLoaded); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.loadMessage(QStringLiteral("nonexistent@example.org"), 1); + + QCOMPARE(loaded.count(), 1); + QVERIFY(loaded.first().at(0).value>().isEmpty()); + QCOMPARE(errors.count(), 0); +} + void TestNotmuchWorker::loadThreadTreeReportsReplyDepth() { // Thread A is a root plus one reply carrying In-Reply-To, which is what -- cgit v1.2.3 From 7c3648676e188344dabb24e084f91b2b47e87633 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 11:22:39 +0200 Subject: feat(ui): scope actions to the selected row kind and name it tagSelected resolved rows to threads with threadAt(index.row()), which is wrong for a message row: a child's row number indexes its siblings, so acting on a reply tagged whichever thread sat at that position in the list. It now routes through ThreadListModel::scopeFor, and a message row's change is sent as message ids down applyTags with its own MessageTagCommand for undo. MessageTagCommand stores message ids where ThreadTagCommand stores thread ids, and that difference is the point rather than an inconsistency: re-resolving the thread on undo would restore tags across every sibling the action never touched. sendMessageTagChange deliberately skips the optimistic model update. applyTagChange is keyed by thread and would repaint the whole row as though every message in it had changed, which for a one-message edit is a lie the user watches correct itself on the next query. It keeps the two things that are NOT optional: the edited-account set, resolved through the containing thread since the account is a property of the thread, and holding the edit when a sync holds notmuch's write lock, since the worker's read-write open blocks rather than failing. The scope is now stated before an action and after it, naming both the message count and whether a whole thread went. This is what stands in for the confirmation dialog CLAUDE.md rules out: undo is the safety net, and undo is only usable if the user can tell that something larger than they meant has just happened. Selecting a single message reports no count at all, since reading one message is not a bulk action. A mutation that routed message rows down the thread path SURVIVED the whole suite: undo depth and status text are identical either way while every sibling gets tagged. anActionOnAMessageRowTagsThatMessageNotTheThread exists because that gap was found, and asserts on the ids actually sent. anActionOnAThreadRowSaysItHitTheWholeThread reads the status bar BEFORE draining the event loop. This binary has no worker, backlog item 36, so the queued write reaches a database that has never heard of the thread and answers with errorOccurred, which overwrites the status bar: draining first asserts on that error and fails against correct code. --- src/mainwindow.cpp | 168 ++++++++++++++++++++++++++++++------- src/mainwindow.h | 65 ++++++++++++++ src/threadlistmodel.cpp | 11 +++ src/threadlistmodel.h | 5 ++ tests/test_mainwindow.cpp | 210 ++++++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 431 insertions(+), 28 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index d9aa57a..423aa7b 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1607,33 +1607,82 @@ void MainWindow::showThreadContextMenu(const QPoint &pos) void MainWindow::onSelectionChanged() { - const int selected = m_threadView->selectionModel()->selectedRows().size(); - if (selected <= 1) { - // Clearing the count here would wipe whatever the last action reported - // ("Archive: 3 threads"), which is the more useful message once the - // selection is gone. Only a count this function wrote is taken back. - if (m_statusLabel->text() == m_selectionMessage) - m_statusLabel->clear(); - m_selectionMessage.clear(); + const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); + const int selected = rows.size(); + if (selected == 1) { + // One row selected. With two kinds of row this is exactly where the + // scope became ambiguous: a thread root stands for every message in it, + // a message row for one, and the keypress looks identical. Naming it + // here is what this project does instead of a confirmation dialog, + // which CLAUDE.md rules out for tag mutations. + const ActionScope scope = m_model->scopeFor(rows); + + if (scope.wholeThread) { + m_selectionMessage = + tr("1 thread selected (%n message(s))", "", scope.messageCount); + m_statusLabel->setText(m_selectionMessage); + m_statusTimer->stop(); + m_transientMessage.clear(); + } else { + // Reading one message is not a bulk action and gets no count. + if (m_statusLabel->text() == m_selectionMessage) + m_statusLabel->clear(); + m_selectionMessage.clear(); + } - // Collapsing a multi-row selection back to one row has to load that - // row here, and cannot be left to onThreadSelected. currentRowChanged - // is emitted BEFORE the selection model is updated (verified against - // Qt 6.11), so when a click collapses three rows to one, that handler - // still sees three selected, takes the multi-select branch and returns - // without loading anything. Only this signal sees the real count. + // Collapsing a multi-row selection back to one row has to load that row + // here, and cannot be left to onThreadSelected: currentRowChanged is + // emitted BEFORE the selection model is updated (verified against + // Qt 6.11), so that handler still sees the old count and returns + // without loading anything. + // + // Compared per row kind. A message row's row number indexes its + // siblings, so threadAt() on one answers about an unrelated thread and + // the comparison below would be against the wrong id. const QModelIndex current = m_threadView->currentIndex(); - if (current.isValid() - && m_model->threadAt(current.row()).threadId != m_currentThreadId) { - onThreadSelected(current, QModelIndex()); + if (current.isValid()) { + const bool changed = + m_model->isMessageRow(current) + ? m_model->messageAt(current).messageId != m_currentMessageId + : m_model->threadAt(current.row()).threadId + != m_currentThreadId; + if (changed) + onThreadSelected(current, QModelIndex()); } return; } + if (selected < 1) { + // Nothing selected. Clearing unconditionally would wipe whatever the + // last action reported ("Archive: 3 threads"), which is the more useful + // message once the selection is gone, so only a count this function + // wrote is taken back. + if (m_statusLabel->text() == m_selectionMessage) + m_statusLabel->clear(); + m_selectionMessage.clear(); + return; + } + // The count is the part that actually teaches multi-select: it acknowledges // the selection while it is being built, rather than only after an action // has already been applied to it. - m_selectionMessage = tr("%n thread(s) selected", "", selected); + // + // Reported per row kind rather than as a bare row count, so a mixed + // selection says what it will really touch instead of calling three replies + // "3 threads". + const ActionScope scope = m_model->scopeFor(rows); + if (!scope.threadIds.isEmpty() && scope.messageIds.isEmpty()) { + m_selectionMessage = + tr("%n thread(s) selected (%1 messages)", "", scope.threadIds.size()) + .arg(scope.messageCount); + } else if (scope.threadIds.isEmpty()) { + m_selectionMessage = + tr("%n message(s) selected", "", scope.messageIds.size()); + } else { + m_selectionMessage = + tr("%n thread(s) and %1 message(s) selected", "", + scope.threadIds.size()).arg(scope.messageIds.size()); + } m_statusLabel->setText(m_selectionMessage); // State, not an event: it must persist while the selection does. Cancel any @@ -2457,20 +2506,83 @@ void MainWindow::tagSelected(const QStringList &add, const QStringList &remove, if (rows.isEmpty()) return; - QStringList threadIds; - threadIds.reserve(rows.size()); - for (const QModelIndex &index : rows) - threadIds.append(m_model->threadAt(index.row()).threadId); + // Resolved through the model rather than by mapping rows to threads here. + // A message row's row number indexes its siblings, so the old + // threadAt(index.row()) mapping silently acted on whichever thread sat at + // that position in the list. + const ActionScope scope = m_model->scopeFor(rows); + if (scope.isEmpty()) + return; - sendThreadTagChange(threadIds, add, remove, description); + if (!scope.threadIds.isEmpty()) { + sendThreadTagChange(scope.threadIds, add, remove, description); - // Pushed for undo. The inverse re-resolves the same threads, so it works - // whether or not those rows are still selected. - m_undoStack.push(new ThreadTagCommand(this, threadIds, add, remove, - description)); + // Pushed for undo. The inverse re-resolves the same threads, so it + // works whether or not those rows are still selected. + m_undoStack.push(new ThreadTagCommand(this, scope.threadIds, add, + remove, description)); + } + if (!scope.messageIds.isEmpty()) { + sendMessageTagChange(scope.messageIds, add, remove, description); + m_undoStack.push(new MessageTagCommand(this, scope.messageIds, add, + remove, description)); + } + + // The scope named after the fact, since the selection may well be gone by + // the time the user reads it. This is what stands in for the confirmation + // dialog CLAUDE.md rules out: undo is the safety net, and undo is only + // usable if the user can tell that something larger than they meant has + // just happened. showTransientStatus( - tr("%1: %n thread(s)", "", threadIds.size()).arg(description)); + scope.wholeThread + ? tr("%1: %n message(s) (whole thread)", "", scope.messageCount) + .arg(description) + : tr("%1: %n message(s)", "", scope.messageCount).arg(description)); +} + +void MainWindow::sendMessageTagChange(const QStringList &messageIds, + const QStringList &add, + const QStringList &remove, + const QString &description) +{ + if (messageIds.isEmpty()) + return; + + // No optimistic model update. applyTagChange is keyed by THREAD and would + // repaint the whole row as though every message in it had changed, which + // for a one-message edit is a lie the user would see and then watch + // silently correct itself on the next query. + + // The accounts this touches, resolved through the containing threads: the + // account is a property of the thread, and the sync needs the channel + // whether one message moved or seven. + for (const QString &messageId : messageIds) { + const QString threadId = m_model->threadIdForMessage(messageId); + if (threadId.isEmpty()) + continue; + for (const QString &key : m_model->accountKeysForThread(threadId)) + m_editedAccounts.insert(key); + } + + // Held during a sync for exactly the reason the thread path is: the + // worker's read-write open BLOCKS on notmuch's exclusive lock rather than + // failing, so sending now would freeze the worker for the rest of the run. + if (aSyncHoldsTheWriteLock()) { + m_heldEdits.append(HeldEdit{ + {}, TagChange{ messageIds, add, remove, description } }); + m_statusLabel->setText( + tr("A sync is running; your change will be applied when it " + "finishes.")); + updatePendingIndicator(); + return; + } + + m_pendingThreadIds.clear(); + m_pendingChange = TagChange{ messageIds, add, remove, description }; + + QMetaObject::invokeMethod(m_worker, "applyTags", Qt::QueuedConnection, + Q_ARG(TagChange, m_pendingChange)); } void MainWindow::sendThreadTagChange(const QStringList &threadIds, diff --git a/src/mainwindow.h b/src/mainwindow.h index a4eca20..6b9e557 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -129,6 +129,19 @@ public: /// command was pushed, which is what "this did nothing" has to assert. int undoDepthForTesting() const { return m_undoStack.count(); } + /// The ids the last tag change was sent for, and whether they were thread + /// ids or message ids. + /// + /// Exposed because the difference is invisible from outside otherwise: a + /// message row routed down the thread path produces the same undo depth and + /// the same status text while tagging every sibling in the thread. A + /// mutation that made exactly that change passed the whole suite. + QStringList pendingThreadIdsForTesting() const { return m_pendingThreadIds; } + QStringList pendingMessageIdsForTesting() const + { + return m_pendingChange.messageIds; + } + /// The generation a worker reply must carry to be accepted. /// /// A test seam: onQueryFinished() discards a reply whose generation is @@ -355,6 +368,13 @@ private: const QStringList &remove, const QString &description); + /// The same for individual MESSAGES, without touching the undo stack. + /// Both tagSelected() and MessageTagCommand route through this. + void sendMessageTagChange(const QStringList &messageIds, + const QStringList &add, + const QStringList &remove, + const QString &description); + /// Undoes the optimistic model update for a write the worker rejected. void revertPendingTagChange(); @@ -388,6 +408,7 @@ private: QVector m_heldEdits; friend class ThreadTagCommand; + friend class MessageTagCommand; Config m_config; KeyMap m_keyMap; @@ -629,3 +650,47 @@ private: QString m_description; bool m_firstRedo = true; }; + +/// Undo entry for a tag change over individual MESSAGES. +/// +/// Stores message ids, unlike ThreadTagCommand, and that difference is the +/// point rather than an inconsistency: a message row acts on one message, so +/// re-resolving its thread on undo would restore tags across every sibling the +/// action never touched. +class MessageTagCommand : public QUndoCommand +{ +public: + MessageTagCommand(MainWindow *window, const QStringList &messageIds, + const QStringList &add, const QStringList &remove, + const QString &description) + : QUndoCommand(description), m_window(window), + m_messageIds(messageIds), m_add(add), m_remove(remove), + m_description(description) {} + + /// The stack calls redo() when the command is pushed, by which point the + /// change has already been sent, so the first call is skipped. + void redo() override + { + if (m_firstRedo) { + m_firstRedo = false; + return; + } + m_window->sendMessageTagChange(m_messageIds, m_add, m_remove, + m_description); + } + + void undo() override + { + m_window->sendMessageTagChange( + m_messageIds, m_remove, m_add, + QStringLiteral("Undo %1").arg(m_description)); + } + +private: + MainWindow *m_window; + QStringList m_messageIds; + QStringList m_add; + QStringList m_remove; + QString m_description; + bool m_firstRedo = true; +}; diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index f9efee7..9a74041 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -594,6 +594,17 @@ MessageNode ThreadListModel::messageAt(const QModelIndex &index) const return children.at(index.row()); } +QString ThreadListModel::threadIdForMessage(const QString &messageId) const +{ + for (const ThreadNode &node : m_threads) { + for (const MessageNode &child : node.children) { + if (child.messageId == messageId) + return node.summary.threadId; + } + } + return {}; +} + ActionScope ThreadListModel::scopeFor(const QModelIndexList &selection) const { ActionScope scope; diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index c56e80a..b80488b 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -188,6 +188,11 @@ public: /// is not a message row. MessageNode messageAt(const QModelIndex &index) const; + /// The thread a loaded message row belongs to, or empty when no expanded + /// thread holds it. Only expanded threads have message rows at all, so a + /// message the user could select is always findable here. + QString threadIdForMessage(const QString &messageId) const; + /// Resolves a selection into what an action should touch. /// /// Mixed selections are honoured as given: a thread root and an unrelated diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 3a375d4..0a916c4 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -102,6 +102,10 @@ private slots: void replyRowsKeepTheirTextUnderTheThreadLine(); void clickingTheExpanderTogglesTheThread(); void selectingAMessageRowTargetsThatMessageNotItsThread(); + void selectingAThreadRowNamesHowManyMessagesItStandsFor(); + void selectingAMessageRowReportsNoBulkCount(); + void anActionOnAThreadRowSaysItHitTheWholeThread(); + void anActionOnAMessageRowTagsThatMessageNotTheThread(); void markAllReadIsDisabledUntilTheQueryFinishes(); void markAllReadActsOnEveryRowAndUndoesInOneStep(); void markAllReadDoesNothingWhenNothingIsUnread(); @@ -801,6 +805,212 @@ void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() QCOMPARE(control, 0); } +void TestMainWindow::selectingAThreadRowNamesHowManyMessagesItStandsFor() +{ + // With two kinds of row selectable, one selected row no longer says how + // much an action will touch. CLAUDE.md forbids a confirmation dialog for + // tag mutations, so the scope is made visible instead: this is the "before" + // half of that, and the count has to come from the thread's own total, not + // from whatever happens to be expanded. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(status); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 7; + model->appendBatch({ t }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + // Guard: nothing is expanded, so a count taken from the loaded children + // would read 0 and this test would be measuring the wrong source. + QCOMPARE(model->rowCount(model->index(0, 0, QModelIndex())), 0); + + selectThreadRow(view, 0); + QApplication::processEvents(); + + QVERIFY2(status->text().contains(QStringLiteral("7")), + qPrintable(QStringLiteral("the status bar says '%1', which does " + "not name the 7 messages the thread " + "stands for") + .arg(status->text()))); +} + +void TestMainWindow::selectingAMessageRowReportsNoBulkCount() +{ + // Reading one message is not a bulk action, so it gets no count. A message + // row reporting "1 thread selected" would be actively wrong about what an + // action would touch. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(status); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 3; + model->appendBatch({ t }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { root, reply }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex threadRow = model->index(0, 0, QModelIndex()); + view->expand(threadRow); + QApplication::processEvents(); + + const QModelIndex messageRow = model->index(0, 0, threadRow); + QVERIFY(model->isMessageRow(messageRow)); + + view->selectionModel()->select( + messageRow, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(messageRow); + QApplication::processEvents(); + + QVERIFY2(!status->text().contains(QStringLiteral("thread")), + qPrintable(QStringLiteral("a single message row reports '%1', " + "which claims a thread-wide scope it " + "does not have") + .arg(status->text()))); +} + +void TestMainWindow::anActionOnAThreadRowSaysItHitTheWholeThread() +{ + // The "after" half. Undo is the safety net this project chose over a + // confirmation dialog, and undo is only usable if the user can tell that + // something bigger than they intended just happened. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + auto *status = window.findChild(QStringLiteral("statusMessage")); + QVERIFY(status); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 7; + model->appendBatch({ t }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + selectThreadRow(view, 0); + QApplication::processEvents(); + + auto *archive = window.findChild(QStringLiteral("archive")); + QVERIFY2(archive, "no archive action to trigger"); + archive->trigger(); + + // Read BEFORE processEvents, deliberately. This binary has no worker + // (backlog item 36), so the queued applyTagsToThreads reaches a throwaway + // database that has never heard of thread t1 and answers with + // errorOccurred, which overwrites the status bar. Draining the event loop + // here would assert on that error rather than on the scope message, and + // the test would fail against correct code. + const QString message = status->text(); + + QVERIFY2(message.contains(QStringLiteral("7")), + qPrintable(QStringLiteral("after archiving a 7-message thread the " + "status bar says '%1', which does not " + "say how much was touched") + .arg(message))); + + // And it must say the whole thread went, not merely how many messages: the + // count alone does not distinguish "7 messages you picked" from "7 messages + // because you picked their thread". + QVERIFY2(message.contains(QStringLiteral("whole thread")), + qPrintable(QStringLiteral("the status bar says '%1', which does " + "not say the action took the whole " + "thread") + .arg(message))); +} + +void TestMainWindow::anActionOnAMessageRowTagsThatMessageNotTheThread() +{ + // The routing itself, which nothing else here can see. A message row sent + // down the THREAD path produces the same undo depth and the same status + // text while tagging every sibling in the conversation: a mutation that did + // exactly that passed the entire suite, so this test exists because that + // gap was found rather than because the path looked risky. + const Config config; + MainWindow window(config); + + auto *model = window.findChild(); + QVERIFY(model); + auto *view = window.findChild(); + QVERIFY(view); + + ThreadSummary t = makeThread(QStringLiteral("t1"), {}); + t.totalCount = 3; + model->appendBatch({ t }); + + MessageNode root; + root.messageId = QStringLiteral("m0@example.org"); + root.threadId = QStringLiteral("t1"); + root.depth = 0; + MessageNode reply; + reply.messageId = QStringLiteral("m1@example.org"); + reply.threadId = QStringLiteral("t1"); + reply.depth = 1; + model->setThreadMessages(QStringLiteral("t1"), { root, reply }); + + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const QModelIndex threadRow = model->index(0, 0, QModelIndex()); + view->expand(threadRow); + QApplication::processEvents(); + + const QModelIndex messageRow = model->index(0, 0, threadRow); + QVERIFY(model->isMessageRow(messageRow)); + + view->selectionModel()->select( + messageRow, + QItemSelectionModel::ClearAndSelect | QItemSelectionModel::Rows); + view->setCurrentIndex(messageRow); + QApplication::processEvents(); + + auto *archive = window.findChild(QStringLiteral("archive")); + QVERIFY(archive); + archive->trigger(); + + // The change must carry the MESSAGE id and no thread id. Sent as a thread + // id it would archive the root and every other reply along with it. + QCOMPARE(window.pendingMessageIdsForTesting(), + QStringList{ QStringLiteral("m1@example.org") }); + QVERIFY2(window.pendingThreadIdsForTesting().isEmpty(), + qPrintable(QStringLiteral("the action was sent for thread(s) %1: a " + "message row must not tag its siblings") + .arg(window.pendingThreadIdsForTesting() + .join(QStringLiteral(", "))))); + + // And it is undoable, on its own terms rather than the thread's. + QCOMPARE(window.undoDepthForTesting(), 1); +} + void TestMainWindow::selectingAMessageRowTargetsThatMessageNotItsThread() { // test_mainwindow has no worker (backlog item 36), so this cannot assert on -- cgit v1.2.3 From 1335d3c7f891a22f61307d2d186483f56e8e558c Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:25:24 +0200 Subject: test(mainwindow): select the flagged-tag row through the tree helper theImportantActionStillWritesTheFlaggedTag arrived from master, where the thread list is a QTableView, onto a branch where item 20 made it a QTreeView. findChild returned null and the test failed on its first QVERIFY, before reaching anything it was written to check. Uses the branch's selectThreadRow helper, which is what every other test here already calls: QTreeView has no selectRow of its own. --- tests/test_mainwindow.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 0a916c4..740e7fa 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -3335,7 +3335,7 @@ void TestMainWindow::theImportantActionStillWritesTheFlaggedTag() auto *model = window.findChild(); QVERIFY(model); - auto *view = window.findChild(); + auto *view = window.findChild(); QVERIFY(view); model->appendBatch({ makeThread(QStringLiteral("t1"), @@ -3345,7 +3345,7 @@ void TestMainWindow::theImportantActionStillWritesTheFlaggedTag() // below would pass against an action that did nothing at all. QVERIFY(!model->threadAt(0).isFlagged()); - view->selectRow(0); + selectThreadRow(view, 0); auto *action = window.findChild(QStringLiteral("flag")); QVERIFY(action); -- cgit v1.2.3 From 320189af0baa392bff7ed89fe17ac5d06455454d Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:46:31 +0200 Subject: refactor(view): stop the view painting, and hit-test the reply count ThreadListView::paintEvent and its band arithmetic are deleted. The view existed to paint a strip across five columns; with one column and one delegate painting the whole card there is nothing to span, and the two faults that arithmetic kept producing go with it: a deleted row cut in half, and every other row showing a bare stripe. What survives is the expander hit-test, because a delegate gets no click of its own without an editor. It now asks CardDelegate for the rect rather than recomputing it, so the drawn target and the clickable one cannot drift. The siblingAtColumn(0) dance is gone: with one column, the index already is column 0. Item 51 closes here rather than separately. A card is exactly viewport width, so the view has no horizontal scroll range for a click to scroll into, and the test asserts that directly. Two rendering tests had to change how they measure, not merely which index they name. The indent test asserted on visualRect, which now reports the SAME rect for a thread and its reply by design, since setIndentation(0) leaves the indent to CardLayout: it reads contentLeft off the layout instead. And the expander test reported zero ink over a card the delegate paints 2183 pixels into, because viewport()->render() returned a blank image, exactly as CLAUDE.md warns; it now paints the delegate into an image directly and carries a guard proving the probe can see ink before it reports finding none. Both were mutation-checked. Two tests are deleted rather than ported. Both existed to prove the row-wide strip spanned columns a delegate could not reach, which is a property of code that no longer exists. --- src/mainwindow.cpp | 121 +++-------- src/threadlistview.cpp | 243 ++------------------- src/threadlistview.h | 52 ++--- tests/test_mainwindow.cpp | 464 ++++++++++++++--------------------------- tests/test_threadlistmodel.cpp | 224 ++++++++------------ 5 files changed, 311 insertions(+), 793 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 423aa7b..30148d4 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -42,7 +42,7 @@ #include #include #include -#include +#include #include #include #include @@ -52,6 +52,8 @@ #include "mimeparser.h" #include "notmuchworker.h" #include "querycompleter.h" +#include "carddelegate.h" +#include "cardlayout.h" #include "tagchip.h" #include "tagdialog.h" #include "threadlistmodel.h" @@ -157,20 +159,9 @@ void MainWindow::restoreUiState() m_splitter->restoreState(splitter); } - // A header blob saved against a different set of columns must be - // discarded, not restored. QHeaderView::restoreState() returns TRUE for a - // blob with fewer sections than the model and applies the old widths to - // the wrong columns: adding the attachment column in front shifted every - // saved width one place right, silently mangling the layout with no error - // to detect it by (verified on Qt 6.11). The column count is stored - // alongside and the blob is only used when it still matches. - const QByteArray header = state.value(QStringLiteral("threadlist/header")) - .toByteArray(); - const int savedColumns = - state.value(QStringLiteral("threadlist/columns")).toInt(); - if (!header.isEmpty() && savedColumns == ThreadListModel::ColumnCount) { - m_threadView->header()->restoreState(header); - } + // No thread-list header state is read. The pane is one column drawn whole + // by CardDelegate, so there are no widths to restore; a blob saved by an + // older version is simply ignored (item 53's Upgrading note). // The config value is the starting point for a profile that has never // zoomed; once the user does, the state file is what they last had. @@ -187,11 +178,6 @@ void MainWindow::saveUiState() const state.setValue(QStringLiteral("window/geometry"), saveGeometry()); state.setValue(QStringLiteral("window/state"), saveState()); state.setValue(QStringLiteral("window/splitter"), m_splitter->saveState()); - state.setValue(QStringLiteral("threadlist/header"), - m_threadView->header()->saveState()); - // Guards the blob above: see restoreUiState(). - state.setValue(QStringLiteral("threadlist/columns"), - int(ThreadListModel::ColumnCount)); state.setValue(QStringLiteral("message/zoom"), m_messageView->zoomFactor()); } @@ -543,83 +529,42 @@ void MainWindow::buildUi() // delegate is confined to one column's rectangle. m_threadView = new ThreadListView(central); m_threadView->setModel(m_model); + m_threadView->setItemDelegate(new CardDelegate(this)); + m_threadView->setHeaderHidden(true); m_threadView->setSelectionBehavior(QAbstractItemView::SelectRows); m_threadView->setSelectionMode(QAbstractItemView::ExtendedSelection); - m_threadView->header()->setStretchLastSection(false); - // Every column Interactive, Subject included: Stretch and ResizeToContents - // both compute a width and discard the user's drag. Nothing absorbs spare - // width as a result, so the columns end where they end. - for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { - m_threadView->header()->setSectionResizeMode( - column, QHeaderView::Interactive); - } - // The expander goes on the subject column, not on column 0. Column 0 is the - // narrow attachment marker, and an expander there has no room: it pushes the - // paperclip out of a 28px column entirely. - m_threadView->setTreePosition(ThreadListModel::SubjectColumn); - - // No style-drawn branch decoration. SubjectDelegate draws the expander - // itself, because drawBranches runs BEFORE the row's cells and the - // delegate's own background paints straight over anything put there: a - // 60-pixel triangle survived as 8 pixels, indistinguishable from the - // near-invisible dot this replaces. Leaving both enabled would draw the - // theme's dot underneath the delegate's glyph. + // No style-drawn branch decoration. CardDelegate draws the expander itself, + // because drawBranches runs BEFORE the row's cells and the delegate's own + // background paints straight over anything put there: a 60-pixel triangle + // once survived as 8. Leaving both enabled would draw the theme's dot + // underneath the delegate's glyph. m_threadView->setRootIsDecorated(false); - // Wider than Qt's default 20px, and the reason is specific rather than - // aesthetic. A thread row carries an account chip in front of its subject - // and a reply row does not, so a reply's text already starts roughly a - // chip's width (~60px) to the LEFT of its thread's. At the default indent - // the 20px shift is swallowed by that difference and the replies read as - // flush with the thread, or even outdented. Verified against the running - // app, not assumed: visualRect reported a correct 20px indent while the - // rendered text showed none, because the geometry is indented and the - // delegate then lays the text out from its own left edge. - m_threadView->setIndentation(SubjectDelegate::kReplyIndent); - - // Two delegates, and the split is not cosmetic. RowStyleDelegate carries - // only the selection fix every column needs: the read/unread dimming - // arrives as a Qt::ForegroundRole, which Qt's painting prefers over the - // highlight, leaving a selected read row grey on the selection colour. - // - // SubjectDelegate adds the account chip and the tag pills, and must go on - // the subject column ALONE. It reads AccountLabelRole, a property of the - // row rather than of a cell, so installed view-wide it draws the chip into - // every column: tried once, and the list came out with a chip repeated - // four times per row. - m_threadView->setItemDelegate(new RowStyleDelegate(this)); - m_threadView->setItemDelegateForColumn(ThreadListModel::SubjectColumn, - new SubjectDelegate(this)); + // Zero, because CardLayout draws the indent itself. Qt's own indentation + // would shift the card's rect, and every rect on the card is measured from + // that rect's left edge, so the two would compound. + m_threadView->setIndentation(0); // One height for every row. A QTreeView has no vertical header to carry a - // default section size, so the height comes from uniformRowHeights plus the - // delegate's own sizeHint. uniformRowHeights is not merely an optimisation - // here: without it the tree measures every row separately and the tag strip, - // which is painted OUTSIDE any cell, is not accounted for in any of those - // measurements, so rows collapse to text height and the strip is clipped. + // default section size, so the height comes from uniformRowHeights plus + // CardDelegate::sizeHint. m_threadView->setUniformRowHeights(true); - // Widening a column past the viewport scrolls rather than squeezing the - // others. Per-pixel so the scroll does not jump a whole column at a time. - // Banding, so the eye can follow a row across four columns and a pill - // strip without losing it. The colour comes from the palette's - // AlternateBase, so it follows the desktop theme. + + // Banding, so the eye can follow a card across the pane. The colour comes + // from the palette's AlternateBase, so it follows the desktop theme. m_threadView->setAlternatingRowColors(true); - m_threadView->setHorizontalScrollBarPolicy(Qt::ScrollBarAsNeeded); - m_threadView->setHorizontalScrollMode(QAbstractItemView::ScrollPerPixel); - - // Starting widths only; a drag overrides them, and they are what the - // saved-widths item will persist. - // Without this the attachment column cannot be narrow at all: the default - // minimum section size is 58px on this platform, and setColumnWidth() - // clamps to it silently rather than reporting the smaller value back. - m_threadView->header()->setMinimumSectionSize(24); - m_threadView->setColumnWidth(ThreadListModel::AttachmentColumn, 28); - m_threadView->setColumnWidth(ThreadListModel::FlagColumn, 28); - m_threadView->setColumnWidth(ThreadListModel::DateColumn, 130); - m_threadView->setColumnWidth(ThreadListModel::AuthorsColumn, 180); - m_threadView->setColumnWidth(ThreadListModel::SubjectColumn, 520); + // A card is exactly viewport width, so there is nothing to scroll to + // sideways. Turning the bar off is what closes item 51: a click used to + // scroll the list horizontally, because the subject column was wider than + // the viewport and auto-scroll brought the clicked index fully into view. + m_threadView->setHorizontalScrollBarPolicy(Qt::ScrollBarAlwaysOff); + + // Scrolling a whole card at a time rather than a fraction of one, so a + // card is never left half above the top edge. + m_threadView->verticalScrollBar()->setSingleStep( + CardLayout::heightFor(m_threadView->font())); // Replies are loaded when a thread is expanded, not with the query. // Walking the reply tree of every thread in a 10k-thread result would cost @@ -1061,7 +1006,7 @@ void MainWindow::buildMenus() m_threadContextMenu->addAction(m_actions.value(QStringLiteral("select_all"))); m_threadView->setContextMenuPolicy(Qt::CustomContextMenu); - connect(m_threadView, &QTableView::customContextMenuRequested, + connect(m_threadView, &QWidget::customContextMenuRequested, this, &MainWindow::showThreadContextMenu); // The frequent subset only. A toolbar holding every action is as diff --git a/src/threadlistview.cpp b/src/threadlistview.cpp index 1ffec04..5a3ce77 100644 --- a/src/threadlistview.cpp +++ b/src/threadlistview.cpp @@ -18,39 +18,34 @@ #include "threadlistview.h" -#include "tagchip.h" +#include "carddelegate.h" #include "threadlistmodel.h" #include -#include -#include -#include void ThreadListView::mousePressEvent(QMouseEvent *event) { const QModelIndex index = indexAt(event->pos()); - // Only a thread row, only the subject column, only the strip the delegate - // reserved for the glyph. Anything wider would swallow clicks meant to - // select the row, which is what the rest of the subject cell is for. + // The reply count IS the expander. Anything outside its rect selects the + // card and opens it, which is what the rest of the card is for. if (event->button() == Qt::LeftButton && index.isValid() - && !index.parent().isValid() - && index.column() == ThreadListModel::SubjectColumn - && index.data(ThreadListModel::HasRepliesRole).toBool()) { - - const QRect rect = visualRect(index); - if (event->pos().x() >= rect.left() - && event->pos().x() < rect.left() + SubjectDelegate::kExpanderWidth) { - // Column 0, not the clicked index. Expansion state belongs to the - // ROW, and QTreeView keys it on the first column: asking - // isExpanded() about the subject-column index always answers false, - // so every click expanded again instead of toggling. - const QModelIndex row = index.siblingAtColumn(0); - setExpanded(row, !isExpanded(row)); - - // Swallowed, so the click that opened a thread does not also load - // it into the message pane: expanding is a request to see the - // thread's shape, not to read it. + && index.data(ThreadListModel::ReplyCountRole).toInt() > 0) { + + QStyleOptionViewItem option; + initViewItemOption(&option); + option.rect = visualRect(index); + // State_Open decides which way the glyph points, and the rect is the + // same either way, but pass it so the layout sees the true state. + if (isExpanded(index)) + option.state |= QStyle::State_Open; + + if (CardDelegate::expanderRectFor(option, index) + .contains(event->pos())) { + setExpanded(index, !isExpanded(index)); + // Swallowed, so expanding does not also load the thread into the + // message pane: it is a request to see the thread's shape, not to + // read it. event->accept(); return; } @@ -58,203 +53,3 @@ void ThreadListView::mousePressEvent(QMouseEvent *event) QTreeView::mousePressEvent(event); } - -void ThreadListView::paintEvent(QPaintEvent *event) -{ - QTreeView::paintEvent(event); - - if (!model()) - return; - - QPainter painter(viewport()); - - // Two fonts, deliberately. The row's own font fixes where the text band - // ends, and the pills are drawn a size smaller: at the same size they read - // as a second row of content competing with the subject, rather than as - // annotation beneath it. - const QFontMetrics rowMetrics(font()); - const QFont pillFont = SubjectDelegate::pillFont(font()); - const QFontMetrics metrics(pillFont); - painter.setFont(pillFont); - - // Only the rows actually on screen, walked by INDEX rather than by row - // number. A tree numbers rows per parent, so row 0 exists once per expanded - // thread and the old flat 0..N walk would paint the first thread's strip - // over every one of them. - QModelIndex walk = indexAt(QPoint(0, 0)); - - // Counts the rows actually painted, for the alternating colour. In a tree - // that has to follow VISUAL position: row 0 under three different threads - // is three different stripes, and using index.row() would give all three - // the same one. - int visualRow = 0; - - // The spine's extent, collected across the reply rows and drawn once after - // the loop. Per-row segments leave a gap at every row boundary and read as - // a column of dashes rather than as one line. - int spineX = -1; - int spineTop = std::numeric_limits::max(); - int spineBottom = std::numeric_limits::min(); - - for (; walk.isValid(); walk = indexBelow(walk), ++visualRow) { - const QRect rowRect = visualRect(walk); - if (rowRect.top() > viewport()->height()) - break; - - // A message row: no tag strip, but it does get the band filled to its - // own tint and a thread line down its left. - // - // The band has to be filled here for the same reason a thread row's is. - // The cells paint the tint per cell, so nothing covers the width to the - // right of the last column or the lower band the strip normally - // occupies, and an untouched reply row comes out tinted across its text - // and bare underneath it. - if (walk.parent().isValid()) { - // Only the band BELOW the text, never the whole row. paintEvent - // runs after the cells, so filling the row's full height paints - // over the sender and subject the delegate just drew: measured at - // zero surviving text pixels, a block of blank tinted rows. - const int bandTop = rowRect.top() + SubjectDelegate::kRowPadding - + rowMetrics.height(); - const QRect band(columnViewportPosition(ThreadListModel::DateColumn), - bandTop, - viewport()->width() - - columnViewportPosition( - ThreadListModel::DateColumn), - rowRect.bottom() - bandTop + 1); - - if (selectionModel() - && selectionModel()->isSelected( - walk.siblingAtColumn(ThreadListModel::SubjectColumn))) { - painter.fillRect(band, palette().brush(QPalette::Highlight)); - } else { - painter.fillRect(band, ThreadListModel::replyBackground()); - } - - // The spine is NOT drawn here. Drawing it per row leaves a gap - // wherever consecutive rows do not abut exactly, which is every row - // boundary once the rows carry padding: the result reads as a - // column of dashes rather than as one line. It is drawn as a single - // continuous run after this loop, from the collected extents below. - const int subjectLeft = - columnViewportPosition(ThreadListModel::SubjectColumn); - const int lineX = subjectLeft + SubjectDelegate::kExpanderWidth / 2; - - if (spineX < 0) - spineX = lineX; - spineTop = qMin(spineTop, rowRect.top()); - spineBottom = qMax(spineBottom, rowRect.bottom() + 1); - - // A stub out to the row, so each reply is visibly attached to the - // spine rather than merely beside it. Drawn in the LOWER band, not - // at the row's midpoint: the midpoint crosses the sender text, and - // this paints after the cells. - const int stubY = bandTop + (rowRect.bottom() - bandTop) / 2; - painter.setPen(QPen(ThreadListModel::threadLineColour(), 2)); - painter.drawLine(lineX, stubY, - subjectLeft + SubjectDelegate::kReplyIndent - - TagChip::kSpacing * 2, - stubY); - continue; - } - - const QModelIndex index = walk.siblingAtColumn( - ThreadListModel::SubjectColumn); - - const int rowTop = rowRect.top(); - const int height = rowRect.height(); - if (height <= 0) - continue; - - // The strip's band, filled to match the row before anything is drawn - // on it. - // - // A QTableView paints alternating colours and the selection PER CELL, - // so nothing paints the width to the right of the last column, and - // nothing paints the band at all where a column does not reach. Left - // unfilled, an alternate-coloured or selected row shows the viewport - // background in a strip across its lower half. Filled for every - // visible row, not only tagged ones, since an untagged row has the - // same band to account for. - // Starting at the date column, NOT at the viewport edge. The two - // leading columns hold the attachment and flag glyphs, centred in the - // full row height, so a band drawn over them cuts those glyphs in half. - const int bandLeft = - columnViewportPosition(ThreadListModel::DateColumn); - const QRect band(bandLeft, rowTop + SubjectDelegate::kRowPadding - + rowMetrics.height(), - viewport()->width() - bandLeft, - height - SubjectDelegate::kRowPadding - - rowMetrics.height()); - - // The model's own row colour wins where it has one: a deleted or spam - // thread fills its cells with crimson or orange, and painting the base - // colour across the band beneath them would cut the row in half. - const QVariant background = index.data(Qt::BackgroundRole); - - if (background.isValid()) - painter.fillRect(band, background.value()); - // isSelected on the index, not isRowSelected(int): a QTreeView has no - // such overload, and a row number alone cannot name a row in a tree - // anyway since it is only unique under one parent. - else if (selectionModel() && selectionModel()->isSelected(index)) - painter.fillRect(band, palette().brush(QPalette::Highlight)); - else if (alternatingRowColors() && (visualRow % 2)) - painter.fillRect(band, palette().brush(QPalette::AlternateBase)); - else - painter.fillRect(band, palette().brush(QPalette::Base)); - - const QStringList tags = - index.data(ThreadListModel::PillTagsRole).toStringList(); - if (tags.isEmpty()) - continue; - - const QVariantList colours = - index.data(ThreadListModel::PillColoursRole).toList(); - - // The band the cells leave free, below the text they draw in the - // upper one. Measured from SubjectDelegate by both sides, so neither - // can drift into the other's half. The row's own font metrics set the - // text band; the strip's smaller font must not be used for it, or the - // pills ride up over the date and sender. - const int top = rowTop + SubjectDelegate::kRowPadding - + rowMetrics.height() + TagChip::kSpacing; - - // Aligned with the first text column rather than the viewport edge: - // the two leading columns are narrow markers for the attachment and - // flag glyphs, and a strip starting at x=0 paints straight over them. - // Indented past the date column's own left edge rather than flush with - // it: a chip starting exactly where the column does reads as part of - // the column rather than as a strip laid under the row. - int x = columnViewportPosition(ThreadListModel::DateColumn) - + TagChip::kSpacing * 2; - const int available = viewport()->width() - TagChip::kSpacing; - - for (int i = 0; i < tags.size(); ++i) { - const QSize size = TagChip::sizeFor(metrics, tags.at(i)); - - // Stop rather than wrap or elide. A row that grew to fit its tags - // would break the uniform height the list depends on, and half a - // chip reads as a rendering fault. - if (x + size.width() > available) - break; - - const QColor colour = i < colours.size() - ? colours.at(i).value() - : QColor(0x55, 0x55, 0x5f); - - TagChip::paint(&painter, QRect(x, top, size.width(), size.height()), - tags.at(i), colour); - x += size.width() + TagChip::kSpacing; - } - } - - // One continuous spine over every visible reply row, drawn last so no - // cell fill can break it. Segments drawn per row left a dash at every row - // boundary, which read as a dotted decoration rather than as the structure - // holding the block together. - if (spineX >= 0 && spineBottom > spineTop) { - painter.setPen(QPen(ThreadListModel::threadLineColour(), 2)); - painter.drawLine(spineX, spineTop, spineX, spineBottom); - } -} diff --git a/src/threadlistview.h b/src/threadlistview.h index 0ebe3ea..3215153 100644 --- a/src/threadlistview.h +++ b/src/threadlistview.h @@ -20,34 +20,19 @@ #include -/// The thread list, with a row-wide strip of tag chips under each row's cells. +/// The thread list. /// -/// The strip is painted by the VIEW rather than by a delegate, and that is the -/// whole reason this class exists. A delegate is handed one cell's rectangle -/// and cannot paint outside its column, so pills drawn from the subject -/// column's delegate stop at that column's edge, losing the last tags of a -/// well-tagged thread, and start at that column's left edge, which puts them -/// under the subject instead of under the row. Painting after the cells lets -/// the strip run the full width, which is what the layout asks for: +/// It exists for ONE reason now: the expander is drawn by CardDelegate, and a +/// delegate gets no click of its own without an editor, so the view has to own +/// the hit-test. Everything else it used to do is gone. /// -/// [ date ][ from ][ subject ...................... ] -/// [ pill ][ pill ][ pill ] -/// -/// The cells confine themselves to the upper band so the lower one is free; -/// SubjectDelegate::kRowPadding and rowHeightFor() are the shared measurements -/// that keep the two halves agreeing. -/// -/// A QTreeView rather than a QTableView since item 20: a thread's replies are -/// child rows, and a table can neither indent nor expand. The strip survived -/// the port because every geometry call it needs (visualRect, -/// columnViewportPosition, indexAt, indexBelow) exists on both. What did NOT -/// survive is anything keyed on a row NUMBER: a tree numbers rows per parent, -/// so row 0 exists once per expanded thread and a flat 0..N walk paints the -/// first thread's strip over every one of them. The walk below goes by index. -/// -/// The strip is painted for THREAD rows only. It carries the thread's tags, so -/// one under each reply would stripe the list and repeat identical tags down -/// the whole expansion. +/// Until item 53 this class also painted a row-wide strip of tag chips after +/// the cells, because a delegate cannot paint outside its column and the strip +/// spanned all five. With one column and one delegate painting the whole card, +/// that reason is gone and so is the paintEvent, along with the two faults it +/// kept producing: a deleted row cut in half, and every other row showing a +/// bare stripe, both from the view having to re-honour alternating colours, +/// the selection and BackgroundRole across cells it did not own. class ThreadListView : public QTreeView { Q_OBJECT @@ -55,16 +40,11 @@ public: using QTreeView::QTreeView; protected: - void paintEvent(QPaintEvent *event) override; - - /// Toggles a thread when its expander glyph is clicked. + /// Toggles a thread when its reply count is clicked. /// - /// The view owns this because the glyph is drawn by SubjectDelegate and a - /// delegate gets no click of its own without an editor. Being VISIBLE and - /// being CLICKABLE are separate properties: setRootIsDecorated(false), - /// needed to stop the style drawing its own indicator underneath ours, also - /// removed the style's hit area, so the expander painted correctly and did - /// nothing at all. + /// Being VISIBLE and being CLICKABLE are separate properties: + /// setRootIsDecorated(false), needed to stop the style drawing its own + /// indicator underneath, also removed the style's hit area, so an expander + /// once painted correctly and did nothing at all. void mousePressEvent(QMouseEvent *event) override; - }; diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 740e7fa..d77cee3 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -46,6 +46,12 @@ #include "mainwindow.h" #include "messageview.h" #include "notmuchworker.h" +#include "carddelegate.h" +#include "cardlayout.h" + +#include +#include +#include #include "tagchip.h" #include "threadlistmodel.h" #include "threadlistview.h" @@ -95,10 +101,9 @@ private slots: void anUnobservableLockTableLeavesTheSyncButtonUsable(); void theStatusBarFollowsTheSyncPhase(); void aSelectedReadThreadIsNotDimmedIntoTheHighlight(); - void thePillRowSpansTheWholeWidthNotOneColumn(); void childRowsAreIndentedUnderTheirThread(); void aThreadWithRepliesDrawsAVisibleExpander(); - void noTagStripIsPaintedUnderAMessageRow(); + void cardsNeverScrollSideways(); void replyRowsKeepTheirTextUnderTheThreadLine(); void clickingTheExpanderTogglesTheThread(); void selectingAMessageRowTargetsThatMessageNotItsThread(); @@ -451,11 +456,12 @@ void TestMainWindow::headerStateFromADifferentColumnLayoutIsDiscarded() window.close(); } - // Forge a state file from an older layout: same blob, wrong column count. + // Forge a state file from the five-column layout. Nothing reads these keys + // any more, and that is exactly what must be verified: a blob saved by an + // older version has to be ignored rather than applied to a one-column view. { QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); - state.setValue(QStringLiteral("threadlist/columns"), - int(ThreadListModel::ColumnCount) - 1); + state.setValue(QStringLiteral("threadlist/columns"), 5); state.setValue(QStringLiteral("threadlist/header"), QByteArray("not a header this model could have saved")); } @@ -466,9 +472,7 @@ void TestMainWindow::headerStateFromADifferentColumnLayoutIsDiscarded() auto *view = reopened.findChild(); QVERIFY(view); - QCOMPARE(view->columnWidth(ThreadListModel::AttachmentColumn), 28); - QCOMPARE(view->columnWidth(ThreadListModel::DateColumn), 130); - QCOMPARE(view->columnWidth(ThreadListModel::SubjectColumn), 520); + QCOMPARE(view->model()->columnCount(), 1); QFile::remove(MainWindow::uiStatePath()); QStandardPaths::setTestModeEnabled(false); @@ -548,83 +552,6 @@ static ThreadSummary makeThread(const QString &id, const QStringList &tags) return thread; } -void TestMainWindow::thePillRowSpansTheWholeWidthNotOneColumn() -{ - // The pills are a row-wide strip under the cells, not content of the - // subject cell. Drawn from the subject column's delegate they stop at that - // column's edge, so a thread with several tags loses the last of them; and - // they inherit the column's left edge, which puts them under the subject - // rather than under the row. - // - // The property: pills appear to the LEFT of where the subject column - // starts, which no per-cell delegate on that column could produce. - const Config config; - MainWindow window(config); - - auto *model = window.findChild(); - QVERIFY(model); - auto *view = window.findChild(); - QVERIFY(view); - - ThreadSummary thread = makeThread(QStringLiteral("t1"), {}); - thread.tags = QStringList{ QStringLiteral("mailing-list/SBo"), - QStringLiteral("signed") }; - model->appendBatch({ thread }); - - window.resize(1400, 300); - window.show(); - QVERIFY(QTest::qWaitForWindowExposed(&window)); - QApplication::processEvents(); - - const int subjectLeft = - view->columnViewportPosition(ThreadListModel::SubjectColumn); - QVERIFY2(subjectLeft > 40, - qPrintable(QStringLiteral("the subject column starts at x=%1, too " - "close to the left edge to tell a " - "row-wide strip from a subject-cell one") - .arg(subjectLeft))); - // The strip must have somewhere to paint that the subject cell does not - // reach, or this test cannot fail. - QVERIFY2(subjectLeft < view->viewport()->width(), - qPrintable(QStringLiteral("the subject column is off-screen " - "(x=%1, viewport %2), so nothing it " - "draws is measurable") - .arg(subjectLeft) - .arg(view->viewport()->width()))); - - QImage shot(view->viewport()->size(), QImage::Format_ARGB32); - shot.fill(Qt::transparent); - view->viewport()->render(&shot); - - // Count pixels matching the tag colours EXACTLY, not "saturated" pixels. - // A looser test counts the antialiased edge of the selection highlight - // blending into the background, which is several hundred distinct - // near-background colours and passes whatever the strip does. Both earlier - // versions of this test did precisely that. - QSet pillColours; - const QVariantList colours = - model->index(0, ThreadListModel::SubjectColumn) - .data(ThreadListModel::PillColoursRole).toList(); - QVERIFY2(!colours.isEmpty(), "the model supplied no pill colours"); - for (const QVariant &colour : colours) - pillColours.insert(colour.value().rgb()); - - const int rowHeight = threadRowHeight(view, 0); - QVERIFY(rowHeight > 0); - - int chipPixels = 0; - for (int y = 0; y < qMin(rowHeight, shot.height()); ++y) { - for (int x = 0; x < qMin(subjectLeft, shot.width()); ++x) { - if (pillColours.contains(shot.pixel(x, y) | 0xff000000)) - ++chipPixels; - } - } - - QVERIFY2(chipPixels > 0, - "no pill-coloured pixels left of the subject column: the strip is " - "still confined to that cell rather than spanning the row"); -} - void TestMainWindow::childRowsAreIndentedUnderTheirThread() { const Config config; @@ -661,14 +588,14 @@ void TestMainWindow::childRowsAreIndentedUnderTheirThread() view->expand(root); QApplication::processEvents(); - // Measured on the TREE POSITION column, not on column 0. A QTreeView - // indents only the column carrying the expander, verified against Qt 6.11: - // with setTreePosition(4), column 0 reports the same left edge for a thread - // and its reply (0 and 0) while column 4 reports 420 and 440. Asserting on - // column 0 therefore fails against a perfectly indented tree. - const int treeColumn = ThreadListModel::SubjectColumn; - const QModelIndex rootCell = model->index(0, treeColumn, QModelIndex()); - const QModelIndex child = model->index(0, treeColumn, root); + // One column, and setIndentation(0): Qt indents nothing, CardLayout draws + // the indent itself. So visualRect reports the SAME rect for a thread and + // its reply, and the indent has to be read off the layout rather than off + // the geometry. That is the trap CLAUDE.md records in reverse: there, + // visualRect reported an indent the text did not have; here it reports + // none while the text is indented. + const QModelIndex rootCell = model->index(0, 0, QModelIndex()); + const QModelIndex child = model->index(0, 0, root); QVERIFY(child.isValid()); // Guards before the claim: a probe that cannot see both rows can report @@ -679,55 +606,84 @@ void TestMainWindow::childRowsAreIndentedUnderTheirThread() "the reply row has no height: it is collapsed or off-screen, and " "an indent test against it would pass without drawing anything"); - QVERIFY2(view->visualRect(child).left() > view->visualRect(rootCell).left(), - "the reply is not indented relative to its thread"); - - // The geometry being indented is NOT the same as the reply LOOKING - // indented, and asserting only the former shipped a build with no visible - // nesting at all. A thread row draws an account chip before its subject and - // a reply row does not, so the reply's text starts about a chip's width to - // the left of the thread's; at Qt's default 20px indent that difference - // swallows the shift entirely. + // The indent is NOT in the geometry. setIndentation(0) means visualRect + // reports the same left edge for both rows, deliberately: CardLayout draws + // the indent inside the card's own rect. Asserting on visualRect here + // would fail against a perfectly indented list, which is the mirror of the + // trap CLAUDE.md records for item 20, where visualRect reported an indent + // the text did not have. // - // So the real property: where the TEXT lands. The reply's subject must - // begin to the right of the thread's, which is what the eye reads as - // nesting. - const int chipWidth = - TagChip::sizeFor(QFontMetrics(view->font()), - model->data(rootCell, ThreadListModel::AccountLabelRole) - .toString()).width(); - QVERIFY2(chipWidth > 0, - "the thread row has no account chip, so this test cannot measure " - "the offset it is meant to compensate for"); - - const int threadTextLeft = view->visualRect(rootCell).left() + chipWidth; - QVERIFY2(view->visualRect(child).left() > threadTextLeft, + // So the real property, as before: where the TEXT lands. It is read off + // the layout, which is what the delegate paints from. + CardLayout::Input threadIn; + threadIn.isMessage = false; + threadIn.depth = 0; + CardLayout::Input replyIn; + replyIn.isMessage = true; + replyIn.depth = + model->data(child, ThreadListModel::MessageDepthRole).toInt(); + QVERIFY2(replyIn.depth > 0, + "the reply reports depth 0, so there is no nesting to measure"); + + const QRect rect = view->visualRect(rootCell); + const CardLayout threadCard = + CardLayout::compute(threadIn, rect, view->font()); + const CardLayout replyCard = + CardLayout::compute(replyIn, rect, view->font()); + + QVERIFY2(replyCard.contentLeft > threadCard.contentLeft, qPrintable(QStringLiteral("the reply's text starts at x=%1, not " - "right of the thread's text at x=%2: the " - "indent does not beat the account chip " - "and the nesting is invisible") - .arg(view->visualRect(child).left()) - .arg(threadTextLeft))); + "right of the thread's at x=%2: the " + "nesting is invisible") + .arg(replyCard.contentLeft) + .arg(threadCard.contentLeft))); + + // And the spine that makes the nesting read as one block rather than as an + // arbitrary offset. + QCOMPARE(replyCard.spines.size(), replyIn.depth); +} + +void TestMainWindow::cardsNeverScrollSideways() +{ + const Config config; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + auto *view = window.findChild(); + QVERIFY(view); + + auto *model = window.findChild(); + QVERIFY(model); + // A long subject, so the guard below is not vacuous: this is exactly the + // content that used to make the subject column wider than the viewport. + model->appendBatch({ makeThread( + QStringLiteral("t1"), + QStringList{ QStringLiteral("inbox") }) }); + QApplication::processEvents(); + + // Item 51: clicking a row used to scroll the list sideways, because the + // subject column was wider than the viewport and auto-scroll brought the + // clicked index fully into view. A card is exactly viewport width, so + // there is nowhere to scroll to. + QVERIFY2(view->visualRect(model->index(0, 0)).height() > 0, + "no card is drawn, so there is no layout to assert about"); + QCOMPARE(view->horizontalScrollBar()->minimum(), + view->horizontalScrollBar()->maximum()); } void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() { // The expander is the ONLY thing saying a thread can be opened, and it took - // four wrong attempts to get on screen, each of which looked correct in - // code: - // - // - QTreeView::drawBranches, the documented hook, runs BEFORE the row's - // cells, so with the expander on a content column the delegate's own - // background paints over it. A 60-pixel triangle survived as 8. - // - Sizing it from the row rather than the branch rect put most of it - // outside that rect. - // - Moving it into the delegate but calling it from only one of the two - // branches left every real row without one, since every real row has an - // account chip and takes the other branch. + // four wrong attempts to get on screen before item 53, each of which looked + // correct in code and none of which a geometry or role assertion could see. + // So this counts painted pixels. // - // None of those is visible to a test that asserts on geometry or on model - // roles, so this one counts painted pixels of the palette colour the glyph - // is drawn in. + // Painted through the DELEGATE rather than through viewport()->render(). + // The viewport render returns a blank image here: CLAUDE.md records that it + // does so in several ordinary situations, and this test proved it again, + // reporting zero ink over a card the delegate demonstrably paints 2183 + // pixels into. A probe that sees nothing cannot report on anything. const Config config; MainWindow window(config); @@ -737,7 +693,7 @@ void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() QVERIFY(view); // Two threads: one with replies, one without. The second is the control, - // and without it a test that counts text pixels would pass on any row. + // and without it a test that counts ink would pass on any card. ThreadSummary withReplies = makeThread( QStringLiteral("t1"), QStringList{ TagColors::tagForAccountKey(QStringLiteral("work")) }); @@ -748,61 +704,66 @@ void TestMainWindow::aThreadWithRepliesDrawsAVisibleExpander() lone.totalCount = 1; model->appendBatch({ withReplies, lone }); - window.resize(1400, 300); - window.show(); - QVERIFY(QTest::qWaitForWindowExposed(&window)); - QApplication::processEvents(); - - const QModelIndex first = - model->index(0, ThreadListModel::SubjectColumn, QModelIndex()); - const QModelIndex second = - model->index(1, ThreadListModel::SubjectColumn, QModelIndex()); - - // Guards: both rows on screen, and the model agreeing about which has - // replies. Without these a zero count could mean anything. - QVERIFY2(view->visualRect(first).height() > 0, "the first row is not drawn"); - QVERIFY2(view->visualRect(second).height() > 0, - "the control row is not drawn"); - QVERIFY(model->data(first, ThreadListModel::HasRepliesRole).toBool()); - QVERIFY(!model->data(second, ThreadListModel::HasRepliesRole).toBool()); - - QImage shot(view->viewport()->size(), QImage::Format_ARGB32); - shot.fill(Qt::transparent); - view->viewport()->render(&shot); - - // The exact colour the glyph is filled with, matched exactly rather than by - // a brightness threshold, which would count antialiased subject text. - const QRgb glyph = view->palette().color(QPalette::Text).rgb(); - - // Only the strip in front of the subject text, so the subject's own glyphs - // cannot be counted. kExpanderWidth is the room the delegate reserves. - const auto countGlyphPixels = [&](const QModelIndex &index) { - const QRect rect = view->visualRect(index); + const QModelIndex first = model->index(0, 0, QModelIndex()); + const QModelIndex second = model->index(1, 0, QModelIndex()); + + // Guards: the model agrees about which thread has replies, and only that + // one is offered an expander at all. + QCOMPARE(model->data(first, ThreadListModel::ReplyCountRole).toInt(), 2); + QCOMPARE(model->data(second, ThreadListModel::ReplyCountRole).toInt(), 0); + + const QFont font = view->font(); + const int height = CardLayout::heightFor(font); + + const auto inkInExpander = [&](const QModelIndex &index) { + QImage shot(400, height, QImage::Format_ARGB32); + shot.fill(Qt::white); + QPainter painter(&shot); + QStyleOptionViewItem option; + option.rect = QRect(0, 0, 400, height); + option.font = font; + option.palette = QApplication::palette(); + option.state = QStyle::State_Enabled; + CardDelegate delegate; + delegate.paint(&painter, option, index); + painter.end(); + + const QRect rect = CardDelegate::expanderRectFor(option, index); int found = 0; - for (int y = rect.top(); y < qMin(rect.bottom(), shot.height()); ++y) { - for (int x = rect.left(); - x < qMin(rect.left() + SubjectDelegate::kExpanderWidth, - shot.width()); + for (int y = rect.top(); y <= rect.bottom() && y < shot.height(); ++y) { + for (int x = rect.left(); x <= rect.right() && x < shot.width(); ++x) { - if ((shot.pixel(x, y) | 0xff000000) == (glyph | 0xff000000)) + if ((shot.pixel(x, y) | 0xff000000) != 0xffffffffu) ++found; } } - return found; + + // Guard on the probe itself: prove it can see the card's own text + // before trusting it about the expander. A probe that finds no ink + // anywhere reports "nothing was drawn" whatever the delegate did. + int anyInk = 0; + for (int y = 0; y < shot.height(); ++y) + for (int x = 0; x < shot.width(); ++x) + if ((shot.pixel(x, y) | 0xff000000) != 0xffffffffu) + ++anyInk; + return std::pair(found, anyInk); }; - const int drawn = countGlyphPixels(first); - const int control = countGlyphPixels(second); + const auto [drawn, drawnAnywhere] = inkInExpander(first); + const auto [control, controlAnywhere] = inkInExpander(second); - QVERIFY2(drawn > 12, - qPrintable(QStringLiteral("only %1 expander pixels: the glyph is " - "clipped or painted over, which is how " - "it shipped as an invisible dot") - .arg(drawn))); + QVERIFY2(drawnAnywhere > 0 && controlAnywhere > 0, + "the probe finds no ink on either card, so it cannot report on " + "the expander either"); - // The control must have none, or the count above is measuring something - // every row draws. - QCOMPARE(control, 0); + QVERIFY2(drawn > 12, + qPrintable(QStringLiteral("only %1 pixels in the expander's rect: " + "the reply count is clipped or painted " + "over").arg(drawn))); + QVERIFY2(control == 0, + qPrintable(QStringLiteral("a thread with no replies drew %1 " + "pixels where an expander would go") + .arg(control))); } void TestMainWindow::selectingAThreadRowNamesHowManyMessagesItStandsFor() @@ -1110,7 +1071,7 @@ void TestMainWindow::clickingTheExpanderTogglesTheThread() const QModelIndex root = model->index(0, 0, QModelIndex()); const QModelIndex subject = - model->index(0, ThreadListModel::SubjectColumn, QModelIndex()); + model->index(0, 0, QModelIndex()); const QRect rect = view->visualRect(subject); // Guards: the row is drawn, it claims to have replies, and it starts @@ -1119,11 +1080,15 @@ void TestMainWindow::clickingTheExpanderTogglesTheThread() QVERIFY(model->data(subject, ThreadListModel::HasRepliesRole).toBool()); QVERIFY(!view->isExpanded(root)); - // Aimed at the glyph itself: the delegate reserves kExpanderWidth at the - // left of the subject cell and centres the triangle in it. - const QPoint hit(rect.left() + SubjectDelegate::kExpanderWidth / 2, - rect.top() + SubjectDelegate::kRowPadding - + QFontMetrics(view->font()).height() / 2); + // Aimed at the rect the delegate reports, not at one reconstructed here: + // the drawn target and the clickable one cannot drift if both come from + // the same call. + QStyleOptionViewItem option; + option.rect = rect; + option.font = view->font(); + const QRect expander = CardDelegate::expanderRectFor(option, subject); + QVERIFY2(!expander.isEmpty(), "the card offers no expander to click"); + const QPoint hit = expander.center(); QTest::mouseClick(view->viewport(), Qt::LeftButton, Qt::NoModifier, hit); QApplication::processEvents(); @@ -1177,7 +1142,7 @@ void TestMainWindow::replyRowsKeepTheirTextUnderTheThreadLine() QApplication::processEvents(); const QModelIndex child = - model->index(0, ThreadListModel::AuthorsColumn, root); + model->index(0, 0, root); const QRect rect = view->visualRect(child); QVERIFY2(rect.height() > 0, "the reply row is not on screen"); @@ -1203,121 +1168,6 @@ void TestMainWindow::replyRowsKeepTheirTextUnderTheThreadLine() .arg(textPixels))); } -void TestMainWindow::noTagStripIsPaintedUnderAMessageRow() -{ - // The strip is a row-wide band of the THREAD's tags. Painted under every - // reply as well it would stripe the list and repeat identical tags down the - // whole expansion. - // - // TWO independent guards stop that, and this test is aimed at the SECOND: - // the model returns no pills for a child row, and the view skips child rows - // in its walk. Asserting against the real model tests only the first, and - // the view's guard can be deleted without the test noticing: verified by - // mutation, which passed with the skip removed. So the model is replaced - // here by one that hands out pills for EVERY row, thread and reply alike, - // leaving the view's own skip as the only thing that can keep the reply - // rows clean. - /// Hands out the same pills for a message row as for a thread row, which - /// the real model never does. Without this the view's skip is unobservable. - class PillsEverywhereModel : public ThreadListModel - { - public: - QVariant data(const QModelIndex &index, int role) const override - { - if (role == PillTagsRole) { - return QStringList{ QStringLiteral("mailing-list/SBo"), - QStringLiteral("signed") }; - } - if (role == PillColoursRole) { - return QVariantList{ QVariant::fromValue(QColor(Qt::magenta)), - QVariant::fromValue(QColor(Qt::cyan)) }; - } - return ThreadListModel::data(index, role); - } - }; - - PillsEverywhereModel model; - ThreadListView view; - view.setModel(&model); - view.setTreePosition(ThreadListModel::SubjectColumn); - view.setUniformRowHeights(true); - - // The delegates MainWindow installs, and not optional here. The strip's - // band is measured against SubjectDelegate::rowHeightFor; without the - // delegate the rows take the default height, the band overflows into the - // row below, and the thread's own strip paints across the reply. That - // reads exactly like a missing skip in the walk and is not one. - view.setItemDelegate(new RowStyleDelegate(&view)); - view.setItemDelegateForColumn(ThreadListModel::SubjectColumn, - new SubjectDelegate(&view)); - view.setColumnWidth(ThreadListModel::AttachmentColumn, 28); - view.setColumnWidth(ThreadListModel::FlagColumn, 28); - view.setColumnWidth(ThreadListModel::DateColumn, 130); - view.setColumnWidth(ThreadListModel::AuthorsColumn, 180); - view.setColumnWidth(ThreadListModel::SubjectColumn, 520); - - ThreadSummary thread = makeThread(QStringLiteral("t1"), {}); - thread.tags = QStringList{ QStringLiteral("mailing-list/SBo"), - QStringLiteral("signed") }; - model.appendBatch({ thread }); - - MessageNode first; - first.messageId = QStringLiteral("m0@example.org"); - first.threadId = QStringLiteral("t1"); - first.depth = 0; - MessageNode reply; - reply.messageId = QStringLiteral("m1@example.org"); - reply.threadId = QStringLiteral("t1"); - reply.depth = 1; - model.setThreadMessages(QStringLiteral("t1"), { first, reply }); - - view.resize(1400, 300); - view.show(); - QVERIFY(QTest::qWaitForWindowExposed(&view)); - - const QModelIndex root = model.index(0, 0, QModelIndex()); - view.expand(root); - QApplication::processEvents(); - - const QModelIndex child = model.index(0, 0, root); - const QRect childRect = view.visualRect(child); - QVERIFY2(childRect.height() > 0, "the reply row is not on screen"); - - // The exact colours the stub supplies, so an antialiased edge of anything - // else cannot be counted as a pill. - QSet pillColours; - pillColours.insert(QColor(Qt::magenta).rgb()); - pillColours.insert(QColor(Qt::cyan).rgb()); - - QImage shot(view.viewport()->size(), QImage::Format_ARGB32); - shot.fill(Qt::transparent); - view.viewport()->render(&shot); - - // Guard proving the probe can see pills at all: the THREAD row must have - // them, or a zero count under the reply proves nothing about the reply. - const QRect rootRect = view.visualRect(root); - int threadPills = 0; - for (int y = rootRect.top(); y < qMin(rootRect.bottom(), shot.height()); ++y) { - for (int x = 0; x < shot.width(); ++x) { - if (pillColours.contains(shot.pixel(x, y) | 0xff000000)) - ++threadPills; - } - } - QVERIFY2(threadPills > 0, - "no pill pixels under the THREAD row either, so this probe cannot " - "tell a missing strip from a broken render"); - - int replyPills = 0; - for (int y = childRect.top(); y < qMin(childRect.bottom(), shot.height()); ++y) { - for (int x = 0; x < shot.width(); ++x) { - if (pillColours.contains(shot.pixel(x, y) | 0xff000000)) - ++replyPills; - } - } - - QCOMPARE(replyPills, 0); -} - void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() { // Read threads carry a dimmed Qt::ForegroundRole, blended against the @@ -1383,15 +1233,15 @@ void TestMainWindow::aSelectedReadThreadIsNotDimmedIntoTheHighlight() QVERIFY2(delegate, "the thread view has no styled delegate"); const QModelIndex index = - model->index(0, ThreadListModel::SubjectColumn); + model->index(0, 0); // initStyleOption is protected, so the resolved palette is reached the way // the painter does: through a subclass that exposes it. - struct Probe : SubjectDelegate { - using SubjectDelegate::initStyleOption; + struct Probe : CardDelegate { + using CardDelegate::initStyleOption; }; const auto *probe = static_cast( - static_cast(delegate)); + static_cast(delegate)); probe->initStyleOption(&selected, index); probe->initStyleOption(&unselected, index); diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 2e3eede..49b8894 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -47,22 +47,20 @@ private slots: void appendingEmptyBatchIsNoOp(); void clearResetsModel(); void reportsSubjectAndAuthors(); - void subjectShowsMessageCountOnlyForRealThreads(); + void theReplyCountExcludesTheRootMessage(); void unreadThreadsRenderBold(); void readThreadsAreDimmedAndUnreadAreNot(); void flaggedThreadsShowAStar(); void pillTagsExcludeWhatTheRowAlreadyShows(); - void theStarColumnIsNarrowAndCarriesNoText(); void theUnreadCueDoesNotDependOnFontWeight(); void aDoomedThreadKeepsItsContrastEvenWhenRead(); - void tagsAreTheFirstColumnAndSubjectTheLast(); void accountTagBecomesAChipLabel(); void unreadStylingSurvivesAnAccountChip(); void accountChipUsesTheConfiguredColour(); void deletedThreadsAreRedAndStruckThrough(); - void attachmentColumnIsFirstAndMarksOnlyTaggedThreads(); + void attachmentIsMarkedOnlyOnTaggedThreads(); void spamThreadsAreOrangeAndStruckThrough(); - void doomedStylingCoversEveryColumn(); + void doomedStylingCoversTheWholeCard(); void ordinaryThreadsCarryNoRowColour(); void threadIdIsReachableFromAnIndex(); void invalidIndexesReturnNothing(); @@ -120,7 +118,7 @@ void TestThreadListModel::repliesBecomeChildRowsUnderTheirThread() QCOMPARE(model.rowCount(root), 2); const QModelIndex child = - model.index(0, ThreadListModel::SubjectColumn, root); + model.index(0, 0, root); QVERIFY(child.isValid()); QCOMPARE(model.parent(child), model.index(0, 0, QModelIndex())); @@ -159,20 +157,17 @@ void TestThreadListModel::messageRowsShowTheirOwnSenderAndSubject() QStringLiteral("Re: A subject")) }); const QModelIndex root = model.index(0, 0, QModelIndex()); - const QModelIndex authors = - model.index(0, ThreadListModel::AuthorsColumn, root); - const QModelIndex subject = - model.index(0, ThreadListModel::SubjectColumn, root); + const QModelIndex reply = model.index(0, 0, root); - QCOMPARE(model.data(authors, Qt::DisplayRole).toString(), + QCOMPARE(model.data(reply, ThreadListModel::SendersRole).toString(), QStringLiteral("Bob ")); - QCOMPARE(model.data(subject, Qt::DisplayRole).toString(), + QCOMPARE(model.data(reply, ThreadListModel::SubjectRole).toString(), QStringLiteral("Re: A subject")); // No tag strip under a child row. The strip is a row-wide band carrying the // THREAD's tags; one under every reply would stripe the list and repeat the // same tags down the whole expansion. - QVERIFY(model.data(subject, ThreadListModel::PillTagsRole) + QVERIFY(model.data(reply, ThreadListModel::PillTagsRole) .toStringList().isEmpty()); } @@ -397,10 +392,10 @@ void TestThreadListModel::rootRowsSurviveTheTreeConversion() // A tree model reports its roots under an INVALID parent. QCOMPARE(model.rowCount(QModelIndex()), 1); - QCOMPARE(model.columnCount(QModelIndex()), ThreadListModel::ColumnCount); + QCOMPARE(model.columnCount(QModelIndex()), 1); const QModelIndex root = - model.index(0, ThreadListModel::SubjectColumn, QModelIndex()); + model.index(0, 0, QModelIndex()); QVERIFY(root.isValid()); QVERIFY(!model.parent(root).isValid()); QCOMPARE(model.data(root, ThreadListModel::ThreadIdRole).toString(), @@ -422,7 +417,7 @@ void TestThreadListModel::startsEmpty() { ThreadListModel model; QCOMPARE(model.rowCount(), 0); - QCOMPARE(model.columnCount(), ThreadListModel::ColumnCount); + QCOMPARE(model.columnCount(), 1); } void TestThreadListModel::appendsBatches() @@ -467,21 +462,21 @@ void TestThreadListModel::reportsSubjectAndAuthors() ThreadListModel model; model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("hello")) }); - const QModelIndex authors = model.index(0, ThreadListModel::AuthorsColumn); - QCOMPARE(model.data(authors, Qt::DisplayRole).toString(), + // One index, every field, by role. The card draws them all at once, so + // reading them through Qt::DisplayRole as five columns did is no longer + // possible: DisplayRole answers the subject alone. + const QModelIndex card = model.index(0, 0); + QCOMPARE(model.data(card, ThreadListModel::SendersRole).toString(), QStringLiteral("Alice")); + QVERIFY(model.data(card, ThreadListModel::DateRole).toDateTime().isValid()); + QCOMPARE(model.data(card, ThreadListModel::SubjectRole).toString(), + QStringLiteral("hello")); - const QModelIndex date = model.index(0, ThreadListModel::DateColumn); - QVERIFY(!model.data(date, Qt::DisplayRole).toString().isEmpty()); - - // Tags are no longer a column; they reach the strip under the message - // pane through a role instead. - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); - QCOMPARE(model.data(subject, ThreadListModel::TagsRole).toStringList(), + QCOMPARE(model.data(card, ThreadListModel::TagsRole).toStringList(), QStringList({ QStringLiteral("inbox"), QStringLiteral("unread") })); } -void TestThreadListModel::subjectShowsMessageCountOnlyForRealThreads() +void TestThreadListModel::theReplyCountExcludesTheRootMessage() { ThreadListModel model; @@ -491,12 +486,18 @@ void TestThreadListModel::subjectShowsMessageCountOnlyForRealThreads() multi.totalCount = 4; model.appendBatch({ single, multi }); - QCOMPARE(model.data(model.index(0, ThreadListModel::SubjectColumn), - Qt::DisplayRole).toString(), - QStringLiteral("alone")); - QCOMPARE(model.data(model.index(1, ThreadListModel::SubjectColumn), - Qt::DisplayRole).toString(), - QStringLiteral("group (4)")); + // The count used to be a "(4)" suffix on the subject. It is the expander + // on the card's second line now, and it counts REPLIES: totalCount + // includes the root message, which is the card itself. + QCOMPARE(model.data(model.index(0, 0), + ThreadListModel::ReplyCountRole).toInt(), 0); + QCOMPARE(model.data(model.index(1, 0), + ThreadListModel::ReplyCountRole).toInt(), 3); + + // And the subject is bare, with no count spliced into it. + QCOMPARE(model.data(model.index(1, 0), + ThreadListModel::SubjectRole).toString(), + QStringLiteral("group")); } void TestThreadListModel::unreadThreadsRenderBold() @@ -507,11 +508,11 @@ void TestThreadListModel::unreadThreadsRenderBold() model.appendBatch({ read, makeThread(QStringLiteral("t2"), QStringLiteral("unread")) }); const QVariant readFont = - model.data(model.index(0, ThreadListModel::SubjectColumn), Qt::FontRole); + model.data(model.index(0, 0), Qt::FontRole); QVERIFY(!readFont.isValid()); const QVariant unreadFont = - model.data(model.index(1, ThreadListModel::SubjectColumn), Qt::FontRole); + model.data(model.index(1, 0), Qt::FontRole); QVERIFY(unreadFont.isValid()); QVERIFY(unreadFont.value().bold()); } @@ -534,10 +535,10 @@ void TestThreadListModel::readThreadsAreDimmedAndUnreadAreNot() { read, makeThread(QStringLiteral("t2"), QStringLiteral("unread")) }); const QVariant readFg = - model.data(model.index(0, ThreadListModel::SubjectColumn), + model.data(model.index(0, 0), Qt::ForegroundRole); const QVariant unreadFg = - model.data(model.index(1, ThreadListModel::SubjectColumn), + model.data(model.index(1, 0), Qt::ForegroundRole); QVERIFY2(readFg.isValid(), "a read thread carries no dimming"); @@ -559,16 +560,17 @@ void TestThreadListModel::flaggedThreadsShowAStar() QStringLiteral("flagged") }; model.appendBatch({ plain, starred }); - const QString none = - model.data(model.index(0, ThreadListModel::FlagColumn), - Qt::DisplayRole).toString(); - const QString star = - model.data(model.index(1, ThreadListModel::FlagColumn), - Qt::DisplayRole).toString(); - - QVERIFY2(none.isEmpty(), "an unflagged thread shows something in the column"); - QVERIFY2(!star.isEmpty(), "a flagged thread shows nothing"); - QCOMPARE(star, ThreadListModel::flagGlyph()); + QVERIFY2(!model.data(model.index(0, 0), + ThreadListModel::IsFlaggedRole).toBool(), + "an unflagged thread reports itself flagged"); + QVERIFY2(model.data(model.index(1, 0), + ThreadListModel::IsFlaggedRole).toBool(), + "a flagged thread does not report itself flagged"); + + // The glyph the delegate draws from that flag must be something a font can + // render: an unrenderable codepoint shows as tofu, which reads as + // breakage rather than as a mark. + QVERIFY(!ThreadListModel::flagGlyph().isEmpty()); } void TestThreadListModel::pillTagsExcludeWhatTheRowAlreadyShows() @@ -592,7 +594,7 @@ void TestThreadListModel::pillTagsExcludeWhatTheRowAlreadyShows() model.appendBatch({ thread }); const QStringList pills = - model.data(model.index(0, ThreadListModel::SubjectColumn), + model.data(model.index(0, 0), ThreadListModel::PillTagsRole).toStringList(); QVERIFY2(pills.contains(QStringLiteral("SBo")), qPrintable(pills.join(','))); @@ -622,29 +624,6 @@ void TestThreadListModel::pillTagsExcludeWhatTheRowAlreadyShows() QCOMPARE(pills, sorted); } -void TestThreadListModel::theStarColumnIsNarrowAndCarriesNoText() -{ - // A marker column, like the paperclip beside it: centred, and never - // carrying the subject or anything else that would want width. - ThreadListModel model; - ThreadSummary starred = makeThread(QStringLiteral("t1"), - QStringLiteral("starred")); - starred.tags = QStringList{ QStringLiteral("flagged") }; - model.appendBatch({ starred }); - - const QModelIndex index = model.index(0, ThreadListModel::FlagColumn); - QCOMPARE(model.data(index, Qt::TextAlignmentRole).toInt(), - int(Qt::AlignCenter)); - - // The glyph is one character, whether it is the star or its fallback: a - // column sized for a marker cannot hold a word. - QCOMPARE(ThreadListModel::flagGlyph().size(), 1); - - // And it says what it means, for anyone who cannot tell the glyph apart - // from the paperclip beside it. - QVERIFY(!model.data(index, Qt::ToolTipRole).toString().isEmpty()); -} - void TestThreadListModel::theUnreadCueDoesNotDependOnFontWeight() { // The property that matters, stated directly: strip every font from the @@ -657,17 +636,14 @@ void TestThreadListModel::theUnreadCueDoesNotDependOnFontWeight() model.appendBatch( { read, makeThread(QStringLiteral("t2"), QStringLiteral("unread")) }); - for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { - const QVariant readFg = - model.data(model.index(0, column), Qt::ForegroundRole); - const QVariant unreadFg = - model.data(model.index(1, column), Qt::ForegroundRole); + const QVariant readFg = + model.data(model.index(0, 0), Qt::ForegroundRole); + const QVariant unreadFg = + model.data(model.index(1, 0), Qt::ForegroundRole); - QVERIFY2(readFg != unreadFg, - qPrintable(QStringLiteral("column %1 renders read and unread " - "identically once the font is " - "ignored").arg(column))); - } + QVERIFY2(readFg != unreadFg, + "read and unread cards render identically once the font is " + "ignored"); } void TestThreadListModel::aDoomedThreadKeepsItsContrastEvenWhenRead() @@ -684,32 +660,11 @@ void TestThreadListModel::aDoomedThreadKeepsItsContrastEvenWhenRead() model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("deleted") }, {}); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QCOMPARE(model.data(subject, Qt::ForegroundRole).value().color(), QColor(Qt::white)); } -void TestThreadListModel::tagsAreTheFirstColumnAndSubjectTheLast() -{ - // Subject stretches to fill the view, so whatever sits after it is pushed - // off-screen. Tags used to be there, which is why acting on a thread - // looked like it did nothing: the only column that changed was invisible. - QCOMPARE(ThreadListModel::SubjectColumn, ThreadListModel::ColumnCount - 1); - - ThreadListModel model; - model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("hello")) }); - QCOMPARE(model.headerData(ThreadListModel::SubjectColumn, Qt::Horizontal, - Qt::DisplayRole).toString(), - QStringLiteral("Subject")); - - // No tags column at all: spelling out a dozen tags per row consumed most - // of the list's width and was unreadable. - for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { - QVERIFY(model.headerData(column, Qt::Horizontal, Qt::DisplayRole) - .toString() != QStringLiteral("Tags")); - } -} - void TestThreadListModel::accountTagBecomesAChipLabel() { // The account tag is a different taxonomy from a functional one: which @@ -721,7 +676,7 @@ void TestThreadListModel::accountTagBecomesAChipLabel() QStringLiteral("account-webmail-personal") }; model.appendBatch({ thread }); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QCOMPARE(model.data(subject, ThreadListModel::AccountLabelRole).toString(), QStringLiteral("webmail-personal")); QVERIFY(model.data(subject, ThreadListModel::AccountColourRole) @@ -732,7 +687,7 @@ void TestThreadListModel::accountTagBecomesAChipLabel() ThreadSummary untagged = makeThread(QStringLiteral("t2"), QStringLiteral("hi")); untagged.tags = QStringList{ QStringLiteral("inbox") }; plain.appendBatch({ untagged }); - QVERIFY(plain.data(plain.index(0, ThreadListModel::SubjectColumn), + QVERIFY(plain.data(plain.index(0, 0), ThreadListModel::AccountLabelRole).toString().isEmpty()); } @@ -748,7 +703,7 @@ void TestThreadListModel::unreadStylingSurvivesAnAccountChip() QStringLiteral("account-webmail-personal") }; model.appendBatch({ thread }); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QVERIFY(!model.data(subject, ThreadListModel::AccountLabelRole) .toString().isEmpty()); @@ -771,7 +726,7 @@ void TestThreadListModel::accountChipUsesTheConfiguredColour() thread.tags = QStringList{ QStringLiteral("account-webmail-personal") }; model.appendBatch({ thread }); - QCOMPARE(model.data(model.index(0, ThreadListModel::SubjectColumn), + QCOMPARE(model.data(model.index(0, 0), ThreadListModel::AccountColourRole).value(), QColor(QStringLiteral("#cc0000"))); } @@ -783,7 +738,7 @@ void TestThreadListModel::deletedThreadsAreRedAndStruckThrough() thread.tags = QStringList{ QStringLiteral("inbox") }; model.appendBatch({ thread }); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QVERIFY(!model.data(subject, Qt::BackgroundRole).isValid()); model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("deleted") }, {}); @@ -808,7 +763,7 @@ void TestThreadListModel::spamThreadsAreOrangeAndStruckThrough() model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("spam") }, {}); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QCOMPARE(model.data(subject, Qt::BackgroundRole).value().color(), ThreadListModel::spamColour()); QVERIFY(model.data(subject, Qt::FontRole).value().strikeOut()); @@ -817,10 +772,12 @@ void TestThreadListModel::spamThreadsAreOrangeAndStruckThrough() QVERIFY(ThreadListModel::spamColour() != ThreadListModel::deletedColour()); } -void TestThreadListModel::doomedStylingCoversEveryColumn() +void TestThreadListModel::doomedStylingCoversTheWholeCard() { - // A cue on one column would vanish the moment that column scrolled out of - // view, which is the bug this whole change exists to fix. + // The cue is on the card itself. It used to be asserted per column, + // because a cue on one column vanished the moment that column scrolled out + // of view; one column cannot scroll away, but the roles still have to be + // answered or a deleted card looks untouched. ThreadListModel model; ThreadSummary thread = makeThread(QStringLiteral("t1"), QStringLiteral("doomed")); thread.tags = QStringList{ QStringLiteral("inbox") }; @@ -828,13 +785,11 @@ void TestThreadListModel::doomedStylingCoversEveryColumn() model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("deleted") }, {}); - for (int column = 0; column < ThreadListModel::ColumnCount; ++column) { - const QModelIndex index = model.index(0, column); - QVERIFY2(model.data(index, Qt::BackgroundRole).isValid(), - qPrintable(QStringLiteral("column %1 has no background").arg(column))); - QVERIFY2(model.data(index, Qt::FontRole).value().strikeOut(), - qPrintable(QStringLiteral("column %1 is not struck through").arg(column))); - } + const QModelIndex index = model.index(0, 0); + QVERIFY2(model.data(index, Qt::BackgroundRole).isValid(), + "a deleted card has no background"); + QVERIFY2(model.data(index, Qt::FontRole).value().strikeOut(), + "a deleted card is not struck through"); } void TestThreadListModel::ordinaryThreadsCarryNoRowColour() @@ -848,7 +803,7 @@ void TestThreadListModel::ordinaryThreadsCarryNoRowColour() model.applyTagChange(QStringLiteral("t1"), { QStringLiteral("deleted") }, {}); model.applyTagChange(QStringLiteral("t1"), {}, { QStringLiteral("deleted") }); - const QModelIndex subject = model.index(0, ThreadListModel::SubjectColumn); + const QModelIndex subject = model.index(0, 0); QVERIFY(!model.data(subject, Qt::BackgroundRole).isValid()); const QVariant font = model.data(subject, Qt::FontRole); QVERIFY(!font.isValid() || !font.value().strikeOut()); @@ -871,7 +826,7 @@ void TestThreadListModel::threadIdIsReachableFromAnIndex() model.appendBatch({ makeThread(QStringLiteral("t1"), QStringLiteral("one")), makeThread(QStringLiteral("t2"), QStringLiteral("two")) }); - const QModelIndex index = model.index(1, ThreadListModel::SubjectColumn); + const QModelIndex index = model.index(1, 0); QCOMPARE(model.data(index, ThreadListModel::ThreadIdRole).toString(), QStringLiteral("t2")); } @@ -889,7 +844,7 @@ void TestThreadListModel::invalidIndexesReturnNothing() // the reset in clear() before data() ever sees it. data() still checks its // own bounds, but that guard is unreachable defence, not something these // assertions can falsify. - QVERIFY(!model.index(0, ThreadListModel::ColumnCount).isValid()); + QVERIFY(!model.index(0, 1).isValid()); QVERIFY(!model.index(5, 0).isValid()); QVERIFY(!model.index(-1, 0).isValid()); @@ -953,9 +908,9 @@ void TestThreadListModel::tagChangeSignalsExactlyTheChangedRow() const QModelIndex bottomRight = changed.first().at(1).value(); QCOMPARE(topLeft.row(), 1); QCOMPARE(bottomRight.row(), 1); + // One column, so the range is a single index: the card repaints whole. QCOMPARE(topLeft.column(), 0); - // The whole row repaints: unread state changes the font of every column. - QCOMPARE(bottomRight.column(), ThreadListModel::ColumnCount - 1); + QCOMPARE(bottomRight.column(), 0); } void TestThreadListModel::tagChangeForUnknownThreadIsIgnored() @@ -1006,12 +961,11 @@ void TestThreadListModel::modelPassesQtTester() model.clear(); } -void TestThreadListModel::attachmentColumnIsFirstAndMarksOnlyTaggedThreads() +void TestThreadListModel::attachmentIsMarkedOnlyOnTaggedThreads() { - // Leftmost, and narrow: the point is to see an attachment without opening - // the thread, which only works if the column is never scrolled away. - QCOMPARE(ThreadListModel::AttachmentColumn, 0); - + // The mark is drawn on the card's second line by CardDelegate. What the + // model owes it is the flag and the glyph, which is what this asserts: + // the column that used to carry it is gone. ThreadSummary plain = makeThread(QStringLiteral("t1"), QStringLiteral("no attachment")); ThreadSummary withFile = makeThread(QStringLiteral("t2"), @@ -1024,13 +978,12 @@ void TestThreadListModel::attachmentColumnIsFirstAndMarksOnlyTaggedThreads() model.appendBatch({ plain, withFile }); const QModelIndex plainCell = - model.index(0, ThreadListModel::AttachmentColumn); + model.index(0, 0); const QModelIndex fileCell = - model.index(1, ThreadListModel::AttachmentColumn); + model.index(1, 0); - QVERIFY(model.data(plainCell, Qt::DisplayRole).toString().isEmpty()); - QCOMPARE(model.data(fileCell, Qt::DisplayRole).toString(), - ThreadListModel::attachmentGlyph()); + QVERIFY(!model.data(plainCell, ThreadListModel::HasAttachmentRole).toBool()); + QVERIFY(model.data(fileCell, ThreadListModel::HasAttachmentRole).toBool()); // The glyph must be something a font can draw. An unrenderable codepoint // shows as a tofu box, which reads as breakage rather than as a marker. @@ -1040,11 +993,6 @@ void TestThreadListModel::attachmentColumnIsFirstAndMarksOnlyTaggedThreads() // have an attachment on hover. QVERIFY(model.data(plainCell, Qt::ToolTipRole).toString().isEmpty()); QVERIFY(!model.data(fileCell, Qt::ToolTipRole).toString().isEmpty()); - - // The header carries no text: a label would set a minimum width far wider - // than the icon and defeat the narrow column. - QVERIFY(model.headerData(ThreadListModel::AttachmentColumn, Qt::Horizontal, - Qt::DisplayRole).toString().isEmpty()); } void TestThreadListModel::modelHasOneColumn() -- cgit v1.2.3 From b59b9ec616fb5c965cfd32385879162989589bb6 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:49:21 +0200 Subject: 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. --- src/keymap.cpp | 36 +++++++++++++ src/keymap.h | 9 ++++ src/mainwindow.cpp | 45 ++++++++++------ tests/test_mainwindow.cpp | 130 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 203 insertions(+), 17 deletions(-) (limited to 'tests/test_mainwindow.cpp') 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 + #include QStringList KeyMap::knownActions() @@ -64,6 +66,18 @@ QList> 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 KeyMap::sequencesFor(const QString &action) const +{ + const QKeySequence primary = sequenceFor(action); + if (primary.isEmpty()) + return {}; + + QList all{ primary }; + QList 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 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 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(); + f.model = window.findChild(); + + 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(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(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(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{ + "next_thread", "Alt+Down" }, + { "prev_thread", "Alt+Up" } }) { + auto *action = + window.findChild(QString::fromLatin1(pair.first)); + QVERIFY2(action, pair.first); + const QList 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; -- cgit v1.2.3 From 91311854173c06b5007302341fcf1840585d9d37 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:53:13 +0200 Subject: feat(ui): let the user choose newest or oldest first Two entries, straight to notmuch. This adds a feature rather than replacing one: the column header was decorative and nothing implemented click-to-sort, so removing the header with the grid lost nothing. Stored in uistate.conf, never in the hand-edited config, and range-guarded on read: a stale file can hold anything, which is the lesson item 58 recorded. SortOrder needed qRegisterMetaType despite carrying Q_ENUM. Q_ENUM gives the enum a meta-object entry, not a metatype registered under the name invokeMethod resolves, so the queued runQuery would have dropped its sort argument at runtime and every query would have silently run newest-first. Nothing in the suite exercises a real worker thread, so this was asserted directly rather than left to a warning nobody would see. It is registered beside the type rather than in MainWindow's constructor: a first attempt put it there and passed only because the test that catches it never constructs a MainWindow. The account dropdown's entries now carry their account's colour as a swatch, which is what makes the accent bar on a card mean anything: a colour down a card's edge says nothing until something maps it to a name. Raw colour here rather than the blended line colour, since a swatch is a filled patch like a chip rather than a thin line. Its test builds its own two-account config: reading the environment's made it SKIP wherever no accounts are configured, which is a test that asserts nothing while reporting success. --- src/mainwindow.cpp | 42 ++++++++++++++++++++- src/mainwindow.h | 1 + src/notmuchworker.cpp | 16 ++++++++ tests/test_mainwindow.cpp | 90 ++++++++++++++++++++++++++++++++++++++++++++ tests/test_notmuchworker.cpp | 22 +++++++++++ 5 files changed, 169 insertions(+), 2 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 57a6988..5881fae 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -163,6 +163,12 @@ void MainWindow::restoreUiState() // by CardDelegate, so there are no widths to restore; a blob saved by an // older version is simply ignored (item 53's Upgrading note). + // Range-guarded on read: a stale or hand-edited file can hold anything, + // and setCurrentIndex() on a value with no row silently selects nothing. + const int sort = + state.value(QStringLiteral("threadlist/sortOrder"), 0).toInt(); + m_sortOrder->setCurrentIndex(sort == 1 ? 1 : 0); + // The config value is the starting point for a profile that has never // zoomed; once the user does, the state file is what they last had. // clampZoom() rejects the garbage a hand-edited file can hold. @@ -178,6 +184,8 @@ void MainWindow::saveUiState() const state.setValue(QStringLiteral("window/geometry"), saveGeometry()); state.setValue(QStringLiteral("window/state"), saveState()); state.setValue(QStringLiteral("window/splitter"), m_splitter->saveState()); + state.setValue(QStringLiteral("threadlist/sortOrder"), + m_sortOrder->currentIndex()); state.setValue(QStringLiteral("message/zoom"), m_messageView->zoomFactor()); } @@ -419,9 +427,34 @@ void MainWindow::buildUi() // Query row. auto *queryRow = new QHBoxLayout; m_accountBox = new QComboBox(central); + m_accountBox->setObjectName(QStringLiteral("accountBox")); m_accountBox->addItem(tr("All accounts"), QString()); - for (const Account &account : m_config.accounts()) + for (const Account &account : m_config.accounts()) { m_accountBox->addItem(account.key, account.key); + // The RAW account colour here, not CardDelegate's blended line colour: + // a swatch is a filled patch like a chip, not a thin line, so it wants + // the colour the account was actually given. Qt renders a + // DecorationRole colour as a swatch itself, with no delegate. + // + // This is what makes the accent bar on a card mean anything: a colour + // down a card's edge says nothing until something maps it to a name. + m_accountBox->setItemData( + m_accountBox->count() - 1, + m_tagColors.colourFor(TagColors::tagForAccountKey(account.key)), + Qt::DecorationRole); + } + + // Sort order. Two entries, straight to notmuch: this ADDS a feature rather + // than replacing one, since the old column header was decorative and + // nothing implemented click-to-sort. + m_sortOrder = new QComboBox(central); + m_sortOrder->setObjectName(QStringLiteral("sortOrder")); + // Order matters: the index is what uistate.conf stores. + m_sortOrder->addItem(tr("Newest first")); + m_sortOrder->addItem(tr("Oldest first")); + m_sortOrder->setToolTip(tr("The order threads are listed in")); + connect(m_sortOrder, &QComboBox::currentIndexChanged, + this, &MainWindow::runCurrentQuery); m_queryEdit = new QLineEdit(central); m_queryEdit->setPlaceholderText(tr("notmuch query, e.g. tag:inbox")); @@ -510,6 +543,7 @@ void MainWindow::buildUi() // would squeeze the field, but three is the real-world case today. Item 23 // already specifies buttons-plus-menu and is where that belongs. queryRow->addWidget(m_accountBox); + queryRow->addWidget(m_sortOrder); queryRow->addWidget(m_queryEdit, 1); for (const SavedQuery &saved : m_config.savedQueries()) { auto *button = new QPushButton(saved.name, central); @@ -1465,9 +1499,13 @@ void MainWindow::runCurrentQuery() m_queryComplete = false; updateViewWideActions(); + const auto sort = m_sortOrder->currentIndex() == 1 + ? NotmuchWorker::OldestFirst + : NotmuchWorker::NewestFirst; QMetaObject::invokeMethod(m_worker, "runQuery", Qt::QueuedConnection, Q_ARG(QString, query), - Q_ARG(quint64, m_generation)); + Q_ARG(quint64, m_generation), + Q_ARG(NotmuchWorker::SortOrder, sort)); } void MainWindow::onThreadsReady(const QVector &threads, diff --git a/src/mainwindow.h b/src/mainwindow.h index 6b9e557..ccac5ad 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -468,6 +468,7 @@ private: /// placeholder's own text wraps every couple of words. static constexpr int kMinMessagePaneWidth = 300; QComboBox *m_accountBox = nullptr; + QComboBox *m_sortOrder = nullptr; QLabel *m_statusLabel = nullptr; /// Expires a transient status message. See showTransientStatus(). diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index a6b0a29..d34c032 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -119,9 +119,25 @@ void walkReplies(notmuch_messages_t *messages, int depth, } // namespace +/// Registers SortOrder for queued calls, once, before main() runs. +/// +/// Q_ENUM alone is NOT enough for a queued Q_ARG: it gives the enum a +/// meta-object entry, not a metatype registered under the name invokeMethod +/// resolves, so MainWindow's queued runQuery would drop its sort argument at +/// runtime with a warning and every query would silently run newest-first. +/// +/// Here rather than in MainWindow's constructor, because the registration +/// belongs to the type rather than to one consumer: a caller that never +/// constructs a MainWindow (a test, or a future headless mode) needs it too, +/// and that is exactly how the first attempt at this passed by accident and +/// failed under test. +static const int kSortOrderMetaType = + qRegisterMetaType("NotmuchWorker::SortOrder"); + NotmuchWorker::NotmuchWorker(const QString ¬muchConfigPath, QObject *parent) : QObject(parent), m_configPath(notmuchConfigPath) { + Q_UNUSED(kSortOrderMetaType); } NotmuchWorker::~NotmuchWorker() diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index b3511ec..cdb08ca 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -51,6 +51,7 @@ #include #include +#include #include #include "tagchip.h" #include "threadlistmodel.h" @@ -107,6 +108,8 @@ private slots: void nextThreadLeavesTheLastReply(); void altDownSkipsReplies(); void bothThreadStepBindingsReachTheAction(); + void sortChoiceSurvivesRestart(); + void accountEntriesCarryTheirColour(); void replyRowsKeepTheirTextUnderTheThreadLine(); void clickingTheExpanderTogglesTheThread(); void selectingAMessageRowTargetsThatMessageNotItsThread(); @@ -773,6 +776,93 @@ void TestMainWindow::bothThreadStepBindingsReachTheAction() } } +void TestMainWindow::sortChoiceSurvivesRestart() +{ + QStandardPaths::setTestModeEnabled(true); + QFile::remove(MainWindow::uiStatePath()); + + { + const Config config; + MainWindow window(config); + auto *sort = window.findChild(QStringLiteral("sortOrder")); + QVERIFY(sort); + QCOMPARE(sort->count(), 2); + QCOMPARE(sort->currentIndex(), 0); // Newest first by default. + sort->setCurrentIndex(1); + window.close(); + } + + const Config config; + MainWindow second(config); + auto *sort = second.findChild(QStringLiteral("sortOrder")); + QVERIFY(sort); + QCOMPARE(sort->currentIndex(), 1); + + // A stale or hand-edited file can hold anything, which is the lesson item + // 58 recorded: an out-of-range value must fall back rather than select a + // row that does not exist. + { + QSettings state(MainWindow::uiStatePath(), QSettings::IniFormat); + state.setValue(QStringLiteral("threadlist/sortOrder"), 47); + } + MainWindow third(config); + auto *thirdSort = third.findChild(QStringLiteral("sortOrder")); + QCOMPARE(thirdSort->currentIndex(), 0); + + QFile::remove(MainWindow::uiStatePath()); + QStandardPaths::setTestModeEnabled(false); +} + +void TestMainWindow::accountEntriesCarryTheirColour() +{ + // Its own config, not the environment's. Reading the real one made this + // SKIP wherever no accounts are configured, which is a test that asserts + // nothing while reporting success. + QTemporaryDir dir; + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + { + QSettings s(path, QSettings::IniFormat); + s.beginGroup(QStringLiteral("account.work")); + s.setValue(QStringLiteral("maildir"), QStringLiteral("work")); + s.setValue(QStringLiteral("color"), QStringLiteral("#3d7fd1")); + s.endGroup(); + s.beginGroup(QStringLiteral("account.personal")); + s.setValue(QStringLiteral("maildir"), QStringLiteral("personal")); + s.endGroup(); + } + + Config config; + config.load(path); + QCOMPARE(config.accounts().size(), 2); + + MainWindow window(config); + auto *box = window.findChild(QStringLiteral("accountBox")); + QVERIFY(box); + QCOMPARE(box->count(), 3); + + // "All accounts" is not an account and carries no swatch. + QVERIFY(!box->itemData(0, Qt::DecorationRole).isValid()); + + // Every real account does, including the one with no color= key: + // colourFor() never fails, deriving a stable colour from the tag name, so + // adding an account and forgetting to colour it degrades to something + // usable rather than to nothing. + QSet seen; + for (int i = 1; i < box->count(); ++i) { + const QVariant swatch = box->itemData(i, Qt::DecorationRole); + QVERIFY2(swatch.isValid(), + qPrintable(QStringLiteral("account %1 carries no swatch") + .arg(box->itemText(i)))); + const QColor colour = swatch.value(); + QVERIFY(colour.isValid()); + seen.insert(colour.rgb()); + } + + // Guard: two accounts sharing one colour would make the swatches useless + // as a key to the accent bars, and would let a broken lookup pass. + QCOMPARE(seen.size(), 2); +} + void TestMainWindow::cardsNeverScrollSideways() { const Config config; diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index 3342013..88dcf0b 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -40,6 +40,7 @@ private slots: void unreadableConfigEmitsError(); void queryPassesGenerationThrough(); void oldestFirstReversesTheOrder(); + void theSortOrderCrossesAQueuedCall(); void loadThreadReturnsMessagesOldestFirst(); void loadThreadMarksMatchedMessages(); @@ -346,6 +347,27 @@ void TestNotmuchWorker::oldestFirstReversesTheOrder() QCOMPARE(oldest.last().threadId, newest.first().threadId); } +void TestNotmuchWorker::theSortOrderCrossesAQueuedCall() +{ + // MainWindow reaches the worker with invokeMethod(..., QueuedConnection) + // across a thread boundary, and a Q_ARG whose type the meta-object system + // does not know FAILS AT RUNTIME with a warning, not at compile time. So + // the enum's registration is asserted here rather than assumed from Q_ENUM. + QVERIFY2(QMetaType::fromName("NotmuchWorker::SortOrder").isValid(), + "SortOrder is not a registered metatype, so the queued runQuery " + "call will drop its sort argument at runtime"); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy ready(&worker, &NotmuchWorker::threadsReady); + + // The real call shape, invoked by NAME exactly as MainWindow does. + QVERIFY(QMetaObject::invokeMethod( + &worker, "runQuery", Qt::DirectConnection, + Q_ARG(QString, QStringLiteral("*")), Q_ARG(quint64, 1), + Q_ARG(NotmuchWorker::SortOrder, NotmuchWorker::OldestFirst))); + QCOMPARE(ready.size(), 1); +} + void TestNotmuchWorker::loadThreadReturnsMessagesOldestFirst() { const QString threadId = threadIdOf(QStringLiteral("Release notes")); -- cgit v1.2.3 From 93e6a533f4b0cc1a75000b2f8c77181dfc56e199 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 09:23:50 +0200 Subject: fix(ui): expand flat threads, reach the first message, localise the date Four faults from the first hand test, two of them behavioural. An expander that opened onto nothing. setThreadMessages kept only nodes with depth > 0, and notmuch_thread_get_toplevel_messages returns every message at depth 0 when a thread carries no usable In-Reply-To, so a flat thread contributed no children while its card still advertised the count. Measured in the user's database: of 396 inbox threads three are flat, one of them nine messages long, and every two-message thread of that kind was affected, which is exactly why the fault looked like "the expander only works with more than one reply". The rule is now position, not depth: every message except the first, which is the root card itself. That is also the correct rule rather than a workaround, since the row under the root is the second message however notmuch chose to nest it. The thread's first message was unreachable. Selecting a root card loaded the whole thread, so the pane showed every message with only the last expanded, and no row in the list offered the first one: the reply rows are messages two onward. The root card now renders its own message, which is what the card already claims to be. It keeps its thread id, unlike the message-row path, so mark-read and the tag-change repaint still work; that is asserted, because clearing it is the obvious way to write this and silently disables both. Before the replies are loaded the model has no first message to name and the whole thread stays the honest answer. Dates ignored the locale. One hardcoded "yyyy-MM-dd hh:mm" produced a US-looking format on an Italian desktop; QLocale::system() now formats it, and the width reserved for the date comes from the same function so a longer locale cannot clip. The expander was a bare number on the card's own background. It is a pill now, carrying "3 replies" (and "1 reply", singular), sized from the label actually drawn and measured in both glyph states so it does not resize under the pointer on click. Its fill is blended from Text toward Base rather than taken from QPalette::Button, which is #2b2b2b against a Base of #2b2b2b on the user's theme: byte identical, so the pill was invisible. A theme may make any two roles equal; a blend is defined against the surface it sits on and cannot collide with it. Checked by rendering both a dark and a light palette and looking. --- src/carddelegate.cpp | 53 ++++++++++++++++++++---- src/cardlayout.cpp | 58 +++++++++++++++++++++++--- src/cardlayout.h | 31 ++++++++++++++ src/mainwindow.cpp | 23 ++++++++++- src/threadlistmodel.cpp | 37 +++++++++++++---- src/threadlistmodel.h | 9 ++++ tests/test_cardlayout.cpp | 75 ++++++++++++++++++++++++++++++++++ tests/test_mainwindow.cpp | 39 +++++++++++++++++- tests/test_threadlistmodel.cpp | 93 +++++++++++++++++++++++++++++++++++++++--- 9 files changed, 388 insertions(+), 30 deletions(-) (limited to 'tests/test_mainwindow.cpp') diff --git a/src/carddelegate.cpp b/src/carddelegate.cpp index a3a846d..f13205e 100644 --- a/src/carddelegate.cpp +++ b/src/carddelegate.cpp @@ -157,7 +157,7 @@ void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, const QDateTime date = index.data(ThreadListModel::DateRole).toDateTime(); painter->drawText(card.dateRect, Qt::AlignVCenter | Qt::AlignRight, - date.toString(QStringLiteral("yyyy-MM-dd hh:mm"))); + CardLayout::formatDate(date)); // Line 2: the flag mark, the subject, the attachment mark. QString subject = index.data(ThreadListModel::SubjectRole).toString(); @@ -178,16 +178,51 @@ void CardDelegate::paint(QPainter *painter, const QStyleOptionViewItem &option, metrics.elidedText(line2, Qt::ElideRight, card.subjectRect.width())); - // The reply count, which is also the expander. + // The reply count, which is also the expander, drawn as a PILL. + // + // A bare "3" on the card's own background read as an unexplained number + // beside the subject and gave no hint that it could be clicked. The chip + // shape says "this is a control", matching the tag chips on line 3, and the + // word says what the number counts. if (!card.expanderRect.isEmpty()) { - painter->setFont(CardLayout::smallFont(chrome.font)); const int count = index.data(ThreadListModel::ReplyCountRole).toInt(); - const QString glyph = (option.state & QStyle::State_Open) - ? QStringLiteral("▾") - : QStringLiteral("▸"); - painter->drawText(card.expanderRect, Qt::AlignVCenter | Qt::AlignRight, - QStringLiteral("%1 %2").arg(glyph).arg(count)); - painter->setFont(chrome.font); + const QString label = CardLayout::expanderLabel( + count, option.state & QStyle::State_Open); + + painter->save(); + painter->setFont(CardLayout::smallFont(chrome.font)); + + // Blended from Text toward Base rather than taken from a palette ROLE. + // QPalette::Button is the role this obviously wants and it is + // #2b2b2b against a Base of #2b2b2b on the user's theme: byte + // identical, so the pill was invisible. A theme is free to make any two + // roles equal, and several do; a blend cannot collide with the surface + // it sits on because it is defined relative to it. + // + // Toward Text, so it darkens on a light theme and lightens on a dark + // one, the same trick replyBackground() and threadLineColour() use. + const QColor base = option.palette.color(QPalette::Base); + const QColor text = option.palette.color(QPalette::Text); + constexpr qreal kFillWeight = 0.18; + const QColor fill = QColor::fromRgbF( + text.redF() * kFillWeight + base.redF() * (1.0 - kFillWeight), + text.greenF() * kFillWeight + base.greenF() * (1.0 - kFillWeight), + text.blueF() * kFillWeight + base.blueF() * (1.0 - kFillWeight)); + painter->setRenderHint(QPainter::Antialiasing, true); + painter->setPen(Qt::NoPen); + painter->setBrush(fill); + // Fully rounded ends, the same shape TagChip paints: the radius is half + // the height, so the pill cannot look like a rectangle with soft corners. + const qreal radius = card.expanderRect.height() / 2.0; + painter->drawRoundedRect(card.expanderRect, radius, radius); + + // The pen is restored from the card's own text colour rather than + // ButtonText, which belongs to the role that just proved unreliable. + painter->setPen(option.state & QStyle::State_Selected + ? option.palette.highlightedText().color() + : text); + painter->drawText(card.expanderRect, Qt::AlignCenter, label); + painter->restore(); } painter->restore(); diff --git a/src/cardlayout.cpp b/src/cardlayout.cpp index 1a79e3b..0e118ab 100644 --- a/src/cardlayout.cpp +++ b/src/cardlayout.cpp @@ -19,6 +19,44 @@ #include "cardlayout.h" #include +#include + +QString CardLayout::formatDate(const QDateTime &date) +{ + // The system locale's own short format, not a hardcoded pattern: an + // Italian desktop writes 10/08/2025, not 2025-08-10, and a mail client + // that disagrees with every other application on screen is simply wrong. + return QLocale::system().toString(date, QLocale::ShortFormat); +} + +QString CardLayout::expanderLabel(int replyCount, bool expanded) +{ + // "3 replies", not a bare "3". The count alone reads as an unexplained + // number beside the subject, and the word is what says the card opens. + // + // Not translated through tr() here because CardLayout is a plain struct + // rather than a QObject; the delegate is where a translated build would + // wrap this, and the string is deliberately kept in one place so there is + // exactly one thing to change. + const QString glyph = expanded ? QStringLiteral("\u25be") + : QStringLiteral("\u25b8"); + const QString word = replyCount == 1 ? QStringLiteral("reply") + : QStringLiteral("replies"); + return QStringLiteral("%1 %2 %3").arg(glyph).arg(replyCount).arg(word); +} + +QString CardLayout::widestDateSample() +{ + // A real date run through the same formatter, with the wide digits and a + // two-digit day and month, so the reserved width matches what is drawn + // whatever the locale's pattern turns out to be. Guessing a pattern here + // would reintroduce the clipping this exists to prevent. + static const QString sample = [] { + const QDateTime wide(QDate(2028, 12, 28), QTime(22, 58)); + return formatDate(wide); + }(); + return sample; +} QFont CardLayout::smallFont(const QFont &cardFont) { @@ -96,8 +134,8 @@ CardLayout CardLayout::compute(const Input &input, const QRect &rect, // painted. QFont dateFont = font; dateFont.setBold(true); - const int dateWidth = QFontMetrics(dateFont).horizontalAdvance( - QStringLiteral("8888-88-88 88:88")); + const int dateWidth = + QFontMetrics(dateFont).horizontalAdvance(widestDateSample()); out.dateRect = QRect(right - dateWidth, lineOneTop, dateWidth, metrics.height()); out.senderRect = QRect(out.contentLeft, lineOneTop, @@ -105,10 +143,20 @@ CardLayout CardLayout::compute(const Input &input, const QRect &rect, - kPaddingX), metrics.height()); - // The expander is the reply count, on line two and on the right. + // The expander is the reply count as a PILL, on line two and on the right. + // + // Sized from the label actually drawn rather than from a fixed sample, so + // the background and the text inside it cannot disagree. Both states of the + // glyph are measured because the rect must not change width when the card + // is expanded: a pill that resized on click would shift the subject's + // elision under the pointer. if (input.replyCount > 0) { - const int countWidth = smallMetrics.horizontalAdvance( - QStringLiteral("▾ 8888 replies")); + const int collapsed = smallMetrics.horizontalAdvance( + expanderLabel(input.replyCount, false)); + const int expanded = smallMetrics.horizontalAdvance( + expanderLabel(input.replyCount, true)); + const int countWidth = + qMax(collapsed, expanded) + kPillPaddingX * 2; out.expanderRect = QRect(right - countWidth, lineTwoTop, countWidth, metrics.height()); } diff --git a/src/cardlayout.h b/src/cardlayout.h index d06ed92..ef6a563 100644 --- a/src/cardlayout.h +++ b/src/cardlayout.h @@ -18,6 +18,7 @@ #pragma once +#include #include #include #include @@ -116,4 +117,34 @@ struct CardLayout static CardLayout compute(const Input &input, const QRect &rect, const QFont &font); + + /// How a card writes a date, in the user's own locale. + /// + /// Never a hardcoded pattern. "yyyy-MM-dd hh:mm" is a US-looking format + /// that an Italian desktop does not use, and the whole point of asking the + /// system locale is that the user reads dates the way their desktop writes + /// them everywhere else. + /// + /// Shared with the layout so the width reserved for the date and the text + /// drawn into it come from one place: a locale whose short format is + /// longer than the reserved rect would clip, which is exactly the fault + /// bold text produced. + static QString formatDate(const QDateTime &date); + + /// The widest string formatDate() can return, for reserving space. + static QString widestDateSample(); + + /// The expander's label: the reply count with its glyph, as drawn. + /// + /// Shared with the layout for the same reason as formatDate: the rect + /// reserved for the pill and the text put inside it must come from one + /// place, or a count wider than the sample the layout guessed at spills + /// out of its own background. + /// + /// `expanded` chooses which way the triangle points. + static QString expanderLabel(int replyCount, bool expanded); + + /// Padding inside the expander pill, matching a tag chip's, so the two read + /// as the same kind of object on the card. + static constexpr int kPillPaddingX = 8; }; diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 5881fae..b7efec5 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1755,10 +1755,31 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, } const ThreadSummary thread = m_model->threadAt(current.row()); - m_currentMessageId.clear(); m_currentThreadId = thread.threadId; m_messageView->setTags(thread.tags); scheduleMarkRead(thread); + + // The root card IS the thread's first message, so selecting it renders + // that message rather than the whole conversation. Loading the thread here + // made the first message unreachable: the pane showed every message with + // only the last expanded, and no row in the list offered the first one, + // since the reply rows are messages two onward. + // + // Known only once the replies have been loaded, which happens when the + // thread is expanded. Until then the thread is the honest answer: it + // contains the first message, where a guess might not. + const QString firstId = + m_model->data(current, ThreadListModel::MessageIdRole).toString(); + if (!firstId.isEmpty()) { + m_currentMessageId = firstId; + QMetaObject::invokeMethod(m_worker, "loadMessage", + Qt::QueuedConnection, + Q_ARG(QString, firstId), + Q_ARG(quint64, m_generation)); + return; + } + + m_currentMessageId.clear(); QMetaObject::invokeMethod(m_worker, "loadThread", Qt::QueuedConnection, Q_ARG(QString, m_currentThreadId), Q_ARG(QString, m_lastQuery), diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index d8412f9..bdc7e96 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -365,8 +365,13 @@ QVariant ThreadListModel::data(const QModelIndex &index, int role) const if (role == IsMessageRole) return false; - if (role == MessageIdRole) - return QString(); + if (role == MessageIdRole) { + // The thread's FIRST message, once known, because the root card is + // that message: selecting it renders one message rather than the whole + // conversation. Empty before the replies are loaded, which is the + // caller's signal to load the thread instead of guessing at a message. + return m_threads.at(index.row()).first.messageId; + } if (role == MessageDepthRole) return 0; @@ -542,7 +547,7 @@ void ThreadListModel::appendBatch(const QVector &batch) const int first = m_threads.size(); beginInsertRows({}, first, first + batch.size() - 1); for (const ThreadSummary &summary : batch) - m_threads.append(ThreadNode{ summary, {}, false }); + m_threads.append(ThreadNode{ summary, {}, {}, false }); endInsertRows(); } @@ -570,12 +575,26 @@ void ThreadListModel::setThreadMessages(const QString &threadId, endRemoveRows(); } - QVector children; - children.reserve(nodes.size()); - for (const MessageNode &node : nodes) { - if (node.depth > 0) - children.append(node); - } + // Every message EXCEPT the first, which is the root card itself. + // + // Selecting on depth > 0 instead was wrong, and wrong in a way that + // only showed on real mail: notmuch_thread_get_toplevel_messages + // returns every message at depth 0 when a thread carries no usable + // In-Reply-To, so a flat thread contributed no children at all. The + // card advertised "3 replies" and expanded onto nothing. Measured in + // the user's database: of 396 inbox threads, three are flat, one of + // them nine messages long, and every two-message thread of this kind + // was affected, which is why the fault looked like "the expander only + // works with more than one reply". + // + // Position also happens to be the right rule rather than a workaround. + // The root card IS the thread's first message, so the row under it is + // the second message whatever depth notmuch assigns it. + QVector children = nodes.mid(1); + + // Kept so the root card can render its own message. It is the card the + // user clicks to read the thread's opening message. + m_threads[row].first = nodes.isEmpty() ? MessageNode() : nodes.first(); if (!children.isEmpty()) { beginInsertRows(parent, 0, children.size() - 1); diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index 9cb774e..f381ffa 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -240,6 +240,15 @@ private: ThreadSummary summary; QVector children; ///< Empty until the thread is expanded. + /// The thread's FIRST message, which the root card itself draws. + /// + /// Kept because the root card is that message: selecting it must + /// render one message rather than the whole conversation, and without + /// this the first message of every thread is unreachable, since the + /// only rows offering a message are the replies and it is not one of + /// them. Empty until the replies are loaded. + MessageNode first; + /// Distinguishes "this thread has no replies" from "its replies have /// not been asked for yet". Without it an expander would be drawn over /// every thread, including the ones that turn out to be single diff --git a/tests/test_cardlayout.cpp b/tests/test_cardlayout.cpp index c38f728..48bba25 100644 --- a/tests/test_cardlayout.cpp +++ b/tests/test_cardlayout.cpp @@ -19,6 +19,7 @@ #include "cardlayout.h" #include +#include #include class TestCardLayout : public QObject @@ -32,10 +33,12 @@ private slots: void indentStopsAtTheCap(); void expanderSitsOnTheSecondLine(); void expanderIsEmptyWithoutReplies(); + void theExpanderReadsAsAPillWithAWord(); void dateIsFlushRight(); void threadCardCarriesAnAccentBar(); void replyCardCarriesNoAccentBar(); void theDateFitsWhenTheCardIsBold(); + void theDateFollowsTheSystemLocale(); }; namespace { @@ -183,6 +186,42 @@ void TestCardLayout::expanderIsEmptyWithoutReplies() QVERIFY(card.expanderRect.isEmpty()); } +void TestCardLayout::theExpanderReadsAsAPillWithAWord() +{ + // A bare "3" beside the subject reads as an unexplained number and gives + // no hint that it can be clicked. The label carries the word, and the rect + // carries padding for the pill drawn behind it. + QCOMPARE(CardLayout::expanderLabel(3, false), + QStringLiteral("\u25b8 3 replies")); + QCOMPARE(CardLayout::expanderLabel(3, true), + QStringLiteral("\u25be 3 replies")); + + // Singular, because "1 replies" is the kind of detail that makes an + // interface look unfinished. + QCOMPARE(CardLayout::expanderLabel(1, false), + QStringLiteral("\u25b8 1 reply")); + + const QFont font; + const int h = CardLayout::heightFor(font); + const CardLayout card = + CardLayout::compute(threadInput(), QRect(0, 0, 400, h), font); + const QFontMetrics small(CardLayout::smallFont(font)); + + // The rect must hold the label AND its padding, or the pill's background + // is narrower than the text sitting on it. + QVERIFY2(card.expanderRect.width() + >= small.horizontalAdvance(CardLayout::expanderLabel(3, false)) + + CardLayout::kPillPaddingX * 2, + "the expander rect is too narrow for its own label and padding"); + + // And it must NOT change width when the card opens: a pill that resized on + // click would shift the subject's elision under the pointer. + CardLayout::Input open = threadInput(); + const CardLayout expanded = + CardLayout::compute(open, QRect(0, 0, 400, h), font); + QCOMPARE(expanded.expanderRect.width(), card.expanderRect.width()); +} + void TestCardLayout::dateIsFlushRight() { const QFont font; @@ -232,6 +271,42 @@ void TestCardLayout::replyCardCarriesNoAccentBar() QCOMPARE(reply.spines.size(), 1); } +void TestCardLayout::theDateFollowsTheSystemLocale() +{ + const QDateTime when(QDate(2025, 8, 10), QTime(6, 26)); + + // The system locale's own rendering, whatever it is. Asserting a specific + // string would only restate the hardcoded pattern this replaced, and would + // fail on any machine but the one that wrote it. + QCOMPARE(CardLayout::formatDate(when), + QLocale::system().toString(when, QLocale::ShortFormat)); + + // The specific fault: an ISO-looking pattern on a desktop that does not + // use one. Guarded so this test says nothing on a locale that genuinely + // formats that way. + if (QLocale::system().toString(when, QLocale::ShortFormat) + != QStringLiteral("2025-08-10 06:26")) { + QVERIFY2(CardLayout::formatDate(when) + != QStringLiteral("2025-08-10 06:26"), + "the date is hardcoded to yyyy-MM-dd hh:mm rather than " + "following the desktop's locale"); + } + + // And the reserved width has to follow the same formatter, or a locale + // whose dates are longer clips them exactly as the bold font did. + QFont font; + const int h = CardLayout::heightFor(font); + const CardLayout card = + CardLayout::compute(threadInput(), QRect(0, 0, 400, h), font); + QFont bold = font; + bold.setBold(true); + QVERIFY2(card.dateRect.width() + >= QFontMetrics(bold).horizontalAdvance( + CardLayout::formatDate(when)), + "the reserved date width is narrower than this locale's own " + "formatting of a date"); +} + void TestCardLayout::theDateFitsWhenTheCardIsBold() { // An UNREAD card draws BOLD, and bold is wider. The layout is computed from diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index cdb08ca..922705c 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -105,6 +105,7 @@ private slots: void childRowsAreIndentedUnderTheirThread(); void aThreadWithRepliesDrawsAVisibleExpander(); void cardsNeverScrollSideways(); + void selectingARootCardKeepsItsThreadForMarkRead(); void nextThreadLeavesTheLastReply(); void altDownSkipsReplies(); void bothThreadStepBindingsReachTheAction(); @@ -667,8 +668,12 @@ NavFixture buildNavFixture(MainWindow &window) f.view = window.findChild(); f.model = window.findChild(); + // unread, so a selection arms the mark-read timer: scheduleMarkRead() + // returns early for a thread that is already read, and a fixture without + // it would make a mark-read assertion pass for the wrong reason. ThreadSummary first = makeThread(QStringLiteral("T1"), - QStringList{ QStringLiteral("inbox") }); + QStringList{ QStringLiteral("inbox"), + QStringLiteral("unread") }); first.totalCount = 2; ThreadSummary second = makeThread(QStringLiteral("T2"), QStringList{ QStringLiteral("inbox") }); @@ -693,6 +698,38 @@ NavFixture buildNavFixture(MainWindow &window) } // namespace +void TestMainWindow::selectingARootCardKeepsItsThreadForMarkRead() +{ + // A root card is BOTH a message and a thread: it renders the thread's + // first message, and it is still the thread that gets marked read and + // repainted on a tag change. The message-row path deliberately clears the + // current thread id; doing that here too would silently disable mark-read + // and the tag-change repaint for every thread root in the list. + const Config config; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + const NavFixture f = buildNavFixture(window); + f.view->setCurrentIndex(f.root); + QApplication::processEvents(); + + // Guard: the fixture loads replies, so the root knows its own message and + // the branch under test is the one that runs. + QVERIFY2(!f.model->data(f.root, ThreadListModel::MessageIdRole) + .toString().isEmpty(), + "the root card does not know its first message, so this exercises " + "the fallback rather than the path it is written for"); + + // A mark-read timer armed for the thread is what proves the thread id + // survived: scheduleMarkRead() is only reached on the thread-row path. + auto *timer = window.findChild(QStringLiteral("markReadTimer")); + QVERIFY(timer); + QVERIFY2(timer->isActive(), + "no mark-read timer for a selected root card: its thread id was " + "cleared along with the switch to rendering one message"); +} + void TestMainWindow::nextThreadLeavesTheLastReply() { const Config config; diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 49b8894..aa71080 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -32,6 +32,8 @@ private slots: void repliesBecomeChildRowsUnderTheirThread(); void messageRowsShowTheirOwnSenderAndSubject(); void modelHasOneColumn(); + void aFlatThreadStillListsItsReplies(); + void theRootCardKnowsItsOwnMessage(); void replyShowsOnlyItsOwnTags(); void replySharingEveryThreadTagShowsNone(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); @@ -131,10 +133,13 @@ void TestThreadListModel::repliesBecomeChildRowsUnderTheirThread() QCOMPARE(model.data(child, ThreadListModel::ThreadIdRole).toString(), QStringLiteral("t1")); - // A thread root is not a message row and carries no message id. + // A thread root is not a message ROW, but it does carry a message id: the + // root card is the thread's first message, and selecting it renders that + // message alone. It used to answer nothing here, which is what made the + // first message of every thread unreachable. QVERIFY(!model.data(root, ThreadListModel::IsMessageRole).toBool()); - QVERIFY(model.data(root, ThreadListModel::MessageIdRole) - .toString().isEmpty()); + QCOMPARE(model.data(root, ThreadListModel::MessageIdRole).toString(), + QStringLiteral("m0@example.org")); QAbstractItemModelTester tester( &model, QAbstractItemModelTester::FailureReportingMode::Warning); @@ -1022,6 +1027,72 @@ void TestThreadListModel::modelHasOneColumn() QCOMPARE(index.data(ThreadListModel::ReplyCountRole).toInt(), 0); } +void TestThreadListModel::aFlatThreadStillListsItsReplies() +{ + // A thread whose messages carry no reply structure: notmuch returns them + // all from get_toplevel_messages at depth 0, which is what happens when the + // mail has no usable In-Reply-To. Measured in the user's own database: + // of 396 inbox threads, three are like this, one of them nine messages + // deep, and every one of them showed a reply count that expanded to + // nothing because the model kept only nodes with depth > 0. + ThreadListModel model; + ThreadSummary thread = makeThread(QStringLiteral("t1"), + QStringLiteral("flat thread")); + thread.totalCount = 3; + model.appendBatch({ thread }); + + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 0), + makeNode(QStringLiteral("m2@example.org"), 0) }); + + const QModelIndex root = model.index(0, 0); + + // Two children, not zero: the FIRST message is the root card itself, and + // the rest are its replies however flat the thread is. + QCOMPARE(model.rowCount(root), 2); + QCOMPARE(model.index(0, 0, root).data(ThreadListModel::MessageIdRole) + .toString(), + QStringLiteral("m1@example.org")); + + // And the count the card advertises must agree with the rows beneath it, + // or the expander opens onto nothing. + QCOMPARE(root.data(ThreadListModel::ReplyCountRole).toInt(), + model.rowCount(root)); +} + +void TestThreadListModel::theRootCardKnowsItsOwnMessage() +{ + // The root card IS the thread's first message, so it has to be able to say + // which message that is. Without this the pane renders the whole thread + // when the root is selected, and the first message is unreachable: the + // only rows offering it are the replies, and it is not one of them. + ThreadListModel model; + ThreadSummary thread = makeThread(QStringLiteral("t1"), + QStringLiteral("a subject")); + thread.totalCount = 2; + model.appendBatch({ thread }); + + const QModelIndex root = model.index(0, 0); + + // Before the replies are loaded there is nothing to report, and the caller + // must fall back to loading the whole thread rather than a wrong message. + QVERIFY(root.data(ThreadListModel::MessageIdRole).toString().isEmpty()); + + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + + QCOMPARE(root.data(ThreadListModel::MessageIdRole).toString(), + QStringLiteral("m0@example.org")); + + // And it is the FIRST message, not just any of them: the reply must still + // report its own. + QCOMPARE(model.index(0, 0, root).data(ThreadListModel::MessageIdRole) + .toString(), + QStringLiteral("m1@example.org")); +} + void TestThreadListModel::replyShowsOnlyItsOwnTags() { ThreadListModel model; @@ -1040,7 +1111,14 @@ void TestThreadListModel::replyShowsOnlyItsOwnTags() // Two the thread already has, one it does not. reply.tags = { QStringLiteral("inbox"), QStringLiteral("work"), QStringLiteral("todo") }; - model.setThreadMessages(QStringLiteral("T1"), { reply }); + + // Led by the thread's FIRST message, which is what the worker sends and + // what the root card draws. setThreadMessages drops it by position. + MessageNode root; + root.messageId = QStringLiteral("M1"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + model.setThreadMessages(QStringLiteral("T1"), { root, reply }); const QModelIndex threadIndex = model.index(0, 0); QVERIFY(model.hasChildren(threadIndex)); @@ -1073,7 +1151,12 @@ void TestThreadListModel::replySharingEveryThreadTagShowsNone() reply.threadId = QStringLiteral("T1"); reply.depth = 1; reply.tags = { QStringLiteral("inbox"), QStringLiteral("work") }; - model.setThreadMessages(QStringLiteral("T1"), { reply }); + + MessageNode root; + root.messageId = QStringLiteral("M1"); + root.threadId = QStringLiteral("T1"); + root.depth = 0; + model.setThreadMessages(QStringLiteral("T1"), { root, reply }); const QModelIndex replyIndex = model.index(0, 0, model.index(0, 0)); QVERIFY(replyIndex.isValid()); -- cgit v1.2.3