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

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):
     ('a<Ctrl+a>b', ['a', 'ctrl+a', 'b']),
     ('<Ctrl+a>a', ['ctrl+a', 'a']),
     ('a<Ctrl+a>', ['a', 'ctrl+a']),
+    ('<Ctrl-a>', ['ctrl+a']),
+    ('<Num-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, '', '<Shift+Tab>'),
         ('', Qt.Key_Backtab, Qt.ControlModifier | Qt.ShiftModifier, '',
          '<Control+Shift+Tab>'),
+
+        # 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', [
-        ('<Control-x>', keyutils.KeySequence(Qt.ControlModifier | Qt.Key_X)),
-        ('<Meta-x>', keyutils.KeySequence(Qt.MetaModifier | Qt.Key_X)),
         ('<Ctrl-Alt-y>',
          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)),
 
+        ('<Shift-x>', keyutils.KeySequence(Qt.ShiftModifier | Qt.Key_X)),
+        ('<Alt-x>', keyutils.KeySequence(Qt.AltModifier | Qt.Key_X)),
+        ('<Control-x>', keyutils.KeySequence(Qt.ControlModifier | Qt.Key_X)),
+        ('<Meta-x>', keyutils.KeySequence(Qt.MetaModifier | Qt.Key_X)),
+        ('<Num-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)),