diff options
| author | Danilo M. <danix@danix.xyz> | 2026-08-02 17:08:31 +0200 |
|---|---|---|
| committer | Danilo M. <danix@danix.xyz> | 2026-08-02 17:08:31 +0200 |
| commit | db1eb37b26f9ac14be4e1a4e5edbf5b6e77f8db7 (patch) | |
| tree | 79a0a1c07b04241f27405be8e829571fa330b267 | |
| parent | dd7845d847a3d6f961f0302551c33245d1f12bdc (diff) | |
| download | qtmaildir-db1eb37b26f9ac14be4e1a4e5edbf5b6e77f8db7.tar.gz qtmaildir-db1eb37b26f9ac14be4e1a4e5edbf5b6e77f8db7.zip | |
fix: report collisions between INI overrides bound to the same key
loadOverrides() inserted straight into m_bindings, so two override
lines that normalize to the same QKeySequence (e.g. "y" and "Y", both
"Y" per QKeySequence) silently overwrote each other with zero warning,
contradicting the "a typo cannot bind silently" contract on
knownActions().
Track sequences seen within the current override pass separately from
m_bindings (which already holds the defaults) so overriding a default
key stays silent, but two colliding override lines produce one warning
naming both actions.
| -rw-r--r-- | src/keymap.cpp | 18 | ||||
| -rw-r--r-- | tests/test_keymap.cpp | 47 |
2 files changed, 65 insertions, 0 deletions
diff --git a/src/keymap.cpp b/src/keymap.cpp index 6e43f1b..205d2f5 100644 --- a/src/keymap.cpp +++ b/src/keymap.cpp @@ -48,6 +48,14 @@ void KeyMap::loadOverrides(QSettings &settings) { const QStringList known = knownActions(); + // Sequences bound so far *within this override pass*. Defaults already + // sit in m_bindings before this runs, so a plain m_bindings.contains() + // check would misfire on every legitimate override of a default (e.g. + // "j=archive" overriding the default 'j' binding). Only a collision + // between two entries in this same pass (e.g. two INI keys that + // normalize to the same QKeySequence, such as "y" and "Y") is a bug. + QHash<QKeySequence, QString> seenThisPass; + settings.beginGroup(QStringLiteral("keys")); const QStringList keys = settings.childKeys(); for (const QString &key : keys) { @@ -71,6 +79,16 @@ void KeyMap::loadOverrides(QSettings &settings) continue; } + const auto previous = seenThisPass.constFind(sequence); + if (previous != seenThisPass.constEnd()) { + m_warnings.append( + QStringLiteral("Key sequence '%1' bound to both '%2' and '%3' " + "in [keys]; keeping '%2'") + .arg(key, previous.value(), action)); + continue; + } + seenThisPass.insert(sequence, action); + m_bindings.insert(sequence, action); } settings.endGroup(); diff --git a/tests/test_keymap.cpp b/tests/test_keymap.cpp index 10863f2..229fba6 100644 --- a/tests/test_keymap.cpp +++ b/tests/test_keymap.cpp @@ -13,6 +13,7 @@ private slots: void chordSequenceParses(); void unknownActionIsReported(); void invalidSequenceIsReported(); + void collidingOverridesAreReported(); }; void TestKeyMap::defaultsAreLoaded() @@ -127,5 +128,51 @@ void TestKeyMap::invalidSequenceIsReported() QCOMPARE(map.warnings().size(), 1); } +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. + { + 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.endGroup(); + } + + KeyMap map; + QSettings s(path, QSettings::IniFormat); + map.loadOverrides(s); + + QCOMPARE(map.warnings().size(), 1); + QVERIFY(map.warnings().first().contains(QStringLiteral("archive"))); + QVERIFY(map.warnings().first().contains(QStringLiteral("delete"))); + } + + // Overriding a default is not a collision: loadDefaults() puts 'j' in + // the map first, then the override pass rebinds the same 'j'. That must + // stay silent (regression check for iniOverridesDefault's scenario). + { + QTemporaryDir dir; + const QString path = dir.filePath(QStringLiteral("t.conf")); + { + QSettings s(path, QSettings::IniFormat); + s.beginGroup(QStringLiteral("keys")); + s.setValue(QStringLiteral("j"), QStringLiteral("archive")); + s.endGroup(); + } + + KeyMap map; + map.loadDefaults(); + QSettings s(path, QSettings::IniFormat); + map.loadOverrides(s); + + QCOMPARE(map.warnings().size(), 0); + } +} + QTEST_MAIN(TestKeyMap) #include "test_keymap.moc" |
