From bb3e119a55345e997efe80ec275e67acf7a04851 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/mainwindow.cpp | 2 + src/notmuchworker.cpp | 93 ++++++++++++++++++++++++++++++++++++++++++++ src/notmuchworker.h | 17 ++++++++ src/types.h | 1 + tests/test_notmuchworker.cpp | 52 +++++++++++++++++++++++++ 5 files changed, 165 insertions(+) diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index f830683..7866524 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -289,8 +289,10 @@ MainWindow::MainWindow(const Config &config, QWidget *parent) qRegisterMetaType(); qRegisterMetaType(); qRegisterMetaType(); + qRegisterMetaType(); qRegisterMetaType>(); qRegisterMetaType>(); + qRegisterMetaType>(); m_keyMap.loadDefaults(); { diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 752a52c..47bbb62 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -69,6 +69,54 @@ bool collectMessageIds(notmuch_database_t *db, const QString &query, return true; } +/// Walks a thread's reply structure depth-first, appending each message with +/// its depth. +/// +/// Takes RAW notmuch_message_t*, deliberately, against the rule that every +/// handle in this file is RAII-owned. Messages reached through a thread belong +/// to that thread and are freed with it (notmuch.h:1637), so wrapping one in +/// NmMessage would call notmuch_message_destroy on memory the thread frees +/// again. The NmThread in the caller is what keeps every pointer here alive, +/// and this must not outlive it. +/// +/// No match-set argument, unlike loadThread. A row is drawn for every message +/// in the thread regardless of the query: the list is where the user goes to +/// SEE the thread's shape, and hiding replies that did not match would make the +/// reply count disagree with the rows beneath it. +void walkReplies(notmuch_messages_t *messages, int depth, + QVector *out) +{ + for (; notmuch_messages_valid(messages); + notmuch_messages_move_to_next(messages)) { + + notmuch_message_t *message = notmuch_messages_get(messages); + if (!message) + continue; + + MessageNode node; + node.messageId = + QString::fromUtf8(notmuch_message_get_message_id(message)); + node.threadId = + QString::fromUtf8(notmuch_message_get_thread_id(message)); + node.filePath = + QString::fromUtf8(notmuch_message_get_filename(message)); + node.from = + QString::fromUtf8(notmuch_message_get_header(message, "from")); + node.subject = + QString::fromUtf8(notmuch_message_get_header(message, "subject")); + node.date = + QDateTime::fromSecsSinceEpoch(notmuch_message_get_date(message)); + node.tags = tagsOf(message); + node.depth = depth; + out->append(node); + + // NULL is a legitimate "no replies" here: notmuch_messages_valid + // accepts it and returns FALSE (notmuch.h:1630), so a leaf needs no + // guard of its own. + walkReplies(notmuch_message_get_replies(message), depth + 1, out); + } +} + } // namespace NotmuchWorker::NotmuchWorker(const QString ¬muchConfigPath, QObject *parent) @@ -242,6 +290,51 @@ void NotmuchWorker::loadThread(const QString &threadId, emit threadLoaded(result, generation); } +void NotmuchWorker::loadThreadTree(const QString &threadId, + const QString &matchQuery, + quint64 generation) +{ + // Accepted for signature symmetry with loadThread, and unused on purpose: + // see walkReplies on why every message in the thread gets a row. + Q_UNUSED(matchQuery); + + if (!openReadOnly()) + return; + + const QString query = QStringLiteral("thread:%1").arg(threadId); + NmQuery nmQuery(notmuch_query_create(m_db, query.toUtf8().constData())); + if (!nmQuery) { + emit errorOccurred( + QStringLiteral("Cannot load thread %1").arg(threadId)); + return; + } + + // search_threads, not search_messages. The messages have to come from a + // notmuch_thread_t or notmuch_message_get_replies returns NULL for every + // one of them and the walk below produces a flat list at depth 0. + notmuch_threads_t *rawThreads = nullptr; + if (notmuch_query_search_threads(nmQuery.get(), &rawThreads) + != NOTMUCH_STATUS_SUCCESS) { + emit errorOccurred( + QStringLiteral("Cannot search thread %1").arg(threadId)); + return; + } + NmThreads threads(rawThreads); + + QVector nodes; + if (notmuch_threads_valid(threads.get())) { + // Held for the whole walk: every message pointer inside belongs to this + // thread and dies with it. + NmThread thread(notmuch_threads_get(threads.get())); + if (thread) { + walkReplies(notmuch_thread_get_toplevel_messages(thread.get()), 0, + &nodes); + } + } + + emit threadTreeLoaded(nodes, generation); +} + void NotmuchWorker::applyTagsToThreads(const QStringList &threadIds, const QStringList &add, const QStringList &remove, 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); diff --git a/src/types.h b/src/types.h index 6b2ca4c..ef822b9 100644 --- a/src/types.h +++ b/src/types.h @@ -124,5 +124,6 @@ struct DatabaseStats Q_DECLARE_METATYPE(ThreadSummary) Q_DECLARE_METATYPE(MessageRef) +Q_DECLARE_METATYPE(MessageNode) Q_DECLARE_METATYPE(TagChange) Q_DECLARE_METATYPE(DatabaseStats) diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index b419915..0f47c55 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -58,6 +58,9 @@ private slots: void requestAllTagsReturnsSortedTags(); void requestAllTagsOnUnreadableConfigEmitsError(); + void loadThreadTreeReportsReplyDepth(); + void loadThreadTreeCarriesTheFactsARowNeeds(); + void requestCountsAnswersOneCountPerQuery(); void requestCountsKeepsPositionOnAnInvalidQuery(); void requestDatabaseStatsCountsMessagesNotThreads(); @@ -158,6 +161,55 @@ QStringList TestNotmuchWorker::tagsOf(const QString &messageId) return {}; } +void TestNotmuchWorker::loadThreadTreeReportsReplyDepth() +{ + // Thread A is a root plus one reply carrying In-Reply-To, which is what + // notmuch threads on. Without that header the two would be separate threads + // and this test would assert nothing about depth. + const QString threadId = threadIdOf(QStringLiteral("Release notes")); + QVERIFY(!threadId.isEmpty()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy loaded(&worker, &NotmuchWorker::threadTreeLoaded); + worker.loadThreadTree(threadId, QString(), 1); + + QCOMPARE(loaded.count(), 1); + const auto nodes = loaded.first().at(0).value>(); + + QCOMPARE(nodes.size(), 2); + QCOMPARE(nodes.at(0).messageId, QStringLiteral("a1@example.org")); + QCOMPARE(nodes.at(0).depth, 0); + QCOMPARE(nodes.at(1).messageId, QStringLiteral("a2@example.org")); + QCOMPARE(nodes.at(1).depth, 1); +} + +void TestNotmuchWorker::loadThreadTreeCarriesTheFactsARowNeeds() +{ + // A row is drawn without opening the message, so the walk has to read the + // headers. loadThread does not, which is why a separate signal exists. + const QString threadId = threadIdOf(QStringLiteral("Release notes")); + QVERIFY(!threadId.isEmpty()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy loaded(&worker, &NotmuchWorker::threadTreeLoaded); + worker.loadThreadTree(threadId, QString(), 1); + + QCOMPARE(loaded.count(), 1); + const auto nodes = loaded.first().at(0).value>(); + QCOMPARE(nodes.size(), 2); + + const MessageNode &reply = nodes.at(1); + QVERIFY(reply.from.contains(QStringLiteral("bob@example.org"))); + QCOMPARE(reply.subject, QStringLiteral("Re: Release notes")); + QVERIFY(reply.date.isValid()); + QVERIFY(!reply.filePath.isEmpty()); + + // Every node names its thread, so a batch does not need the caller to keep + // track of which thread it asked about. + QCOMPARE(reply.threadId, threadId); + QCOMPARE(nodes.at(0).threadId, threadId); +} + void TestNotmuchWorker::queryReturnsAllThreads() { const QVector threads = runQuery(QStringLiteral("*")); -- cgit v1.2.3