diff options
| -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> |