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 <noreply@anthropic.com>

6 files changed, 232 insertions, 6 deletions

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<QPair<QString, QString>> 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<QString, QStringList> byOrigin;
+    QHash<QString, QStringList> 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.