From b3834835edb32188bb53dcaab502fef689e6b07d Mon Sep 17 00:00:00 2001
From: Florian Bruhin <git@the-compiler.org>
Date: Fri, 2 Mar 2018 14:16:40 +0100
Subject: [PATCH] Bring back keyutils.is_modifier() and modifier handling

Turns out when we press yY, we get three events:

Qt.Key_Y, Qt.NoModifier
Qt.Key_Shift, Qt.ShiftModifier
Qt.Key_Y, Qt.ShiftModifier

If we don't ignore the second one, our keychain will be interrupted by the Shift
keypress.
---
 qutebrowser/keyinput/basekeyparser.py     |  4 +++
 qutebrowser/keyinput/keyutils.py          | 30 ++++++++++++++++-------
 tests/unit/keyinput/conftest.py           |  1 +
 tests/unit/keyinput/test_basekeyparser.py | 11 +++++++++
 tests/unit/keyinput/test_keyutils.py      |  9 +++++++
 5 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/qutebrowser/keyinput/basekeyparser.py b/qutebrowser/keyinput/basekeyparser.py
index a8501ab4c..2c934617b 100644
--- a/qutebrowser/keyinput/basekeyparser.py
+++ b/qutebrowser/keyinput/basekeyparser.py
@@ -128,6 +128,10 @@ class BaseKeyParser(QObject):
         txt = str(keyutils.KeyInfo.from_event(e))
         self._debug_log("Got key: 0x{:x} / text: '{}'".format(key, txt))
 
+        if keyutils.is_modifier_key(key):
+            self._debug_log("Ignoring, only modifier")
+            return QKeySequence.NoMatch
+
         if (txt.isdigit() and self._supports_count and not
                 (not self._count and txt == '0')):
             assert len(txt) == 1, txt
diff --git a/qutebrowser/keyinput/keyutils.py b/qutebrowser/keyinput/keyutils.py
index 1cb941ac7..e456da045 100644
--- a/qutebrowser/keyinput/keyutils.py
+++ b/qutebrowser/keyinput/keyutils.py
@@ -29,10 +29,29 @@ from PyQt5.QtGui import QKeySequence, QKeyEvent
 from qutebrowser.utils import utils
 
 
+# Map Qt::Key values to their Qt::KeyboardModifier value.
+_MODIFIER_MAP = {
+    Qt.Key_Shift: Qt.ShiftModifier,
+    Qt.Key_Control: Qt.ControlModifier,
+    Qt.Key_Alt: Qt.AltModifier,
+    Qt.Key_Meta: Qt.MetaModifier,
+    Qt.Key_Mode_switch: Qt.GroupSwitchModifier,
+}
+
+
 def is_printable(key):
     return key <= 0xff
 
 
+def is_modifier_key(key):
+    """Test whether the given key is a modifier.
+
+    This only considers keys which are part of Qt::KeyboardModifiers, i.e. which
+    would interrupt a key chain like "yY" when handled.
+    """
+    return key in _MODIFIER_MAP
+
+
 def _key_to_string(key):
     """Convert a Qt::Key member to a meaningful name.
 
@@ -195,18 +214,11 @@ class KeyInfo:
             A name of the key (combination) as a string.
         """
         key_string = _key_to_string(self.key)
-        modifier_map = {
-            Qt.Key_Shift: Qt.ShiftModifier,
-            Qt.Key_Control: Qt.ControlModifier,
-            Qt.Key_Alt: Qt.AltModifier,
-            Qt.Key_Meta: Qt.MetaModifier,
-            Qt.Key_Mode_switch: Qt.GroupSwitchModifier,
-        }
         modifiers = int(self.modifiers)
 
-        if self.key in modifier_map:
+        if self.key in _MODIFIER_MAP:
             # Don't return e.g. <Shift+Shift>
-            modifiers &= ~modifier_map[self.key]
+            modifiers &= ~_MODIFIER_MAP[self.key]
         elif is_printable(self.key):
             # "normal" binding
             # FIXME Add a test to make sure Tab doesn't become TAB
diff --git a/tests/unit/keyinput/conftest.py b/tests/unit/keyinput/conftest.py
index 0713c5d26..f78ed61ad 100644
--- a/tests/unit/keyinput/conftest.py
+++ b/tests/unit/keyinput/conftest.py
@@ -27,6 +27,7 @@ BINDINGS = {'prompt': {'<Ctrl-a>': 'message-info ctrla',
                        'ba': 'message-info ba',
                        'ax': 'message-info ax',
                        'ccc': 'message-info ccc',
+                       'yY': 'yank -s',
                        '0': 'message-info 0'},
             'command': {'foo': 'message-info bar',
                         '<Ctrl+X>': 'message-info ctrlx'},
diff --git a/tests/unit/keyinput/test_basekeyparser.py b/tests/unit/keyinput/test_basekeyparser.py
index 5da4efa9b..872709d6e 100644
--- a/tests/unit/keyinput/test_basekeyparser.py
+++ b/tests/unit/keyinput/test_basekeyparser.py
@@ -241,6 +241,17 @@ class TestKeyChain:
         handle_text((Qt.Key_B, 'b'))
         assert not keyparser.execute.called
 
+    def test_binding_with_shift(self, keyparser, fake_keyevent_factory):
+        """Simulate a binding which involves shift."""
+        keyparser.handle(
+            fake_keyevent_factory(Qt.Key_Y, text='y'))
+        keyparser.handle(
+            fake_keyevent_factory(Qt.Key_Shift, Qt.ShiftModifier, text=''))
+        keyparser.handle(
+            fake_keyevent_factory(Qt.Key_Y, Qt.ShiftModifier, text='Y'))
+
+        keyparser.execute.assert_called_once_with('yank -s', None)
+
 
 class TestCount:
 
diff --git a/tests/unit/keyinput/test_keyutils.py b/tests/unit/keyinput/test_keyutils.py
index 154330af2..220d16dc6 100644
--- a/tests/unit/keyinput/test_keyutils.py
+++ b/tests/unit/keyinput/test_keyutils.py
@@ -200,3 +200,12 @@ def test_normalize_keystr(orig, normalized):
 ])
 def test_is_printable(key, printable):
     assert keyutils.is_printable(key) == printable
+
+
+@pytest.mark.parametrize('key, ismodifier', [
+    (Qt.Key_Control, True),
+    (Qt.Key_X, False),
+    (Qt.Key_Super_L, False),  # Modifier but not in _MODIFIER_MAP
+])
+def test_is_modifier_key(key, ismodifier):
+    assert keyutils.is_modifier_key(key) == ismodifier