From f051dd657757bca3ac7e36454d7f2fc21f322f73 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Sat, 29 Aug 2026 09:49:29 +0200 Subject: fix: judge a conversation's trash state on all of its messages Item 178. everySelectedRowIsInATrashFolder() read ThreadSummary::firstMessagePath for any row that was not a message row. That was correct while a thread row MEANT that message (item 108) and stopped being correct when item 177 made it mean the conversation. A conversation is in the trash only when ALL of its messages are, so a partly trashed thread answered on whichever message the query returned first: Delete could be hidden on a conversation that still had mail outside the trash, and Restore offered on one that mostly did not. Not data-affecting. Both actions are no-ops in the wrong direction: Delete on already-trashed mail takes moveMessages()' already-there branch, and Restore on mail that was never trashed finds nothing to move. qtmaildir cannot produce such a thread itself, since Delete is absent on a reply row and Restore is thread-scoped. Two things outside it can: another client trashing a single message, and a reply arriving after the conversation was trashed. ThreadDigest already walks every message of the selected conversation for its sender counts, and a filename is served from the index like everything else in it, so the paths ride along on a request the selection already makes rather than costing a walk on every query. ThreadDigest::messagePaths is relative to the mail root, for the reason firstMessagePath records: an absolute path matches no account and silently resolves every row to none. MainWindow keeps them beside the dashboard's thread id and clears them when the dashboard is left, so a late digest cannot answer about another row. One limit, stated in the code rather than hidden. The digest is requested only for a single selected conversation row, so that is the only case with a real answer; any other selection falls back to the summary's one path. That fallback IS the pre-177 answer and is wrong in exactly the same partial case, which is the point: a multi-row selection is left no worse than it was, rather than given a second, differently wrong rule of its own. Making it exhaustive costs a per-query walk over every message, which is what this avoids. Two tests, both mutation-checked. The worker test puts its two messages in different folders, since two in one folder answer identically whichever way the code resolves them. The window test asserts both directions, so a fix that simply hid Delete everywhere would fail it, and sets totalCount explicitly: a summary left at the default is a message row, and the test would otherwise exercise the other branch and pass for the wrong reason. Suite: 42 of 43, with undoMovesTheMessageBack failing as it does on master (item 136). --- tests/test_notmuchworker.cpp | 72 ++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 72 insertions(+) (limited to 'tests/test_notmuchworker.cpp') diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index 1766589..7148f0c 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -123,6 +123,7 @@ private slots: void aDigestCountsSendersAndUnread(); void aDigestCapsItsUnreadListButNotItsCount(); + void aDigestCarriesEveryMessagePath(); void aOneMessageThreadGivesASaneSpan(); private: @@ -2242,6 +2243,77 @@ void TestNotmuchWorker::aDigestCountsSendersAndUnread() QVERIFY(digest.firstTimestamp <= digest.lastTimestamp); } +/// Item 178. Delete and Restore judge a CONVERSATION, and the summary carries +/// one message's path, so a partly-trashed thread answered on whichever +/// message the query returned first. +/// +/// The paths are collected by the walk the digest already makes over every +/// message, so this costs no extra query and no extra file read: a filename +/// comes from the INDEX, like everything else in ThreadDigest. +/// +/// The fixture puts the two messages in DIFFERENT folders deliberately. Two +/// messages in one folder answer identically whichever way the code resolves +/// them, which is the trap AGENTS.md records for item 87's opposite states. +void TestNotmuchWorker::aDigestCarriesEveryMessagePath() +{ + NotmuchFixture fixture; + QVERIFY(fixture.addMessage(QStringLiteral("inbox"), + QStringLiteral("p0@example.org"), + QStringLiteral("Split thread"), + QStringLiteral("alice@example.org"), + QStringLiteral("Mon, 24 Aug 2026 10:00:00 +0200"), + QStringLiteral("Root."), false)); + QVERIFY(fixture.addMessage(QStringLiteral("Trash"), + QStringLiteral("p1@example.org"), + QStringLiteral("Re: Split thread"), + QStringLiteral("bob@example.org"), + QStringLiteral("Tue, 25 Aug 2026 10:00:00 +0200"), + QStringLiteral("Trashed reply."), false, + QStringLiteral("p0@example.org"))); + QVERIFY2(fixture.index(), qPrintable(fixture.error())); + + NotmuchWorker worker(fixture.configPath()); + QSignalSpy spy(&worker, &NotmuchWorker::threadDigestLoaded); + + const QString threadId = worker.threadIdForTesting( + QStringLiteral("id:p0@example.org")); + QVERIFY(!threadId.isEmpty()); + worker.loadThreadDigest(threadId, 1); + + QCOMPARE(spy.count(), 1); + const ThreadDigest digest = spy.at(0).at(0).value(); + + QCOMPARE(digest.totalCount, 2); + QCOMPARE(digest.messagePaths.size(), 2); + + // RELATIVE to the mail root, for the reason ThreadSummary::firstMessagePath + // records: the UI knows an account only by its `maildir`, itself a + // database-relative prefix, so an absolute path here matches no account and + // silently resolves every row to none. + for (const QString &path : digest.messagePaths) { + QVERIFY2(!path.startsWith(QLatin1Char('/')), + qPrintable(QStringLiteral("absolute path: %1").arg(path))); + } + + // One in each folder, which is what makes a partly-trashed conversation + // answerable at all. + int inInbox = 0; + int inTrash = 0; + for (const QString &path : digest.messagePaths) { + // The mail root IS the fixture's maildir, so a relative path begins + // with the folder name and carries no leading separator. + if (path.startsWith(QStringLiteral("inbox/"))) + ++inInbox; + if (path.startsWith(QStringLiteral("Trash/"))) + ++inTrash; + } + QVERIFY2(inInbox + inTrash == 2, + qPrintable(QStringLiteral("unexpected paths: %1") + .arg(digest.messagePaths.join(QStringLiteral(", "))))); + QCOMPARE(inInbox, 1); + QCOMPARE(inTrash, 1); +} + void TestNotmuchWorker::aDigestCapsItsUnreadListButNotItsCount() { NotmuchFixture fixture; -- cgit v1.2.3