diff options
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability-closed.md | 96 | ||||
| -rw-r--r-- | docs/superpowers/plans/2026-08-03-post-0.1.0-usability.md | 74 | ||||
| -rw-r--r-- | src/mainwindow.cpp | 78 | ||||
| -rw-r--r-- | src/mainwindow.h | 14 | ||||
| -rw-r--r-- | tests/test_mainwindow.cpp | 162 |
5 files changed, 348 insertions, 76 deletions
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. diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index dd7bc68..89c01eb 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -905,8 +905,10 @@ void MainWindow::buildUi() // entry to "Mark as unread" with the same row still selected. Keyed on // the model rather than on each of the six call sites that apply an // optimistic update, so a new one cannot forget. - connect(m_model, &QAbstractItemModel::dataChanged, this, - [this]() { refreshUnreadAction(); }); + connect(m_model, &QAbstractItemModel::dataChanged, this, [this]() { + refreshUnreadAction(); + refreshTrashActions(); + }); connect(m_threadView, &QAbstractItemView::doubleClicked, this, &MainWindow::onRowDoubleClicked); @@ -3517,6 +3519,60 @@ void MainWindow::showThreadContextMenu(const QPoint &pos) m_threadContextMenu->popup(m_threadView->viewport()->mapToGlobal(pos)); } +bool MainWindow::everySelectedRowIsInATrashFolder() const +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return false; + + for (const QModelIndex &index : rows) { + // The row's own file: a reply row's message, a thread row's displayed + // message. Same rule as everySelectedRowHasTag(), and for the same + // reason: a thread row acts on the message its card shows. + const QString path = + m_model->isMessageRow(index) + ? m_model->messageAt(index).filePath + : m_model->threadFor(index).firstMessagePath; + if (path.isEmpty()) + return false; + + const Account account = accountForMessagePath(path); + if (account.maildir.isEmpty() || account.trash.isEmpty()) + return false; + + // Compared as a path segment, never with startsWith(): `trash-old` + // starts with `trash` and is a different folder. The same trap the + // attachment-save check records. + const QString prefix = account.maildir + QLatin1Char('/') + + account.trash + QLatin1Char('/'); + // accountForMessagePath() accepts both shapes, so this must too: a + // thread row's path is database-relative and a reply row's absolute. + if (!path.contains(prefix)) + return false; + } + return true; +} + +void MainWindow::refreshTrashActions() +{ + const bool inTrash = everySelectedRowIsInATrashFolder(); + const bool haveSelection = + !m_threadView->selectionModel()->selectedRows().isEmpty(); + + // Delete on mail already in the trash reported success and did nothing: + // moveMessages() finds the file already in the destination and takes its + // early-return branch, which counts an unsynced change for a move that + // never happened (item 168). + if (auto *del = m_actions.value(QStringLiteral("delete"))) + del->setVisible(!haveSelection || !inTrash); + + // The mirror, which shipped beside it: Restore was added unconditionally + // to both menus and so was offered on mail that was never deleted. + if (auto *restore = m_actions.value(QStringLiteral("restore"))) + restore->setVisible(!haveSelection || inTrash); +} + void MainWindow::refreshUnreadAction() { // The user's design (item 112 and its duplicates 99/147): the label says @@ -3553,6 +3609,7 @@ void MainWindow::onSelectionChanged() // selectedRows() there sees the PREVIOUS selection and would label the // action for the rows the user just left (CLAUDE.md, verified Qt 6.11). refreshUnreadAction(); + refreshTrashActions(); const QModelIndexList rows = m_threadView->selectionModel()->selectedRows(); const int selected = rows.size(); @@ -5388,8 +5445,23 @@ void MainWindow::trashMessages(const QStringList &messageIds, return; for (auto it = byTrash.cbegin(); it != byTrash.cend(); ++it) { + // `unread` goes with it (item 168, the user's request). Deleting is a + // decision about the message, so the unread count must not go on + // including what the user threw away. + // + // In the SAME change rather than as a second write, so one undo + // returns the folder and the tag together: TagChange::inverted() + // gives it back only if it travelled with the move. + // + // This rewrites the Maildir filename, because + // maildir.synchronize_flags is true, and so reaches the server on the + // next mbsync. That is the same mechanism the post-new hook REFUSES + // to touch, and the difference is who is acting: the hook tags + // arriving mail unattended, while this is an explicit gesture on a + // message in front of the user. sendMove(it.value(), it.key(), - { QStringLiteral("deleted"), kOriginTagPlaceholder() }, {}, + { QStringLiteral("deleted"), kOriginTagPlaceholder() }, + { QStringLiteral("unread") }, tr("Delete"), false, wholeThreadIds); } diff --git a/src/mainwindow.h b/src/mainwindow.h index 42708d9..a5a8c31 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -909,6 +909,20 @@ private: /// single state. Called whenever the selection changes. void refreshUnreadAction(); + /// Hides Delete on mail already in the trash, and Restore on mail that + /// was never there (item 168). Each is offered only where it means + /// something, the same rule refreshUnreadAction() applies to the label. + void refreshTrashActions(); + + /// Whether every selected row's file already sits in its account's trash + /// folder. Empty selection answers false. + /// + /// The question is about the PATH, never the `deleted` TAG: a message + /// trashed by another client carries no such tag at all, which is why the + /// trash view is path-based (item 103), and asking the tag would offer + /// Delete on exactly the mail a trash view is full of. + bool everySelectedRowIsInATrashFolder() const; + void editTagsOnSelection(); /// Set once the user has answered the exit prompt, or once a sync started diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 6020a1d..f76ff70 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -384,6 +384,9 @@ private slots: void editTagsOnAReplyCountsItsOwnThreadNotTheFirstInTheList(); void markCurrentThreadReadResolvesTheThreadThroughTheIndex(); void deletingAReplyRepaintsThatReplyRow(); + void deleteIsHiddenOnMailAlreadyInTheTrash(); + void restoreIsHiddenOnMailThatWasNeverDeleted(); + void deleteAlsoMarksTheMessageRead(); void emptyTrashAsksBeforeDestroyingAnything(); void theUnreadLabelSaysWhichDirectionItWillGo(); void theUnreadLabelFollowsAWriteWithoutReselecting(); @@ -5214,6 +5217,165 @@ void TestMainWindow::deletingAReplyRepaintsThatReplyRow() "deleting one reply marked its whole thread deleted"); } +/// A window whose one account owns `acct/`, with its trash at `acct/trash`. +/// +/// Delete and Restore both ask about a row's PATH, so a test for either needs +/// a config that says which prefix is a trash folder. Bare-window tests carry +/// no account at all and would answer "not in the trash" for every row. +static Config configWithTrash(QTemporaryDir &dir) +{ + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + QFile file(path); + if (file.open(QIODevice::WriteOnly | QIODevice::Text)) { + QTextStream out(&file); + out << "[account.acct]\n" + << "maildir = acct\n" + << "trash = trash\n" + << "inbox = inbox\n"; + } + Config config; + config.load(path); + return config; +} + +/// One thread row whose displayed message sits at `filePath`. +static ThreadSummary threadAtPath(const QString &id, const QString &filePath, + const QStringList &tags = {}) +{ + ThreadSummary thread = makeThread(id, tags); + thread.firstMessagePath = filePath; + thread.firstMessageTags = tags; + return thread; +} + +void TestMainWindow::deleteIsHiddenOnMailAlreadyInTheTrash() +{ + // Item 168, from the user: "I noticed I can hit delete via context menu on + // a message already in the trash." + // + // It was not dangerous, which is the part that made it survive: the file + // is already in the destination, so moveMessages() takes its + // already-there branch, reports the message as moved and counts an + // unsynced change for a move that never happened. The menu claimed to + // have done something and nothing had. + // + // The question is about the PATH, never the `deleted` TAG: a message + // trashed by another client carries no such tag, which is why item 103 + // made the trash view path-based, and asking the tag would offer Delete on + // exactly the mail a trash view is full of. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const Config config = configWithTrash(dir); + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *deleteAction = + window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(deleteAction); + + model->appendBatch({ + threadAtPath(QStringLiteral("t1"), + QStringLiteral("acct/inbox/cur/1:2,S")), + threadAtPath(QStringLiteral("t2"), + QStringLiteral("acct/trash/cur/2:2,S")), + }); + + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(deleteAction->isVisible(), + "Delete is hidden on mail that is NOT in the trash, so this test " + "cannot tell the two cases apart"); + + view->setCurrentIndex(model->index(1, 0, {})); + QVERIFY2(!deleteAction->isVisible(), + "Delete is still offered on a message already in the trash, " + "where it reports success and does nothing"); + + // A folder whose name STARTS with the trash folder's is a different + // folder. Without the trailing separator `acct/trash-old` matches + // `acct/trash` and Delete silently disappears from mail that was never + // trashed, which is the quiet half of the same mistake. + model->appendBatch({ threadAtPath(QStringLiteral("t3"), + QStringLiteral("acct/trash-old/cur/3:2,S")) }); + view->setCurrentIndex(model->index(2, 0, {})); + QVERIFY2(deleteAction->isVisible(), + "Delete vanished on mail in acct/trash-old, which is not the " + "trash: the prefix was compared without its separator"); +} + +void TestMainWindow::restoreIsHiddenOnMailThatWasNeverDeleted() +{ + // The mirror, shipped beside it: `restore` was added unconditionally to + // both menus, so it was offered on mail that was never deleted, where it + // has as little meaning as Delete has in the trash. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const Config config = configWithTrash(dir); + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + auto *restore = window.findChild<QAction *>(QStringLiteral("restore")); + QVERIFY(restore); + + model->appendBatch({ + threadAtPath(QStringLiteral("t1"), + QStringLiteral("acct/inbox/cur/1:2,S")), + threadAtPath(QStringLiteral("t2"), + QStringLiteral("acct/trash/cur/2:2,S")), + }); + + view->setCurrentIndex(model->index(1, 0, {})); + QVERIFY2(restore->isVisible(), "Restore is hidden on trashed mail"); + + view->setCurrentIndex(model->index(0, 0, {})); + QVERIFY2(!restore->isVisible(), + "Restore is still offered on mail that was never deleted"); +} + +void TestMainWindow::deleteAlsoMarksTheMessageRead() +{ + // The user's second request on the same tangent: "messages moved to the + // trash should be automatically marked -unread". Deleting is a decision + // about the message, so the unread count must not go on including what + // the user threw away. + // + // Asserted on the undo TEXT and depth rather than on the tags: the write + // is a move, which a bare window cannot complete, but the tag change it + // composes is pushed as one command either way. One command, not two, is + // the property that matters: undo has to return the folder AND the tag + // together. + QTemporaryDir dir; + QVERIFY(dir.isValid()); + const Config config = configWithTrash(dir); + MainWindow window(config); + + auto *model = window.findChild<ThreadListModel *>(); + QVERIFY(model); + auto *view = window.findChild<QTreeView *>(); + QVERIFY(view); + + model->appendBatch({ threadAtPath(QStringLiteral("t1"), + QStringLiteral("acct/inbox/cur/1:2,S"), + { QStringLiteral("unread") }) }); + const QModelIndex row = model->index(0, 0, {}); + view->setCurrentIndex(row); + + QVERIFY2(model->threadFor(row).isUnread(), + "the fixture is already read, so this test cannot see the tag go"); + + auto *deleteAction = window.findChild<QAction *>(QStringLiteral("delete")); + QVERIFY(deleteAction); + deleteAction->trigger(); + + QVERIFY2(!model->threadFor(row).isUnread(), + "Delete left the message unread in the trash"); +} + void TestMainWindow::emptyTrashAsksBeforeDestroyingAnything() { // Item 118, and the one place this application asks. CLAUDE.md rules out |