diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-25 18:49:55 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-25 18:49:55 +0200 |
| commit | dfb36213c03e678178ec1abb2327266d23015bc0 (patch) | |
| tree | 4c4bf7c50e509af78d9a9be09c819b81c9ba90de | |
| parent | 3955ff80e3b0d558cd854c4517d1835514010b90 (diff) | |
| download | qtmaildir-dfb36213c03e678178ec1abb2327266d23015bc0.tar.gz qtmaildir-dfb36213c03e678178ec1abb2327266d23015bc0.zip | |
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HFuRPtzFrSxCQjFk6tq7gD
| -rw-r--r-- | CLAUDE.md | 12 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 82 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 109 | ||||
| -rw-r--r-- | src/keymap.cpp | 4 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 109 | ||||
| -rw-r--r-- | src/mainwindow.h | 16 | ||||
| -rw-r--r-- | src/notmuchworker.cpp | 101 | ||||
| -rw-r--r-- | src/notmuchworker.h | 26 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 85 | ||||
| -rw-r--r-- | tests/test_notmuchworker.cpp | 131 | ||||
| -rw-r--r-- | translations/qtmaildir_it_IT.ts | 50 |
11 files changed, 689 insertions, 36 deletions
@@ -236,7 +236,17 @@ open handle per process, so that close-first ordering is required, not stylistic **No dry-run, no destructive-action confirmation.** Those gates belonged to the retired `mailctl` CLI, where they restrained an agent; a human at a GUI gets **undo** instead — every mutation pushes its inverse (`TagChange::inverted()`) onto a `QUndoStack`. Do not add -confirmation dialogs for tag mutations. All actions funnel through one `applyTags` path; +confirmation dialogs for tag mutations. + +**There is exactly ONE exception, and its shape is the rule's own logic rather +than a hole in it.** `empty_trash` (item 118) destroys files and index entries, +so it has no inverse to push, and the protection the rule actually provides — +that a user never loses work to a keystroke — has to come from somewhere else. +It therefore asks, naming the count and the account, defaulting to Cancel, and +it carries **no default shortcut** for the same reason. `NotmuchWorker::purgeMessages()` +is a separate entry point from `moveMessages()` deliberately: the two look +alike and only one of them can be undone. A second confirmation anywhere is a +defect unless the action is likewise irreversible. All actions funnel through one `applyTags` path; multi-row selections go through `applyTagsToThreads`, which resolves every thread in ONE combined `thread:a or thread:b` query rather than one query per thread. 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 206a550..8092fb4 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 @@ -8313,3 +8313,85 @@ reporting 0 unfinished. **Size: S.** Done, at roughly twice the entry's scope because the entry's scope was wrong. + +## 118. No way to empty the trash from inside the app + +**Observed (user, 2026-08-17):** raised while reviewing item 103's spec, as +something that had been forgotten rather than newly noticed: "we could add +'Empty Trash' to the backlog as a future item. I forgot it existed, but I don't +want to squeeze it in this spec." + +**Blocked on 103**, which creates the trash folder this would empty. Until that +ships there is nothing to empty: Delete writes a tag and moves no file, so no +account has a populated trash folder except through another client. + +**Deliberately excluded from 103's spec**, at the user's request and recorded in +its "Out of scope" section. Worth keeping separate for a reason beyond scope +control: emptying the trash is the first action in this application that would +destroy mail with no undo. Every mutation so far is a tag or, after 103, a move, +and both are reversible. A purge is not. + +**Approach, unspecified.** The shape depends on decisions not yet made, and the +spec for 103 answers none of them: + +- **Local or remote.** Deleting the files locally and letting `Expunge Both` + carry it to the server is one thing; asking the provider to empty its own + trash is another, and mbsync offers no verb for the latter. The first is + probably what "Empty Trash" should mean here. +- **Whether the no-confirmation rule survives it.** It does not, on the face of + it. `CLAUDE.md` grants undo in place of confirmation dialogs, and this is the + action where undo cannot exist. That makes it the second item, after 103, that + re-examines the rule rather than assuming it, and unlike 103 it will probably + have to break it. +- **Per-account or all-accounts**, which should follow whatever the Trash filter + does once 103 ships rather than being decided independently. + +**Built 2026-08-25**, unblocked by 103. The three questions the entry left open +were put to the user and answered: + +- **Local, and let the sync carry it.** The files go, and a channel with + `Expunge Both` propagates that to the server. mbsync offers no verb for + asking a provider to empty its own trash, so the alternative was to delete + locally and not care, which brings the mail back on the next sync and reads + as the action having silently failed. +- **It confirms**, naming the count and the account, defaulting to Cancel, with + no default shortcut. CLAUDE.md now records this as the ONE exception to the + no-confirmation rule, in the same paragraph that states the rule, so the next + reader meets both together. +- **Scoped to the account selector**, like every other account-aware surface, + which is what the entry asked for. + +`NotmuchWorker::purgeMessages()` is a separate 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 only, never a folder sweep, so the blast +radius is what the dialog enumerated and the user confirmed. It deletes EVERY +file of a message: notmuch deduplicates by Message-ID, and leaving one behind +would leave the message alive in the folder the user emptied, which is the same +one-message-many-files property item 166 turned on. + +`resolveQueryMessages()` is a four-line wrapper over the existing private +`resolveQuery()`, so enumerating what is about to be destroyed needed no new +walk. The count in the dialog comes from the DATABASE rather than the model, +which holds whatever the current view is showing and is usually not the trash. + +**A defect surfaced while writing the tests**, and it is the one worth +remembering: the first version counted a message whose file was already gone as +destroyed, so the number reported for an irreversible action overstated it. An +absent file is correctly not an ERROR, since the index can name a path a sync +has removed; the mistake was treating "not an error" as "destroyed". The +mutation that restores it now fails. + +**A second defect was found by the user's own hand test**: the mail was +destroyed correctly and the LIST went on showing it until they re-ran the query +themselves. A purge is the one mutation with no optimistic update available, +because it removes rows rather than changing them, so `messagesPurged` re-runs +the current query. Nothing was connected to that signal at all, which is the +kind of gap a green suite is happy to keep. + +Verified against the live index after the user emptied one real account's +trash: zero files on disk, zero in the index. + +**Item 168 was filed from the same hand test**, on Delete being offered on mail +already in the trash. + +**Size: S.** Done. 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 ab346f5..37dc190 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 @@ -184,7 +184,7 @@ taking that too literally. | 115 | A copy from the message pane gives no confirmation | presentation | XS | **done** 2026-08-19, unreleased. Four entries report, each naming what it copied; connected to the page's own QActions, so the entry is covered wherever it is triggered from | | 116 | Copy image copies markup instead of the image | defect | XS | **dropped** 2026-08-17, same day. NOT A DEFECT: `wl-paste --list-types` run immediately after a copy reports `image/png`, `application/x-qt-image` and 30 more image flavours. The clipboard is correct and Chromium is behaving. The earlier "text only" reading was taken minutes late off a clipboard that had been overwritten, and a whole cause was theorised on it | | 117 | The message pane offers no Select all | workflow | XS | **done** 2026-08-19, unreleased. `addPaneActions()` supplies it. The call site is NOT covered by a test and cannot be: the production menu needs a real context-menu event. Stated in the test rather than faked | -| 118 | No way to empty the trash from inside the app | workflow | S | open, 2026-08-17. **Blocked on 103**, which creates the trash in the first place. Deliberately left out of 103's spec at the user's request rather than squeezed in | +| 118 | No way to empty the trash from inside the app | workflow | S | **done 2026-08-25**, unreleased. Unblocked by 103. `Message > Empty trash...`, scoped to the account selector, no shortcut. The one confirmation in this application, and CLAUDE.md now records it as the single exception rather than leaving it to be discovered. Found a defect while testing: the count claimed messages whose files were already gone | | 119 | The unsynced-changes count cannot be opened to see what it counts | information | S | open, 2026-08-19, from the notes. One of the four things it sums carries no message ids at all, so a list cannot be complete without a change to how the count is kept | | 121 | The thread list shows nothing while a query is running | feedback | S | open, 2026-08-20, from the notes. Follows item 74, which fixed the status-bar half and left the list itself blank | @@ -241,6 +241,7 @@ taking that too literally. | 165 | A draft gets a new Message-ID on every autosave | enhancement | ? | open, 2026-08-25, found while hand-testing 163 and 164. `MessageBuilder::build()` generates an id unconditionally and every autosave calls it, so each revision is a distinct MESSAGE to notmuch and to the server rather than a new version of one. Invisible while the file is replaced correctly, which item 163's fix restores; it is what turned that fork into two messages rather than one duplicated file. Needs a DECISION on what a draft's identity is before any code: a stable id reused at send, a stable id discarded at send, or the status quo. Neither `ComposeContext` nor `OutgoingMessage` has a field to carry an id, so it is not a changed call site | | 166 | Mail you send to your own other account loses `inbox` | defect | S | **done 2026-08-25**, unreleased. `sent_only()` keeps a message only when EVERY file is inside a sent folder, which is what the carve-out's docstring already claimed. No query can express it, measured; the root comes from `database.mail_root`, with a split-index fixture the ordinary layout cannot provide. Verified read-only against the live index: 780 of 807 still stripped, 27 spared, no arrival affected | | 167 | No way to tell one build of an unreleased version from another | enhancement | XS | **done 2026-08-25**, unreleased. The user chose a counter over a git description: `QTMAILDIR_BUILD_NUMBER`, a cmake option ON by default, increments a counter in the BUILD directory on every build and writes `buildnumber.h`. `QTMAILDIR_VERSION_DISPLAY` carries it; `QTMAILDIR_VERSION` stays clean and is what the window title, `applicationVersion` and the release procedure use | +| 168 | Delete is offered on mail already in the trash, and does nothing | defect | S | open, found 2026-08-25 by the user while hand-testing 118. `moveMessages()` takes its already-in-destination branch, reports the message as moved and counts an unsynced change for a move that never happened. Not dangerous, it purges nothing. Restore has the mirror of the same problem: it is offered on mail that was never deleted. The user chose to hide each where it has no meaning, the same principle as 112's hidden unread entry. **Carries a second request**: Delete should also drop `unread` | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -672,40 +673,6 @@ make Save image work must not make Save link reachable again. The test fails if it does, which is the point: the handler is per-profile, so the natural implementation would light up both entries at once. -## 118. No way to empty the trash from inside the app - -**Observed (user, 2026-08-17):** raised while reviewing item 103's spec, as -something that had been forgotten rather than newly noticed: "we could add -'Empty Trash' to the backlog as a future item. I forgot it existed, but I don't -want to squeeze it in this spec." - -**Blocked on 103**, which creates the trash folder this would empty. Until that -ships there is nothing to empty: Delete writes a tag and moves no file, so no -account has a populated trash folder except through another client. - -**Deliberately excluded from 103's spec**, at the user's request and recorded in -its "Out of scope" section. Worth keeping separate for a reason beyond scope -control: emptying the trash is the first action in this application that would -destroy mail with no undo. Every mutation so far is a tag or, after 103, a move, -and both are reversible. A purge is not. - -**Approach, unspecified.** The shape depends on decisions not yet made, and the -spec for 103 answers none of them: - -- **Local or remote.** Deleting the files locally and letting `Expunge Both` - carry it to the server is one thing; asking the provider to empty its own - trash is another, and mbsync offers no verb for the latter. The first is - probably what "Empty Trash" should mean here. -- **Whether the no-confirmation rule survives it.** It does not, on the face of - it. `CLAUDE.md` grants undo in place of confirmation dialogs, and this is the - action where undo cannot exist. That makes it the second item, after 103, that - re-examines the rule rather than assuming it, and unlike 103 it will probably - have to break it. -- **Per-account or all-accounts**, which should follow whatever the Trash filter - does once 103 ships rather than being decided independently. - -**Size: S**, provisionally, and not worth sizing properly until 103 exists. - ## 119. The unsynced-changes count cannot be opened to see what it counts **Observed (user, from the notes):** "the bottom left statusbar message needs to @@ -1380,3 +1347,75 @@ id and a draft of a reply carries both. - Item 163's fix stands on its own and this does not block it: the file is replaced correctly now, so the fork this would have mitigated no longer happens by that route. + +## 168. Delete is offered on mail already in the trash, and does nothing + +**Observed (user, 2026-08-25, while hand-testing item 118):** "I noticed I can +hit delete via context menu on a message already in the trash. Seems like a +bug, unless that action doesn't do for one message what Empty trash does for +the whole view." + +It does not, and the guess in the second half is worth recording as the reason +this matters: the user's mental model was that Delete on already-trashed mail +might PURGE it. It does not, and nothing about the menu says so. + +**Cause (verified in code, 2026-08-25.)** `moveMessages()` compares the file's +directory against the destination and takes an early-return branch when they +match (`notmuchworker.cpp`, the "already where it was asked to go" branch, +added when a fresh Maildir name made a path comparison useless). That branch +appends the id to `moved` and records an origin, so the message is reported as +having moved when nothing happened. The UI counts an unsynced change for it. + +Nothing is destroyed and nothing is corrupted; the cost is a menu entry that +lies about having done something, and a pending-changes count that overstates +what a sync has to carry. + +**The mirror of the same defect is already shipped beside it.** `restore` is +added unconditionally to both the Message menu (`mainwindow.cpp:1956`) and the +thread context menu (`mainwindow.cpp:2119`), so it is offered on mail that was +never deleted, where it has as little meaning as Delete has in the trash. + +**Approach.** The user chose to hide each action where it has no meaning, +which is the principle item 112 established for the unread entry: an action +with no honest meaning for the selection is absent rather than present and +inert. + +- Delete is hidden when every selected row is already in a trash folder. +- Restore is hidden when no selected row is. +- The test for both needs a MIXED selection as well as uniform ones, for the + reason item 112 records: a selection whose rows agree answers identically + whichever way the predicate is computed. + +**Constraints.** + +- **The question is about the PATH, not the tag.** A message trashed by + another client carries no `deleted` tag at all, which is why item 103 made + the trash view path-based. Asking `tags.contains("deleted")` here would + offer Delete on exactly the mail the user is most likely to be looking at + in a trash view. +- **`selectionTagPresence()` is the wrong instrument** for the same reason, + though it is the right shape. A path predicate needs the row's path, which + `MessageNode` carries. +- Deciding this does not require deciding item 118's relationship to it: a + purge stays an explicit whole-view action, and hiding Delete does not make + Delete a purge. + +**A second request, from the same tangent (user, 2026-08-25):** "messages moved +to the trash should be automatically marked `-unread`." Deleting is a decision +about the message, so leaving it bold and unread in the trash is noise; the +count of unread mail should not include what the user threw away. + +It is one line where Delete already composes its tag change, and it carries a +constraint worth stating rather than discovering. `maildir.synchronize_flags` +is true, so removing `unread` REWRITES the Maildir filename and reaches the +server on the next mbsync. That is acceptable here and is a deliberate +exception: it is the same mechanism the `post-new` hook refuses to touch on +arriving mail, for the good reason that the hook acts unattended on mail the +user has not seen. A Delete is an explicit gesture on a message in front of +them, which is the difference. + +Undo must put it back. `TagChange::inverted()` already does, provided the +removal travels as part of the SAME change rather than as a second write, so +one undo returns both the folder and the tag. + +**Size: S** for the visibility half, XS for the `unread` half. diff --git a/src/keymap.cpp b/src/keymap.cpp index 269a7d5..6605882 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -33,6 +33,10 @@ QStringList KeyMap::knownActions() QStringLiteral("delete"), QStringLiteral("restore"), QStringLiteral("cleanup_stranded"), + // Item 118. No default binding, deliberately: this is the one action + // that destroys mail with no undo, and a chord is how it would be run + // by accident. Menu only, which item 132 made a legitimate choice. + QStringLiteral("empty_trash"), QStringLiteral("spam"), QStringLiteral("toggle_unread"), QStringLiteral("mark_all_read"), diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index fd954b0..dd7bc68 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -1588,6 +1588,19 @@ void MainWindow::registerActions() [this]() { showStrandedDeletedMail(); }); + // The ONE irreversible action in this application, and the only one that + // asks before it runs (item 118). 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 the user never loses work to a + // keystroke, which here is what the dialog provides. + // + // No default shortcut, for the same reason: a chord is how this would be + // run by accident. + addAction(QStringLiteral("empty_trash"), tr("Empt&y trash..."), + tr("Permanently delete every message in the trash"), [this]() { + emptyTrash(); + }); addAction(QStringLiteral("spam"), tr("Mark &spam"), tr("Add spam and remove inbox"), [this]() { tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, @@ -1957,6 +1970,7 @@ void MainWindow::buildMenus() // It replaces the whole view like a filter does, so a sixth button beside // the five filters would read as one of them. messageMenu->addAction(m_actions.value(QStringLiteral("cleanup_stranded"))); + messageMenu->addAction(m_actions.value(QStringLiteral("empty_trash"))); messageMenu->addAction(m_actions.value(QStringLiteral("tag_rules"))); auto *viewMenu = menuBar()->addMenu(tr("&View")); @@ -2017,6 +2031,7 @@ void MainWindow::buildMenus() // nothing, so an icon from the delete family would promise the one // thing it deliberately does not do. { QStringLiteral("cleanup_stranded"), QStringLiteral("system-search") }, + { QStringLiteral("empty_trash"), QStringLiteral("edit-delete-shred") }, { QStringLiteral("undo"), QStringLiteral("edit-undo") }, { QStringLiteral("spam"), QStringLiteral("mail-mark-junk") }, { QStringLiteral("flag"), QStringLiteral("mail-mark-important") }, @@ -2540,6 +2555,18 @@ void MainWindow::wireWorker() connect(m_worker, &NotmuchWorker::messagesMovedFrom, this, &MainWindow::onMessagesMoved); + // A purge removes rows rather than changing them, so there is no + // optimistic update to apply: the only honest view is the one the query + // gives now. Without this the list went on showing mail that no longer + // existed until the user refreshed by hand, which is how the user found + // it. + connect(m_worker, &NotmuchWorker::messagesPurged, this, + [this](const QStringList &messageIds) { + showTransientStatus( + tr("Deleted %n message(s) permanently", "", messageIds.size())); + runCurrentQuery(); + }); + connect(m_worker, &NotmuchWorker::threadMessagesResolved, this, &MainWindow::onThreadMessagesResolved); @@ -5464,6 +5491,11 @@ void MainWindow::onThreadMessagesResolved(const QStringList &messageIds, const QStringList threadScope = m_pendingThreadScope; m_pendingThreadScope.clear(); + if (requestTag == QStringLiteral("empty_trash")) { + confirmAndPurge(messageIds); + return; + } + if (requestTag == QStringLiteral("delete_thread")) { trashMessages(messageIds, pathById, messageIds.size(), threadScope); return; @@ -5692,6 +5724,83 @@ void MainWindow::restoreSelectedFromTrash() Q_ARG(QString, QStringLiteral("restore_messages"))); } +void MainWindow::purgeForTesting(const QStringList &messageIds) +{ + if (!m_worker || messageIds.isEmpty()) + return; + QMetaObject::invokeMethod(m_worker, "purgeMessages", Qt::QueuedConnection, + Q_ARG(QStringList, messageIds)); +} + +void MainWindow::emptyTrash() +{ + // Scoped to the account selector, like every other account-aware surface: + // the All accounts view empties every configured trash, a selected + // account empties only its own. The user sees which in the dialog. + const QString accountKey = m_accountBox->currentData().toString(); + const QString query = accountKey.isEmpty() + ? m_config.allTrashQuery() + : m_config.account(accountKey).trashQuery(); + + // An account with no trash folder configured produces an EMPTY query, and + // an empty notmuch query matches EVERYTHING. Refusing here rather than + // relying on the worker's own guard, so the message names the cause. + if (query.isEmpty()) { + showTransientStatus(tr("No trash folder is configured")); + return; + } + + if (!m_worker) { + showTransientStatus(tr("Not connected to the mail index")); + return; + } + + // Enumerated before it is counted, and counted from the DATABASE: the + // number in the dialog has to be the number destroyed, and the model + // holds whatever the current view is showing, which is usually not the + // trash at all. + QMetaObject::invokeMethod(m_worker, "resolveQueryMessages", + Qt::QueuedConnection, + Q_ARG(QString, query), + Q_ARG(QString, QStringLiteral("empty_trash"))); +} + +void MainWindow::confirmAndPurge(const QStringList &messageIds) +{ + if (messageIds.isEmpty()) { + showTransientStatus(tr("The trash is already empty")); + return; + } + + const QString accountKey = m_accountBox->currentData().toString(); + const QString where = accountKey.isEmpty() + ? tr("every account") + : m_accountBox->currentText(); + + QMessageBox box(this); + box.setObjectName(QStringLiteral("emptyTrashConfirmation")); + box.setIcon(QMessageBox::Warning); + box.setWindowTitle(tr("Empty trash")); + box.setText(tr("Permanently delete %n message(s) from the trash of %1?", + "", messageIds.size()) + .arg(where)); + // Said plainly, because it is the only place in this application where it + // is true. + box.setInformativeText(tr("This cannot be undone.")); + box.addButton(QMessageBox::Cancel); + QPushButton *confirm = + box.addButton(tr("Delete permanently"), QMessageBox::DestructiveRole); + // Cancel is the default, so Return does not destroy mail. + box.setDefaultButton(QMessageBox::Cancel); + box.exec(); + + if (box.clickedButton() != confirm) + return; + + QMetaObject::invokeMethod(m_worker, "purgeMessages", Qt::QueuedConnection, + Q_ARG(QStringList, messageIds)); +} + void MainWindow::showStrandedDeletedMail() { // Not scoped to the selected account, deliberately. The stranded mail is diff --git a/src/mainwindow.h b/src/mainwindow.h index d3c5d15..42708d9 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -169,6 +169,11 @@ public: /// command was pushed, which is what "this did nothing" has to assert. int undoDepthForTesting() const { return m_undoStack.count(); } + /// Runs a purge without the confirmation, which a test cannot drive: a + /// modal blocks the thread it is shown on (item 84). What this exists to + /// cover is what happens AFTER the user confirms. + void purgeForTesting(const QStringList &messageIds); + /// The text of the command on top of the undo stack. /// /// A test seam for the DIRECTION a toggle chose. Delete and Undelete both @@ -1032,6 +1037,17 @@ private: /// without moving. void showStrandedDeletedMail(); + /// Asks the worker what is in the trash. The answer arrives at + /// onThreadMessagesResolved() tagged `empty_trash` and goes to + /// confirmAndPurge(): the count in the dialog has to be what will actually + /// be destroyed, so it comes from the database rather than from the model, + /// which holds whatever the current view happens to show. + void emptyTrash(); + + /// The confirmation, and the only one in this application. Destroys + /// nothing if the user declines. + void confirmAndPurge(const QStringList &messageIds); + /// Moves each resolved message home, using the tags and paths the WORKER /// reported rather than anything the model holds. void restoreResolvedMessages(const QStringList &messageIds, diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index f3a76ea..8ab3ab5 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -920,6 +920,99 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, emit messagesMovedFrom(origins, destFolder); } +void NotmuchWorker::purgeMessages(const QStringList &messageIds) +{ + if (messageIds.isEmpty()) + return; + + // Same handle ordering as applyTags() and moveMessages(): notmuch allows + // one open handle per process, so the read-only one closes first. + close(); + + const QByteArray configPath = configPathArg(); + notmuch_database_t *db = nullptr; + char *error = nullptr; + const notmuch_status_t status = notmuch_database_open_with_config( + nullptr, + NOTMUCH_DATABASE_MODE_READ_WRITE, + configPath.isEmpty() ? nullptr : configPath.constData(), + nullptr, + &db, + &error); + + if (status != NOTMUCH_STATUS_SUCCESS) { + emit errorOccurred( + QStringLiteral("Cannot open database for writing: %1") + .arg(QString::fromUtf8(error ? error + : notmuch_status_to_string(status)))); + free(error); + return; + } + + QStringList purged; + for (const QString &id : messageIds) { + notmuch_message_t *raw = nullptr; + // find_message reports SUCCESS with a null message for an unknown id, + // so both are checked. A stale id does not abort the batch: the live + // ids beside it still have to go. + if (notmuch_database_find_message(db, id.toUtf8().constData(), &raw) + != NOTMUCH_STATUS_SUCCESS || !raw) { + continue; + } + NmMessage message(raw); + + // EVERY file, not just the first. notmuch deduplicates by Message-ID, + // so one message can have several files; unlinking one would leave the + // message alive in the folder the user emptied, which reads as the + // purge having silently skipped it. This is the same one-message, + // many-files property that item 166 turned on. + QStringList files; + for (NmFilenames names(notmuch_message_get_filenames(message.get())); + notmuch_filenames_valid(names.get()); + notmuch_filenames_move_to_next(names.get())) { + files.append(QString::fromUtf8(notmuch_filenames_get(names.get()))); + } + + // The handle is released before the files go out from under it. + message.reset(); + + bool removedAny = false; + for (const QString &file : files) { + // A file already gone is not an ERROR: the index can name a path a + // sync has since removed, and the goal state (no file) is reached + // either way. Reporting it would teach the user to ignore the one + // message that matters here. + // + // It is not a DESTRUCTION either, which is a separate point and + // the one a first version got wrong. The count reaches the user as + // the size of an irreversible act, so it must say what this run + // actually destroyed, not what was already absent when it started. + if (!QFile::exists(file)) { + notmuch_database_remove_message(db, file.toUtf8().constData()); + continue; + } + if (!QFile::remove(file)) { + emit errorOccurred(QStringLiteral("Cannot delete %1") + .arg(QFileInfo(file).fileName())); + continue; + } + removedAny = true; + // The index entry for that path. When the last filename goes, so + // does the message and every tag on it, which is exactly what is + // wanted here and is the thing moveMessages() has to avoid. + notmuch_database_remove_message(db, file.toUtf8().constData()); + } + + if (removedAny) + purged.append(id); + } + + notmuch_database_close(db); + notmuch_database_destroy(db); + + emit messagesPurged(purged); +} + void NotmuchWorker::indexDraftFile(const QString &path, const QString &previousPath) { @@ -1025,6 +1118,14 @@ void NotmuchWorker::resolveMessages(const QStringList &messageIds, resolveQuery(terms.join(QStringLiteral(" or ")), requestTag); } +void NotmuchWorker::resolveQueryMessages(const QString &query, + const QString &requestTag) +{ + if (query.isEmpty()) + return; + resolveQuery(query, requestTag); +} + void NotmuchWorker::resolveThreadMessages(const QStringList &threadIds, const QString &requestTag) { diff --git a/src/notmuchworker.h b/src/notmuchworker.h index 3ccf8e5..2efddaa 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -134,6 +134,22 @@ public slots: /// it, so removing before indexing loses the message's tags. void moveMessages(const QStringList &messageIds, const QString &destFolder); + /// Destroys mail: removes each file from disk and each message from the + /// index. **This is the only irreversible operation in the application** + /// (item 118), which is why it is a separate entry point rather than a + /// flag on moveMessages(): the two look alike and one of them can be + /// undone. + /// + /// Named ids only, never a folder-wide sweep, so the blast radius is + /// whatever the caller enumerated and confirmed. A message with several + /// files loses every file it has, since leaving one behind would leave + /// the message alive in a folder the user emptied. + /// + /// The caller is responsible for confirming: CLAUDE.md rules out + /// confirmation dialogs for mutations because undo replaces them, and + /// this is the one action where undo cannot exist. + void purgeMessages(const QStringList &messageIds); + /// Indexes one freshly written file, so it appears in a `path:` query /// without a full `notmuch new` (item 158). /// @@ -190,6 +206,12 @@ public slots: void resolveMessages(const QStringList &messageIds, const QString &requestTag); + /// The same walk for an arbitrary QUERY, which is what Empty Trash needs: + /// it has to enumerate what it is about to destroy before it can say how + /// much that is, and the answer must not come from the model, which holds + /// whatever the current view happens to be showing. + void resolveQueryMessages(const QString &query, const QString &requestTag); + private: /// The shared walk behind resolveMessages() and resolveThreadMessages(): /// runs `query` and emits threadMessagesResolved() with each match's id, @@ -273,6 +295,10 @@ signals: /// than aborting the batch. void messagesMoved(const QStringList &messageIds, const QString &destFolder); + /// What a purge actually destroyed. Unlike a move there is no new path to + /// observe afterwards, so this is the only report the UI has. + void messagesPurged(const QStringList &messageIds); + /// The same move, reported per message with the folder it came FROM. /// /// Emitted alongside messagesMoved rather than replacing it: that signal's 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<QAction *>(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<QMenu *> menus = window.findChildren<QMenu *>(); + 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<ThreadListModel *>(); + QVERIFY(model); + auto *queryEdit = + window.findChild<QLineEdit *>(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 diff --git a/translations/qtmaildir_it_IT.ts b/translations/qtmaildir_it_IT.ts index a976f4e..22f8485 100644 --- a/translations/qtmaildir_it_IT.ts +++ b/translations/qtmaildir_it_IT.ts @@ -552,6 +552,41 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> </translation> </message> <message> + <source>No trash folder is configured</source> + <translation>Nessuna cartella cestino configurata</translation> + </message> + <message> + <source>Not connected to the mail index</source> + <translation>Non connesso all'indice della posta</translation> + </message> + <message> + <source>The trash is already empty</source> + <translation>Il cestino è già vuoto</translation> + </message> + <message> + <source>every account</source> + <translation>ogni account</translation> + </message> + <message> + <source>Empty trash</source> + <translation>Svuota cestino</translation> + </message> + <message numerus="yes"> + <source>Permanently delete %n message(s) from the trash of %1?</source> + <translation> + <numerusform>Eliminare definitivamente %n messaggio dal cestino di %1?</numerusform> + <numerusform>Eliminare definitivamente %n messaggi dal cestino di %1?</numerusform> + </translation> + </message> + <message> + <source>This cannot be undone.</source> + <translation>Questa operazione non può essere annullata.</translation> + </message> + <message> + <source>Delete permanently</source> + <translation>Elimina definitivamente</translation> + </message> + <message> <source>Mail tagged deleted but not in a trash folder. Select what should go and press Delete.</source> <translation>Posta etichettata come eliminata ma non in un cestino. Seleziona cosa deve essere rimosso e premi Elimina.</translation> </message> @@ -752,6 +787,14 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation>Mostra la posta etichettata come eliminata che non si trova in un cestino</translation> </message> <message> + <source>Empt&y trash...</source> + <translation>S&vuota cestino...</translation> + </message> + <message> + <source>Permanently delete every message in the trash</source> + <translation>Elimina definitivamente ogni messaggio nel cestino</translation> + </message> + <message> <source>Add spam and remove inbox on whole threads</source> <translation>Aggiunge spam e rimuove inbox su intere conversazioni</translation> </message> @@ -1043,6 +1086,13 @@ Il messaggio È stato inviato. Non inviarlo di nuovo.</translation> <translation><h3>qtmaildir %1</h3><p>Un client di posta Qt6 per Maildir indicizzate con notmuch.</p><p>Legge e organizza la posta locale. Scaricamento e invio sono affidati a script esterni.</p><p>Copyright &copy; 2026 Danilo M. &lt;danix@danix.xyz&gt;<br>Distribuito secondo la GNU General Public License versione 2.</p><p>Sviluppato con l'assistenza dell'IA. Tutto il codice è riveduto, testato e curato dal manutentore.</p></translation> </message> <message numerus="yes"> + <source>Deleted %n message(s) permanently</source> + <translation> + <numerusform>%n messaggio eliminato definitivamente</numerusform> + <numerusform>%n messaggi eliminati definitivamente</numerusform> + </translation> + </message> + <message numerus="yes"> <source>%n unread</source> <translation> <numerusform>%n non letto</numerusform> |
