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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HFuRPtzFrSxCQjFk6tq7gD

1 files changed, 75 insertions, 3 deletions

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);
     }