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 +++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 45 insertions(+), 1 deletion(-) (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()) { -- cgit v1.2.3