diff options
| -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"); |