diff options
| -rw-r--r-- | src/keymap.cpp | 112 | ||||
| -rw-r--r-- | src/keymap.h | 27 | ||||
| -rw-r--r-- | 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<QPair<QString, QString>> KeyMap::defaultBindings() { - const QHash<QString, QString> 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 <QHash> #include <QKeySequence> +#include <QList> +#include <QPair> #include <QStringList> 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<QPair<QString, QString>> 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+<letter>. 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+<key>, 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+<key> 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<QString> 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(); } |