From 7074a4c57343777fe08f5cdd3174563a46007484 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sun, 6 Sep 2026 15:07:35 +0200 Subject: fix: give the Sent and Drafts views one row per message, in date order Both views are lists of the user's own messages and are flat, but walkThreads() emitted one ThreadSummary per THREAD and then picked a single matched message to stand for it, breaking at the first one the oldest-first walk reached. A conversation replied to twice therefore produced one row: dated by the thread, opening the OLDER of the two messages, with the newer one reachable nowhere in the view. Reported against real mail, where a message sent at 12:42 was missing while the row above it, dated 12:42, opened a message from three weeks earlier. The same wrongly chosen message supplied firstMessagePath, so Delete or Archive on such a row would have moved a file the user was not looking at, silently, and mbsync would have carried it to the server. That half was never visible. The Sent branch now emits one summary per matched message, each carrying its own id, tags, sender, path, date and subject. withRecipients still selects the branch, so Sent and Drafts both get this and no second flag can disagree with the flat-mode flag. Ordering was a second defect under the same item, found by hand once the rows appeared: notmuch_query_set_sort is a THREAD sort, so every row of a thread inherits that thread's single position and an older reply drew above a newer one. Sorting each thread's rows in place is not enough either, since a message from another thread dated between them still cannot land between them. Flat rows are collected and sorted as one list before emitting. ThreadListModel::rowKeyFor() is the second consequence and would have broken quietly: two rows now share a threadId, and reconcile() keyed its QHash on exactly that, so a sync would have dropped one of them by a different route. It answers what makes a row unique, the message id in flat mode and the thread id otherwise. Tests cover both halves over new fixture threads F and G. The cross-thread ordering assertion passed for the wrong reason at first, because the existing fixture threads happen not to interleave; thread G exists to break that and failed the moment it was added. oldestFirstReversesTheOrder is corrected rather than satisfied: OLDEST_FIRST orders threads by their oldest message while NEWEST_FIRST orders by their newest, so the two lists mirror each other only while no thread's date span contains another's, which this fixture is the first to violate. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Jq9gXquUo9W4KXDagJXMmn --- src/notmuchworker.cpp | 120 ++++++++++++++++++++++++++++++++++++++---------- src/threadlistmodel.cpp | 28 ++++++++--- src/threadlistmodel.h | 5 ++ 3 files changed, 122 insertions(+), 31 deletions(-) (limited to 'src') diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 3a0b2c1..c8a2485 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -27,6 +27,8 @@ #include +#include + #include #include #include @@ -468,6 +470,11 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, batch.reserve(kBatchSize); int total = 0; + // A flat view's rows, held back until every thread has been walked so they + // can be ordered against each other rather than thread by thread. Empty + // for a threaded query, which emits in notmuch's own order as it goes. + QVector flatRows; + for (; notmuch_threads_valid(threads.get()); notmuch_threads_move_to_next(threads.get())) { @@ -500,17 +507,40 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, // Two different questions, and the Sent view asks the second one. A // normal row stands for the thread's OPENING message. A Sent row // stands for what the USER sent, usually a reply and often not the - // opening message at all, so it takes the first message the query - // MATCHED. withRecipients is exactly the Sent query, which is why it - // selects between them rather than carrying a second flag that could - // disagree with it. + // opening message at all. + // + // **One row per MATCHED MESSAGE, not per thread** (item 191). The Sent + // and Drafts views are flat (`Config::generatorIsFlat`), so they are + // lists of messages and a conversation the user replied to twice owes + // them two rows. This branch used to stop at the first match, which + // made the second reply reachable NOWHERE in the view: measured on the + // developer's real mail, a message sent at 12:42 was missing while the + // row above it, dated 12:42, opened a message from three weeks + // earlier. The date came from the thread and the body from the one + // chosen message, and the two cannot agree once a thread matches + // twice. + // + // `date` and `subject` are therefore overridden per message here. + // Everything above this point is thread-wide and correct for a + // threaded view; for a flat row the MESSAGE is the row. // // Note notmuch_thread_get_matched_messages returns a COUNT, not an // iterator; there is no matched-messages list. The match state is a - // per-message flag, so the Sent branch walks in oldest-first order and - // stops at the first match. Measured at 0.146s against a 0.143s - // baseline over 4,515 threads: the walk stops early and reads the - // index, so it is as free as the toplevel call. + // per-message flag, so this walks the thread in oldest-first order and + // takes every message carrying it. The walk is the same one that was + // measured at 0.146s against a 0.143s baseline over 4,515 threads; it + // no longer stops early, but it still reads only the index. + // + // **The rows are collected and sorted as ONE list, because the sort + // notmuch applied is a THREAD sort.** `notmuch_query_set_sort` orders + // the threads this loop visits; it says nothing about the messages + // inside one, and it gives every row of a thread that thread's single + // position. Emitting them in the thread's own oldest-first walk put a + // reply from three weeks ago ABOVE the one sent today, both sitting + // where their shared thread sorted. Sorting each thread's rows + // in place does not fix it either: a message from ANOTHER thread dated + // between them still cannot land between them. A flat view is a list + // of messages, so it has to be ordered as one. if (withRecipients) { notmuch_messages_t *all = notmuch_thread_get_messages(thread.get()); for (; all && notmuch_messages_valid(all); @@ -522,24 +552,43 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, notmuch_message_get_flag_st(message, NOTMUCH_MESSAGE_FLAG_MATCH, &matched); - if (matched) { - summary.firstMessageId = QString::fromUtf8( - notmuch_message_get_message_id(message)); - // The card's own tags, beside the thread's union above. - // Same walk, same index read, no extra query. - summary.firstMessageTags = tagsOf(message); - // The card's sender, for the avatar hash (item 169). Same - // walk, and From is in the index like the tags. - summary.firstMessageSender = senderAddressOf(message); - // Which account this belongs to, for Delete's destination. - summary.firstMessagePath = QDir(dbRoot).relativeFilePath( - QString::fromUtf8( - notmuch_message_get_filename(message))); - break; - } + if (!matched) + continue; + + // A copy per matched message, so each row carries the + // thread-wide fields set above and its own identity below. + ThreadSummary row = summary; + row.firstMessageId = QString::fromUtf8( + notmuch_message_get_message_id(message)); + // The card's own tags, beside the thread's union above. + // Same walk, same index read, no extra query. + row.firstMessageTags = tagsOf(message); + // The card's sender, for the avatar hash (item 169). Same + // walk, and From is in the index like the tags. + row.firstMessageSender = senderAddressOf(message); + // Which account this belongs to, for Delete's destination. + row.firstMessagePath = QDir(dbRoot).relativeFilePath( + QString::fromUtf8( + notmuch_message_get_filename(message))); + // The row IS this message, so it is dated and titled by it. + // Reading the thread's newest date here is what put a sent + // message under a stranger's date. + row.date = QDateTime::fromSecsSinceEpoch( + notmuch_message_get_date(message)); + const char *subject = + notmuch_message_get_header(message, "subject"); + if (subject && *subject) + row.subject = QString::fromUtf8(subject); + + flatRows.append(row); } - } else if (notmuch_messages_t *top = - notmuch_thread_get_toplevel_messages(thread.get())) { + + // Sorted and emitted once the whole query has been walked, below. + continue; + } + + if (notmuch_messages_t *top = + notmuch_thread_get_toplevel_messages(thread.get())) { if (notmuch_messages_valid(top)) { if (notmuch_message_t *first = notmuch_messages_get(top)) { summary.firstMessageId = QString::fromUtf8( @@ -568,6 +617,27 @@ void NotmuchWorker::runQuery(const QString &query, quint64 generation, } } + // The flat views (Sent, Drafts) are lists of MESSAGES, so they are ordered + // by each row's own date across the whole result. std::stable_sort so rows + // sharing a timestamp keep the order the walk found them in rather than + // swapping between identical queries. + if (!flatRows.isEmpty()) { + std::stable_sort(flatRows.begin(), flatRows.end(), + [sort](const ThreadSummary &a, const ThreadSummary &b) { + return sort == OldestFirst ? a.date < b.date : a.date > b.date; + }); + + for (const ThreadSummary &row : flatRows) { + batch.append(row); + ++total; + if (batch.size() >= kBatchSize) { + emit threadsReady(batch, generation); + batch.clear(); + batch.reserve(kBatchSize); + } + } + } + if (!batch.isEmpty()) emit threadsReady(batch, generation); diff --git a/src/threadlistmodel.cpp b/src/threadlistmodel.cpp index 5c44dd4..ad1359b 100644 --- a/src/threadlistmodel.cpp +++ b/src/threadlistmodel.cpp @@ -820,6 +820,22 @@ void ThreadListModel::appendBatch(const QVector &batch) endInsertRows(); } +QString ThreadListModel::rowKeyFor(const ThreadSummary &summary) const +{ + // What makes a ROW unique, which is not always its thread. A flat view + // (Sent, Drafts) emits one row per matched MESSAGE since item 191, so two + // rows can share a threadId and keying on that alone silently drops one of + // them on every reconcile: the QHash below would map both to one row and + // the second would look like a thread that had vanished. + // + // Falls back to the thread id when a flat row somehow carries no message + // id, which keeps the old behaviour rather than collapsing every such row + // onto one empty key. + if (m_flatMode && !summary.firstMessageId.isEmpty()) + return summary.firstMessageId; + return summary.threadId; +} + void ThreadListModel::reconcile(const QVector &threads) { // Removals first, walking BACKWARDS. Each beginRemoveRows renumbers @@ -832,14 +848,14 @@ void ThreadListModel::reconcile(const QVector &threads) QSet wanted; wanted.reserve(threads.size()); for (const ThreadSummary &summary : threads) - wanted.insert(summary.threadId); + wanted.insert(rowKeyFor(summary)); for (int row = m_threads.size() - 1; row >= 0; --row) { - if (wanted.contains(m_threads.at(row).summary.threadId)) + if (wanted.contains(rowKeyFor(m_threads.at(row).summary))) continue; int first = row; while (first > 0 - && !wanted.contains(m_threads.at(first - 1).summary.threadId)) + && !wanted.contains(rowKeyFor(m_threads.at(first - 1).summary))) --first; beginRemoveRows({}, first, row); m_threads.remove(first, row - first + 1); @@ -852,7 +868,7 @@ void ThreadListModel::reconcile(const QVector &threads) QHash present; present.reserve(m_threads.size()); for (int row = 0; row < m_threads.size(); ++row) - present.insert(m_threads.at(row).summary.threadId, row); + present.insert(rowKeyFor(m_threads.at(row).summary), row); // Insertions, forwards, at the position the RESULT gives them. Walking the // result in order means each new thread is placed against rows already @@ -860,7 +876,7 @@ void ThreadListModel::reconcile(const QVector &threads) // know what that order means. for (int target = 0; target < threads.size(); ++target) { const ThreadSummary &summary = threads.at(target); - const auto it = present.constFind(summary.threadId); + const auto it = present.constFind(rowKeyFor(summary)); if (it == present.constEnd()) { const int at = qMin(target, m_threads.size()); @@ -903,7 +919,7 @@ void ThreadListModel::reconcile(const QVector &threads) if (entry.value() >= target && entry.value() < row) ++entry.value(); } - present[summary.threadId] = target; + present[rowKeyFor(summary)] = target; row = target; } diff --git a/src/threadlistmodel.h b/src/threadlistmodel.h index fee4a1f..bed099d 100644 --- a/src/threadlistmodel.h +++ b/src/threadlistmodel.h @@ -430,6 +430,11 @@ private: bool loaded = false; }; + /// What makes a ROW unique: the message id in a flat view, the thread id + /// otherwise. Two flat rows can share a thread since item 191, so + /// reconcile() cannot key on threadId alone without dropping one of them. + QString rowKeyFor(const ThreadSummary &summary) const; + /// A newly arrived thread, with its card's own message seeded from the /// query. /// -- cgit v1.2.3