From bc9b22fda26ac47d5870099b281b63592996722d Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:13:30 +0200 Subject: feat(types): add MessageNode for message rows in the thread list A message row has to be drawn without opening the message, so it needs sender, subject and date. MessageRef carries none of them: it exists for rendering a thread into the pane and holds only id, path, tags and matched. depth defaults to 0, the thread's first message, which the root row stands for rather than a child row. threadId is carried so a batch of nodes names the thread it belongs to without the caller tracking it alongside. --- src/types.h | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) (limited to 'src/types.h') diff --git a/src/types.h b/src/types.h index 999d3f3..6b2ca4c 100644 --- a/src/types.h +++ b/src/types.h @@ -63,6 +63,37 @@ struct MessageRef bool matched = true; }; +/// One message as a row in the thread list. +/// +/// Separate from MessageRef, which exists for RENDERING a thread and carries +/// only what the message pane needs. A row has to be drawn without opening the +/// message at all, so the display facts live here. +struct MessageNode +{ + QString messageId; + QString threadId; ///< The thread this message belongs to. + QString from; + QString subject; + QDateTime date; + QStringList tags; + QString filePath; + + /// Reply depth within the thread. 0 is the thread's first message, which + /// occupies the ROOT row rather than a child row: the user's model is + /// "N replies", so a thread of 7 shows 1 root and 6 descendants. + int depth = 0; + + bool isUnread() const { return tags.contains(QStringLiteral("unread")); } + bool isFlagged() const { return tags.contains(QStringLiteral("flagged")); } + + /// notmuch applies "attachment" while indexing, so this needs no MIME + /// parsing, exactly as on ThreadSummary. + bool hasAttachment() const + { + return tags.contains(QStringLiteral("attachment")); + } +}; + /// One tag mutation, kept so it can be inverted for undo. struct TagChange { -- cgit v1.2.3 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/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(+) (limited to 'src/types.h') diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 11de1d4..13406e4 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 From c80c060a593fb802f7149223d0d2f8495bc1f443 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 8 Aug 2026 10:24:16 +0200 Subject: feat(model): resolve action scope from the selected row kind ActionScope is what an action is about to touch, resolved from the selection in one place so no call site reinvents the mapping. A thread root contributes the whole thread, a message row contributes one message, and messageCount is what the status bar reports. The count comes from totalCount, not from the loaded children. A thread that was never expanded still has all of its messages, and counting only the rows that happen to be on screen would understate what the action does: mutation-checked, and the wrong version reports 1 message where 7 are about to be tagged. A mixed selection is honoured as given rather than escalated to thread scope or narrowed to message scope. Silently widening it would defeat the reason the scope is shown at all. --- src/threadlistmodel.cpp | 36 ++++++++++++++++++ src/threadlistmodel.h | 8 ++++ src/types.h | 26 +++++++++++++ tests/test_threadlistmodel.cpp | 83 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 153 insertions(+) (limited to 'src/types.h') diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 6fc2177..19e1153 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -498,6 +498,42 @@ MessageNode ThreadListModel::messageAt(const QModelIndex &index) const return children.at(index.row()); } +ActionScope ThreadListModel::scopeFor(const QModelIndexList &selection) const +{ + ActionScope scope; + + for (const QModelIndex &index : selection) { + if (isMessageRow(index)) { + const MessageNode node = messageAt(index); + if (node.messageId.isEmpty() + || scope.messageIds.contains(node.messageId)) + continue; + scope.messageIds.append(node.messageId); + scope.messageCount += 1; + continue; + } + + if (index.row() < 0 || index.row() >= m_threads.size()) + continue; + + const ThreadSummary &summary = m_threads.at(index.row()).summary; + if (scope.threadIds.contains(summary.threadId)) + continue; + + scope.threadIds.append(summary.threadId); + + // totalCount, not the loaded children: a thread that was never expanded + // still has all of its messages, and counting only what happens to be + // on screen would understate what the action does. Floored at 1, since + // a summary with no count still stands for at least the message that + // produced it. + scope.messageCount += qMax(1, summary.totalCount); + scope.wholeThread = true; + } + + return scope; +} + ThreadSummary ThreadListModel::threadAt(int row) const { if (row < 0 || row >= m_threads.size()) diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index a7ce5d3..bc9fcbf 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -160,6 +160,14 @@ public: /// is not a message row. MessageNode messageAt(const QModelIndex &index) const; + /// Resolves a selection into what an action should touch. + /// + /// Mixed selections are honoured as given: a thread root and an unrelated + /// reply act on that whole thread and that one message. Nothing is + /// escalated or narrowed silently, which is the point of the scope being + /// visible in the first place. + ActionScope scopeFor(const QModelIndexList &selection) const; + /// The account keys behind a thread's account tags, for item 49's /// per-account sync. /// diff --git a/src/types.h b/src/types.h index ef822b9..d04670e 100644 --- a/src/types.h +++ b/src/types.h @@ -94,6 +94,32 @@ struct MessageNode } }; +/// What an action is about to touch, resolved from the selection. +/// +/// Exists because the thread list holds two kinds of row since item 20, so a +/// keypress alone no longer says whether it hit one message or seven. Actions +/// take one of these rather than a bare list of thread ids, and the status bar +/// reports it: this project's answer to that ambiguity is to make the scope +/// visible, not to add a confirmation dialog. See CLAUDE.md on why. +struct ActionScope +{ + QStringList threadIds; ///< Whole threads to act on. + QStringList messageIds; ///< Individual messages to act on. + + /// Messages the action will touch in total, for the status bar. A whole + /// thread contributes all of its messages, a message row contributes one. + int messageCount = 0; + + /// True when any whole thread is in scope, which drives the + /// "(whole thread)" suffix in the status bar. + bool wholeThread = false; + + bool isEmpty() const + { + return threadIds.isEmpty() && messageIds.isEmpty(); + } +}; + /// One tag mutation, kept so it can be inverted for undo. struct TagChange { diff --git a/tests/test_threadlistmodel.cpp b/tests/test_threadlistmodel.cpp index b4be527..74b8dd1 100644 --- a/tests/test_threadlistmodel.cpp +++ b/tests/test_threadlistmodel.cpp @@ -32,6 +32,9 @@ private slots: void repliesBecomeChildRowsUnderTheirThread(); void messageRowsShowTheirOwnSenderAndSubject(); void reloadingAThreadReplacesItsRepliesRatherThanRepeatingThem(); + void scopeFollowsTheSelectedRowKind(); + void scopeCountsEveryMessageOfAnUnexpandedThread(); + void scopeHonoursAMixedSelectionWithoutEscalating(); void startsEmpty(); void accountKeysComeFromTheAccountTags(); void accountKeysCoverAThreadSpanningTwoAccounts(); @@ -193,6 +196,86 @@ void TestThreadListModel::reloadingAThreadReplacesItsRepliesRatherThanRepeatingT Q_UNUSED(tester); } +void TestThreadListModel::scopeFollowsTheSelectedRowKind() +{ + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")); + t.totalCount = 3; + model.appendBatch({ t }); + model.setThreadMessages(QStringLiteral("t1"), + { makeNode(QStringLiteral("m0@example.org"), 0), + makeNode(QStringLiteral("m1@example.org"), 1) }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + const QModelIndex child = model.index(0, 0, root); + + // A thread root acts on the whole thread, and reports every message it + // stands for so the status bar can say so. + const ActionScope threadScope = model.scopeFor({ root }); + QCOMPARE(threadScope.threadIds, QStringList{ QStringLiteral("t1") }); + QVERIFY(threadScope.messageIds.isEmpty()); + QCOMPARE(threadScope.messageCount, 3); + QVERIFY(threadScope.wholeThread); + + // A message row acts on that message alone. + const ActionScope messageScope = model.scopeFor({ child }); + QVERIFY(messageScope.threadIds.isEmpty()); + QCOMPARE(messageScope.messageIds, + QStringList{ QStringLiteral("m1@example.org") }); + QCOMPARE(messageScope.messageCount, 1); + QVERIFY(!messageScope.wholeThread); +} + +void TestThreadListModel::scopeCountsEveryMessageOfAnUnexpandedThread() +{ + // totalCount, not the loaded children. A thread that was never expanded + // still has all of its messages, and counting only what happens to be on + // screen would understate what the action is about to do. + ThreadListModel model; + ThreadSummary t = makeThread(QStringLiteral("t1"), + QStringLiteral("A subject")); + t.totalCount = 7; + model.appendBatch({ t }); + + const QModelIndex root = model.index(0, 0, QModelIndex()); + QCOMPARE(model.rowCount(root), 0); // guard: nothing expanded + + const ActionScope scope = model.scopeFor({ root }); + QCOMPARE(scope.messageCount, 7); +} + +void TestThreadListModel::scopeHonoursAMixedSelectionWithoutEscalating() +{ + // Selecting a thread root and an unrelated reply acts on that whole thread + // AND that one message. Nothing is escalated to thread scope or narrowed to + // message scope silently, which is the point of the scope being visible. + ThreadListModel model; + ThreadSummary t1 = makeThread(QStringLiteral("t1"), QStringLiteral("One")); + t1.totalCount = 2; + ThreadSummary t2 = makeThread(QStringLiteral("t2"), QStringLiteral("Two")); + t2.totalCount = 5; + model.appendBatch({ t1, t2 }); + + MessageNode reply = makeNode(QStringLiteral("m1@example.org"), 1); + reply.threadId = QStringLiteral("t2"); + model.setThreadMessages(QStringLiteral("t2"), + { makeNode(QStringLiteral("m0@example.org"), 0), + reply }); + + const QModelIndex firstRoot = model.index(0, 0, QModelIndex()); + const QModelIndex secondRoot = model.index(1, 0, QModelIndex()); + const QModelIndex reply1 = model.index(0, 0, secondRoot); + + const ActionScope scope = model.scopeFor({ firstRoot, reply1 }); + QCOMPARE(scope.threadIds, QStringList{ QStringLiteral("t1") }); + QCOMPARE(scope.messageIds, QStringList{ QStringLiteral("m1@example.org") }); + + // 2 from the whole thread plus 1 for the lone message. + QCOMPARE(scope.messageCount, 3); + QVERIFY(scope.wholeThread); +} + void TestThreadListModel::accountKeysComeFromTheAccountTags() { // Item 49 reads this to decide which mbsync channels a sync needs. Only -- cgit v1.2.3