Refactor modified keys

docs/en/keys.md

@@ -32,12 +32,6 @@ objects have a few core pieces of information:
   computer, which will translate that integer to something meaningful - for
   example, `code=4` becomes `a` on a US QWERTY/Dvorak keyboard).
 
-- Their attached modifiers (to implement things like shifted keys or `KC.HYPR`,
-  which are single key presses sending along more than one key in a single HID
-  report. For almost all purposes outside of KMK core,
-  this field should be ignored - it can be safely populated through far more
-  sane means than futzing with it by hand.
-
 - Handlers for "press" (sometimes known as "keydown") and "release" (sometimes
   known as "keyup") events. KMK provides handlers for standard keyboard
   functions and some special override keys (like `KC.GESC`, which is an enhanced

kmk/hid.py

@@ -127,9 +127,6 @@ def create_report(self, keys_pressed, axes):
                 self.add_pd(key)
             else:
                 self.add_key(key)
-                if key.has_modifiers:
-                    for mod in key.has_modifiers:
-                        self.add_modifier(mod)
 
         for axis in axes:
             self.move_axis(axis)
@@ -175,23 +172,15 @@ def clear_non_modifiers(self):
 
     def add_modifier(self, modifier):
         if isinstance(modifier, ModifierKey):
-            if modifier.code == ModifierKey.FAKE_CODE:
-                for mod in modifier.has_modifiers:
-                    self.report_mods[0] |= mod
-            else:
-                self.report_mods[0] |= modifier.code
+            self.report_mods[0] |= modifier.code
         else:
             self.report_mods[0] |= modifier
 
         return self
 
     def remove_modifier(self, modifier):
         if isinstance(modifier, ModifierKey):
-            if modifier.code == ModifierKey.FAKE_CODE:
-                for mod in modifier.has_modifiers:
-                    self.report_mods[0] ^= mod
-            else:
-                self.report_mods[0] ^= modifier.code
+            self.report_mods[0] ^= modifier.code
         else:
             self.report_mods[0] ^= modifier
 

kmk/keys.py

@@ -286,7 +286,9 @@ def maybe_make_shifted_key(candidate: str) -> Optional[Key]:
 
     for code, names in codes:
         if candidate in names:
-            return make_key(code=code, names=names, has_modifiers={KC.LSFT.code})
+            return make_key(
+                code=code, names=names, key_type=ModifiedKey, modifier=KC.LSFT
+            )
 
 
 def maybe_make_international_key(candidate: str) -> Optional[Key]:
@@ -442,7 +444,6 @@ class Key:
     def __init__(
         self,
         code: int,
-        has_modifiers: Optional[list[Key, ...]] = None,
         on_press: Callable[
             [object, Key, Keyboard, ...], None
         ] = handlers.default_pressed,
@@ -452,8 +453,6 @@ def __init__(
         meta: object = object(),
     ):
         self.code = code
-        self.has_modifiers = has_modifiers
-        # cast to bool() in case we get a None value
 
         self._on_press = on_press
         self._on_release = on_release
@@ -463,7 +462,7 @@ def __call__(self) -> Key:
         return self
 
     def __repr__(self):
-        return f'Key(code={self.code}, has_modifiers={self.has_modifiers})'
+        return f'Key(code={self.code})'
 
     def on_press(self, keyboard: Keyboard, coord_int: Optional[int] = None) -> None:
         self._on_press(self, keyboard, KC, coord_int)
@@ -473,40 +472,57 @@ def on_release(self, keyboard: Keyboard, coord_int: Optional[int] = None) -> Non
 
 
 class ModifierKey(Key):
-    FAKE_CODE = const(-1)
+    def __call__(self, key: Key) -> Key:
+        # don't duplicate when applying the same modifier twice
+        if (
+            isinstance(key, ModifiedKey)
+            and key.modifier.code & self.code == key.modifier.code
+        ):
+            return key
+        elif isinstance(key, ModifierKey) and key.code & self.code == key.code:
+            return key
 
-    def __call__(
-        self,
-        modified_key: Optional[Key] = None,
-    ) -> Key:
-        if modified_key is None:
-            return super().__call__()
-
-        modifiers = set()
-        code = modified_key.code
-
-        if self.code != ModifierKey.FAKE_CODE:
-            modifiers.add(self.code)
-        if self.has_modifiers:
-            modifiers |= self.has_modifiers
-        if modified_key.has_modifiers:
-            modifiers |= modified_key.has_modifiers
-
-        if isinstance(modified_key, ModifierKey):
-            if modified_key.code != ModifierKey.FAKE_CODE:
-                modifiers.add(modified_key.code)
-            code = ModifierKey.FAKE_CODE
-
-        return type(modified_key)(
-            code=code,
-            has_modifiers=modifiers,
-            on_press=modified_key._on_press,
-            on_release=modified_key._on_release,
-            meta=modified_key.meta,
-        )
+        return ModifiedKey(key, self)
 
     def __repr__(self):
-        return f'ModifierKey(code={self.code}, has_modifiers={self.has_modifiers})'
+        return f'ModifierKey(code={self.code})'
+
+
+class ModifiedKey(Key):
+    meta = None
+    code = -1
+
+    def __init__(self, code: [Key, int], modifier: [ModifierKey]):
+        # generate from code by maybe_make_shifted_key
+        if isinstance(code, int):
+            key = Key(code=code)
+        else:
+            key = code
+
+        # stack modified keys
+        if isinstance(key, ModifiedKey):
+            modifier = ModifierKey(key.modifier.code | modifier.code)
+            key = key.key
+
+        self.key = key
+        self.modifier = modifier
+
+    def on_press(self, keyboard: Keyboard, coord_int: Optional[int] = None) -> None:
+        self.modifier.on_press(keyboard, coord_int)
+        self.key.on_press(keyboard, coord_int)
+
+    def on_release(self, keyboard: Keyboard, coord_int: Optional[int] = None) -> None:
+        self.key.on_release(keyboard, coord_int)
+        self.modifier.on_release(keyboard, coord_int)
+
+    def __repr__(self):
+        return (
+            'ModifiedKey(key='
+            + str(self.key)
+            + ', modifier='
+            + str(self.modifier)
+            + ')'
+        )
 
 
 class ConsumerKey(Key):

kmk/kmk_keyboard.py

@@ -246,11 +246,9 @@ def resume_process_key(
         self._resume_buffer.append(ksf)
 
     def remove_key(self, keycode: Key) -> None:
-        self.keys_pressed.discard(keycode)
         self.process_key(keycode, False)
 
     def add_key(self, keycode: Key) -> None:
-        self.keys_pressed.add(keycode)
         self.process_key(keycode, True)
 
     def tap_key(self, keycode: Key) -> None:

kmk/modules/string_substitution.py

@@ -5,7 +5,7 @@
     pass
 from micropython import const
 
-from kmk.keys import KC, Key, ModifierKey
+from kmk.keys import KC, Key, ModifiedKey, ModifierKey
 from kmk.modules import Module
 
 
@@ -27,6 +27,11 @@ def __init__(self, key_code: Key, is_shifted: bool) -> None:
 
     def __eq__(self, other: any) -> bool:  # type: ignore
         try:
+            if isinstance(self.key_code, ModifiedKey):
+                return (
+                    self.key_code.key.code == other.key_code.key.code
+                    and self.is_shifted == other.is_shifted
+                )
             return (
                 self.key_code.code == other.key_code.code
                 and self.is_shifted == other.is_shifted
@@ -45,7 +50,9 @@ def __init__(self, string: str) -> None:
             key_code = KC[char]
             if key_code == KC.NO:
                 raise ValueError(f'Invalid character in dictionary: {char}')
-            shifted = char.isupper() or key_code.has_modifiers == {2}
+            shifted = char.isupper() or (
+                isinstance(key_code, ModifiedKey) and key_code.modifier == KC.LSHIFT
+            )
             self._characters.append(Character(key_code, shifted))
 
     def next_character(self) -> None:

tests/test_autoshift.py

@@ -76,7 +76,7 @@ def test_hold_shifted_hold_alpha(self):
         self.kb.test(
             '',
             [(2, True), (0, True), t_after, (2, False), (0, False)],
-            [{KC.LSHIFT, KC.HASH}, {KC.LSHIFT, KC.HASH, KC.A}, {KC.A}, {}],
+            [{KC.LSHIFT, KC.N3}, {KC.LSHIFT, KC.N3, KC.A}, {KC.A}, {}],
         )
 
     def test_hold_internal(self):

tests/test_kmk_keys.py

@@ -1,6 +1,6 @@
 import unittest
 
-from kmk.keys import KC, Key, ModifierKey, make_key
+from kmk.keys import KC, Key, ModifiedKey, ModifierKey, make_key
 from tests.keyboard_test import KeyboardTest
 
 
@@ -14,13 +14,13 @@ def test_basic_kmk_keyboard(self):
                     KC.RALT(KC.HASH),
                     KC.RALT(KC.LSFT(KC.N3)),
                     KC.RALT(KC.LSFT),
-                    # Note: this is correct, if unusual, syntax. It's a useful test because it failed silently on previous builds.
-                    KC.RALT(KC.LSFT)(KC.N3),
                     KC.RALT,
                     KC.TRNS,
                 ]
             ],
+            debug_enabled=False,
         )
+
         keyboard.test(
             'Shifted key',
             [(0, True), (0, False)],
@@ -32,6 +32,7 @@ def test_basic_kmk_keyboard(self):
                 {},
             ],
         )
+
         keyboard.test(
             'AltGr+Shifted key',
             [(1, True), (1, False)],
@@ -44,6 +45,7 @@ def test_basic_kmk_keyboard(self):
                 {},
             ],
         )
+
         keyboard.test(
             'AltGr+Shift+key',
             [(2, True), (2, False)],
@@ -56,6 +58,7 @@ def test_basic_kmk_keyboard(self):
                 {},
             ],
         )
+
         keyboard.test(
             'Shift+AltGr',
             [(3, True), (3, False)],
@@ -67,21 +70,10 @@ def test_basic_kmk_keyboard(self):
                 {},
             ],
         )
-        keyboard.test(
-            'AltGr+Shift+key, alternate chaining',
-            [(4, True), (4, False)],
-            [
-                {
-                    KC.N3,
-                    KC.LSFT,
-                    KC.RALT,
-                },
-                {},
-            ],
-        )
+
         keyboard.test(
             'AltGr',
-            [(5, True), (5, False)],
+            [(4, True), (4, False)],
             [
                 {
                     KC.RALT,
@@ -92,13 +84,13 @@ def test_basic_kmk_keyboard(self):
 
         keyboard.test(
             'Transparent',
-            [(6, True)],
+            [(5, True)],
             [{}],
         )
-        self.assertEqual(keyboard.keyboard._coordkeys_pressed, {6: KC.TRNS})
+        self.assertEqual(keyboard.keyboard._coordkeys_pressed, {5: KC.TRNS})
 
         assert isinstance(KC.RGUI, ModifierKey)
-        assert isinstance(KC.RALT(KC.RGUI), ModifierKey)
+        assert isinstance(KC.RALT(KC.RGUI), ModifiedKey)
         assert isinstance(KC.Q, Key)
         assert not isinstance(KC.Q, ModifierKey)
         assert isinstance(KC.RALT(KC.Q), Key)
@@ -143,7 +135,8 @@ def test_custom_key(self):
                 'EURO',
                 '€',
             ),
-            has_modifiers={KC.LSFT.code, KC.ROPT.code},
+            key_type=ModifiedKey,
+            modifier=KC.LSFT(KC.ROPT),
         )
         assert created is KC.get('EURO')
         assert created is KC.get('€')
@@ -186,7 +179,8 @@ def test_custom_key(self):
                 'EURO',
                 '€',
             ),
-            has_modifiers={KC['LSFT'].code, KC['ROPT'].code},
+            key_type=ModifiedKey,
+            modifier=KC.LSFT(KC.ROPT),
         )
         assert created is KC['EURO']
         assert created is KC['€']
@@ -234,7 +228,8 @@ def test_custom_key(self):
                 'EURO',
                 '€',
             ),
-            has_modifiers={KC.get('LSFT').code, KC.get('ROPT').code},
+            key_type=ModifiedKey,
+            modifier=KC.LSFT(KC.ROPT),
         )
         assert created is KC.get('EURO')
         assert created is KC.get('€')