From 29fdd1acc410da016c44f3cc921f538b575520c7 Mon Sep 17 00:00:00 2001 From: Florian Bruhin Date: Mon, 5 Mar 2018 22:11:26 +0100 Subject: [PATCH] Make sure all keyboard modifiers are handled correctly This handles Qt.KeypadModifier (Num+...) correctly, adds tests for converting modifiers to strings, and strips Qt.GroupSwitchModifier as QKeySequence doesn't know about it. Fixes #3675 --- qutebrowser/keyinput/keyutils.py | 74 ++++++++++++++++++++-------- tests/unit/keyinput/key_data.py | 34 ++++++++++++- tests/unit/keyinput/test_keyutils.py | 38 ++++++++++++-- 3 files changed, 121 insertions(+), 25 deletions(-) diff --git a/qutebrowser/keyinput/keyutils.py b/qutebrowser/keyinput/keyutils.py index 6f63a914a..f40314709 100644 --- a/qutebrowser/keyinput/keyutils.py +++ b/qutebrowser/keyinput/keyutils.py @@ -51,6 +51,19 @@ def is_modifier_key(key): return key in _MODIFIER_MAP +def _check_valid_utf8(s, data): + """Make sure the given string is valid UTF-8. + + Makes sure there are no chars where Qt did fall back to weird UTF-16 + surrogates. + """ + try: + s.encode('utf-8') + except UnicodeEncodeError as e: # pragma: no cover + raise ValueError("Invalid encoding in 0x{:x} -> {}: {}" + .format(data, s, e)) + + def _key_to_string(key): """Convert a Qt::Key member to a meaningful name. @@ -131,7 +144,27 @@ def _key_to_string(key): if key in special_names: return special_names[key] - return QKeySequence(key).toString() + result = QKeySequence(key).toString() + _check_valid_utf8(result, key) + return result + + +def _modifiers_to_string(modifiers): + """Convert the given Qt::KeyboardModifiers to a string. + + Handles Qt.GroupSwitchModifier because Qt doesn't handle that as a + modifier. + """ + if modifiers & Qt.GroupSwitchModifier: + modifiers &= ~Qt.GroupSwitchModifier + result = 'AltGr+' + else: + result = '' + + result += QKeySequence(modifiers).toString() + + _check_valid_utf8(result, modifiers) + return result class KeyParseError(Exception): @@ -191,7 +224,7 @@ def _parse_special_key(keystr): for (orig, repl) in replacements: keystr = keystr.replace(orig, repl) - for mod in ['ctrl', 'meta', 'alt', 'shift']: + for mod in ['ctrl', 'meta', 'alt', 'shift', 'num']: keystr = keystr.replace(mod + '-', mod + '+') return keystr @@ -246,7 +279,7 @@ class KeyInfo: key_string = key_string.lower() # "special" binding - modifier_string = QKeySequence(modifiers).toString() + modifier_string = _modifiers_to_string(modifiers) return '<{}{}>'.format(modifier_string, key_string) def text(self): @@ -411,33 +444,32 @@ class KeySequence: raise utils.Unreachable("self={!r} other={!r}".format(self, other)) def append_event(self, ev): - """Create a new KeySequence object with the given QKeyEvent added. - - We need to do some sophisticated checking of modifiers here: - - We don't care about a shift modifier with symbols (Shift-: should match - a : binding even though we typed it with a shift on an US-keyboard) - - However, we *do* care about Shift being involved if we got an - upper-case letter, as Shift-A should match a Shift-A binding, but not - an "a" binding. - - In addition, Shift also *is* relevant when other modifiers are - involved. - Shift-Ctrl-X should not be equivalent to Ctrl-X. - - We also change Qt.Key_Backtab to Key_Tab here because nobody would - configure "Shift-Backtab" in their config. - """ + """Create a new KeySequence object with the given QKeyEvent added.""" key = ev.key() modifiers = ev.modifiers() if key == 0x0: raise KeyParseError(None, "Got nil key!") + # We always remove Qt.GroupSwitchModifier because QKeySequence has no + # way to mention that in a binding anyways... + modifiers &= ~Qt.GroupSwitchModifier + + # We change Qt.Key_Backtab to Key_Tab here because nobody would + # configure "Shift-Backtab" in their config. if modifiers & Qt.ShiftModifier and key == Qt.Key_Backtab: key = Qt.Key_Tab + # We don't care about a shift modifier with symbols (Shift-: should + # match a : binding even though we typed it with a shift on an + # US-keyboard) + # + # However, we *do* care about Shift being involved if we got an + # upper-case letter, as Shift-A should match a Shift-A binding, but not + # an "a" binding. + # + # In addition, Shift also *is* relevant when other modifiers are + # involved. Shift-Ctrl-X should not be equivalent to Ctrl-X. if (modifiers == Qt.ShiftModifier and is_printable(ev.key()) and not ev.text().isupper()): diff --git a/tests/unit/keyinput/key_data.py b/tests/unit/keyinput/key_data.py index ec2f92c1b..bf1ccdede 100644 --- a/tests/unit/keyinput/key_data.py +++ b/tests/unit/keyinput/key_data.py @@ -37,7 +37,7 @@ class Key: name: The name returned by str(KeyInfo) with that key. text: The text returned by KeyInfo.text(). uppertext: The text returned by KeyInfo.text() with shift. - member: Filled by the test fixture, the numeric value. + member: The numeric value. """ attribute = attr.ib() @@ -54,6 +54,28 @@ class Key: self.name = self.attribute +@attr.s +class Modifier: + + """A modifier with expected values. + + Attributes: + attribute: The name of the Qt::KeyboardModifier attribute + ('Shift' -> Qt.ShiftModifier) + name: The name returned by str(KeyInfo) with that modifier. + member: The numeric value. + """ + + attribute = attr.ib() + name = attr.ib(None) + member = attr.ib(None) + + def __attrs_post_init__(self): + self.member = getattr(Qt, self.attribute + 'Modifier') + if self.name is None: + self.name = self.attribute + + # From enum Key in qt5/qtbase/src/corelib/global/qnamespace.h KEYS = [ ### misc keys @@ -589,3 +611,13 @@ KEYS = [ # 0x0 is used by Qt for unknown keys... Key(attribute='', name='nil', member=0x0, qtest=False), ] + + +MODIFIERS = [ + Modifier('Shift'), + Modifier('Control', 'Ctrl'), + Modifier('Alt'), + Modifier('Meta'), + Modifier('Keypad', 'Num'), + Modifier('GroupSwitch', 'AltGr'), +] diff --git a/tests/unit/keyinput/test_keyutils.py b/tests/unit/keyinput/test_keyutils.py index 71f36accd..8ed205fca 100644 --- a/tests/unit/keyinput/test_keyutils.py +++ b/tests/unit/keyinput/test_keyutils.py @@ -40,6 +40,14 @@ def qt_key(request): return key +@pytest.fixture(params=key_data.MODIFIERS, ids=lambda m: m.attribute) +def qt_mod(request): + """Get all existing modifiers from key_data.py.""" + mod = request.param + assert mod.member is not None + return mod + + @pytest.fixture(params=[key for key in key_data.KEYS if key.qtest], ids=lambda k: k.attribute) def qtest_key(request): @@ -47,7 +55,7 @@ def qtest_key(request): return request.param -def test_key_data(): +def test_key_data_keys(): """Make sure all possible keys are in key_data.KEYS.""" key_names = {name[len("Key_"):] for name, value in sorted(vars(Qt).items()) @@ -57,6 +65,17 @@ def test_key_data(): assert not diff +def test_key_data_modifiers(): + """Make sure all possible modifiers are in key_data.MODIFIERS.""" + mod_names = {name[:-len("Modifier")] + for name, value in sorted(vars(Qt).items()) + if isinstance(value, Qt.KeyboardModifier) and + value not in [Qt.NoModifier, Qt.KeyboardModifierMask]} + mod_data_names = {mod.attribute for mod in sorted(key_data.MODIFIERS)} + diff = mod_names - mod_data_names + assert not diff + + class KeyTesterWidget(QWidget): """Widget to get the text of QKeyPressEvents. @@ -113,6 +132,10 @@ class TestKeyToString: def test_to_string(self, qt_key): assert keyutils._key_to_string(qt_key.member) == qt_key.name + def test_modifiers_to_string(self, qt_mod): + expected = qt_mod.name + '+' + assert keyutils._modifiers_to_string(qt_mod.member) == expected + def test_missing(self, monkeypatch): monkeypatch.delattr(keyutils.Qt, 'Key_AltGr') # We don't want to test the key which is actually missing - we only @@ -160,6 +183,8 @@ def test_key_parse_error(keystr, expected): ('ab', ['a', 'ctrl+a', 'b']), ('a', ['ctrl+a', 'a']), ('a', ['a', 'ctrl+a']), + ('', ['ctrl+a']), + ('', ['num+a']), ]) def test_parse_keystr(keystr, parts): assert list(keyutils._parse_keystring(keystr)) == parts @@ -337,6 +362,9 @@ class TestKeySequence: ('', Qt.Key_Backtab, Qt.ShiftModifier, '', ''), ('', Qt.Key_Backtab, Qt.ControlModifier | Qt.ShiftModifier, '', ''), + + # Stripping of Qt.GroupSwitchModifier + ('', Qt.Key_A, Qt.GroupSwitchModifier, 'a', 'a'), ]) def test_append_event(self, old, key, modifiers, text, expected): seq = keyutils.KeySequence.parse(old) @@ -352,8 +380,6 @@ class TestKeySequence: seq.append_event(event) @pytest.mark.parametrize('keystr, expected', [ - ('', keyutils.KeySequence(Qt.ControlModifier | Qt.Key_X)), - ('', keyutils.KeySequence(Qt.MetaModifier | Qt.Key_X)), ('', keyutils.KeySequence(Qt.ControlModifier | Qt.AltModifier | Qt.Key_Y)), ('x', keyutils.KeySequence(Qt.Key_X)), @@ -364,6 +390,12 @@ class TestKeySequence: keyutils.KeySequence(Qt.ControlModifier | Qt.Key_X, Qt.MetaModifier | Qt.Key_Y)), + ('', keyutils.KeySequence(Qt.ShiftModifier | Qt.Key_X)), + ('', keyutils.KeySequence(Qt.AltModifier | Qt.Key_X)), + ('', keyutils.KeySequence(Qt.ControlModifier | Qt.Key_X)), + ('', keyutils.KeySequence(Qt.MetaModifier | Qt.Key_X)), + ('', keyutils.KeySequence(Qt.KeypadModifier | Qt.Key_X)), + ('>', keyutils.KeySequence(Qt.Key_Greater)), ('<', keyutils.KeySequence(Qt.Key_Less)), ('a>', keyutils.KeySequence(Qt.Key_A, Qt.Key_Greater)),