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 +++++++++++++++++++++++++++++++++++++---------- src/keymap.h | 27 ++++++++++++ tests/test_keymap.cpp | 118 +++++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 225 insertions(+), 32 deletions(-) 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; diff --git a/src/keymap.h b/src/keymap.h index 564eb10..1c7df5f 100644 --- a/src/keymap.h +++ b/src/keymap.h @@ -20,6 +20,8 @@ #include #include +#include +#include #include class QSettings; @@ -33,6 +35,11 @@ public: /// anything not in this set, so a typo in the config cannot bind silently. static QStringList knownActions(); + /// The built-in bindings, in menu order: {sequence, action}. The single + /// source of truth for the defaults, so the menus, the shortcut reference + /// and loadDefaults() cannot disagree about them. + static QList> defaultBindings(); + void loadDefaults(); /// Reads the [keys] group. Invalid sequences and unknown action names are @@ -42,6 +49,26 @@ public: /// Empty string when nothing is bound. QString actionFor(const QKeySequence &sequence) const; + /// The sequence currently bound to an action, empty if none. The reverse + /// of actionFor(): menus need a shortcut for an action they already know. + /// When several sequences are bound to one action, returns the shortest + /// text, so the menu shows a stable choice rather than a hash-order one. + QKeySequence sequenceFor(const QString &action) const; + + /// The built-in sequence for an action, ignoring any user override. + static QKeySequence defaultSequenceFor(const QString &action); + + /// Every action name carrying a built-in binding. + static QStringList defaultActions(); + + /// Normalizes a configured key string into the sequence a real keypress + /// produces. QKeySequence::fromString() discards the case of a bare + /// letter, so "N" parses to plain Key_N, which no keystroke ever emits: + /// typing a capital sends Shift+N. A bare uppercase letter is therefore + /// rewritten to Shift+. Returns an empty sequence for input + /// fromString() cannot parse. + static QKeySequence normalizeSequence(const QString &text); + QStringList warnings() const { return m_warnings; } private: diff --git a/tests/test_keymap.cpp b/tests/test_keymap.cpp index 7fc31ef..e63c151 100644 --- a/tests/test_keymap.cpp +++ b/tests/test_keymap.cpp @@ -32,18 +32,110 @@ private slots: void unknownActionIsReported(); void invalidSequenceIsReported(); void collidingOverridesAreReported(); + void bareCapitalMatchesShiftedPress(); + void defaultsDoNotCollide(); + void everyDefaultIsAKnownAction(); }; void TestKeyMap::defaultsAreLoaded() { KeyMap map; map.loadDefaults(); - QCOMPARE(map.actionFor(QKeySequence(QStringLiteral("j"))), + QCOMPARE(map.actionFor(QKeySequence(QStringLiteral("Ctrl+J"))), QStringLiteral("next_thread")); - QCOMPARE(map.actionFor(QKeySequence(QStringLiteral("a"))), + QCOMPARE(map.actionFor(QKeySequence(QStringLiteral("Ctrl+E"))), QStringLiteral("archive")); } +void TestKeyMap::bareCapitalMatchesShiftedPress() +{ + // Typing a capital produces Shift+, but QKeySequence::fromString() + // discards the case of a bare letter: "N" and "n" both parse to plain + // Key_N, which no keypress can ever produce. A user who writes "N = flag" + // would get a binding that silently never fires. Normalizing a bare + // capital to Shift+ is what they meant. + QTemporaryDir dir; + const QString path = dir.filePath(QStringLiteral("t.conf")); + { + QSettings s(path, QSettings::IniFormat); + s.beginGroup(QStringLiteral("keys")); + s.setValue(QStringLiteral("N"), QStringLiteral("flag")); + s.endGroup(); + } + + KeyMap map; + map.loadDefaults(); + QSettings s(path, QSettings::IniFormat); + map.loadOverrides(s); + + // The sequence a real Shift+N keypress produces. + QKeyEvent press(QEvent::KeyPress, Qt::Key_N, Qt::ShiftModifier); + QCOMPARE(map.actionFor(QKeySequence(press.keyCombination())), + QStringLiteral("flag")); + + // A lowercase binding stays unshifted, so the two remain distinguishable. + QVERIFY(map.warnings().isEmpty()); + + // "y" and "Y" are two different keys, not a collision: the second would + // have silently displaced the first before normalization. + QTemporaryDir caseDir; + const QString casePath = caseDir.filePath(QStringLiteral("case.conf")); + { + QSettings s(casePath, QSettings::IniFormat); + s.beginGroup(QStringLiteral("keys")); + s.setValue(QStringLiteral("y"), QStringLiteral("archive")); + s.setValue(QStringLiteral("Y"), QStringLiteral("delete")); + s.endGroup(); + } + KeyMap caseMap; + QSettings caseSettings(casePath, QSettings::IniFormat); + caseMap.loadOverrides(caseSettings); + + QKeyEvent lower(QEvent::KeyPress, Qt::Key_Y, Qt::NoModifier); + QKeyEvent upper(QEvent::KeyPress, Qt::Key_Y, Qt::ShiftModifier); + QCOMPARE(caseMap.actionFor(QKeySequence(lower.keyCombination())), + QStringLiteral("archive")); + QCOMPARE(caseMap.actionFor(QKeySequence(upper.keyCombination())), + QStringLiteral("delete")); + QVERIFY(caseMap.warnings().isEmpty()); +} + +void TestKeyMap::defaultsDoNotCollide() +{ + // Two defaults on one sequence means one of them is unreachable, and the + // QHash would silently keep whichever was inserted last. + KeyMap map; + map.loadDefaults(); + + QSet actions; + for (const QString &action : KeyMap::knownActions()) { + const QKeySequence seq = map.defaultSequenceFor(action); + if (seq.isEmpty()) + continue; // Not every action carries a default. + QVERIFY2(map.actionFor(seq) == action, + qPrintable(QStringLiteral("default '%1' for '%2' resolves to '%3'") + .arg(seq.toString(), action, map.actionFor(seq)))); + actions.insert(action); + } + QVERIFY(!actions.isEmpty()); +} + +void TestKeyMap::everyDefaultIsAKnownAction() +{ + // A default bound to a name loadOverrides() would reject as unknown. + KeyMap map; + map.loadDefaults(); + const QStringList known = KeyMap::knownActions(); + for (const QString &action : known) + QVERIFY(!action.isEmpty()); + + for (const QString &action : map.defaultActions()) { + QVERIFY2(known.contains(action), + qPrintable(QStringLiteral("default binds unknown action '%1'") + .arg(action))); + } +} + void TestKeyMap::iniOverridesDefault() { QTemporaryDir dir; @@ -51,7 +143,7 @@ void TestKeyMap::iniOverridesDefault() { QSettings s(path, QSettings::IniFormat); s.beginGroup(QStringLiteral("keys")); - s.setValue(QStringLiteral("j"), QStringLiteral("archive")); + s.setValue(QStringLiteral("Ctrl+J"), QStringLiteral("archive")); s.endGroup(); } @@ -60,10 +152,10 @@ void TestKeyMap::iniOverridesDefault() QSettings s(path, QSettings::IniFormat); map.loadOverrides(s); - QCOMPARE(map.actionFor(QKeySequence(QStringLiteral("j"))), + QCOMPARE(map.actionFor(QKeySequence(QStringLiteral("Ctrl+J"))), QStringLiteral("archive")); // An untouched default survives. - QCOMPARE(map.actionFor(QKeySequence(QStringLiteral("k"))), + QCOMPARE(map.actionFor(QKeySequence(QStringLiteral("Ctrl+K"))), QStringLiteral("prev_thread")); } @@ -148,16 +240,22 @@ void TestKeyMap::invalidSequenceIsReported() void TestKeyMap::collidingOverridesAreReported() { - // "y" and "Y" both normalize to the same QKeySequence ("Y"), so binding - // both in [keys] is a genuine collision that must not silently drop one. + // Two spellings of one sequence. "Ctrl+Y" and "ctrl+y" parse identically, + // so binding both in [keys] is a genuine collision that must not silently + // drop one. + // + // Note "y" and "Y" are NOT a collision any more: normalizeSequence() + // rewrites a bare capital to Shift+Y, which is the key a user actually + // presses, leaving the two distinct. Before that they both folded to + // plain Key_Y and one was lost. { QTemporaryDir dir; const QString path = dir.filePath(QStringLiteral("t.conf")); { QSettings s(path, QSettings::IniFormat); s.beginGroup(QStringLiteral("keys")); - s.setValue(QStringLiteral("y"), QStringLiteral("archive")); - s.setValue(QStringLiteral("Y"), QStringLiteral("delete")); + s.setValue(QStringLiteral("Ctrl+Y"), QStringLiteral("archive")); + s.setValue(QStringLiteral("ctrl+y"), QStringLiteral("delete")); s.endGroup(); } @@ -179,7 +277,7 @@ void TestKeyMap::collidingOverridesAreReported() { QSettings s(path, QSettings::IniFormat); s.beginGroup(QStringLiteral("keys")); - s.setValue(QStringLiteral("j"), QStringLiteral("archive")); + s.setValue(QStringLiteral("Ctrl+J"), QStringLiteral("archive")); s.endGroup(); } -- cgit v1.2.3