From 601159309118cf65c73f5f50bb3cf216be9f1cbb Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 18 Aug 2026 12:35:05 +0200 Subject: feat(trash): restore mail from the trash view Task 6. Delete moved mail into the trash and the only ways back out were a second press of Delete or Ctrl+Z, both of which act on a row the user has to have deleted in this session. Browsing the trash and putting something back needed an action of its own. `restore` is enabled from the QUERY, not from the selection's tags. The trash view is path-based precisely so that mail trashed by another client appears in it, and such a message carries no tag of ours: deciding from `tag:deleted` would disable Restore on exactly the messages that most need it. isShowingTrash() compares the current query against the trash generator's own, for both the per-account and the all-accounts scope, so it follows the account dropdown like every other filter. A message with NO origin tag is the foreign-trashed case, and it is why this is not simply restoreSelected() under a new name. The two callers want opposite things from a missing origin, which `fallbackToInbox` selects. From the trash view the message is demonstrably in the trash and refusing to move it leaves the user looking at mail they cannot get out, so it goes to the inbox and the status bar says so. From a second press of Delete the message is not in the trash at all and merely wears a stale `deleted` tag from an older version or a hand-written notmuch command; moving that to the inbox would relocate mail the user never asked to move, so the tag comes off and the file stays put. The inbox FOLDER is a new optional per-account `inbox` key, defaulting to "Inbox". It is configurable rather than hardcoded because the name is not ours to assume: naming a folder that does not exist CREATES it, beside the real one, and under mbsync's `Create Both` that folder reaches the mail server. That is not hypothetical, it is what a truncated origin folder did to real mail while this branch was being tested. Unlike `trash` the key is optional, since the default is right for any ordinary Maildir and a wrong value here only affects the fallback. Ctrl+R, which was free. The action is only enabled in the trash view, so the key is inert elsewhere rather than doing something surprising. It sits in the Message menu beside Delete and in the thread context menu, greyed outside the trash rather than hidden: an action that vanishes teaches nothing, while a disabled entry with its shortcut beside it says both that it exists and where it applies. **Adding an action is FIVE places, not four.** knownActions(), defaultBindings() and the icon table are each enforced by a test that fails loudly, and being REACHABLE is a fifth that nothing checked: this shipped registered, bound, iconned, correctly enabled, and present in no menu at all, which a green suite reported as complete. Ctrl+R is not a shortcut anyone guesses, so it was effectively invisible. restoreIsReachableWithoutTheKeyboard() closes that, and deliberately excludes the context menu from its menu-bar assertion, since findChildren returns both and one check would otherwise satisfy the other. Four tests, each mutation-checked. Two worth keeping: the hardcoded "Inbox" mutation fails against the fixture's lowercase folders exactly as it would against a Maildir that spells its inbox differently, and the reachability mutation reproduces the keyboard-only state this shipped in. Co-Authored-By: Claude Opus 5 --- src/keymap.cpp | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'src/keymap.cpp') diff --git a/src/keymap.cpp b/src/keymap.cpp index c731bbb..319bc53 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -31,6 +31,7 @@ QStringList KeyMap::knownActions() QStringLiteral("open_thread"), QStringLiteral("archive"), QStringLiteral("delete"), + QStringLiteral("restore"), QStringLiteral("spam"), QStringLiteral("toggle_unread"), QStringLiteral("mark_all_read"), @@ -98,6 +99,9 @@ QList> KeyMap::defaultBindings() { QStringLiteral("Return"), QStringLiteral("open_thread") }, { QStringLiteral("Ctrl+E"), QStringLiteral("archive") }, { QStringLiteral("Ctrl+D"), QStringLiteral("delete") }, + // Restore is only enabled in the trash view, so its key is dead + // elsewhere rather than doing something surprising. + { QStringLiteral("Ctrl+R"), QStringLiteral("restore") }, { QStringLiteral("Ctrl+Shift+S"), QStringLiteral("spam") }, { QStringLiteral("Ctrl+U"), QStringLiteral("toggle_unread") }, // Shifted against Ctrl+U, which toggles unread on the selection: this -- cgit v1.2.3 From b7d8ca35d74a9531ad292d9e875803964f6e0043 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Tue, 18 Aug 2026 13:01:51 +0200 Subject: feat(delete): bind Del, and resolve a restore against the database Del is the key a user reaches for and Ctrl+D is not a guess anyone makes. Both are bound; Del is listed FIRST because that is the one the menus advertise. Bare, which is safe here for a reason that does not generalise to other bare keys. A QAction shortcut is dispatched before the focused widget sees the key, and Qt withholds only plain LETTERS from editable widgets, so by the argument that made bare Return break the query bar this should delete mail while the user edits a query. It does not: QLineEdit accepts the ShortcutOverride for Delete itself, because Delete is one of its own editing keys, which Return is not. Measured with and without an explicit filter, the action fires 0 times either way, so no filter is added. theDeleteKeyEditsTextInTheQueryBar() pins that Qt behaviour, since the binding rests on it. **Two defects surfaced from the second binding, both real.** An action can now have more than one default, and KeyMap did not allow for it. sequenceFor() decided "is this a built-in?" by comparing against defaultSequenceFor(), which returns only the FIRST default, so the second looked like a user override and won the "a user binding beats the default" rule. The menus advertised Ctrl+D to a user who had configured nothing, and sequenceFor() and defaultSequenceFor() disagreed about an untouched action. isDefaultBinding() asks whether a sequence is ANY of the action's defaults; when two defaults tie, the one defaultBindings() lists first wins, which is the author's stated preference rather than an alphabetical accident. And Restore read each message's origin tag FROM THE MODEL. The model's tags come from the query, so a row whose delete has not been re-queried still carries its pre-delete tags: measured `[inbox,unread]` on a message already sitting in the trash, one run in three. No origin tag was found, the message took the no-origin branch, and Restore moved it to the INBOX instead of the folder it came from, silently, with the origin tag left behind as the only evidence. A restore has to be right about the destination or it is worse than doing nothing. The trash-view Restore now resolves its messages against the DATABASE first, through a new NotmuchWorker::resolveMessages(). That and resolveThreadMessages() share one walk, resolveQuery(), rather than growing a near-duplicate: they differ only in whether the terms are `id:` or `thread:`. restoreSelectedThreads() already worked this way; this is the same reasoning applied to the message-scoped path. The flake was found by running one test five times rather than trusting a single green, and the fix verified the same way: 5 of 5, then the full suite three times over. Co-Authored-By: Claude Opus 5 --- src/keymap.cpp | 46 +++++++++++++++++- src/keymap.h | 8 ++++ src/mainwindow.cpp | 120 +++++++++++++++++++++++++++++++++++++++++++++- src/mainwindow.h | 19 ++++++++ src/notmuchworker.cpp | 27 +++++++++-- src/notmuchworker.h | 18 +++++++ tests/test_mainwindow.cpp | 78 ++++++++++++++++++++++++++++-- 7 files changed, 307 insertions(+), 9 deletions(-) (limited to 'src/keymap.cpp') diff --git a/src/keymap.cpp b/src/keymap.cpp index 319bc53..7a08a58 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -98,6 +98,22 @@ QList> KeyMap::defaultBindings() { QStringLiteral("Alt+Up"), QStringLiteral("prev_thread") }, { QStringLiteral("Return"), QStringLiteral("open_thread") }, { QStringLiteral("Ctrl+E"), QStringLiteral("archive") }, + // Del FIRST, and the order matters twice over. defaultSequenceFor() + // returns the first match, and sequenceFor() prefers any binding that + // is not that default, treating it as a user override; listing Del + // second therefore made it the "override" of Ctrl+D and left the two + // functions disagreeing about which key the menus should advertise. + // First also makes it the ADVERTISED one, which is the point: it is + // the key a user reaches for, and Ctrl+D is not a guess anyone makes. + // + // Bare, which is safe for a reason that does NOT generalise to other + // bare keys. Delete is not a letter, so Qt's protection for editable + // widgets does not cover it, but QLineEdit accepts the + // ShortcutOverride for Delete itself, because it is one of its own + // editing keys. Return is not, which is why that one needed an + // explicit filter in MainWindow::eventFilter() and this one does not. + // Measured both ways; see theDeleteKeyEditsTextInTheQueryBar(). + { QStringLiteral("Del"), QStringLiteral("delete") }, { QStringLiteral("Ctrl+D"), QStringLiteral("delete") }, // Restore is only enabled in the trash view, so its key is dead // elsewhere rather than doing something surprising. @@ -250,7 +266,7 @@ QKeySequence KeyMap::sequenceFor(const QString &action) const if (it.value() != action) continue; - const bool isBuiltIn = !builtIn.isEmpty() && it.key() == builtIn; + const bool isBuiltIn = isDefaultBinding(it.key(), action); if (best.isEmpty()) { best = it.key(); bestIsBuiltIn = isBuiltIn; @@ -260,6 +276,13 @@ QKeySequence KeyMap::sequenceFor(const QString &action) const if (bestIsBuiltIn && !isBuiltIn) { best = it.key(); bestIsBuiltIn = false; + } else if (bestIsBuiltIn && isBuiltIn) { + // Both are defaults, so the ADVERTISED one is whichever + // defaultBindings() lists first: that order is the author's + // preference and is why Del is listed before Ctrl+D. Falling back + // to alphabetical here would advertise Ctrl+D instead. + if (it.key() == builtIn) + best = it.key(); } else if (bestIsBuiltIn == isBuiltIn && it.key().toString() < best.toString()) { best = it.key(); @@ -268,6 +291,27 @@ QKeySequence KeyMap::sequenceFor(const QString &action) const return best; } +bool KeyMap::isDefaultBinding(const QKeySequence &sequence, + const QString &action) +{ + // ANY of the action's defaults, not just the first. + // + // An action can ship with more than one binding: `delete` has Del and + // Ctrl+D. sequenceFor() compares against defaultSequenceFor(), which + // returns only the first, so the second looked like a USER binding and + // won the "a user binding always beats the default" rule. The menus then + // advertised Ctrl+D for a user who had configured nothing, and + // sequenceFor() and defaultSequenceFor() disagreed about an untouched + // action. + for (const auto &binding : defaultBindings()) { + if (binding.second == action + && normalizeSequence(binding.first) == sequence) { + return true; + } + } + return false; +} + QKeySequence KeyMap::defaultSequenceFor(const QString &action) { for (const auto &binding : defaultBindings()) { diff --git a/src/keymap.h b/src/keymap.h index 81e1813..8774209 100644 --- a/src/keymap.h +++ b/src/keymap.h @@ -73,6 +73,14 @@ public: /// The built-in sequence for an action, ignoring any user override. static QKeySequence defaultSequenceFor(const QString &action); + /// Whether `sequence` is ANY of `action`'s default bindings. + /// + /// Not the same question as `sequence == defaultSequenceFor(action)`: an + /// action can ship several, and comparing against only the first makes the + /// others look like user overrides. + static bool isDefaultBinding(const QKeySequence &sequence, + const QString &action); + /// Every action name carrying a built-in binding. static QStringList defaultActions(); diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index da7128f..055e783 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -309,6 +309,18 @@ bool MainWindow::eventFilter(QObject *watched, QEvent *event) keyEvent->accept(); return true; } + // Delete needs NO entry here, and that is worth stating because the + // reasoning that says it does is nearly right. It is bound bare to + // `delete`, and Qt's protection for editable widgets covers plain + // LETTERS only, so by the same argument that made Return a problem it + // should trigger the action while the user edits a query. + // + // It does not, because QLineEdit accepts the ShortcutOverride for + // Delete itself: Delete is one of its own editing keys, which Return + // is not. Measured both ways, with this branch present and absent: + // the action fires 0 times either way and the text is edited either + // way. Adding a guard here would be dead code carrying a test that + // cannot fail. } return QMainWindow::eventFilter(watched, event); @@ -851,7 +863,7 @@ void MainWindow::registerActions() }); addAction(QStringLiteral("restore"), tr("&Restore from trash"), tr("Move the selected messages out of the trash"), [this]() { - restoreSelected(true); + restoreSelectedFromTrash(); }); addAction(QStringLiteral("spam"), tr("Mark &spam"), tr("Add spam and remove inbox"), [this]() { @@ -4414,6 +4426,11 @@ void MainWindow::onThreadMessagesResolved(const QStringList &messageIds, return; } + if (requestTag == QStringLiteral("restore_messages")) { + restoreResolvedMessages(messageIds, paths, tags); + return; + } + if (requestTag != QStringLiteral("undelete_thread")) return; @@ -4531,6 +4548,107 @@ QString MainWindow::inboxFolderFor(const Account &account) const return QStringLiteral("Inbox"); } +void MainWindow::restoreResolvedMessages(const QStringList &messageIds, + const QStringList &paths, + const QStringList &tags) +{ + if (messageIds.size() != paths.size() || messageIds.size() != tags.size()) + return; + + const QString prefix = QStringLiteral("deleted-from:"); + QHash byOrigin; + QHash byInbox; + QStringList stranded; + + for (int i = 0; i < messageIds.size(); ++i) { + const QStringList messageTags = + tags.at(i).split(QLatin1Char('\t'), Qt::SkipEmptyParts); + QString origin; + for (const QString &tag : messageTags) { + if (tag.startsWith(prefix)) { + origin = tag.mid(prefix.length()); + break; + } + } + + const Account account = accountForMessagePath(paths.at(i)); + if (account.maildir.isEmpty()) { + stranded.append(messageIds.at(i)); + continue; + } + + if (origin.isEmpty()) { + // Trashed by another client, so there is no record of where it + // belongs. Inbox is the documented fallback, and it is reported: + // a guess the user is not told about is worse than the guess. + byInbox[account.maildir + QLatin1Char('/') + + account.inboxFolder()] + .append(messageIds.at(i)); + continue; + } + byOrigin[account.maildir + QLatin1Char('/') + origin] + .append(messageIds.at(i)); + } + + for (auto it = byOrigin.cbegin(); it != byOrigin.cend(); ++it) { + // The origin tag is named here rather than left as the placeholder, + // which onMessagesMoved() would resolve to the folder the message is + // coming FROM, namely the trash. + const QString origin = originTagFor(it.key()); + QStringList remove{ QStringLiteral("deleted") }; + if (!origin.isEmpty()) + remove.append(origin); + sendMove(it.value(), it.key(), {}, remove, tr("Restore")); + } + + for (auto it = byInbox.cbegin(); it != byInbox.cend(); ++it) { + sendMove(it.value(), it.key(), {}, { QStringLiteral("deleted") }, + tr("Restore")); + } + + if (!byInbox.isEmpty()) { + m_statusLabel->setText( + tr("%n message(s) had no record of where they came from and were " + "moved to the inbox.", "", int(byInbox.size()))); + } + if (!stranded.isEmpty()) { + m_statusLabel->setText( + tr("%n message(s) could not be restored: they belong to no " + "configured account.", "", int(stranded.size()))); + } +} + +void MainWindow::restoreSelectedFromTrash() +{ + const QModelIndexList rows = + m_threadView->selectionModel()->selectedRows(); + if (rows.isEmpty()) + return; + + const ActionScope scope = m_model->messageScopeFor(rows); + if (scope.messageIds.isEmpty()) + return; + + // Resolved by the WORKER, not read from the model. + // + // The model's tags come from the QUERY, and a row whose delete has not yet + // been re-queried still carries its pre-delete tags: measured + // `[inbox,unread]` on a message already in the trash, one run in three. + // The origin tag is then not found, the message falls into the + // no-origin branch, and Restore sends it to the INBOX instead of the + // folder it came from, silently and irreversibly. + // + // A restore has to be right about the destination or it is worse than + // doing nothing, so it asks the database rather than trusting a view that + // may be a moment behind. restoreSelectedThreads() already worked this + // way; this is the same reasoning applied to the message-scoped path. + m_pendingRestoreIds = scope.messageIds; + QMetaObject::invokeMethod( + m_worker, "resolveMessages", Qt::QueuedConnection, + Q_ARG(QStringList, scope.messageIds), + Q_ARG(QString, QStringLiteral("restore_messages"))); +} + void MainWindow::restoreSelected(bool fallbackToInbox) { const QModelIndexList rows = diff --git a/src/mainwindow.h b/src/mainwindow.h index 937d87c..f9a075d 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -817,6 +817,25 @@ private: /// tag, so the tag comes off and the file stays where it is. void restoreSelected(bool fallbackToInbox = false); + /// Restore as reached from the TRASH VIEW: resolves each selected + /// message against the database first, then moves it. + /// + /// Asynchronous, unlike restoreSelected(), and that is the point. The + /// model's tags come from the query, so a row whose delete has not been + /// re-queried still carries its pre-delete tags; reading the origin from + /// there found none and sent the message to the INBOX instead of the + /// folder it came from, one run in three. + void restoreSelectedFromTrash(); + + /// Moves each resolved message home, using the tags and paths the WORKER + /// reported rather than anything the model holds. + void restoreResolvedMessages(const QStringList &messageIds, + const QStringList &paths, + const QStringList &tags); + + /// The messages a resolveMessages() request was made for. + QStringList m_pendingRestoreIds; + /// The account's inbox FOLDER name, discovered from its inbox query. /// /// Never hardcoded: the real Maildir has `Inbox` and a fixture has diff --git a/src/notmuchworker.cpp b/src/notmuchworker.cpp index 62174c9..2c03226 100644 --- a/src/notmuchworker.cpp +++ b/src/notmuchworker.cpp @@ -776,15 +776,26 @@ void NotmuchWorker::moveMessages(const QStringList &messageIds, emit messagesMovedFrom(origins, destFolder); } +void NotmuchWorker::resolveMessages(const QStringList &messageIds, + const QString &requestTag) +{ + if (messageIds.isEmpty()) + return; + + QStringList terms; + terms.reserve(messageIds.size()); + for (const QString &id : messageIds) + terms.append(QStringLiteral("id:%1").arg(id)); + + resolveQuery(terms.join(QStringLiteral(" or ")), requestTag); +} + void NotmuchWorker::resolveThreadMessages(const QStringList &threadIds, const QString &requestTag) { if (threadIds.isEmpty()) return; - if (!openReadOnly()) - return; - // One combined query, for the reason applyTagsToThreads() gives: a query // per thread reopens the same Xapian cursor once per selected row. QStringList terms; @@ -792,7 +803,15 @@ void NotmuchWorker::resolveThreadMessages(const QStringList &threadIds, for (const QString &id : threadIds) terms.append(QStringLiteral("thread:%1").arg(id)); - const QString query = terms.join(QStringLiteral(" or ")); + resolveQuery(terms.join(QStringLiteral(" or ")), requestTag); +} + +void NotmuchWorker::resolveQuery(const QString &query, + const QString &requestTag) +{ + if (!openReadOnly()) + return; + NmQuery nmQuery(notmuch_query_create(m_db, query.toUtf8().constData())); if (!nmQuery) { emit errorOccurred(QStringLiteral("Cannot resolve selected threads")); diff --git a/src/notmuchworker.h b/src/notmuchworker.h index dfd4303..9932e59 100644 --- a/src/notmuchworker.h +++ b/src/notmuchworker.h @@ -158,6 +158,24 @@ public slots: void resolveThreadMessages(const QStringList &threadIds, const QString &requestTag); + /// The same walk for a set of MESSAGE ids rather than thread ids. + /// + /// Restore needs each message's tags and path to decide where to send it, + /// and must not read them from the model: the model's tags come from the + /// query, so a row whose delete has not been re-queried still carries its + /// pre-delete tags and the origin tag is missing. A restore that guesses + /// the destination is worse than one that does nothing. + void resolveMessages(const QStringList &messageIds, + const QString &requestTag); + +private: + /// The shared walk behind resolveMessages() and resolveThreadMessages(): + /// runs `query` and emits threadMessagesResolved() with each match's id, + /// database-relative path and tab-joined tags. + void resolveQuery(const QString &query, const QString &requestTag); + +public slots: + /// Every tag in the database, sorted. Feeds query bar completion, which /// cannot offer tag names it has no way to enumerate. Called at startup, /// after a sync, and after a tag mutation introduces an unknown tag. diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index cefd686..85418a4 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -377,6 +377,8 @@ private slots: void deletingAThreadRootTwiceRestoresItRatherThanRedeleting(); void deleteThreadMovesEveryMessageAndRepaintsTheRootCard(); void aFolderNameWithASpaceSurvivesTheRoundTrip(); + void deleteIsBoundToTheDeleteKey(); + void theDeleteKeyEditsTextInTheQueryBar(); void restoreIsReachableWithoutTheKeyboard(); void restoreIsOnlyEnabledInTheTrashView(); void restoreReturnsAMessageToItsOriginFolder(); @@ -9466,6 +9468,68 @@ void TestMainWindow::aFolderNameWithASpaceSurvivesTheRoundTrip() "messages are somewhere mbsync will never sync"); } +void TestMainWindow::deleteIsBoundToTheDeleteKey() +{ + // Del is the key a user reaches for, and Ctrl+D is not a guess anyone + // makes. Both are bound; this asserts the bare one is really there, + // since setShortcut() keeps only the LAST of several and silently drops + // the rest, which would leave the documented binding absent. + const Config config; + MainWindow window(config); + + auto *action = window.findChild(QStringLiteral("delete")); + QVERIFY(action); + + QVERIFY2(action->shortcuts().contains(QKeySequence(Qt::Key_Delete)), + qPrintable(QStringLiteral("delete is not on the Del key; it has: %1") + .arg(QKeySequence::listToString(action->shortcuts())))); +} + +void TestMainWindow::theDeleteKeyEditsTextInTheQueryBar() +{ + // `delete` is bound to bare Del, and a QAction shortcut is dispatched + // BEFORE the focused widget sees the key. Qt withholds only plain LETTERS + // from editable widgets, so by the argument that made bare Return break + // the query bar, Delete should move mail to the trash while the user is + // editing a query. + // + // It does not: QLineEdit accepts the ShortcutOverride for Delete itself, + // because Delete is one of its own editing keys, which Return is not. That + // is a property of Qt rather than of this code, which is exactly why it is + // pinned here: it is the assumption the bare binding rests on, and if a + // future Qt or a future focus proxy changes it, mail gets deleted while + // someone types. + // + // Asserted on the ACTION not firing, not on the ShortcutOverride phase. A + // probe on the override reports notify=1 accepted=1 whether or not this + // window filters the key, since QLineEdit accepts it either way, so it + // cannot distinguish the two and passes against any implementation. + // Measured, while trying to write this test the obvious way. + const Config config; + MainWindow window(config); + window.show(); + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + auto *deleteAction = window.findChild(QStringLiteral("delete")); + QVERIFY(queryEdit && deleteAction); + + int fired = 0; + QObject::connect(deleteAction, &QAction::triggered, + [&fired]() { ++fired; }); + + queryEdit->setFocus(); + QTRY_VERIFY(queryEdit->hasFocus()); + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->setCursorPosition(0); + + QTest::keyClick(queryEdit, Qt::Key_Delete); + + QCOMPARE(fired, 0); + QCOMPARE(queryEdit->text(), QStringLiteral("ag:inbox")); +} + void TestMainWindow::restoreIsReachableWithoutTheKeyboard() { // Restore shipped as a keyboard shortcut and nothing else: registered, @@ -9603,15 +9667,23 @@ void TestMainWindow::restoreReturnsAMessageToItsOriginFolder() || folderHasMessageFile(root + QStringLiteral("/acct/inbox/new"), stem), 15000); + // Waited on the ORIGIN tag, not on `deleted`. + // + // Both come off in one write, but the file rename and the tag write are + // separate operations and the assertions below raced the second one: + // measured 1 failure in 3 runs waiting on `deleted` alone, reporting the + // origin tag still present. Waiting on the tag this test is actually about + // removes the race rather than papering over it with a longer timeout. + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, QStringLiteral("id:ro1@example.org and " + "tag:\"deleted-from:inbox\"")) == 0, + 15000); QTRY_VERIFY_WITH_TIMEOUT( notmuchCount(cfg, QStringLiteral("id:ro1@example.org and tag:deleted")) == 0, 15000); QCOMPARE(notmuchCount(cfg, QStringLiteral("id:ro1@example.org")), 1); - QCOMPARE(notmuchCount(cfg, QStringLiteral("id:ro1@example.org and " - "tag:\"deleted-from:inbox\"")), - 0); QVERIFY(!folderHasMessageFile(root + QStringLiteral("/acct/Trash/cur"), stem)); } -- cgit v1.2.3 From 44b341f774a706d70509751b0251793e1c5a34f5 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Wed, 19 Aug 2026 09:54:53 +0200 Subject: feat: find mail tagged deleted but never moved to trash Every version before item 103 tagged a message `deleted` and left its file exactly where it was, so deleted mail accumulated in the inboxes with only a chip to say otherwise. `Find stranded deleted mail` runs the query that finds it: tagged `deleted`, and not inside any configured trash folder. It reports and moves nothing. Acting on its own would be a bulk delete with no selection behind it, and the user asked for something they could come back to and review. Repeatable rather than a startup migration, for the same reason: mail reaches this state again whenever another client tags without moving. A menu entry only, at the user's request, so it cannot be confused with the Trash filter beside the other four. Also adds everyActionIsReachableFromAMenu(), which asserts the fifth registration site nothing enforced. CLAUDE.md documents four places; a menu is the fifth, and `restore` shipped on this branch reachable by a chord and by nothing a user could see. The new test found three more of the same: open_thread, clear_pane and clear_selection were all keyboard-only. All three now sit in the View menu. Co-Authored-By: Claude Opus 5 --- src/keymap.cpp | 6 ++ src/mainwindow.cpp | 52 ++++++++++++ src/mainwindow.h | 11 +++ tests/test_mainwindow.cpp | 206 ++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 275 insertions(+) (limited to 'src/keymap.cpp') diff --git a/src/keymap.cpp b/src/keymap.cpp index 7a08a58..76c6b60 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -32,6 +32,7 @@ QStringList KeyMap::knownActions() QStringLiteral("archive"), QStringLiteral("delete"), QStringLiteral("restore"), + QStringLiteral("cleanup_stranded"), QStringLiteral("spam"), QStringLiteral("toggle_unread"), QStringLiteral("mark_all_read"), @@ -118,6 +119,11 @@ QList> KeyMap::defaultBindings() // Restore is only enabled in the trash view, so its key is dead // elsewhere rather than doing something surprising. { QStringLiteral("Ctrl+R"), QStringLiteral("restore") }, + // Item 103's cleanup. A chord rather than a plain key: it replaces the + // whole view, and it is reached from a menu far more often than from + // the keyboard. Ctrl+Shift+D is message_details and Ctrl+Alt+D is + // delete_thread, so this takes the T of "trash". + { QStringLiteral("Ctrl+Alt+T"), QStringLiteral("cleanup_stranded") }, { QStringLiteral("Ctrl+Shift+S"), QStringLiteral("spam") }, { QStringLiteral("Ctrl+U"), QStringLiteral("toggle_unread") }, // Shifted against Ctrl+U, which toggles unread on the selection: this diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 055e783..a1c01c3 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -865,6 +865,12 @@ void MainWindow::registerActions() tr("Move the selected messages out of the trash"), [this]() { restoreSelectedFromTrash(); }); + addAction(QStringLiteral("cleanup_stranded"), + tr("Find &stranded deleted mail"), + tr("Show mail tagged deleted that is not in a trash folder"), + [this]() { + showStrandedDeletedMail(); + }); addAction(QStringLiteral("spam"), tr("Mark &spam"), tr("Add spam and remove inbox"), [this]() { tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, @@ -1168,11 +1174,23 @@ void MainWindow::buildMenus() // Separated from the entries above: those act on the selection, this edits // a rule store shared with mailctl and changes nothing that is on screen. messageMenu->addSeparator(); + // A MENU entry and nothing else, at the user's request: "the cleanup + // should be a menu entry only, not to be confused with the filter Trash". + // 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("tag_rules"))); auto *viewMenu = menuBar()->addMenu(tr("&View")); viewMenu->addAction(m_actions.value(QStringLiteral("prev_thread"))); viewMenu->addAction(m_actions.value(QStringLiteral("next_thread"))); + viewMenu->addAction(m_actions.value(QStringLiteral("open_thread"))); + viewMenu->addSeparator(); + // The two clears. Both shipped keyboard-only, which is what + // everyActionIsReachableFromAMenu() exists to stop: an action reachable + // only by a chord is an action nobody discovers. + viewMenu->addAction(m_actions.value(QStringLiteral("clear_pane"))); + viewMenu->addAction(m_actions.value(QStringLiteral("clear_selection"))); viewMenu->addSeparator(); viewMenu->addAction(m_actions.value(QStringLiteral("toggle_html"))); viewMenu->addAction(m_actions.value(QStringLiteral("load_remote"))); @@ -1217,6 +1235,10 @@ void MainWindow::buildMenus() // The inverse of delete, and the theme's own name for it: the icon // every desktop uses for taking something back out of the wastebasket. { QStringLiteral("restore"), QStringLiteral("edit-undelete") }, + // A SEARCH, not a delete. The action reports what it finds and moves + // 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("undo"), QStringLiteral("edit-undo") }, { QStringLiteral("spam"), QStringLiteral("mail-mark-junk") }, { QStringLiteral("flag"), QStringLiteral("mail-mark-important") }, @@ -4649,6 +4671,36 @@ void MainWindow::restoreSelectedFromTrash() Q_ARG(QString, QStringLiteral("restore_messages"))); } +void MainWindow::showStrandedDeletedMail() +{ + // Not scoped to the selected account, deliberately. The stranded mail is + // an artefact of an old version rather than a view of anything, and the + // user wants to see all of it at once; the account dropdown is still there + // to narrow it by hand afterwards. + const QString trash = m_config.allTrashQuery(); + + // No account configures a trash folder: everything tagged `deleted` is by + // definition stranded, since there is nowhere for it to have gone. An + // empty exclusion must never be written as `not ()`, which notmuch parses + // without complaint and matches nothing, reporting a clean database. + const QString query = + trash.isEmpty() + ? QStringLiteral("tag:deleted") + : QStringLiteral("tag:deleted and not (%1)").arg(trash); + + // Into the bar, like a filter: what ran is visible and editable, and + // AlreadyScoped stops runQuery() wrapping it in the selected account's + // path, which would hide every other account's stranded mail. + m_queryEdit->setText(query); + runQuery(FlatResult::No, AccountScope::AlreadyScoped); + + // After runQuery(), which sets "Searching...": set before it, this would + // be overwritten and the user would be told nothing about what they are + // looking at. + m_statusLabel->setText(tr("Mail tagged deleted but not in a trash folder. " + "Select what should go and press Delete.")); +} + void MainWindow::restoreSelected(bool fallbackToInbox) { const QModelIndexList rows = diff --git a/src/mainwindow.h b/src/mainwindow.h index f9a075d..5b1621f 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -827,6 +827,17 @@ private: /// folder it came from, one run in three. void restoreSelectedFromTrash(); + /// Runs the query that finds mail tagged `deleted` whose file never left + /// its original folder, which is what every version before item 103 left + /// behind. It REPORTS and moves nothing: acting on its own would be a bulk + /// delete with no selection behind it, and the user asked for something + /// they could come back to and review. + /// + /// Repeatable rather than a one-time startup migration, for the same + /// reason: mail reaches this state again whenever another client tags + /// without moving. + void showStrandedDeletedMail(); + /// 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/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 85418a4..8a41c85 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -27,6 +27,7 @@ #include #include #include +#include #include #include #include @@ -330,6 +331,7 @@ private slots: void aCronSyncDoesNotClearAnEditMadeWhileItRan(); void everyActionCarriesAnIcon(); + void everyActionIsReachableFromAMenu(); void theToolbarDoesNotOverrideTheDesktopButtonStyle(); void theImportantActionIsLabelledImportant(); void theImportantActionStillWritesTheFlaggedTag(); @@ -383,6 +385,8 @@ private slots: void restoreIsOnlyEnabledInTheTrashView(); void restoreReturnsAMessageToItsOriginFolder(); void restoreFallsBackToInboxWithoutAnOriginTag(); + void theCleanupQueryFindsStrandedMail(); + void theCleanupQueryExcludesMailAlreadyInTrash(); private: /// Owns the throwaway lock table init() points every test at. A pointer @@ -6384,6 +6388,75 @@ void TestMainWindow::aCronSyncDoesNotClearAnEditMadeWhileItRan() // Items 56 and 57. +void TestMainWindow::everyActionIsReachableFromAMenu() +{ + // The fourth registration site nothing enforced. CLAUDE.md says adding an + // action is four places: knownActions(), defaultBindings(), the icon table + // and the action itself. It is FIVE, and the fifth is a menu. + // + // Found the hard way on the trash branch: `restore` shipped keyboard-only, + // reachable by a chord and by nothing a user could see or discover, and no + // test noticed. The three existing coverage tests each assert a different + // property and all three pass against an action that appears nowhere in + // the interface. + // + // The MENU rather than the toolbar, since the toolbar is a small + // deliberate subset and always will be. Every menu is walked, submenus + // included, because the five whole-thread actions live only in the "Whole + // thread" submenu. + const Config config; + MainWindow window(config); + + auto *bar = window.menuBar(); + QVERIFY(bar); + + QSet reachable; + QList pending; + const auto topLevel = bar->actions(); + for (QAction *action : topLevel) { + if (action->menu()) + pending.append(action->menu()); + } + QVERIFY2(!pending.isEmpty(), "the menu bar holds no menus"); + + while (!pending.isEmpty()) { + QMenu *menu = pending.takeFirst(); + const auto entries = menu->actions(); + for (QAction *entry : entries) { + if (QMenu *sub = entry->menu()) { + pending.append(sub); + // An action owning a menu emits no `triggered`, so it is the + // submenu that makes its children reachable and never the + // parent entry itself. Not counted as reachable. + continue; + } + reachable.insert(entry); + } + } + + // The guard, before anything is asserted about what is missing: a walk + // that found nothing would report every action as unreachable and read as + // a catastrophic regression rather than as a broken probe. + QVERIFY2(reachable.size() > 10, + qPrintable(QStringLiteral("the menu walk found only %1 entries") + .arg(reachable.size()))); + + QStringList unreachable; + for (const QString &name : KeyMap::knownActions()) { + auto *action = window.findChild(name); + QVERIFY2(action, qPrintable(QStringLiteral("no action named %1").arg(name))); + if (!reachable.contains(action)) + unreachable.append(name); + } + + QVERIFY2(unreachable.isEmpty(), + qPrintable(QStringLiteral("%1 action(s) reach no menu, so they " + "exist only for whoever already knows " + "the chord: %2") + .arg(unreachable.size()) + .arg(unreachable.join(QStringLiteral(", "))))); +} + void TestMainWindow::everyActionCarriesAnIcon() { // Item 56. The complaint was inconsistency, not absence: eight actions had @@ -8814,6 +8887,22 @@ static int notmuchCount(const QString &configPath, const QString &query) return ok ? count : -1; } +/// Applies a tag change with the notmuch binary, for the one thing the UI +/// cannot produce any more: a message tagged `deleted` while its file is still +/// in the inbox. That is the state the OLD Delete left mail in, and the state +/// the cleanup action exists to find, so a test for it has to write it +/// directly rather than through an action that now moves the file too. +static bool notmuchTag(const QString &configPath, const QStringList &args) +{ + QProcess process; + QProcessEnvironment env = QProcessEnvironment::systemEnvironment(); + env.insert(QStringLiteral("NOTMUCH_CONFIG"), configPath); + process.setProcessEnvironment(env); + process.start(QStringLiteral("notmuch"), + QStringList{ QStringLiteral("tag") } + args); + return process.waitForFinished(15000) && process.exitCode() == 0; +} + static bool folderHasMessageFile(const QString &dir, const QString &stem) { QDir directory(dir); @@ -10107,4 +10196,121 @@ void TestMainWindow::deleteWithoutATrashFolderSaysSoRatherThanDoingNothing() QStringLiteral("notrash.example.org"))); } +void TestMainWindow::theCleanupQueryFindsStrandedMail() +{ + // The state 848 real messages are in today: tagged `deleted` by a version + // of Delete that only ever tagged, with the file still sitting in the + // inbox. Nothing moves them on their own, so the action reports them and + // the user decides. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("strand@example.org"), + QStringLiteral("Tagged but never moved"), + QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("keep@example.org"), + QStringLiteral("Perfectly ordinary mail"), + QStringLiteral("other@example.org"), + QStringLiteral("Fri, 14 Aug 2026 11:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct"), + QStringLiteral("Trash")), + qPrintable(backed.error())); + + const QString cfg = backed.fixture().configPath(); + QVERIFY(notmuchTag(cfg, { QStringLiteral("+deleted"), + QStringLiteral("--"), + QStringLiteral("id:strand@example.org") })); + // The guard, before anything is asserted about what the action finds: one + // message is stranded and one is not, so a query that simply returns + // everything cannot pass. + QCOMPARE(notmuchCount(cfg, QStringLiteral("tag:deleted")), 1); + + MainWindow window(backed.config()); + auto *model = window.findChild(); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + auto *cleanup = + window.findChild(QStringLiteral("cleanup_stranded")); + QVERIFY(model && queryEdit); + QVERIFY2(cleanup, "there is no cleanup_stranded action"); + + cleanup->trigger(); + + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + // The query lands in the bar, like every other generated query, so what + // ran is visible and the user can edit it. + QVERIFY2(queryEdit->text().contains(QStringLiteral("tag:deleted")), + qPrintable(QStringLiteral("the bar holds '%1'") + .arg(queryEdit->text()))); + QVERIFY2(queryEdit->text().contains(QStringLiteral("not ")), + qPrintable(QStringLiteral("the bar holds '%1'") + .arg(queryEdit->text()))); + + // It reports and moves NOTHING. A cleanup that acted on its own would be a + // bulk delete with no selection behind it, which is the opposite of what + // the user asked for. + const QString mail = backed.fixture().maildirPath(); + QVERIFY(folderHasMessageFile(mail + QStringLiteral("/acct/inbox/new"), + QStringLiteral("strand.example.org")) + || folderHasMessageFile(mail + QStringLiteral("/acct/inbox/cur"), + QStringLiteral("strand.example.org"))); + QCOMPARE(notmuchCount(cfg, QStringLiteral("path:\"acct/Trash/**\"")), 0); +} + +void TestMainWindow::theCleanupQueryExcludesMailAlreadyInTrash() +{ + // Properly trashed mail carries the tag AND sits in the folder. Without + // the exclusion this reports every deleted message ever, which makes the + // action useless the moment Delete starts working. + WorkerBackedWindow backed; + QVERIFY(backed.fixture().addMessage( + QStringLiteral("acct/inbox"), QStringLiteral("cln1@example.org"), + QStringLiteral("Going to the trash"), + QStringLiteral("sender@example.org"), + QStringLiteral("Fri, 14 Aug 2026 10:00:00 +0200"), + QStringLiteral("Body text."))); + QVERIFY2(backed.build(QStringLiteral("acct"), QStringLiteral("acct"), + QStringLiteral("Trash")), + qPrintable(backed.error())); + + MainWindow window(backed.config()); + auto *model = window.findChild(); + auto *view = window.findChild(); + auto *queryEdit = + window.findChild(QStringLiteral("queryEdit")); + QVERIFY(model && view && queryEdit); + + const QString cfg = backed.fixture().configPath(); + const QString mail = backed.fixture().maildirPath(); + + queryEdit->setText(QStringLiteral("tag:inbox")); + queryEdit->returnPressed(); + QTRY_VERIFY_WITH_TIMEOUT(model->rowCount(QModelIndex()) == 1, 15000); + view->setCurrentIndex(model->index(0, 0, QModelIndex())); + window.findChild(QStringLiteral("delete"))->trigger(); + + // Asked of the database, never of the list: rowCount() reads 0 for the + // whole interval before the worker answers, so "the cleanup found + // nothing" would pass against a delete that never happened. + QTRY_VERIFY_WITH_TIMEOUT( + folderHasMessageFile(mail + QStringLiteral("/acct/Trash/cur"), + QStringLiteral("cln1.example.org")), + 15000); + QTRY_VERIFY_WITH_TIMEOUT( + notmuchCount(cfg, QStringLiteral("tag:deleted")) == 1, 15000); + + auto *cleanup = + window.findChild(QStringLiteral("cleanup_stranded")); + QVERIFY2(cleanup, "there is no cleanup_stranded action"); + cleanup->trigger(); + + // The query the action ran, asked of notmuch directly. The list is the + // wrong instrument for an emptiness claim, for the reason above. + QTRY_VERIFY_WITH_TIMEOUT(!queryEdit->text().isEmpty(), 15000); + QCOMPARE(notmuchCount(cfg, queryEdit->text()), 0); +} + #include "test_mainwindow.moc" -- cgit v1.2.3