From c49d1317f95e435e5b5af0d0352e6743a5d57025 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:16:25 +0200 Subject: feat(worker): load a thread as a reply tree with per-message depth loadThread could not be extended to do this. It walks notmuch_query_search_messages, and a message obtained that way returns NULL from notmuch_message_get_replies (notmuch.h:1617-1628), so that walk cannot produce reply depth at all. The tree comes from notmuch_thread_get_toplevel_messages instead, and the pane keeps the flat list it wants. walkReplies takes raw notmuch_message_t*, against this file's rule that every handle is RAII-owned. Messages reached through a thread are freed with it (notmuch.h:1637), so an NmMessage wrapper would destroy memory the thread frees again. The NmThread in the caller is what keeps them alive. Every message in the thread gets a node regardless of the query: the list is where the reply count is read, and hiding unmatched replies would make that count disagree with the rows under it. Both tests mutation-checked. Flattening depth fails the depth assertion, and skipping the thread walk fails it too, so neither passes against the two mistakes the notmuch API invites. --- src/notmuchworker.h | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) (limited to 'src/notmuchworker.h') diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 7d6a587..99cad04 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -55,6 +55,21 @@ public slots: void loadThread(const QString &threadId, const QString &matchQuery, quint64 generation); + /// Loads a thread as a reply TREE, for the message rows in the list. + /// + /// Separate from loadThread rather than replacing it, for a reason that is + /// not stylistic: loadThread walks notmuch_query_search_messages, and a + /// message obtained that way returns NULL from + /// notmuch_message_get_replies (notmuch.h:1617-1628), so that walk cannot + /// produce reply depth at all. The tree has to come from + /// notmuch_thread_get_toplevel_messages instead. The message pane still + /// wants the flat list; only the list wants the tree. + /// + /// matchQuery is accepted for signature symmetry with loadThread and is + /// deliberately unused: see the comment on the walk in the .cpp. + void loadThreadTree(const QString &threadId, const QString &matchQuery, + 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`. @@ -100,6 +115,8 @@ signals: void threadsReady(const QVector &threads, quint64 generation); void queryFinished(int totalThreads, quint64 generation); void threadLoaded(const QVector &messages, quint64 generation); + void threadTreeLoaded(const QVector &nodes, + quint64 generation); void tagsApplied(const TagChange &change); void allTagsReady(const QStringList &tags, quint64 generation); -- 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/notmuchworker.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 c56d826673ec1bbd821cf703e6ee12cbef1a7ffc Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 10 Aug 2026 08:29:57 +0200 Subject: feat(worker): let a query choose newest or oldest first Sorting was hardcoded NEWEST_FIRST. Two orders only: notmuch's other two are MESSAGE_ID and UNSORTED, neither of which is an order a human wants, and sorting by sender or subject would have to happen in the model after results arrive, which fights the batching that makes a large query paint immediately. loadThread keeps OLDEST_FIRST unconditionally: a thread reads chronologically whichever way the list is sorted. --- src/notmuchworker.cpp | 7 +++++-- src/notmuchworker.h | 16 +++++++++++++++- tests/test_notmuchworker.cpp | 28 +++++++++++++++++++++++++--- 3 files changed, 45 insertions(+), 6 deletions(-) (limited to 'src/notmuchworker.h') diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 7b999cf..a6b0a29 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -170,7 +170,8 @@ void NotmuchWorker::close() } } -void NotmuchWorker::runQuery(const QString &query, quint64 generation) +void NotmuchWorker::runQuery(const QString &query, quint64 generation, + SortOrder sort) { if (!openReadOnly()) return; @@ -180,7 +181,9 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation) emit errorOccurred(QStringLiteral("Invalid query: %1").arg(query)); return; } - notmuch_query_set_sort(nmQuery.get(), NOTMUCH_SORT_NEWEST_FIRST); + notmuch_query_set_sort(nmQuery.get(), + sort == OldestFirst ? NOTMUCH_SORT_OLDEST_FIRST + : NOTMUCH_SORT_NEWEST_FIRST); notmuch_threads_t *rawThreads = nullptr; const notmuch_status_t status = diff --git a/src/notmuchworker.h b/src/notmuchworker.h index df1799d..1d8c8c0 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -44,10 +44,24 @@ public: /// Threads emitted per threadsReady() signal. static constexpr int kBatchSize = 200; + /// The sort orders offered to the user. + /// + /// Two, not four. notmuch also has NOTMUCH_SORT_MESSAGE_ID and + /// NOTMUCH_SORT_UNSORTED, and neither is an order a human wants. Sorting + /// by sender or subject is deliberately absent: notmuch cannot do it, so + /// the model would have to sort after results arrive, which fights the + /// batching that makes a 10k-thread query paint immediately. + enum SortOrder { + NewestFirst, + OldestFirst, + }; + Q_ENUM(SortOrder) + public slots: /// Runs a query. generation lets the UI discard results from a superseded /// query without the worker needing to know about cancellation. - void runQuery(const QString &query, quint64 generation); + void runQuery(const QString &query, quint64 generation, + SortOrder sort = NewestFirst); /// Loads the messages of one thread, oldest first. matchQuery is the /// user's current query; messages matching it render expanded, the rest diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index c84e262..3342013 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -39,6 +39,7 @@ private slots: void malformedQueryYieldsNoThreads(); void unreadableConfigEmitsError(); void queryPassesGenerationThrough(); + void oldestFirstReversesTheOrder(); void loadThreadReturnsMessagesOldestFirst(); void loadThreadMarksMatchedMessages(); @@ -73,7 +74,9 @@ private: QStringList tagsOf(const QString &messageId); QVector messagesOfThread(const QString &threadId, const QString &matchQuery = QString()); - QVector runQuery(const QString &query); + QVector runQuery( + const QString &query, + NotmuchWorker::SortOrder sort = NotmuchWorker::NewestFirst); QString threadIdOf(const QString &subject); NotmuchFixture m_fixture; @@ -113,13 +116,14 @@ void TestNotmuchWorker::initTestCase() QVERIFY2(m_fixture.index(), qPrintable(m_fixture.error())); } -QVector TestNotmuchWorker::runQuery(const QString &query) +QVector TestNotmuchWorker::runQuery( + const QString &query, NotmuchWorker::SortOrder sort) { NotmuchWorker worker(m_fixture.configPath()); QSignalSpy ready(&worker, &NotmuchWorker::threadsReady); QSignalSpy finished(&worker, &NotmuchWorker::queryFinished); - worker.runQuery(query, 1); + worker.runQuery(query, 1, sort); QVector all; for (const QList &args : ready) @@ -324,6 +328,24 @@ void TestNotmuchWorker::queryPassesGenerationThrough() QCOMPARE(finished.first().at(1).value(), quint64(42)); } +void TestNotmuchWorker::oldestFirstReversesTheOrder() +{ + const QVector newest = runQuery(QStringLiteral("*")); + const QVector oldest = + runQuery(QStringLiteral("*"), NotmuchWorker::OldestFirst); + + QCOMPARE(oldest.size(), newest.size()); + + // The guard: with fewer than two threads, or with every thread carrying + // the same date, a reversal is indistinguishable from no sorting at all + // and every assertion below would pass against a hardcoded order. + QVERIFY(newest.size() >= 2); + QVERIFY(newest.first().date != newest.last().date); + + QCOMPARE(oldest.first().threadId, newest.last().threadId); + QCOMPARE(oldest.last().threadId, newest.first().threadId); +} + void TestNotmuchWorker::loadThreadReturnsMessagesOldestFirst() { const QString threadId = threadIdOf(QStringLiteral("Release notes")); -- cgit v1.2.3