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. --- src/mainwindow.h | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) (limited to 'src/mainwindow.h') diff --git a/src/mainwindow.h b/src/mainwindow.h index d8401a3..992cdd6 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -41,7 +41,7 @@ class QAction; class QLineEdit; class QMenu; -class QTableView; +class ThreadListView; class QLabel; class QPushButton; class QComboBox; @@ -212,6 +212,15 @@ private: /// A missing or rejected blob leaves the buildUi() defaults in place. void restoreUiState(); + /// The thread row containing an index: itself for a thread row, its parent + /// for a message row. + QModelIndex threadRowOf(const QModelIndex &index) const; + + /// Selects a whole row. QTreeView has no selectRow of its own. + void selectRowAt(const QModelIndex &index); + + /// Selects the top-level thread row at `row`. + void selectThreadRow(int row); void saveUiState() const; void registerActions(); @@ -410,7 +419,10 @@ private: QLineEdit *m_queryEdit = nullptr; QueryCompleter *m_queryCompleter = nullptr; - QTableView *m_threadView = nullptr; + /// Its own type, not the QTreeView base. The strip painting and the + /// expander column are ThreadListView's, and holding the base here only + /// hid that from every reader. + ThreadListView *m_threadView = nullptr; /// Right-click menu for the thread list, holding the same QActions the /// menu bar does. -- cgit v1.2.3 From 98250d51021aee8929d9f5084a440647c43132b0 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:37:48 +0200 Subject: feat(ui): load a thread's replies when its row is expanded Replies are fetched on expansion rather than with the query: walking the reply tree of every thread in a 10k-thread result would cost more than the query and almost none of it would be looked at. hasChildren is what makes that lazy loading work, and its absence would have shipped the feature unreachable. rowCount is 0 until the worker has walked the thread, so a view left to infer the expander from rowCount alone draws none, the user can never expand, and the replies are never requested. It answers from the summary's totalCount before loading and from the children afterwards, so a thread whose count included duplicates stops offering an expander that opens onto nothing. onThreadTreeLoaded reads the thread id from the reply rather than remembering it from the request. Two expansions can be in flight at once, and pairing them by order would attach one thread's replies to the other. --- src/mainwindow.cpp | 40 ++++++++++++++++++++++++++++++++++++++++ src/mainwindow.h | 7 +++++++ src/threadlistmodel.cpp | 29 +++++++++++++++++++++++++++++ src/threadlistmodel.h | 10 ++++++++++ tests/test_threadlistmodel.cpp | 41 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 127 insertions(+) (limited to 'src/mainwindow.h') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 740edbe..720ec60 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -606,6 +606,12 @@ void MainWindow::buildUi() m_threadView->setColumnWidth(ThreadListModel::AuthorsColumn, 180); m_threadView->setColumnWidth(ThreadListModel::SubjectColumn, 520); + // 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 + // far more than the query itself and almost none of it would be looked at. + connect(m_threadView, &QTreeView::expanded, + this, &MainWindow::onThreadExpanded); + connect(m_threadView->selectionModel(), &QItemSelectionModel::currentRowChanged, this, &MainWindow::onThreadSelected); @@ -1298,6 +1304,8 @@ void MainWindow::wireWorker() this, &MainWindow::onThreadsReady); connect(m_worker, &NotmuchWorker::queryFinished, this, &MainWindow::onQueryFinished); + connect(m_worker, &NotmuchWorker::threadTreeLoaded, + this, &MainWindow::onThreadTreeLoaded); connect(m_worker, &NotmuchWorker::threadLoaded, this, &MainWindow::onThreadLoaded); connect(m_worker, &NotmuchWorker::errorOccurred, @@ -1667,6 +1675,38 @@ void MainWindow::onThreadSelected(const QModelIndex ¤t, Q_ARG(quint64, m_generation)); } +void MainWindow::onThreadExpanded(const QModelIndex &index) +{ + if (!index.isValid() || m_model->isMessageRow(index)) + return; + + const QString threadId = + m_model->data(index, ThreadListModel::ThreadIdRole).toString(); + if (threadId.isEmpty()) + return; + + QMetaObject::invokeMethod(m_worker, "loadThreadTree", Qt::QueuedConnection, + Q_ARG(QString, threadId), + Q_ARG(QString, m_lastQuery), + Q_ARG(quint64, m_generation)); +} + +void MainWindow::onThreadTreeLoaded(const QVector &nodes, + quint64 generation) +{ + // The same generation guard every other worker reply carries: an expansion + // whose query has since been replaced must not insert rows into the new + // result, where that thread may not even appear. + if (generation != m_generation || nodes.isEmpty()) + return; + + // Every node in one reply belongs to one thread, so the first one names it. + // Read from the node rather than remembered from the request: two + // expansions can be in flight at once, and pairing them by order would + // attach one thread's replies to the other. + m_model->setThreadMessages(nodes.first().threadId, nodes); +} + void MainWindow::onThreadLoaded(const QVector &messages, quint64 generation) { diff --git a/src/mainwindow.h b/src/mainwindow.h index 992cdd6..7014855 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -164,6 +164,13 @@ private slots: /// the click lands inside. void showThreadContextMenu(const QPoint &pos); void onThreadLoaded(const QVector &messages, quint64 generation); + + /// Asks the worker for a thread's reply tree when its row is expanded. + void onThreadExpanded(const QModelIndex &index); + + /// Fills in the expanded thread's message rows. + void onThreadTreeLoaded(const QVector &nodes, + quint64 generation); void onWorkerError(const QString &message); void onSyncFinished(bool success, int exitCode); diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 19e1153..d9882dc 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -151,6 +151,35 @@ int ThreadListModel::rowCount(const QModelIndex &parent) const return m_threads.at(parent.row()).children.size(); } +bool ThreadListModel::hasChildren(const QModelIndex &parent) const +{ + if (!parent.isValid()) + return !m_threads.isEmpty(); + + // A message row is always a leaf. Reply depth is drawn from the node's own + // depth, not from further nesting, so nothing hangs under a reply. + if (parent.parent().isValid()) + return false; + + if (parent.column() != 0) + return false; + + if (parent.row() < 0 || parent.row() >= m_threads.size()) + return false; + + const ThreadNode &node = m_threads.at(parent.row()); + + // Once loaded the children are the truth, including "there are none", which + // is how a thread whose totalCount counted duplicates stops offering an + // expander that opens onto nothing. + if (node.loaded) + return !node.children.isEmpty(); + + // Before loading, the summary's count is all there is. A thread of one + // message has no replies and must not offer an expander. + return node.summary.totalCount > 1; +} + int ThreadListModel::columnCount(const QModelIndex &parent) const { // Every level has the same columns. Returning 0 for a valid parent, as the diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index bc9fcbf..e39ade6 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -134,6 +134,16 @@ public: int rowCount(const QModelIndex &parent = {}) const override; int columnCount(const QModelIndex &parent = {}) const override; + + /// Whether a thread row should offer an expander. + /// + /// Answered from totalCount rather than from the loaded children, and that + /// is what makes lazy loading possible at all: rowCount is 0 until the + /// worker has walked the thread, so a view left to infer this from rowCount + /// alone draws no expander, the user can never expand, and the replies are + /// never asked for. The count is already in the summary, so this costs + /// nothing. + bool hasChildren(const QModelIndex &parent = {}) const override; QVariant data(const QModelIndex &index, int role) const override; QVariant headerData(int section, Qt::Orientation orientation, int role) const override; diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index 74b8dd1..3d131cf 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -32,6 +32,7 @@ private slots: void repliesBecomeChildRowsUnderTheirThread(); void messageRowsShowTheirOwnSenderAndSubject(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); + void anUnexpandedMultiMessageThreadOffersAnExpander(); void scopeFollowsTheSelectedRowKind(); void scopeCountsEveryMessageOfAnUnexpandedThread(); void scopeHonoursAMixedSelectionWithoutEscalating(); @@ -196,6 +197,46 @@ void TestThreadListModel::reloadingAThreadReplacesItsRepliesRatherThanRepeatingT Q_UNUSED(tester); } +void TestThreadListModel::anUnexpandedMultiMessageThreadOffersAnExpander() +{ + // This is what makes lazy loading work at all. rowCount is 0 until the + // worker has walked the thread, so a view inferring the expander from + // rowCount alone draws none, the user can never expand, and the replies are + // never requested. hasChildren answers from the summary's count instead. + ThreadListModel model; + ThreadSummary many = makeThread(QStringLiteral("t1"), + QStringLiteral("Has replies")); + many.totalCount = 4; + ThreadSummary lone = makeThread(QStringLiteral("t2"), + QStringLiteral("Single message")); + lone.totalCount = 1; + model.appendBatch({ many, lone }); + + const QModelIndex withReplies = model.index(0, 0, QModelIndex()); + const QModelIndex single = model.index(1, 0, QModelIndex()); + + // Guard: neither is expanded, so this really is the unloaded case. + QCOMPARE(model.rowCount(withReplies), 0); + QCOMPARE(model.rowCount(single), 0); + + QVERIFY(model.hasChildren(withReplies)); + QVERIFY(!model.hasChildren(single)); + + // Once loaded the children are the truth, including "there are none": a + // thread whose count included duplicates must stop offering an expander + // that opens onto nothing. + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0) }); + QVERIFY(!model.hasChildren(withReplies)); + + // A message row is always a leaf. + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + QVERIFY(model.hasChildren(withReplies)); + QVERIFY(!model.hasChildren(model.index(0, 0, withReplies))); +} + void TestThreadListModel::scopeFollowsTheSelectedRowKind() { ThreadListModel model; -- 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 'src/mainwindow.h') 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 'src/mainwindow.h') 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 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 'src/mainwindow.h') 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