From dd6e7a051c22e5145c1b23da0614179aef2e1379 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 25 Aug 2026 19:19:21 +0200 Subject: fix: offer Delete and Restore only where they mean something Item 168, found by the user while hand-testing 118: Delete could be triggered on a message already in the trash. Not dangerous, which is how it survived. moveMessages() finds the file already in the destination and takes its early-return branch, so the message is reported as moved, an unsynced change is counted, and nothing happened. Restore had the mirror of the same problem, added unconditionally to both menus and so offered on mail that was never deleted. Each is now hidden where it has no meaning, which is the rule item 112 established for the unread entry. The question is about the PATH, never the deleted tag: a message trashed by another client carries no such tag, which is why the trash view is path-based, and asking the tag would hide Delete on exactly the mail a trash view is full of. Delete also removes unread now, at the user's request on the same tangent. It travels inside the same sendMove() call rather than as a second write, so one undo returns the folder and the tag together. This rewrites the Maildir filename, because maildir.synchronize_flags is true, and so reaches the server: the same mechanism the post-new hook refuses to touch, and the difference is that the hook acts unattended on arriving mail while this is an explicit gesture on a message in front of the user. A mutation survived the first round and found a real hole: comparing the prefix without its trailing separator passed every test, because no fixture had a folder whose name starts with the trash folder's. Under it Delete silently vanished from mail in acct/trash-old, which is not the trash. The fixture carries that row now and all three properties are mutation-checked. The suite is 37 of 38, the failure being item 136 on an unrelated path. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01HFuRPtzFrSxCQjFk6tq7gD --- .../2026-08-03-post-0.1.0-usability-closed.md | 96 ++++++++++++++++++++++ .../plans/2026-08-03-post-0.1.0-usability.md | 74 +---------------- 2 files changed, 97 insertions(+), 73 deletions(-) (limited to 'docs/superpowers') 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 8092fb4..1208c42 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 @@ -8395,3 +8395,99 @@ trash: zero files on disk, zero in the index. already in the trash. **Size: S.** Done. + +## 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. + +**Built 2026-08-25**, both halves, to the user's own choice of "hide each +where it has no meaning". + +`everySelectedRowIsInATrashFolder()` asks each row about its own file, a reply +row's message and a thread row's displayed message, the same rule +`everySelectedRowHasTag()` follows. `refreshTrashActions()` runs beside +`refreshUnreadAction()` on both the selection change and the model's +`dataChanged`, so the entries follow a write as well as a selection. + +The `unread` removal travels inside the SAME `sendMove()` call rather than as a +second write, which is what makes one undo return the folder and the tag +together. + +**A mutation survived the first round and is worth recording**: comparing the +prefix WITHOUT its trailing separator passed every test, because no fixture had +a folder whose name starts with the trash folder's. `acct/trash-old` is a +different folder, and under that mutation Delete silently disappeared from mail +that had never been trashed, which is the quiet half of the same mistake. The +fixture carries that row now and the mutation fails. + +All three properties are mutation-checked: the separator, Restore's visibility, +and the `unread` removal. The suite is 37 of 38, the failure being item 136 on +an unrelated path, and no new user-facing strings were added. + +**Size: S** for the visibility half, XS for the `unread` half. 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 37dc190..437ceda 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 @@ -241,7 +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` | +| 168 | Delete is offered on mail already in the trash, and does nothing | defect | S | **done 2026-08-25**, unreleased. Delete is hidden when every selected row is already in its account's trash, Restore when none is, both keyed on the PATH rather than the `deleted` tag. Delete also drops `unread` now, in the same TagChange so one undo returns the folder and the tag together | Sizes are rough: XS under an hour, S a sitting, M a session. @@ -1347,75 +1347,3 @@ 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. -- cgit v1.2.3