Fix ModifiedKey roll-interaction with non-modified keys

kmk/keys.py

@@ -432,6 +432,9 @@ def __repr__(self):
     def on_press(self, keyboard: Keyboard, coord_int: Optional[int] = None) -> None:
         keyboard.hid_pending = True
         keyboard.keys_pressed.add(self)
+        if keyboard.implicit_modifier is not None:
+            keyboard.keys_pressed.discard(keyboard.implicit_modifier)
+            keyboard.implicit_modifier = None
 
     def on_release(self, keyboard: Keyboard, coord_int: Optional[int] = None) -> None:
         keyboard.hid_pending = True
@@ -457,30 +460,42 @@ def __call__(self, key: Key) -> Key:
 
 
 class ModifiedKey(Key):
-    code = -1
-
     def __init__(self, code: [Key, int], modifier: [ModifierKey]):
         # generate from code by maybe_make_shifted_key
         if isinstance(code, int):
             key = KeyboardKey(code=code)
         else:
             key = code
 
+        # stack modifier keys
+        if isinstance(key, ModifierKey):
+            modifier = ModifierKey(key.code | modifier.code)
+            key = None
         # stack modified keys
-        if isinstance(key, ModifiedKey):
+        elif isinstance(key, ModifiedKey):
             modifier = ModifierKey(key.modifier.code | modifier.code)
             key = key.key
+        # clone modifier so it doesn't override explicit mods
+        else:
+            modifier = ModifierKey(modifier.code)
 
         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)
+        if self.key is not None:
+            self.key.on_press(keyboard, coord_int)
+            if keyboard.implicit_modifier is not None:
+                keyboard.implicit_modifier.on_release(keyboard, coord_int)
+            keyboard.implicit_modifier = self.modifier
 
     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)
+        if self.key is not None:
+            self.key.on_release(keyboard, coord_int)
+            if keyboard.implicit_modifier == self.modifier:
+                keyboard.implicit_modifier = None
 
     def __repr__(self):
         return (

kmk/kmk_keyboard.py

@@ -53,6 +53,7 @@ class KMKKeyboard:
     keys_pressed = set()
     axes = set()
     _coordkeys_pressed = {}
+    implicit_modifier = None
     hid_type = HIDModes.USB
     secondary_hid_type = None
     _hid_helper = None

kmk/modules/string_substitution.py

@@ -51,7 +51,7 @@ def __init__(self, string: str) -> None:
             if key_code == KC.NO:
                 raise ValueError(f'Invalid character in dictionary: {char}')
             shifted = char.isupper() or (
-                isinstance(key_code, ModifiedKey) and key_code.modifier == KC.LSHIFT
+                isinstance(key_code, ModifiedKey) and key_code.modifier.code == 0x02
             )
             self._characters.append(Character(key_code, shifted))
 

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.N3}, {KC.LSHIFT, KC.N3, KC.A}, {KC.A}, {}],
+            [{KC.LSHIFT, KC.N3}, {KC.N3, KC.A}, {KC.A}, {}],
         )
 
     def test_hold_internal(self):

tests/test_kmk_keys.py

@@ -10,91 +10,119 @@ def test_basic_kmk_keyboard(self):
             [],
             [
                 [
-                    KC.HASH,
-                    KC.RALT(KC.HASH),
-                    KC.RALT(KC.LSFT(KC.N3)),
-                    KC.RALT(KC.LSFT),
-                    KC.RALT,
+                    KC.NO,
                     KC.TRNS,
                 ]
             ],
             debug_enabled=False,
         )
 
         keyboard.test(
-            'Shifted key',
-            [(0, True), (0, False)],
+            'No',
+            [(0, True)],
+            [{}],
+        )
+        self.assertEqual(keyboard.keyboard._coordkeys_pressed, {0: KC.NO})
+        keyboard.test(
+            'No',
+            [(0, False)],
+            [{}],
+        )
+
+        keyboard.test(
+            'Transparent',
+            [(1, True)],
+            [{}],
+        )
+        self.assertEqual(keyboard.keyboard._coordkeys_pressed, {1: KC.TRNS})
+
+        assert isinstance(KC.RGUI, ModifierKey)
+        assert isinstance(KC.Q, Key)
+        assert not isinstance(KC.Q, ModifierKey)
+
+    def test_modified_keys(self):
+        keyboard = KeyboardTest(
+            [],
             [
-                {
-                    KC.N3,
+                [
+                    KC.N0,
+                    KC.EXLM,
+                    KC.RALT(KC.AT),
+                    KC.RALT(KC.LSFT),
+                    KC.RALT(KC.LSFT(KC.N4)),
                     KC.LSFT,
-                },
-                {},
+                ]
             ],
+            debug_enabled=False,
         )
 
         keyboard.test(
-            'AltGr+Shifted key',
+            'Shifted key',
             [(1, True), (1, False)],
-            [
-                {
-                    KC.N3,
-                    KC.LSFT,
-                    KC.RALT,
-                },
-                {},
-            ],
+            [{KC.LSFT, KC.N1}, {}],
+        )
+
+        keyboard.test(
+            'Shifted key + key',
+            [(1, True), (0, True), (0, False), (1, False)],
+            [{KC.LSFT, KC.N1}, {KC.N0, KC.N1}, {KC.N1}, {}],
+        )
+
+        keyboard.test(
+            'Shifted key + key rolled',
+            [(1, True), (0, True), (1, False), (0, False)],
+            [{KC.LSFT, KC.N1}, {KC.N0, KC.N1}, {KC.N0}, {}],
         )
 
         keyboard.test(
-            'AltGr+Shift+key',
+            'Shifted key + shift',
+            [(1, True), (5, True), (5, False), (1, False)],
+            [{KC.LSFT, KC.N1}, {KC.N1}, {}],
+        )
+
+        keyboard.test(
+            'Shifted key + shift rolled',
+            [(1, True), (5, True), (1, False), (5, False)],
+            [{KC.LSFT, KC.N1}, {KC.LSFT}, {}],
+        )
+
+        keyboard.test(
+            'Shift + shifted key',
+            [(5, True), (1, True), (5, False), (1, False)],
+            [{KC.LSFT}, {KC.LSFT, KC.N1}, {}],
+        )
+
+        keyboard.test(
+            'Modified shifted key',
             [(2, True), (2, False)],
-            [
-                {
-                    KC.N3,
-                    KC.LSFT,
-                    KC.RALT,
-                },
-                {},
-            ],
+            [{KC.RALT, KC.LSFT, KC.N2}, {}],
         )
 
         keyboard.test(
-            'Shift+AltGr',
+            'Modified modifier',
             [(3, True), (3, False)],
-            [
-                {
-                    KC.LSFT,
-                    KC.RALT,
-                },
-                {},
-            ],
+            [{KC.RALT, KC.LSFT}, {}],
         )
 
         keyboard.test(
-            'AltGr',
-            [(4, True), (4, False)],
-            [
-                {
-                    KC.RALT,
-                },
-                {},
-            ],
+            'Modified modifier + shifted key',
+            [(3, True), (1, True), (1, False), (3, False)],
+            [{KC.RALT, KC.LSFT}, {KC.RALT, KC.LSFT, KC.N1}, {KC.RALT, KC.LSFT}, {}],
         )
 
         keyboard.test(
-            'Transparent',
-            [(5, True)],
-            [{}],
+            'Modified modified key',
+            [(4, True), (4, False)],
+            [{KC.RALT, KC.LSFT, KC.N4}, {}],
         )
-        self.assertEqual(keyboard.keyboard._coordkeys_pressed, {5: KC.TRNS})
 
-        assert isinstance(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)
         assert not isinstance(KC.RALT(KC.Q), ModifierKey)
+        self.assertEqual(KC.LSFT, KC.LSFT(KC.LSFT))
+        self.assertEqual(
+            KC.RALT(KC.LSFT).modifier.code, KC.RALT(KC.LSFT(KC.RALT)).modifier.code
+        )
 
 
 class TestKeys_dot(unittest.TestCase):
@@ -133,7 +161,7 @@ def test_custom_key(self):
             names=('EURO', '€'),
             constructor=ModifiedKey,
             code=KC.N2.code,
-            modifier=KC.LSFT(KC.ROPT),
+            modifier=KC.LSFT(KC.ROPT).modifier,
         )
         assert created is KC.get('EURO')
         assert created is KC.get('€')
@@ -174,7 +202,7 @@ def test_custom_key(self):
             names=('EURO', '€'),
             constructor=ModifiedKey,
             code=KC['N2'].code,
-            modifier=KC.LSFT(KC.ROPT),
+            modifier=KC.LSFT(KC.ROPT).modifier,
         )
         assert created is KC['EURO']
         assert created is KC['€']
@@ -220,7 +248,7 @@ def test_custom_key(self):
             names=('EURO', '€'),
             constructor=ModifiedKey,
             code=KC.get('N2').code,
-            modifier=KC.LSFT(KC.ROPT),
+            modifier=KC.LSFT(KC.ROPT).modifier,
         )
         assert created is KC.get('EURO')
         assert created is KC.get('€')

tests/test_string_substitution.py

@@ -1,10 +1,17 @@
 import unittest
 
-from kmk.keys import ALL_ALPHAS, ALL_NUMBERS, KC
+from kmk.keys import ALL_ALPHAS, ALL_NUMBERS, KC, ModifiedKey
 from kmk.modules.string_substitution import Character, Phrase, Rule, StringSubstitution
 from tests.keyboard_test import KeyboardTest
 
 
+def extract_code(key):
+    if isinstance(key, ModifiedKey):
+        return key.key.code, key.modifier.code
+    else:
+        return key.code
+
+
 class TestStringSubstitution(unittest.TestCase):
     def setUp(self) -> None:
         self.delay = KeyboardTest.loop_delay_ms
@@ -370,13 +377,13 @@ def test_character_constructs_properly(self):
             'unshifted character key code is correct',
         )
         self.assertEqual(
-            shifted_letter.key_code.__dict__,
-            KC.LSHIFT(KC.A).__dict__,
+            extract_code(shifted_letter.key_code),
+            extract_code(KC.LSHIFT(KC.A)),
             'shifted letter key code is correct',
         )
         self.assertEqual(
-            shifted_symbol.key_code.__dict__,
-            KC.LSHIFT(KC.N1).__dict__,
+            extract_code(shifted_symbol.key_code),
+            extract_code(KC.LSHIFT(KC.N1)),
             'shifted symbol key code is correct',
         )
 
@@ -397,8 +404,8 @@ def test_phrase_constructs_properly(self):
         for letter in ALL_ALPHAS:
             phrase = Phrase(letter)
             self.assertEqual(
-                phrase.get_character_at_index(0).key_code.__dict__,
-                KC.LSHIFT(KC[letter]).__dict__,
+                extract_code(phrase.get_character_at_index(0).key_code),
+                extract_code(KC.LSHIFT(KC[letter])),
                 f'Test failed when constructing phrase with upper-case letter {letter}',
             )
         # numbers
@@ -415,8 +422,8 @@ def test_phrase_constructs_properly(self):
             if character.isupper():
                 key = KC.LSHIFT(KC[character])
             self.assertEqual(
-                multi_character_phrase.get_character_at_index(i).key_code.__dict__,
-                key.__dict__,
+                extract_code(multi_character_phrase.get_character_at_index(i).key_code),
+                extract_code(key),
                 f'Test failed when constructing phrase with character {character}',
             )
 
@@ -462,8 +469,8 @@ def test_sanity_check(self):
             if character.isupper():
                 key = KC.LSHIFT(KC[character])
             self.assertEqual(
-                phrase.get_character_at_index(i).key_code.__dict__,
-                key.__dict__,
+                extract_code(phrase.get_character_at_index(i).key_code),
+                extract_code(key),
                 f'Test failed when constructing phrase with character {character}',
             )