From dfb36213c03e678178ec1abb2327266d23015bc0 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 25 Aug 2026 18:49:55 +0200 Subject: feat: empty the trash, the one action that asks first Item 118, unblocked by 103. Message > Empty trash..., scoped to the account selector, with no default shortcut. purgeMessages() is a separate worker entry point from moveMessages() rather than a flag on it, because the two look alike and only one can be undone. It takes named ids, never a folder sweep, so the blast radius is what the dialog enumerated and the user confirmed, and it deletes every file of a message: notmuch deduplicates by Message-ID, so leaving one behind leaves the message alive in the folder the user emptied. It confirms, naming the count and the account, defaulting to Cancel. That breaks CLAUDE.md's no-confirmation rule deliberately and the rule now records it as its single exception, in the same paragraph: a purge has no inverse to push onto the undo stack, so the protection the rule provides has to come from somewhere, and the dialog is where. Two defects found rather than reasoned. The count claimed messages whose files were already gone, overstating an irreversible action; an absent file is correctly not an error, but that is not the same as destroyed. And the user's hand test found the list still showing mail that no longer existed: a purge removes rows rather than changing them, so there is no optimistic update to apply and nothing was connected to messagesPurged at all. It re-runs the current query now. Verified against the live index after the user emptied one real account's trash: zero files on disk, zero in the index. The suite is 37 of 38, the failure being item 136 on an unrelated path. Ten new strings translated, lrelease reports 0 unfinished. Item 168 is filed from the same hand test, on Delete being offered on mail already in the trash. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HFuRPtzFrSxCQjFk6tq7gD --- tests/test_mainwindow.cpp | 85 ++++++++++++++++++++++++++++ tests/test_notmuchworker.cpp | 131 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 216 insertions(+) (limited to 'tests') diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 53eea2f..6020a1d 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -260,6 +260,7 @@ private slots: void narrowingAnEmptyQueryBarIsAPlainSearch(); void aMalformedAccountIsReportedWithoutBlockingTheConstructor(); void aWorkerBackedWindowReturnsRealThreads(); + void aPurgeTakesTheRowsOutOfTheViewWithoutARefresh(); // Compose and send, item 123 task 12. void theMailRootComesFromTheConfigNotTheIndex(); @@ -383,6 +384,7 @@ private slots: void editTagsOnAReplyCountsItsOwnThreadNotTheFirstInTheList(); void markCurrentThreadReadResolvesTheThreadThroughTheIndex(); void deletingAReplyRepaintsThatReplyRow(); + void emptyTrashAsksBeforeDestroyingAnything(); void theUnreadLabelSaysWhichDirectionItWillGo(); void theUnreadLabelFollowsAWriteWithoutReselecting(); void theUnreadActionIsHiddenOnAMixedSelection(); @@ -5212,6 +5214,48 @@ void TestMainWindow::deletingAReplyRepaintsThatReplyRow() "deleting one reply marked its whole thread deleted"); } +void TestMainWindow::emptyTrashAsksBeforeDestroyingAnything() +{ + // Item 118, and the one place this application asks. CLAUDE.md rules out + // confirmation dialogs for mutations because every mutation pushes its + // inverse onto the undo stack; a purge has no inverse, so the rule does + // not reach it. What the rule protects is that a user never loses work to + // a keystroke, and here the dialog is what provides that rather than + // contradicting it. + // + // Asserting the action EXISTS and is wired, not the dialog's buttons: a + // modal cannot be driven from a test without blocking it (item 84), so + // the dialog itself is a hand test. What is pinned here is that nothing + // is destroyed without going through it. + const Config config; + MainWindow window(config); + + auto *action = window.findChild(QStringLiteral("empty_trash")); + QVERIFY2(action, "empty_trash does not exist"); + + // Reachable from a menu, which everyActionIsReachableFromAMenu() also + // enforces globally. Named here as well because an unreachable purge is + // worse than an unreachable anything else: the user cannot discover the + // action, but a stray keybinding still runs it. + bool found = false; + const QList menus = window.findChildren(); + for (QMenu *menu : menus) { + if (menu->actions().contains(action)) { + found = true; + break; + } + } + QVERIFY2(found, "empty_trash is in no menu"); + + // No shortcut, deliberately: this is the one irreversible action, and a + // chord is exactly how it would be run by accident. + QVERIFY2(action->shortcut().isEmpty(), + qPrintable(QStringLiteral("empty_trash carries the shortcut %1; " + "the one irreversible action must not " + "be a keystroke away") + .arg(action->shortcut().toString()))); +} + void TestMainWindow::theUnreadLabelSaysWhichDirectionItWillGo() { // The user's note: "the label for toggle unread should be dynamic. On an @@ -8666,6 +8710,47 @@ void TestMainWindow::aWorkerBackedWindowReturnsRealThreads() QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); } +void TestMainWindow::aPurgeTakesTheRowsOutOfTheViewWithoutARefresh() +{ + // Found by hand: the mail was destroyed correctly and the list went on + // showing it until the user re-ran the query themselves. + // + // A purge is the one mutation with no optimistic update to apply. Every + // other one CHANGES a row, so the model can rewrite it in place; this one + // takes the row away entirely, and the only honest view afterwards is the + // one the query gives now. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/trash"), QStringLiteral("doomed@example.org"), + QStringLiteral("A subject"), QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY2(backed.buildWithAccounts({ { QStringLiteral("acct"), + QStringLiteral("acct"), + QStringLiteral("trash"), + {}, {}, {} } }), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + + auto *model = window.findChild(); + QVERIFY(model); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(queryEdit); + + queryEdit->setText(QStringLiteral("path:\"acct/trash/**\"")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + + // Straight to the purge, bypassing the confirmation: a modal cannot be + // driven from a test without blocking it (item 84), and what is under + // test is what happens AFTER the user has confirmed. + window.purgeForTesting({ QStringLiteral("doomed@example.org") }); + + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 0, 15000); +} + namespace { /// A worker-backed window with one message in one account's maildir. diff --git a/tests/test_notmuchworker.cpp b/tests/test_notmuchworker.cpp index e1a21cd..3f75898 100644 --- a/tests/test_notmuchworker.cpp +++ b/tests/test_notmuchworker.cpp @@ -91,6 +91,11 @@ private slots: void moveMessagesKeepsTheMessagesTags(); void moveMessagesReportsOnlyWhatMoved(); void moveMessagesGivesTheFileAFreshMaildirName(); + void purgeMessagesDeletesTheFileAndTheIndexEntry(); + void purgeMessagesReportsWhatItDestroyed(); + void purgeMessagesLeavesOtherMessagesAlone(); + void purgeMessagesDoesNotClaimAnIdItCouldNotDelete(); + void resolveQueryMessagesRefusesAnEmptyQuery(); void moveMessagesKeepsTheMaildirFlags(); void moveMessagesRecoversWhenASyncRenamedTheFile(); void moveMessagesStillReportsAMessageThatIsReallyGone(); @@ -1285,6 +1290,132 @@ void TestNotmuchWorker::moveMessagesRelocatesTheFile() QVERIFY(!QFile::exists(before)); } +void TestNotmuchWorker::purgeMessagesDoesNotClaimAnIdItCouldNotDelete() +{ + // The report drives what the UI tells the user, and the one number they + // will remember about an irreversible action is how much it destroyed. An + // id whose file the database names but that is not on disk contributes + // nothing: the index entry is still cleaned up, but claiming it as + // destroyed would overstate what happened. + const QString real = QStringLiteral("purge6@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), real), + qPrintable(m_fixture.error())); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy purged(&worker, &NotmuchWorker::messagesPurged); + + // A KNOWN id whose file is already gone, which is the case that reaches + // the removal loop and finds nothing to unlink. An unknown id is skipped + // far earlier and proves nothing about it. + const QString stale = QStringLiteral("purge7@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), stale), + qPrintable(m_fixture.error())); + const QString staleFile = fileOf(stale); + QVERIFY(!staleFile.isEmpty()); + QVERIFY(QFile::remove(staleFile)); + + worker.purgeMessages({ real, stale }); + + QCOMPARE(purged.size(), 1); + const QStringList reported = purged.first().at(0).toStringList(); + QVERIFY2(reported.contains(real), qPrintable(reported.join(QLatin1Char(',')))); + QVERIFY2(!reported.contains(stale), + "claimed to have destroyed a message whose file was already gone"); +} + +void TestNotmuchWorker::resolveQueryMessagesRefusesAnEmptyQuery() +{ + // An EMPTY query means "match everything" to notmuch, and this walk is + // what Empty Trash enumerates from. An account with no trash folder + // configured produces an empty query, so without this guard the dialog + // would offer to destroy the entire Maildir and say so accurately. + QVERIFY2(addMovableMessage(QStringLiteral("trash"), + QStringLiteral("empty1@example.org")), + qPrintable(m_fixture.error())); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy resolved(&worker, &NotmuchWorker::threadMessagesResolved); + + worker.resolveQueryMessages(QString(), QStringLiteral("purge")); + QCOMPARE(resolved.size(), 0); +} + +void TestNotmuchWorker::purgeMessagesDeletesTheFileAndTheIndexEntry() +{ + // Item 118. The one destructive action in this application: the file is + // removed from disk and the message from the index, with no undo. Both + // halves are asserted, because either one alone leaves a visible defect: + // a file without an index entry is invisible mail on disk, and an index + // entry without a file is a row that opens onto nothing. + const QString id = QStringLiteral("purge1@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), id), + qPrintable(m_fixture.error())); + + const QString before = fileOf(id); + QVERIFY(!before.isEmpty()); + QVERIFY(QFile::exists(before)); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.purgeMessages({ id }); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + QVERIFY2(!QFile::exists(before), qPrintable(before)); + QCOMPARE(runQuery(QStringLiteral("id:%1").arg(id)).size(), 0); +} + +void TestNotmuchWorker::purgeMessagesReportsWhatItDestroyed() +{ + // The count the confirmation named has to be the count that happened, and + // the UI has nothing else to report from: unlike a move, there is no new + // path to observe afterwards. + const QString first = QStringLiteral("purge2@example.org"); + const QString second = QStringLiteral("purge3@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), first), + qPrintable(m_fixture.error())); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), second), + qPrintable(m_fixture.error())); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy purged(&worker, &NotmuchWorker::messagesPurged); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + + worker.purgeMessages({ first, second }); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + QCOMPARE(purged.size(), 1); + QStringList reported = purged.first().at(0).toStringList(); + reported.sort(); + QCOMPARE(reported, (QStringList{ first, second })); +} + +void TestNotmuchWorker::purgeMessagesLeavesOtherMessagesAlone() +{ + // The blast radius. A purge names ids, and nothing outside that list may + // be touched: this is the action with no undo, so an over-reach is not + // recoverable. The survivor is in the SAME folder, which is where a + // folder-wide delete would take everything with it. + const QString doomed = QStringLiteral("purge4@example.org"); + const QString survivor = QStringLiteral("purge5@example.org"); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), doomed), + qPrintable(m_fixture.error())); + QVERIFY2(addMovableMessage(QStringLiteral("trash"), survivor), + qPrintable(m_fixture.error())); + + const QString survivorFile = fileOf(survivor); + QVERIFY(!survivorFile.isEmpty()); + + NotmuchWorker worker(m_fixture.configPath()); + QSignalSpy errors(&worker, &NotmuchWorker::errorOccurred); + worker.purgeMessages({ doomed }); + QVERIFY2(errors.isEmpty(), qPrintable(errors.value(0).value(0).toString())); + + QCOMPARE(runQuery(QStringLiteral("id:%1").arg(doomed)).size(), 0); + QCOMPARE(runQuery(QStringLiteral("id:%1").arg(survivor)).size(), 1); + QVERIFY2(QFile::exists(survivorFile), qPrintable(survivorFile)); +} + void TestNotmuchWorker::moveMessagesReindexesAtTheNewPath() { // The half a filesystem check cannot see. A moved file with a stale index -- cgit v1.2.3