From 188287f14f981ed3e8a08bc1514bb2ba6bb76809 Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 3 Aug 2026 14:08:59 +0200 Subject: fix: bind shortcuts users can actually press Typing a capital emits Shift+, but QKeySequence::fromString() folds the case of a bare letter away: "N" parsed to plain Key_N, a combination no keystroke produces. The N, F and G defaults (toggle_unread, flag and sync) therefore never fired, and neither would any hand-written capital in [keys]. normalizeSequence() rewrites a bare capital to Shift+ and is shared by the defaults and the override pass. As a side effect "y" and "Y" become distinct keys rather than a collision that dropped one. Defaults move to modifier shortcuts throughout. A single letter cannot be a QAction shortcut without stealing that letter from every text field in the window, and the menus in the next commit need real accelerators. defaultBindings() is now the one source of truth for them. --- src/keymap.cpp | 112 +++++++++++++++++++++++++++++++++++++++++++++------------ 1 file changed, 90 insertions(+), 22 deletions(-) (limited to 'src/keymap.cpp') diff --git a/src/keymap.cpp b/src/keymap.cpp index 39991dc..e53c2bf 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -41,25 +41,95 @@ QStringList KeyMap::knownActions() }; } -void KeyMap::loadDefaults() +QList> KeyMap::defaultBindings() { - const QHash defaults = { - { QStringLiteral("j"), QStringLiteral("next_thread") }, - { QStringLiteral("k"), QStringLiteral("prev_thread") }, - { QStringLiteral("Return"), QStringLiteral("open_thread") }, - { QStringLiteral("a"), QStringLiteral("archive") }, - { QStringLiteral("d"), QStringLiteral("delete") }, - { QStringLiteral("N"), QStringLiteral("toggle_unread") }, - { QStringLiteral("F"), QStringLiteral("flag") }, - { QStringLiteral("/"), QStringLiteral("focus_query") }, - { QStringLiteral("h"), QStringLiteral("toggle_html") }, - { QStringLiteral("u"), QStringLiteral("undo") }, - { QStringLiteral("G"), QStringLiteral("sync") }, - { QStringLiteral("Ctrl+Q"), QStringLiteral("quit") }, + // Modifier shortcuts throughout, rather than the bare letters of 0.1.0. + // Two reasons. A bare capital never worked: "N" parses to plain Key_N + // while typing a capital emits Shift+N, so toggle_unread, flag and sync + // were dead keys. And a single letter cannot be a QAction shortcut in a + // menu without stealing that letter from every text field in the window. + // + // Ordered as the menus present them; a QList keeps that order, which a + // QHash would not. + return { + { QStringLiteral("Ctrl+J"), QStringLiteral("next_thread") }, + { QStringLiteral("Ctrl+K"), QStringLiteral("prev_thread") }, + { QStringLiteral("Return"), QStringLiteral("open_thread") }, + { QStringLiteral("Ctrl+E"), QStringLiteral("archive") }, + { QStringLiteral("Ctrl+D"), QStringLiteral("delete") }, + { QStringLiteral("Ctrl+Shift+S"), QStringLiteral("spam") }, + { QStringLiteral("Ctrl+U"), QStringLiteral("toggle_unread") }, + { QStringLiteral("Ctrl+I"), QStringLiteral("flag") }, + { QStringLiteral("Ctrl+L"), QStringLiteral("focus_query") }, + { QStringLiteral("Ctrl+H"), QStringLiteral("toggle_html") }, + { QStringLiteral("Ctrl+M"), QStringLiteral("load_remote") }, + { QStringLiteral("Ctrl+Z"), QStringLiteral("undo") }, + { QStringLiteral("Ctrl+G"), QStringLiteral("sync") }, + { QStringLiteral("Ctrl+Q"), QStringLiteral("quit") }, }; +} + +QStringList KeyMap::defaultActions() +{ + QStringList actions; + const auto bindings = defaultBindings(); + actions.reserve(bindings.size()); + for (const auto &binding : bindings) + actions.append(binding.second); + return actions; +} + +QKeySequence KeyMap::normalizeSequence(const QString &text) +{ + const QKeySequence sequence = QKeySequence::fromString(text); + + // fromString() does not return an empty sequence for unparseable input; + // it returns a non-empty one whose toString() is empty (verified on + // Qt 6.11). Both checks are needed to detect garbage. + if (sequence.isEmpty() || sequence.toString().isEmpty()) + return {}; + + // A bare uppercase letter, no modifiers: the user wrote "N" meaning the + // key they press to type a capital N, which is Shift+N. fromString() + // folded the case away, so put the Shift back. + if (text.size() == 1 && text.at(0).isUpper() && text.at(0).isLetter()) + return QKeySequence(sequence[0].key() | Qt::SHIFT); + + return sequence; +} - for (auto it = defaults.cbegin(); it != defaults.cend(); ++it) - m_bindings.insert(QKeySequence::fromString(it.key()), it.value()); +void KeyMap::loadDefaults() +{ + for (const auto &binding : defaultBindings()) + m_bindings.insert(normalizeSequence(binding.first), binding.second); +} + +QKeySequence KeyMap::sequenceFor(const QString &action) const +{ + // Several sequences can point at one action (a default the user did not + // remove, plus their own addition). QHash iteration order is unspecified, + // so pick deterministically rather than taking whichever comes first. + QKeySequence best; + for (auto it = m_bindings.cbegin(); it != m_bindings.cend(); ++it) { + if (it.value() != action) + continue; + const QString candidate = it.key().toString(); + if (best.isEmpty() || candidate.size() < best.toString().size() + || (candidate.size() == best.toString().size() + && candidate < best.toString())) { + best = it.key(); + } + } + return best; +} + +QKeySequence KeyMap::defaultSequenceFor(const QString &action) +{ + for (const auto &binding : defaultBindings()) { + if (binding.second == action) + return normalizeSequence(binding.first); + } + return {}; } void KeyMap::loadOverrides(QSettings &settings) @@ -79,12 +149,10 @@ void KeyMap::loadOverrides(QSettings &settings) for (const QString &key : keys) { const QString action = settings.value(key).toString(); - const QKeySequence sequence = QKeySequence::fromString(key); - // QKeySequence::fromString() does not return an empty sequence for - // unparseable input; it returns a non-empty sequence whose - // toString() is empty (verified on Qt 6.11). Use that to detect - // garbage input instead. - if (sequence.isEmpty() || sequence.toString().isEmpty()) { + // Shares the defaults' normalization, so a hand-written "N" binds the + // key the user actually presses rather than one nothing emits. + const QKeySequence sequence = normalizeSequence(key); + if (sequence.isEmpty()) { m_warnings.append( QStringLiteral("Unparseable key sequence '%1' in [keys]").arg(key)); continue; -- cgit v1.2.3 From 1f2eddff6afcbf4c24f06e982e9169219429a2ed Mon Sep 17 00:00:00 2001 From: "Danilo M." Date: Mon, 3 Aug 2026 14:17:09 +0200 Subject: feat: add menus, a toolbar and a shortcut reference Actions were a QHash of std::function dispatched by an event filter, which nothing could put in a menu. They are QActions now, bound from KeyMap so a [keys] override reaches the menus as well as the keyboard. Menu bar covers every action; the toolbar carries only Sync, Archive, Delete and Undo. Help > Keyboard shortcuts is generated from the actions, so it shows what the keys really do rather than a copy that drifts. spam and load_remote gained defaults, having been unreachable without a hand-written binding. The event filter is gone. Probing showed QAction shortcuts are dispatched before the focused widget sees the key, so they beat QAbstractItemView's type-to-search without one, and Qt already suppresses plain-letter shortcuts while an editable widget has focus. Dropping the filter's blanket guard also lets Ctrl+Q work while the query bar has focus. registeredActionNames() is derived from the actions rather than hand-maintained, so the two drift tests it needed are replaced by checks that a configured binding reaches its action. No confirmation dialogs: tag mutations still answer to undo. --- CHANGELOG.md | 32 ++- README.md | 55 +++-- .../plans/2026-08-03-post-0.1.0-usability.md | 35 ++- src/keymap.cpp | 30 ++- src/mainwindow.cpp | 260 ++++++++++++++------- src/mainwindow.h | 31 ++- tests/test_keymap.cpp | 33 +++ tests/test_mainwindow.cpp | 93 +++++++- 8 files changed, 439 insertions(+), 130 deletions(-) (limited to 'src/keymap.cpp') diff --git a/CHANGELOG.md b/CHANGELOG.md index 90853ce..ce2b744 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,37 @@ point at which they are stable. ## [Unreleased] -Nothing yet. +### Added + +- Menu bar covering every action: File, Edit, Message, View and Help. +- Toolbar with the frequent subset, Sync, Archive, Delete and Undo. +- **Help > Keyboard shortcuts**, listing the current bindings. Generated from + the actions themselves, so it shows configured overrides rather than a + hand-written copy of the defaults. +- **Help > About**. +- Default bindings for `spam` and `load_remote`, which previously had none + and were unreachable until bound by hand. + +### Fixed + +- Three default bindings never fired. Typing a capital sends `Shift`+the key, + but `N`, `F` and `G` were stored as the unshifted key, which no keystroke + produces, leaving `toggle_unread`, `flag` and `sync` dead. A bare capital in + `[keys]` is now read as `Shift`+that letter. As a side effect `y` and `Y` + are two distinct keys rather than a collision that silently dropped one. +- Modifier shortcuts such as `Ctrl+Q` now work while the query bar has focus. + The old event filter suppressed every binding there, not only the plain + letters that would have interfered with typing. + +### Changed + +- Default bindings moved to modifier shortcuts (`Ctrl+E` archive, `Ctrl+D` + delete, and so on). Existing `[keys]` entries are unaffected, and single + letters are still safe to bind. See "Upgrading from 0.1.0" in the README. +- Actions are `QAction`s dispatched by shortcut rather than a hash of + callbacks behind an event filter, which is what lets them appear in menus. + The hand-maintained list of registered action names is now derived from the + actions, so it can no longer drift from them. ## [0.1.0] - 2026-08-03 diff --git a/README.md b/README.md index 4acd215..f7e2444 100644 --- a/README.md +++ b/README.md @@ -104,18 +104,10 @@ Unread = tag:unread Flagged = tag:flagged [keys] +Ctrl+E = archive +Ctrl+D = delete j = next_thread k = prev_thread -Return = open_thread -a = archive -d = delete -N = toggle_unread -F = flag -/ = focus_query -h = toggle_html -u = undo -G = sync -Ctrl+Q = quit ``` Saved-query buttons appear in alphabetical order rather than file order: @@ -128,27 +120,42 @@ Defaults, all rebindable through `[keys]`: | Key | Action | Does | |---|---|---| -| `j` | `next_thread` | Select the next thread | -| `k` | `prev_thread` | Select the previous thread | +| `Ctrl+J` | `next_thread` | Select the next thread | +| `Ctrl+K` | `prev_thread` | Select the previous thread | | `Return` | `open_thread` | Focus the thread list | -| `a` | `archive` | Remove `inbox` from every selected thread | -| `d` | `delete` | Add `deleted` | -| `N` | `toggle_unread` | Toggle `unread` | -| `F` | `flag` | Add `flagged` | -| `/` | `focus_query` | Focus and select the query bar | -| `h` | `toggle_html` | Switch the thread between HTML and plain text | -| `u` | `undo` | Undo the last tag change | -| `G` | `sync` | Run the configured sync command | +| `Ctrl+E` | `archive` | Remove `inbox` from every selected thread | +| `Ctrl+D` | `delete` | Add `deleted` | +| `Ctrl+Shift+S` | `spam` | Add `spam`, remove `inbox` | +| `Ctrl+U` | `toggle_unread` | Toggle `unread` | +| `Ctrl+I` | `flag` | Add `flagged` | +| `Ctrl+L` | `focus_query` | Focus and select the query bar | +| `Ctrl+H` | `toggle_html` | Switch the thread between HTML and plain text | +| `Ctrl+M` | `load_remote` | Load remote images for the current thread | +| `Ctrl+Z` | `undo` | Undo the last tag change | +| `Ctrl+G` | `sync` | Run the configured sync command | | `Ctrl+Q` | `quit` | Quit | -Two further actions exist but have **no default binding**, so they are -unreachable until you bind them: `spam` (adds `spam`, removes `inbox`) and -`load_remote` (the keyboard equivalent of the "Load remote content" -button). +Every action now carries a default binding, and every one appears in a menu. +**Help > Keyboard shortcuts** lists the current bindings, generated from the +actions themselves, so it shows your overrides rather than these defaults. An unknown action name in `[keys]` produces a warning at startup rather than binding silently, so a typo is visible. +### Upgrading from 0.1.0 + +0.1.0 used single letters (`j`, `k`, `a`, `d`, `N`, `F`, `h`, `u`, `G`, `/`). +Those still work if you keep them in `[keys]`, and single letters remain safe +to bind: Qt suppresses a plain-letter shortcut while the query bar has focus, +so typing a query is unaffected. + +Three of the old defaults never actually fired. Typing a capital sends +`Shift`+the key, but `N`, `F` and `G` were stored as the unshifted key, a +combination no keystroke produces, so `toggle_unread`, `flag` and `sync` were +dead. A bare capital in `[keys]` is now read as `Shift`+that letter, which is +what you press, so those bindings work whether you keep the old names or move +to the new defaults. Note this makes `y` and `Y` two different keys. + Tag actions apply to **every selected thread**, not only the focused one. ## Security posture of the message view 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 8847b9b..99e26e0 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 @@ -36,13 +36,13 @@ taking that too literally. |---|------|---------|------|--------| | 1 | Splitter/column widths do not survive restart | persistence | S | open | | 2 | No way to see full message details (From/To/Cc/Subject) | information | M | open | -| 3 | Too few clickable affordances, shortcuts are the only route | discoverability | M | open | +| 3 | Too few clickable affordances, shortcuts are the only route | discoverability | M | **done** | | 4 | Message-pane font size does not survive restart | persistence | S | open | | 5 | Thread list is cramped, poor readability | presentation | S | open | | 6 | Opened message stays unread | behavior | S | open | | 7 | HTML view should be default for HTML messages | behavior | XS | **verify first, may already be done** | -| 8 | No buttons or menu entries for archive, undo, etc | discoverability | M | open | -| 9 | No in-app view of configured shortcuts | discoverability | S | open | +| 8 | No buttons or menu entries for archive, undo, etc | discoverability | M | **done** | +| 9 | No in-app view of configured shortcuts | discoverability | S | **done** | | 10 | Reaching an account's inbox takes two steps | workflow | S | open | | 11 | Icon, `.desktop` file, SlackBuild | packaging | M | open | @@ -167,6 +167,35 @@ smuggle in a "Are you sure?" for Delete. **Verification:** the existing keymap test must still pass unchanged, proving user bindings survive the conversion. That is the load-bearing check here. +### Outcome (done) + +Built as described: menu bar, toolbar, and a generated shortcut reference. +Four things the plan did not anticipate, all verified by probe rather than +assumed: + +- **The event filter was removable, but not for the stated reason.** The plan + worried that `QAction` shortcuts might lose to `QAbstractItemView`'s + type-to-search. They do not: shortcut dispatch runs before the focused + widget sees the key. The filter is gone, and the thread view no longer + needs its own. +- **Qt already solves the query-bar case.** A plain-letter shortcut is + suppressed while an editable widget has focus, so the `hasFocus()` guard + was unnecessary. Removing it also fixed `Ctrl+Q`, which the old filter + swallowed while typing a query. +- **Three default bindings had never worked.** `N`, `F` and `G` stored the + unshifted key, which no keystroke emits, so `toggle_unread`, `flag` and + `sync` were dead in 0.1.0. Fixed in `KeyMap::normalizeSequence()` and + committed separately from the menu work. +- **The drift test did become unnecessary**, as the plan hoped. + `registeredActionNames()` is now derived from the `QAction`s, and + `defaultBindings()` is the single source for the defaults. The two tests + that pinned the hand-maintained lists together were replaced by ones that + check a configured binding actually reaches its action. + +Defaults moved to modifier shortcuts, since a single letter cannot be a menu +accelerator without claiming that letter window-wide. Existing `[keys]` +entries are unaffected. + ## 4. Message-pane font size does not survive restart **Observed:** described as "very annoying", more so than item 1. diff --git a/src/keymap.cpp b/src/keymap.cpp index e53c2bf..42ccd40 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -106,17 +106,33 @@ void KeyMap::loadDefaults() QKeySequence KeyMap::sequenceFor(const QString &action) const { - // Several sequences can point at one action (a default the user did not - // remove, plus their own addition). QHash iteration order is unspecified, - // so pick deterministically rather than taking whichever comes first. + // Several sequences can reach one action: the built-in default, which + // loadOverrides() does not remove, plus whatever the user added. Their + // binding is the one to show and to put on the QAction, or configuring + // "Ctrl+Alt+A = archive" would leave the menu still advertising Ctrl+E. + // + // QHash iteration order is unspecified, so ties are broken on the text + // rather than left to chance. + const QKeySequence builtIn = defaultSequenceFor(action); QKeySequence best; + bool bestIsBuiltIn = false; + for (auto it = m_bindings.cbegin(); it != m_bindings.cend(); ++it) { if (it.value() != action) continue; - const QString candidate = it.key().toString(); - if (best.isEmpty() || candidate.size() < best.toString().size() - || (candidate.size() == best.toString().size() - && candidate < best.toString())) { + + const bool isBuiltIn = !builtIn.isEmpty() && it.key() == builtIn; + if (best.isEmpty()) { + best = it.key(); + bestIsBuiltIn = isBuiltIn; + continue; + } + // A user binding always beats the default. + if (bestIsBuiltIn && !isBuiltIn) { + best = it.key(); + bestIsBuiltIn = false; + } else if (bestIsBuiltIn == isBuiltIn + && it.key().toString() < best.toString()) { best = it.key(); } } diff --git a/src/mainwindow.cpp b/src/mainwindow.cpp index 2a484fb..be80d8a 100644 --- a/src/mainwindow.cpp +++ b/src/mainwindow.cpp @@ -18,13 +18,14 @@ #include "mainwindow.h" +#include #include -#include #include #include -#include #include #include +#include +#include #include #include #include @@ -32,6 +33,7 @@ #include #include #include +#include #include #include "mailsync.h" @@ -41,26 +43,13 @@ #include "threadlistmodel.h" #include "version.h" -QStringList MainWindow::registeredActionNames() +QStringList MainWindow::registeredActionNames() const { - // Keep in sync with registerActions(). Held against KeyMap::knownActions() - // by a test rather than by hope. - return { - QStringLiteral("next_thread"), - QStringLiteral("prev_thread"), - QStringLiteral("open_thread"), - QStringLiteral("archive"), - QStringLiteral("delete"), - QStringLiteral("spam"), - QStringLiteral("toggle_unread"), - QStringLiteral("flag"), - QStringLiteral("focus_query"), - QStringLiteral("toggle_html"), - QStringLiteral("load_remote"), - QStringLiteral("undo"), - QStringLiteral("sync"), - QStringLiteral("quit"), - }; + // Derived from the actions themselves, so it cannot drift from what + // registerActions() really installed. + QStringList names = m_actions.keys(); + names.sort(); + return names; } QString MainWindow::cidPrefixForIndex(int index) @@ -86,19 +75,16 @@ MainWindow::MainWindow(const Config &config, QWidget *parent) buildUi(); registerActions(); + buildMenus(); wireWorker(); showWarnings(); - installEventFilter(this); - - // The thread view needs its own filter, not just the window's. A filter on - // the window only sees key presses the focused child did not consume, and - // QAbstractItemView consumes plain letters for its type-to-search feature: - // with the list focused, 'h' jumped to the next thread whose subject began - // with "h" instead of toggling HTML, and every other single-letter binding - // (j, k, a, d, N, F, u, G) was swallowed the same way. Filtering the view - // itself puts the keymap ahead of that search. - m_threadView->installEventFilter(this); + // No event filter: QAction shortcuts are dispatched before the focused + // widget sees the key, so they beat QAbstractItemView's type-to-search + // without one. Qt also suppresses a plain-letter shortcut while an + // editable widget has focus, so typing in the query bar stays typing; + // modifier shortcuts such as Ctrl+Q still work there, which the old + // filter blocked. if (!m_config.savedQueries().isEmpty()) { m_queryEdit->setText(m_config.savedQueries().first().query); @@ -206,40 +192,77 @@ void MainWindow::buildUi() setWindowTitle(QStringLiteral("qtmaildir %1").arg(QTMAILDIR_VERSION)); } +QAction *MainWindow::addAction(const QString &name, const QString &text, + const QString &description, + const std::function &handler) +{ + auto *action = new QAction(text, this); + action->setObjectName(name); + action->setStatusTip(description); + m_actionDescriptions.insert(name, description); + + // The binding comes from KeyMap, so a [keys] override reaches the menus + // and the shortcut reference as well as the keyboard. + const QKeySequence sequence = m_keyMap.sequenceFor(name); + if (!sequence.isEmpty()) + action->setShortcut(sequence); + + // Shortcuts must work while focus is in the thread list or the message + // view, not only on the window itself. + action->setShortcutContext(Qt::WindowShortcut); + + connect(action, &QAction::triggered, this, handler); + + // Added to the window so the shortcut is live even before the action is + // put in a menu; the ones that never reach a menu depend on this. + QMainWindow::addAction(action); + m_actions.insert(name, action); + return action; +} + void MainWindow::registerActions() { - m_actions[QStringLiteral("focus_query")] = [this]() { + addAction(QStringLiteral("focus_query"), tr("&Find"), + tr("Focus and select the query bar"), [this]() { m_queryEdit->setFocus(); m_queryEdit->selectAll(); - }; - m_actions[QStringLiteral("next_thread")] = [this]() { + }); + addAction(QStringLiteral("next_thread"), tr("&Next thread"), + tr("Select the next thread"), [this]() { const QModelIndex current = m_threadView->currentIndex(); const int row = current.isValid() ? current.row() + 1 : 0; if (row < m_model->rowCount()) m_threadView->selectRow(row); - }; - m_actions[QStringLiteral("prev_thread")] = [this]() { + }); + addAction(QStringLiteral("prev_thread"), tr("&Previous thread"), + tr("Select the previous thread"), [this]() { const QModelIndex current = m_threadView->currentIndex(); if (current.isValid() && current.row() > 0) m_threadView->selectRow(current.row() - 1); - }; - m_actions[QStringLiteral("open_thread")] = [this]() { + }); + addAction(QStringLiteral("open_thread"), tr("&Open thread"), + tr("Focus the thread list"), [this]() { m_threadView->setFocus(); - }; - m_actions[QStringLiteral("archive")] = [this]() { + }); + addAction(QStringLiteral("archive"), tr("&Archive"), + tr("Remove inbox from every selected thread"), [this]() { tagSelected({}, { QStringLiteral("inbox") }, tr("Archive")); - }; - m_actions[QStringLiteral("delete")] = [this]() { + }); + addAction(QStringLiteral("delete"), tr("&Delete"), + tr("Add the deleted tag"), [this]() { tagSelected({ QStringLiteral("deleted") }, {}, tr("Delete")); - }; - m_actions[QStringLiteral("spam")] = [this]() { + }); + addAction(QStringLiteral("spam"), tr("Mark &spam"), + tr("Add spam and remove inbox"), [this]() { tagSelected({ QStringLiteral("spam") }, { QStringLiteral("inbox") }, tr("Mark spam")); - }; - m_actions[QStringLiteral("flag")] = [this]() { + }); + addAction(QStringLiteral("flag"), tr("&Flag"), + tr("Add the flagged tag"), [this]() { tagSelected({ QStringLiteral("flagged") }, {}, tr("Flag")); - }; - m_actions[QStringLiteral("toggle_unread")] = [this]() { + }); + addAction(QStringLiteral("toggle_unread"), tr("Toggle &unread"), + tr("Toggle the unread tag"), [this]() { // The direction comes from the current row, but the change applies to // the whole selection, so a mixed selection lands in one consistent // state rather than each row flipping its own way. @@ -251,28 +274,123 @@ void MainWindow::registerActions() tagSelected({}, { QStringLiteral("unread") }, tr("Mark read")); else tagSelected({ QStringLiteral("unread") }, {}, tr("Mark unread")); - }; - m_actions[QStringLiteral("toggle_html")] = [this]() { + }); + addAction(QStringLiteral("toggle_html"), tr("Toggle &HTML"), + tr("Switch the thread between HTML and plain text"), [this]() { m_messageView->toggleHtml(); - }; - m_actions[QStringLiteral("load_remote")] = [this]() { + }); + addAction(QStringLiteral("load_remote"), tr("Load &remote content"), + tr("Load remote images for the current thread"), [this]() { m_messageView->loadRemoteContent(); - }; - m_actions[QStringLiteral("undo")] = [this]() { + }); + addAction(QStringLiteral("undo"), tr("&Undo"), + tr("Undo the last tag change"), [this]() { if (m_undoStack.canUndo()) m_undoStack.undo(); else m_statusLabel->setText(tr("Nothing to undo")); - }; - m_actions[QStringLiteral("sync")] = [this]() { + }); + addAction(QStringLiteral("sync"), tr("&Sync"), + tr("Run the configured sync command"), [this]() { if (m_sync->isAvailable()) m_sync->start(); - }; - m_actions[QStringLiteral("quit")] = [this]() { close(); }; + }); + addAction(QStringLiteral("quit"), tr("&Quit"), + tr("Quit qtmaildir"), [this]() { close(); }); - // The two lists are maintained by hand and a test pins them together; this - // catches the same drift in a debug run. - Q_ASSERT(m_actions.size() == registeredActionNames().size()); + // A binding the user wrote for an action that does not exist would be + // silently dead. KeyMap warns about unknown names, but only a check here + // catches the reverse: a known action nothing implements. + Q_ASSERT(m_actions.size() == KeyMap::knownActions().size()); +} + +void MainWindow::buildMenus() +{ + auto *fileMenu = menuBar()->addMenu(tr("&File")); + fileMenu->addAction(m_actions.value(QStringLiteral("sync"))); + fileMenu->addSeparator(); + fileMenu->addAction(m_actions.value(QStringLiteral("quit"))); + + auto *editMenu = menuBar()->addMenu(tr("&Edit")); + editMenu->addAction(m_actions.value(QStringLiteral("undo"))); + editMenu->addSeparator(); + editMenu->addAction(m_actions.value(QStringLiteral("focus_query"))); + + auto *messageMenu = menuBar()->addMenu(tr("&Message")); + messageMenu->addAction(m_actions.value(QStringLiteral("archive"))); + messageMenu->addAction(m_actions.value(QStringLiteral("delete"))); + messageMenu->addAction(m_actions.value(QStringLiteral("spam"))); + messageMenu->addSeparator(); + messageMenu->addAction(m_actions.value(QStringLiteral("toggle_unread"))); + messageMenu->addAction(m_actions.value(QStringLiteral("flag"))); + + auto *viewMenu = menuBar()->addMenu(tr("&View")); + viewMenu->addAction(m_actions.value(QStringLiteral("prev_thread"))); + viewMenu->addAction(m_actions.value(QStringLiteral("next_thread"))); + viewMenu->addSeparator(); + viewMenu->addAction(m_actions.value(QStringLiteral("toggle_html"))); + viewMenu->addAction(m_actions.value(QStringLiteral("load_remote"))); + + auto *helpMenu = menuBar()->addMenu(tr("&Help")); + auto *shortcuts = helpMenu->addAction(tr("&Keyboard shortcuts")); + connect(shortcuts, &QAction::triggered, + this, &MainWindow::showShortcutReference); + auto *about = helpMenu->addAction(tr("&About")); + connect(about, &QAction::triggered, this, &MainWindow::showAbout); + + // The frequent subset only. A toolbar holding every action is as + // unreadable as no toolbar. + auto *toolBar = addToolBar(tr("Main")); + toolBar->setObjectName(QStringLiteral("main_toolbar")); + toolBar->setToolButtonStyle(Qt::ToolButtonTextBesideIcon); + toolBar->addAction(m_actions.value(QStringLiteral("sync"))); + toolBar->addSeparator(); + toolBar->addAction(m_actions.value(QStringLiteral("archive"))); + toolBar->addAction(m_actions.value(QStringLiteral("delete"))); + toolBar->addSeparator(); + toolBar->addAction(m_actions.value(QStringLiteral("undo"))); +} + +void MainWindow::showShortcutReference() +{ + // Generated from the actions, so it cannot disagree with what the keys + // really do. A hand-written list would drift the first time a binding + // changed. + QStringList rows; + for (const QString &name : registeredActionNames()) { + const QAction *action = m_actions.value(name); + if (!action) + continue; + const QString sequence = action->shortcut().toString(QKeySequence::NativeText); + rows.append(QStringLiteral("%1%2" + "%3") + .arg(sequence.isEmpty() ? tr("(unbound)") : sequence.toHtmlEscaped(), + m_actionDescriptions.value(name).toHtmlEscaped(), + name.toHtmlEscaped())); + } + + QMessageBox box(this); + box.setWindowTitle(tr("Keyboard shortcuts")); + box.setTextFormat(Qt::RichText); + box.setText(tr("

Keyboard shortcuts

" + "" + "" + "%1
KeyDoesAction name
" + "

Rebind any of these in the [keys] section of " + "qtmaildir.conf, using the action name.

") + .arg(rows.join(QString()))); + box.exec(); +} + +void MainWindow::showAbout() +{ + QMessageBox::about( + this, tr("About qtmaildir"), + tr("

qtmaildir %1

" + "

A Qt6 mail client for notmuch-indexed Maildirs.

" + "

Reads and organizes local mail. Fetching and sending are " + "handled by external scripts.

") + .arg(QStringLiteral(QTMAILDIR_VERSION))); } void MainWindow::wireWorker() @@ -511,23 +629,3 @@ void MainWindow::sendThreadTagChange(const QStringList &threadIds, Q_ARG(QString, description)); } -bool MainWindow::eventFilter(QObject *watched, QEvent *event) -{ - if (event->type() != QEvent::KeyPress) - return QMainWindow::eventFilter(watched, event); - - // The query bar must receive ordinary typing, so single-key bindings are - // suppressed while it has focus. - if (m_queryEdit->hasFocus()) - return QMainWindow::eventFilter(watched, event); - - auto *keyEvent = static_cast(event); - const QKeySequence sequence(keyEvent->keyCombination()); - - const QString action = m_keyMap.actionFor(sequence); - if (action.isEmpty() || !m_actions.contains(action)) - return QMainWindow::eventFilter(watched, event); - - m_actions.value(action)(); - return true; -} diff --git a/src/mainwindow.h b/src/mainwindow.h index 30679bb..30a128a 100644 --- a/src/mainwindow.h +++ b/src/mainwindow.h @@ -30,6 +30,7 @@ #include "keymap.h" #include "types.h" +class QAction; class QLineEdit; class QTableView; class QLabel; @@ -49,10 +50,10 @@ public: explicit MainWindow(const Config &config, QWidget *parent = nullptr); ~MainWindow() override; - /// Every action name registerActions() installs. Exposed so a test can hold - /// it against KeyMap::knownActions(): the two lists are maintained by hand, - /// and a drift either way silently breaks a user's key binding. - static QStringList registeredActionNames(); + /// Every action name registerActions() installs. Derived from the actions + /// themselves rather than hand-maintained, so it cannot drift from what is + /// really registered. + QStringList registeredActionNames() const; /// The cid: namespace prefix for the nth message of a thread. /// @@ -61,9 +62,6 @@ public: /// cid: references from resolving to another's. static QString cidPrefixForIndex(int index); -protected: - bool eventFilter(QObject *watched, QEvent *event) override; - private slots: void runCurrentQuery(); void onThreadsReady(const QVector &threads, quint64 generation); @@ -76,8 +74,17 @@ private slots: private: void buildUi(); void registerActions(); + void buildMenus(); void wireWorker(); void showWarnings(); + void showShortcutReference(); + void showAbout(); + + /// Creates a QAction, binds it to the sequence KeyMap holds for `name`, + /// and registers it. `name` is the action name used in [keys]. + QAction *addAction(const QString &name, const QString &text, + const QString &description, + const std::function &handler); void tagSelected(const QStringList &add, const QStringList &remove, const QString &description); @@ -112,7 +119,15 @@ private: QLabel *m_statusLabel = nullptr; QPlainTextEdit *m_syncLog = nullptr; - QHash> m_actions; + /// Action name (as used in [keys]) to the QAction implementing it. Owned + /// by the window through the QObject parent, not by this hash. + QHash m_actions; + + /// One-line description per action, for the shortcut reference. Kept + /// beside the actions so the dialog is generated, never hand-written in + /// parallel with them. + QHash m_actionDescriptions; + quint64 m_generation = 0; QString m_lastQuery; QString m_currentThreadId; diff --git a/tests/test_keymap.cpp b/tests/test_keymap.cpp index e63c151..c81eeb0 100644 --- a/tests/test_keymap.cpp +++ b/tests/test_keymap.cpp @@ -33,6 +33,7 @@ private slots: void invalidSequenceIsReported(); void collidingOverridesAreReported(); void bareCapitalMatchesShiftedPress(); + void userBindingWinsOverDefaultInMenus(); void defaultsDoNotCollide(); void everyDefaultIsAKnownAction(); }; @@ -100,6 +101,38 @@ void TestKeyMap::bareCapitalMatchesShiftedPress() QVERIFY(caseMap.warnings().isEmpty()); } +void TestKeyMap::userBindingWinsOverDefaultInMenus() +{ + // loadOverrides() adds a binding without removing the default, so two + // sequences reach 'archive'. sequenceFor() is what the menus and the + // shortcut reference display: it must show the user's, not the built-in + // one they were trying to replace. + QTemporaryDir dir; + const QString path = dir.filePath(QStringLiteral("t.conf")); + { + QSettings s(path, QSettings::IniFormat); + s.beginGroup(QStringLiteral("keys")); + s.setValue(QStringLiteral("Ctrl+Alt+A"), QStringLiteral("archive")); + s.endGroup(); + } + + KeyMap map; + map.loadDefaults(); + QSettings s(path, QSettings::IniFormat); + map.loadOverrides(s); + + QCOMPARE(map.sequenceFor(QStringLiteral("archive")), + QKeySequence(QStringLiteral("Ctrl+Alt+A"))); + + // The default still fires; it is only no longer the advertised one. + QCOMPARE(map.actionFor(KeyMap::defaultSequenceFor(QStringLiteral("archive"))), + QStringLiteral("archive")); + + // An action the user left alone still shows its default. + QCOMPARE(map.sequenceFor(QStringLiteral("delete")), + KeyMap::defaultSequenceFor(QStringLiteral("delete"))); +} + void TestKeyMap::defaultsDoNotCollide() { // Two defaults on one sequence means one of them is unreachable, and the diff --git a/tests/test_mainwindow.cpp b/tests/test_mainwindow.cpp index 57eb763..6bfa925 100644 --- a/tests/test_mainwindow.cpp +++ b/tests/test_mainwindow.cpp @@ -18,18 +18,27 @@ #include +#include +#include +#include +#include + +#include "config.h" #include "keymap.h" #include "mainwindow.h" -/// MainWindow is mostly wiring and needs a live QApplication plus a real -/// database, so it is verified manually in Task 13. Two things do not need -/// either, and both are the kind of drift a comment alone does not prevent. +/// MainWindow is mostly wiring, and the parts that need a real database are +/// still verified manually. What is checked here is the action registry: the +/// bindings a user configures reach the QActions the menus and the keyboard +/// both read from, and no action is left unreachable. class TestMainWindow : public QObject { Q_OBJECT private slots: void everyKnownActionIsRegistered(); void everyRegisteredActionIsKnown(); + void everyActionHasAShortcut(); + void configuredBindingReachesTheAction(); void cidPrefixesAreBangFree(); void cidPrefixesAreDistinctPerMessage(); }; @@ -39,8 +48,14 @@ void TestMainWindow::everyKnownActionIsRegistered() // KeyMap::knownActions() is what loadOverrides() validates config bindings // against. An action listed there but never registered means a user can // bind a key in qtmaildir.conf, get no warning, and have it do nothing. + // + // registeredActionNames() is now derived from the QActions themselves, so + // this compares against what the window really installed. + const Config config; + MainWindow window(config); + const QStringList known = KeyMap::knownActions(); - const QStringList registered = MainWindow::registeredActionNames(); + const QStringList registered = window.registeredActionNames(); for (const QString &action : known) { QVERIFY2(registered.contains(action), @@ -53,8 +68,11 @@ void TestMainWindow::everyRegisteredActionIsKnown() { // The reverse drift: an action MainWindow implements but KeyMap rejects. // The user would get "unknown action" for a binding that is really there. + const Config config; + MainWindow window(config); + const QStringList known = KeyMap::knownActions(); - const QStringList registered = MainWindow::registeredActionNames(); + const QStringList registered = window.registeredActionNames(); for (const QString &action : registered) { QVERIFY2(known.contains(action), @@ -63,6 +81,57 @@ void TestMainWindow::everyRegisteredActionIsKnown() } } +void TestMainWindow::everyActionHasAShortcut() +{ + // An action with no binding is unreachable from the keyboard. Every one + // of them carries a default, so an empty shortcut means the default table + // and the action list have drifted apart. + const Config config; + MainWindow window(config); + + for (const QString &name : window.registeredActionNames()) { + const QAction *action = window.findChild(name); + QVERIFY2(action, qPrintable(QStringLiteral("no QAction named '%1'").arg(name))); + QVERIFY2(!action->shortcut().isEmpty(), + qPrintable(QStringLiteral("action '%1' has no shortcut").arg(name))); + } +} + +void TestMainWindow::configuredBindingReachesTheAction() +{ + // The whole point of [keys]: a user's override must end up on the QAction, + // which is what both the keyboard and the menus read. + QTemporaryDir dir; + const QString path = dir.filePath(QStringLiteral("qtmaildir.conf")); + { + QSettings s(path, QSettings::IniFormat); + s.beginGroup(QStringLiteral("keys")); + s.setValue(QStringLiteral("Ctrl+Alt+A"), QStringLiteral("archive")); + s.endGroup(); + } + + // MainWindow reads its keymap from Config::defaultPath(), so point that + // at the temporary file for this test. + const QString previous = qEnvironmentVariable("XDG_CONFIG_HOME"); + QVERIFY(QDir().mkpath(dir.filePath(QStringLiteral("qtmaildir")))); + QVERIFY(QFile::copy(path, dir.filePath(QStringLiteral("qtmaildir/qtmaildir.conf")))); + qputenv("XDG_CONFIG_HOME", dir.path().toUtf8()); + + { + const Config config; + MainWindow window(config); + const QAction *archive = + window.findChild(QStringLiteral("archive")); + QVERIFY(archive); + QCOMPARE(archive->shortcut(), QKeySequence(QStringLiteral("Ctrl+Alt+A"))); + } + + if (previous.isEmpty()) + qunsetenv("XDG_CONFIG_HOME"); + else + qputenv("XDG_CONFIG_HOME", previous.toUtf8()); +} + void TestMainWindow::cidPrefixesAreBangFree() { // MainWindow is the only producer of cidPrefix in the application. The @@ -89,5 +158,17 @@ void TestMainWindow::cidPrefixesAreDistinctPerMessage() } } -QTEST_MAIN(TestMainWindow) +// Constructing a MainWindow needs a QApplication and a platform plugin. The +// test has no display under ctest, so it runs offscreen unless the caller +// asked for something else. +int main(int argc, char *argv[]) +{ + qputenv("QT_QPA_PLATFORM", qgetenv("QT_QPA_PLATFORM").isEmpty() + ? QByteArray("offscreen") + : qgetenv("QT_QPA_PLATFORM")); + QApplication app(argc, argv); + TestMainWindow test; + return QTest::qExec(&test, argc, argv); +} + #include "test_mainwindow.moc" -- cgit v1.2.3