diff options
| author | Danilo M. <danix@danix.xyz> | 2026-09-06 15:07:35 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-09-06 15:07:35 +0200 |
| commit | 7074a4c57343777fe08f5cdd3174563a46007484 (patch) | |
| tree | e70fef4825cb43042c6f855c9465a4adbde771dc | |
| parent | 5843f11f6809762aab52d37fbe54dd9b1792d00c (diff) | |
| download | qtmaildir-7074a4c57343777fe08f5cdd3174563a46007484.tar.gz qtmaildir-7074a4c57343777fe08f5cdd3174563a46007484.zip | |
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jq9gXquUo9W4KXDagJXMmn
| -rw-r--r-- | CHANGELOG.md | 10 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 134 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 1 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 120 | ||||
| -rw-r--r-- | src/threadlistmodel.cpp | 28 | ||||
| -rw-r--r-- | src/threadlistmodel.h | 5 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 186 |
7 files changed, 440 insertions, 44 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index 73e938e..2749e0b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -44,6 +44,16 @@ point at which they are stable. ### Fixed +- **The Sent and Drafts views show every message you sent in a conversation, + not just the first.** Both views are lists of your own messages rather than + of conversations, but a thread you had replied to twice produced a single + row: it was dated by the conversation, opened the OLDER of your two + messages, and the newer one could not be reached from the view at all. Each + matched message is now its own row, carrying its own date, subject, tags and + sender, and the view is ordered by those dates rather than by the dates of + the conversations behind them. This also fixes a quieter problem on the same + rows, where Delete or Archive would have acted on the wrong message's file. + - **A background sync now clears only the accounts it actually carried.** A sync qtmaildir did not start could only be judged from the log, which cannot say which accounts a run covered, so a successful run cleared the pending diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md index 26f90fa..af1de0c 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md @@ -9807,3 +9807,137 @@ where it is unless the user overrules the reasoning above. have their own. **Closed 2026-08-29**, unreleased. Star and Archive added to the ordinary branch; Archive left the main toolbar; `mark_all_read` stays there by the user's decision, and the assertion that it does is now in the suite. + +--- + +## 191. The Sent view collapses two messages you sent in one conversation into one row + +**Observed (user, 2026-09-06, by hand against real mail).** The Sent view for +one account showed a row dated 12:42 today whose subject was right, but whose +message pane rendered a message from three weeks earlier. The message actually +sent at 12:42 appeared nowhere in the view. Thunderbird, pointed at the same +IMAP folder, showed both. The user's own statement of what the view owes them +settled the fix: "the sent view is for my sent messages, if I sent 2 messages in +a thread, I want to see both, not threaded (as the view is not threaded), +correctly tagged and dated." + +**Two wrong diagnoses came first and are worth recording**, because both were +plausible and both were contradicted by measurement. The first read the `draft` +chip on the row and blamed item 165; the chip is real and unrelated. The second +read the row's date, found the message in `Sent/`, and concluded the row was +correct and the user was comparing a thread count against a message count; the +counts do differ for that reason, but that was not what they were reporting. The +user's correction, that the pane opened a different message from the one the row +claimed, is what located the defect. **A row's date and the message its pane +opens are two separate reads, and a defect can sit precisely in the gap.** + +**Cause, verified in the code and against the real index.** +`NotmuchWorker::walkThreads()` (`notmuchworker.cpp`) is built on +`notmuch_query_search_threads`, so it emitted exactly one `ThreadSummary` per +thread. The Sent branch then chose ONE message to stand for that thread: + +``` +if (matched) { summary.firstMessageId = ...; break; } +``` + +`notmuch_thread_get_messages` walks oldest-first, so a thread the user had +replied to twice took their OLDEST reply and stopped. The row's `date` and +`subject` came from the thread (`notmuch_thread_get_newest_date`, +`notmuch_thread_get_subject`) while `firstMessageId` named that older message, +which is why the two disagreed on screen. + +Measured on the developer's database: the conversation reported +`[2/9]` — two matched messages of nine — and `notmuch search --output=messages +--sort=oldest-first` over the sent folder returned the 17 Aug message before the +6 Sep one. The view is already flat (`setFlatMode(m_sentView)`, +`mainwindow.cpp`, and `Config::generatorIsFlat` marks `sent` and `drafts`), so +the model was right about being unthreaded and only the worker was wrong. + +**This was also a data-safety defect, which the visible symptom hid.** +`firstMessagePath`, `firstMessageTags` and `firstMessageSender` were all read +from the same wrongly chosen message. `moveMessages` composes its destination +from that path, so Delete or Archive on such a row would have moved the OLDER +message's file, silently, and mbsync would have carried it to the server. The +row the user was looking at named a file they were not looking at. + +**Fix.** The Sent branch emits one `ThreadSummary` per matched message rather +than one per thread: it no longer breaks, and for each matched message it copies +the thread-wide summary and overrides the row's identity — +`firstMessageId`, `firstMessageTags`, `firstMessageSender`, `firstMessagePath`, +and now `date` and `subject`, read from the message with +`notmuch_message_get_date` and `notmuch_message_get_header`. Batching and the +`total` counter move inside that loop, and the thread iteration `continue`s +past the single-summary append below. + +`withRecipients` continues to select the branch, so Sent and Drafts both get +this and no new flag can disagree with the flat-mode flag: the two questions are +answered by one value, which is why the original branch keyed on it. + +**One consequence needed a second change, and it is the part that would have +broken quietly.** Two rows now share a `threadId`, and +`ThreadListModel::reconcile()` keyed its `QHash<QString,int> present` on exactly +that. Two rows mapping to one key means the second looks like a thread that has +vanished, so a sync would have dropped one of the user's sent messages from the +view again, by a different route. `ThreadListModel::rowKeyFor()` is the single +answer to "what makes a row unique": the message id in flat mode, the thread id +otherwise, falling back to the thread id when a flat row carries no message id. +All four keyed sites in `reconcile()` use it. + +**Verification.** Two tests in `test_notmuchworker.cpp`, over a new fixture +thread F (`f1` sent, `f2` received, `f3` sent) which is the shape the defect +needs and which no existing fixture had: + +- `aSentQueryEmitsOneRowPerMatchedMessage` asserts both ids are present, that + the two rows share a thread id (the property that made the reconcile change + necessary), and that the SAME messages under a non-Sent query still fold into + one row, so this is the flat branch's contract and not a change of meaning + for threaded views. +- `aSentRowCarriesItsOwnMessagesDateAndSubject` asserts each row's date and + subject are its own, which is the half the user saw first. + +`aSentQueryCarriesTheMatchedMessageNotTheThreadsFirst` was RETARGETED rather +than retired: its assertion still holds, but it filtered rows by the thread's +subject, and a flat row is now titled by its own message, so it matched nothing. +It filters on the message id instead and asserts the subject. + +Eight other failures were the new fixture's arithmetic, not the change: three +messages and one thread added, so hardcoded totals moved 5→6 threads and 6→9 +messages. + +**The ordering was a second defect under the same item, found by hand after +the first fix shipped to the user's screen.** With both rows present, the 17 +August one drew ABOVE the 6 September one. The cause is that +`notmuch_query_set_sort` is a THREAD sort: it orders the threads the walk +visits and says nothing about the messages inside one, so every row of a thread +inherits that thread's single position. Sorting each thread's own rows in place +is NOT enough either, and the fixture proved it: a message from another thread +dated between two of a thread's replies still cannot land between them. A flat +view is a list of messages, so `walkThreads()` collects flat rows in `flatRows` +and sorts the whole result once, by each row's own date, before emitting. +`std::stable_sort`, so rows sharing a timestamp keep the walk's order rather +than swapping between identical queries. + +`sentRowsAreOrderedByTheirOwnDate` covers it, in both sort directions and +across threads. **The cross-thread half of it passed for the wrong reason at +first**: fixture threads D, E and F happen not to interleave, so per-thread +sorting satisfied a whole-view order check. Fixture thread G exists purely to +break that, dated between thread F's two messages, and the assertion failed +the moment it was added. A mutation check confirms the within-thread half fails +without the sort. + +**One existing test had to be corrected rather than satisfied, and the +correction is a fact about notmuch worth keeping.** +`oldestFirstReversesTheOrder` asserted the two sort directions produce exactly +reversed lists. They do not: `NOTMUCH_SORT_OLDEST_FIRST` orders threads by +their OLDEST message while `NEWEST_FIRST` orders them by their NEWEST, so the +lists mirror each other only while no thread's date span contains another's. +Thread F starts before thread G and ends after it, which is the first fixture +data to violate that. The test keeps the assertion that does hold +(`oldest.first()` is `newest.last()`) and replaces the mirror with a +monotonicity check. It passed on master and fails here for a reason that is not +a regression: the fixture finally contains the shape that distinguishes the two +sorts. + +**Still open and separate:** the four autosave revisions of that reply sitting +in `Drafts/` with distinct Message-IDs, which is item 165 and is what puts a +`draft` chip on the conversation. diff --git a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md index 8c55aad..43b241e 100644 --- a/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md +++ b/docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md @@ -264,6 +264,7 @@ taking that too literally. | 188 | Does Empty trash respect the account selector? | question | XS | **answered 2026-08-29** by reading the code, no work needed. It does: `MainWindow::emptyTrash()` (`mainwindow.cpp:6567`) reads `m_accountBox->currentData()` and uses `allTrashQuery()` only for All accounts, and the confirmation names which. Recorded so the notes' question has an answer rather than sitting open | | 189 | The message bar carries only Reply, Forward and Delete | presentation | S | **done 2026-08-29**, unreleased. Star and Archive joined the bar's ordinary branch, Archive leaving the main toolbar as Delete did. `mark_all_read` deliberately did NOT move, at the user's decision: it is the one action that ignores the selection. Item 140's toolbar test listed `archive` as a list-wide action and had to be corrected, which is the classification this item changed. Section in the closed file. Original entry: Asks for Star (`flag`) and Archive on the bar, and raises Mark all read as a question. Two of the three are selection-scoped and fit the bar's rule as it stands; **`mark_all_read` does not**, since it deliberately ignores the selection and acts on every row in the view, which is the one action in the window that does. Needs a decision from the user on that one and on whether Archive LEAVES the main toolbar the way Delete did | | 190 | Mark spam is not on the message bar, and its icon was never chosen for one | presentation | XS | open, 2026-09-06, from the notes. The bar's ordinary branch carries Reply, Forward, Star, Archive, Delete after item 189 and `spam` is not among them, though it meets the bar's rule (selection-scoped, undoable). Two halves: put it on the bar, and settle the icon, which the note asks to be "a bug, or a skull, or something that signifies bad/evil" and which is `mail-mark-junk` today, chosen for a menu where the label carries the meaning. **Paired with 187**, which changes what the action DOES (moves the file); ordering is the user's call | +| 191 | The Sent view collapses two messages you sent in one conversation into one row | defect | S | **done 2026-09-06**, unreleased, from a hand test. The Sent and Drafts views are flat, but the worker emitted one summary per THREAD and picked a single matched message to stand for it, oldest-first. A conversation replied to twice showed one row, dated by the thread and opening the OLDER message, and the newer one was reachable nowhere. Also a data-safety defect: `firstMessagePath` named the wrong file, so Delete would have moved it. A second half, found by hand once the rows appeared: the sort notmuch applies is a THREAD sort, so both rows took their thread's position and an older reply drew above a newer one. Flat rows are now sorted as one list. Section in the closed file | Sizes are rough: XS under an hour, S a sitting, M a session. 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 <notmuch.h> +#include <algorithm> + #include <QDateTime> #include <QDir> #include <QDirIterator> @@ -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<ThreadSummary> 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<ThreadSummary> &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<ThreadSummary> &threads) { // Removals first, walking BACKWARDS. Each beginRemoveRows renumbers @@ -832,14 +848,14 @@ void ThreadListModel::reconcile(const QVector<ThreadSummary> &threads) QSet<QString> 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<ThreadSummary> &threads) QHash<QString, int> 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<ThreadSummary> &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<ThreadSummary> &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. /// diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index 7148f0c..970f51d 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -80,6 +80,9 @@ private slots: void loadThreadMatchedOnlyWithNoQueryKeepsEverything(); void recipientsAreAbsentUnlessAskedFor(); + void aSentQueryEmitsOneRowPerMatchedMessage(); + void aSentRowCarriesItsOwnMessagesDateAndSubject(); + void sentRowsAreOrderedByTheirOwnDate(); void recipientsAreFoldedWhenAskedFor(); void recipientsCrossAQueuedCall(); void theFirstRecipientsAddressCrossesForTheAvatar(); @@ -205,6 +208,41 @@ void TestNotmuchWorker::initTestCase() "info@example.net, " "third@example.org"))); + // Thread F: a conversation the user replied to TWICE, which is the shape + // item 191 exists for. f1 is theirs, f2 is the correspondent's, f3 is + // theirs again. A Sent query matches f1 and f3 and must produce TWO rows, + // not one row standing for whichever of them the walk reached first. + QVERIFY(m_fixture.addMessage(QStringLiteral("sent"), QStringLiteral("f1@example.org"), + QStringLiteral("Ticket 1234"), + QStringLiteral("You <you@example.org>"), + QStringLiteral("Sun, 7 Jun 2026 10:00:00 +0000"), + QStringLiteral("my first reply"), false, QString(), + QStringLiteral("Support <support@example.org>"))); + QVERIFY(m_fixture.addMessage(QStringLiteral("inbox"), QStringLiteral("f2@example.org"), + QStringLiteral("Re: Ticket 1234"), + QStringLiteral("Support <support@example.org>"), + QStringLiteral("Mon, 8 Jun 2026 10:00:00 +0000"), + QStringLiteral("their answer"), false, + QStringLiteral("f1@example.org"))); + QVERIFY(m_fixture.addMessage(QStringLiteral("sent"), QStringLiteral("f3@example.org"), + QStringLiteral("Re: Ticket 1234"), + QStringLiteral("You <you@example.org>"), + QStringLiteral("Tue, 9 Jun 2026 14:00:00 +0000"), + QStringLiteral("my second reply"), false, + QStringLiteral("f2@example.org"), + QStringLiteral("Support <support@example.org>"))); + + // Thread G: one sent message dated BETWEEN thread F's two, which is what + // makes the cross-thread ordering assertion mean something. Without it + // the fixture's threads happen not to interleave, and sorting each + // thread's own rows is enough to pass a whole-view order check. + QVERIFY(m_fixture.addMessage(QStringLiteral("sent"), QStringLiteral("g1@example.org"), + QStringLiteral("Interleaved"), + QStringLiteral("You <you@example.org>"), + QStringLiteral("Mon, 8 Jun 2026 12:00:00 +0000"), + QStringLiteral("between f1 and f3"), false, QString(), + QStringLiteral("Someone <someone@example.org>"))); + QVERIFY2(m_fixture.index(), qPrintable(m_fixture.error())); } @@ -508,12 +546,15 @@ void TestNotmuchWorker::aSentQueryCarriesTheMatchedMessageNotTheThreadsFirst() runQuery(QStringLiteral("id:a2@example.org"), NotmuchWorker::NewestFirst, /*withRecipients=*/true); + // Matched on the MESSAGE id rather than on the subject: since item 191 a + // flat row is titled by its own message, so this row reads "Re: Release + // notes" and a subject filter for the thread's title skips it. bool sawIt = false; for (const ThreadSummary &t : asSent) { - if (t.subject != QStringLiteral("Release notes")) + if (t.firstMessageId != QStringLiteral("a2@example.org")) continue; // The REPLY, because that is what matched. Not a1, the thread's first. - QCOMPARE(t.firstMessageId, QStringLiteral("a2@example.org")); + QCOMPARE(t.subject, QStringLiteral("Re: Release notes")); sawIt = true; } QVERIFY2(sawIt, "the thread was not in the results at all"); @@ -530,6 +571,112 @@ void TestNotmuchWorker::aSentQueryCarriesTheMatchedMessageNotTheThreadsFirst() } } +void TestNotmuchWorker::aSentQueryEmitsOneRowPerMatchedMessage() +{ + // Item 191. The Sent view is a list of what the USER sent, and it is flat + // (Config::generatorIsFlat), so a conversation they replied to twice owes + // them two rows. Before this the walk stopped at the first match and the + // second reply was reachable nowhere in the view: measured on the + // developer's real mail, a message sent at 12:42 was absent while the row + // above it, dated 12:42, opened a message from three weeks earlier. + // + // Thread F is that shape: f1 and f3 are the user's, f2 is the reply + // between them. The query matches only the sent folder, as the real Sent + // filter does. + const QVector<ThreadSummary> rows = + runQuery(QStringLiteral("path:\"sent/**\" and subject:\"Ticket 1234\""), + NotmuchWorker::NewestFirst, /*withRecipients=*/true); + + QStringList ids; + for (const ThreadSummary &t : rows) + ids.append(t.firstMessageId); + std::sort(ids.begin(), ids.end()); + + const QStringList expected{ QStringLiteral("f1@example.org"), + QStringLiteral("f3@example.org") }; + QCOMPARE(ids, expected); + + // Both rows name the same thread, which is the property that makes this + // more than a loop: anything keyed on threadId alone now sees a duplicate. + QCOMPARE(rows.size(), 2); + QCOMPARE(rows.at(0).threadId, rows.at(1).threadId); + + // And an ordinary query over the same messages still folds them into ONE + // row, so this is the Sent branch's contract and not a change of meaning + // for the threaded views. + const QVector<ThreadSummary> threaded = + runQuery(QStringLiteral("subject:\"Ticket 1234\""), + NotmuchWorker::NewestFirst, /*withRecipients=*/false); + QCOMPARE(threaded.size(), 1); +} + +void TestNotmuchWorker::aSentRowCarriesItsOwnMessagesDateAndSubject() +{ + // The half the user reported first: a row dated today that opened a + // message from three weeks ago. `date` and `subject` came from the THREAD + // (newest date, thread subject) while the pane rendered `firstMessageId`, + // so the two could not agree once a thread matched twice. + const QVector<ThreadSummary> rows = + runQuery(QStringLiteral("path:\"sent/**\" and subject:\"Ticket 1234\""), + NotmuchWorker::NewestFirst, /*withRecipients=*/true); + QCOMPARE(rows.size(), 2); + + for (const ThreadSummary &t : rows) { + if (t.firstMessageId == QStringLiteral("f1@example.org")) { + QCOMPARE(t.date.toUTC().toString(QStringLiteral("yyyy-MM-dd")), + QStringLiteral("2026-06-07")); + QCOMPARE(t.subject, QStringLiteral("Ticket 1234")); + } else { + QCOMPARE(t.date.toUTC().toString(QStringLiteral("yyyy-MM-dd")), + QStringLiteral("2026-06-09")); + QCOMPARE(t.subject, QStringLiteral("Re: Ticket 1234")); + } + } +} + +void TestNotmuchWorker::sentRowsAreOrderedByTheirOwnDate() +{ + // The sort notmuch applies is a THREAD sort: notmuch_query_set_sort orders + // the threads the walk visits and says nothing about the messages inside + // one. Emitting a thread's matches in its own oldest-first walk therefore + // put the user's older reply ABOVE their newer one, both sitting at the + // position of the thread they share. Reported by hand: a message sent on + // 17/08 drawn above one sent on 06/09. + // + // Thread F holds f1 (7 Jun) and f3 (9 Jun), both the user's. + const QString sent = + QStringLiteral("path:\"sent/**\" and subject:\"Ticket 1234\""); + + QVector<QDateTime> dates; + for (const ThreadSummary &t : runQuery(sent, NotmuchWorker::NewestFirst, + /*withRecipients=*/true)) + dates.append(t.date); + QCOMPARE(dates.size(), 2); + QVERIFY2(dates.at(0) > dates.at(1), + "newest-first put the older of two messages in one thread first"); + + dates.clear(); + for (const ThreadSummary &t : runQuery(sent, NotmuchWorker::OldestFirst, + /*withRecipients=*/true)) + dates.append(t.date); + QCOMPARE(dates.size(), 2); + QVERIFY2(dates.at(0) < dates.at(1), + "oldest-first did not reverse with the requested sort"); + + // And the whole view is ordered, not merely each thread internally: the + // Sent folder's other messages must interleave correctly with these two. + // d1 is 5 Jun and e1 is 6 Jun, so newest-first owes f3, f1, e1, d1. + QVector<QDateTime> all; + for (const ThreadSummary &t : runQuery(QStringLiteral("path:\"sent/**\""), + NotmuchWorker::NewestFirst, + /*withRecipients=*/true)) + all.append(t.date); + QCOMPARE(all.size(), 5); + for (int i = 1; i < all.size(); ++i) + QVERIFY2(all.at(i - 1) >= all.at(i), + "the flat view is not in date order across threads"); +} + void TestNotmuchWorker::queryCarriesTheFirstMessageSender() { // Item 169. The card has no address to hash: `authors` is notmuch's own @@ -655,7 +802,7 @@ void TestNotmuchWorker::loadThreadTreeCarriesTheFactsARowNeeds() void TestNotmuchWorker::queryReturnsAllThreads() { const QVector<ThreadSummary> threads = runQuery(QStringLiteral("*")); - QCOMPARE(threads.size(), 5); + QCOMPARE(threads.size(), 7); } void TestNotmuchWorker::queryFiltersByTag() @@ -724,7 +871,7 @@ void TestNotmuchWorker::queryPassesGenerationThrough() QCOMPARE(ready.size(), 1); QCOMPARE(ready.first().at(1).value<quint64>(), quint64(42)); QCOMPARE(finished.size(), 1); - QCOMPARE(finished.first().at(0).toInt(), 5); + QCOMPARE(finished.first().at(0).toInt(), 7); QCOMPARE(finished.first().at(1).value<quint64>(), quint64(42)); } @@ -743,7 +890,20 @@ void TestNotmuchWorker::oldestFirstReversesTheOrder() QVERIFY(newest.first().date != newest.last().date); QCOMPARE(oldest.first().threadId, newest.last().threadId); - QCOMPARE(oldest.last().threadId, newest.first().threadId); + + // NOT the mirror assertion, and the asymmetry is notmuch's rather than + // ours: NOTMUCH_SORT_OLDEST_FIRST orders threads by their OLDEST message + // while NEWEST_FIRST orders them by their newest, so the two lists are + // reverses of each other only while no thread's span contains another + // thread's. Fixture thread F starts before thread G and ends after it, so + // F is first under newest-first and G is last under oldest-first. Measured + // on this fixture; asserting the mirror here would be asserting that + // notmuch does something it does not do. + // + // What still holds in both directions is that each list is monotonic in + // the sense its own sort defines, which is what a caller relies on. + for (int i = 1; i < newest.size(); ++i) + QVERIFY(newest.at(i - 1).date >= newest.at(i).date); } void TestNotmuchWorker::theSortOrderCrossesAQueuedCall() @@ -975,7 +1135,7 @@ void TestNotmuchWorker::queryStillWorksAfterWrite() worker.runQuery(QStringLiteral("*"), 2); QCOMPARE(ready.size(), 2); - QCOMPARE(ready.at(1).at(0).value<QVector<ThreadSummary>>().size(), 5); + QCOMPARE(ready.at(1).at(0).value<QVector<ThreadSummary>>().size(), 7); worker.applyTags(change.inverted()); } @@ -1323,7 +1483,7 @@ void TestNotmuchWorker::requestCountsAnswersOneCountPerQuery() // Threads, not messages: thread A holds two messages and must count once, // which is the number the pane's "N in inbox" line claims to be showing. const QVector<int> counts = spy.at(0).at(0).value<QVector<int>>(); - QCOMPARE(counts, QVector<int>({ 1, 5, 0 })); + QCOMPARE(counts, QVector<int>({ 1, 7, 0 })); } void TestNotmuchWorker::requestCountsKeepsPositionOnAnInvalidQuery() @@ -1353,7 +1513,7 @@ void TestNotmuchWorker::requestCountsKeepsPositionOnAnInvalidQuery() // The queries either side keep their own answers, which is the property // the pane depends on. QCOMPARE(counts.at(0), 1); - QCOMPARE(counts.at(2), 5); + QCOMPARE(counts.at(2), 7); } void TestNotmuchWorker::messageCountsCountMessagesNotThreads() @@ -1379,12 +1539,12 @@ void TestNotmuchWorker::messageCountsCountMessagesNotThreads() const QVector<int> threadCounts = threads.first().at(0).value<QVector<int>>(); - QCOMPARE(messageCounts, (QVector<int>{ 6 })); + QCOMPARE(messageCounts, (QVector<int>{ 10 })); // The guard that makes this test mean something: if requestMessageCounts // were implemented with count_threads it would return 5 here and match // the thread count, and the assertion above would be the only thing that // caught it. - QCOMPARE(threadCounts, (QVector<int>{ 5 })); + QCOMPARE(threadCounts, (QVector<int>{ 7 })); } void TestNotmuchWorker::messageCountsReportAnInvalidQueryAsMinusOne() @@ -1411,7 +1571,7 @@ void TestNotmuchWorker::messageCountsReportAnInvalidQueryAsMinusOne() QCOMPARE(counts.at(0), 0); // The query beside it keeps its own answer at its own position, which is // what pairs a count with the rule that produced it. - QCOMPARE(counts.at(1), 6); + QCOMPARE(counts.at(1), 10); } void TestNotmuchWorker::requestDatabaseStatsCountsMessagesNotThreads() @@ -1431,8 +1591,8 @@ void TestNotmuchWorker::requestDatabaseStatsCountsMessagesNotThreads() // one counts messages, which is what a user means by "how much mail". A // reimplementation that reused the thread count would report 3 here and be // confidently wrong under the label "messages". - QCOMPARE(stats.messages, 6); - QCOMPARE(stats.threads, 5); + QCOMPARE(stats.messages, 10); + QCOMPARE(stats.threads, 7); QVERIFY2(stats.messages != stats.threads, "messages and threads are equal, so this fixture cannot prove the " "two counts are distinct: add a reply to it"); |
