some fixes, cleaner .change function of mapping

keymapper/gtk/row.py

@@ -91,6 +91,7 @@ class Row(Gtk.ListBoxRow):
         # the newest_keycode is populated since the ui regularly polls it
         # in order to display it in the status bar.
         key = self.get_keycode()
+        previous_type = key[0] if key else None
         previous_keycode = key[1] if key else None
 
         character = self.get_character()
@@ -104,8 +105,10 @@ class Row(Gtk.ListBoxRow):
             return
 
         # keycode is already set by some other row
-        if custom_mapping.get_character(ev_type, new_keycode) is not None:
-            msg = f'Keycode {new_keycode} is already mapped'
+        existing = custom_mapping.get_character(ev_type, new_keycode)
+        if existing is not None:
+            name = to_string(ev_type, new_keycode)
+            msg = f'"{name}" already mapped to {existing}"'
             logger.info(msg)
             self.window.get('status_bar').push(CTX_KEYCODE, msg)
             return
@@ -129,10 +132,9 @@ class Row(Gtk.ListBoxRow):
 
         # else, the keycode has changed, the character is set, all good
         custom_mapping.change(
-            ev_type=ev_type,
-            new_keycode=new_keycode,
+            new=(ev_type, new_keycode),
             character=character,
-            previous_keycode=previous_keycode
+            previous=(previous_type, previous_keycode)
         )
 
     def highlight(self):
@@ -153,10 +155,9 @@ class Row(Gtk.ListBoxRow):
         if key is not None:
             ev_type, keycode = key
             custom_mapping.change(
-                ev_type=ev_type,
-                new_keycode=keycode,
+                new=(ev_type, keycode),
                 character=character,
-                previous_keycode=None
+                previous=(None, None)
             )
 
     def put_together(self, character):

keymapper/gtk/window.py

@@ -140,11 +140,13 @@ class Window:
         """Ensure that one empty row is available at all times."""
         num_rows = len(self.get('key_list').get_children())
 
-        # verify that all mappings are displayed
-        if num_rows < len(custom_mapping):
+        # verify that all mappings are displayed. One of them
+        # is possible the empty row
+        num_maps = len(custom_mapping)
+        if num_rows < num_maps or num_rows > num_maps + 1:
             raise AssertionError(
                 f'custom_mapping contains {len(custom_mapping)} rows, '
-                f'but only {num_rows} are displayed'
+                f'but {num_rows} are displayed'
             )
 
         if num_rows == len(custom_mapping):

keymapper/mapping.py

@@ -25,7 +25,6 @@
 import os
 import json
 import copy
-import evdev
 
 from keymapper.logger import logger
 from keymapper.paths import get_config_path, touch
@@ -51,41 +50,60 @@ class Mapping:
     def __len__(self):
         return len(self._mapping)
 
-    def change(self, ev_type, new_keycode, character, previous_keycode=None):
+    def change(self, new, character, previous=(None, None)):
         """Replace the mapping of a keycode with a different one.
 
         Return True on success.
 
         Parameters
         ----------
-        ev_type : int
-            one of evdev.events, taken from the original source event.
-            Everything will be mapped to EV_KEY.
-        new_keycode : int
-            The source keycode, what the mouse would report without any
+        new : int, int
+            0: type, one of evdev.events, taken from the original source
+            event. Everything will be mapped to EV_KEY.
+            1: The source keycode, what the mouse would report without any
             modification.
         character : string or string[]
             A single character known to xkb or linux.
             Examples: KP_1, Shift_L, a, B, BTN_LEFT.
-        previous_keycode : int or None
+        previous : int, int
             If None, will not remove any previous mapping. If you recently
             used 10 for new_keycode and want to overwrite that with 11,
             provide 5 here.
         """
+        new_type, new_keycode = new
+        prev_type, prev_keycode = previous
+
+        # either both of them are None, or both integers
+        if prev_keycode is None and prev_keycode != prev_type:
+            logger.error('Got (%s, %s) for previous', prev_type, prev_keycode)
+        if new_keycode is None and prev_keycode != new_type:
+            logger.error('Got (%s, %s) for new', new_type, new_keycode)
+
         try:
             new_keycode = int(new_keycode)
-            if previous_keycode is not None:
-                previous_keycode = int(previous_keycode)
+            new_type = int(new_type)
+            if prev_keycode is not None:
+                prev_keycode = int(prev_keycode)
+            if prev_type is not None:
+                prev_type = int(prev_type)
         except ValueError:
-            logger.error('Can only use numbers as keycodes')
+            logger.error('Can only use numbers in the tuples')
             return False
 
         if new_keycode and character:
-            self._mapping[(ev_type, new_keycode)] = character
-            if new_keycode != previous_keycode:
+            logger.debug(
+                'type:%s, code:%s will map to %s, replacing type:%s, code:%s',
+                new_type, new_keycode, character, prev_type, prev_keycode
+            )
+            self._mapping[(new_type, new_keycode)] = character
+            code_changed = new_keycode != prev_keycode
+            if code_changed and prev_keycode is not None:
                 # clear previous mapping of that code, because the line
                 # representing that one will now represent a different one.
-                self.clear(ev_type, previous_keycode)
+                # TODO test that None won't reach the clear function.
+                # TODO test that overwriting an entry with a different type
+                #  works
+                self.clear(prev_type, prev_keycode)
             self.changed = True
             return True
 
@@ -100,7 +118,16 @@ class Mapping:
         ev_type : int
             one of evdev.events
         """
+        assert keycode is not None
+        assert ev_type is not None
+        assert isinstance(ev_type, int)
+        assert isinstance(keycode, int)
+
         if self._mapping.get((ev_type, keycode)) is not None:
+            logger.debug(
+                'type:%s, code:%s will be cleared',
+                ev_type, keycode
+            )
             del self._mapping[(ev_type, keycode)]
             self.changed = True
 

tests/testcases/test_daemon.py

@@ -85,8 +85,8 @@ class TestDaemon(unittest.TestCase):
         keycode_from_2 = 12
         keycode_to_2 = 100
 
-        custom_mapping.change(EV_KEY, keycode_from_1, 'a')
-        custom_mapping.change(EV_KEY, keycode_from_2, 'b')
+        custom_mapping.change((EV_KEY, keycode_from_1), 'a')
+        custom_mapping.change((EV_KEY, keycode_from_2), 'b')
         clear_system_mapping()
         system_mapping['a'] = keycode_to_1
         system_mapping['b'] = keycode_to_2

tests/testcases/test_injector.py

@@ -76,10 +76,7 @@ class TestInjector(unittest.TestCase):
                 }
 
         mapping = Mapping()
-        mapping.change(EV_KEY,
-            new_keycode=80,
-            character='a'
-        )
+        mapping.change((EV_KEY, 80), 'a')
 
         maps_to = system_mapping['a']
 
@@ -99,7 +96,7 @@ class TestInjector(unittest.TestCase):
 
     def test_grab(self):
         # path is from the fixtures
-        custom_mapping.change(EV_KEY, 10, 'a')
+        custom_mapping.change((EV_KEY, 10), 'a')
 
         self.injector = KeycodeInjector('device 1', custom_mapping)
         path = '/dev/input/event10'
@@ -124,7 +121,7 @@ class TestInjector(unittest.TestCase):
 
     def test_skip_unused_device(self):
         # skips a device because its capabilities are not used in the mapping
-        custom_mapping.change(EV_KEY, 10, 'a')
+        custom_mapping.change((EV_KEY, 10), 'a')
         self.injector = KeycodeInjector('device 1', custom_mapping)
         path = '/dev/input/event11'
         device, abs_to_rel = self.injector._prepare_device(path)
@@ -284,11 +281,11 @@ class TestInjector(unittest.TestCase):
         self.assertIn(('b', 0), history)
 
     def test_injector(self):
-        custom_mapping.change(EV_KEY, 8, 'k(KEY_Q).k(w)')
-        custom_mapping.change(EV_KEY, 9, 'a')
+        custom_mapping.change((EV_KEY, 8), 'k(KEY_Q).k(w)')
+        custom_mapping.change((EV_KEY, 9), 'a')
         # one mapping that is unknown in the system_mapping on purpose
         input_b = 10
-        custom_mapping.change(EV_KEY, input_b, 'b')
+        custom_mapping.change((EV_KEY, input_b), 'b')
 
         clear_system_mapping()
         code_a = 100

tests/testcases/test_integration.py

@@ -157,9 +157,9 @@ class TestIntegration(unittest.TestCase):
     def test_select_device(self):
         # creates a new empty preset when no preset exists for the device
         self.window.on_select_device(FakeDropdown('device 1'))
-        custom_mapping.change(EV_KEY, 50, 'q')
-        custom_mapping.change(EV_KEY, 51, 'u')
-        custom_mapping.change(EV_KEY, 52, 'x')
+        custom_mapping.change((EV_KEY, 50), 'q')
+        custom_mapping.change((EV_KEY, 51), 'u')
+        custom_mapping.change((EV_KEY, 52), 'x')
         self.assertEqual(len(custom_mapping), 3)
         self.window.on_select_device(FakeDropdown('device 2'))
         self.assertEqual(len(custom_mapping), 0)
@@ -345,12 +345,12 @@ class TestIntegration(unittest.TestCase):
         remove(row_3, None, 'c', 1)
 
     def test_rename_and_save(self):
-        custom_mapping.change(EV_KEY, 14, 'a', None)
+        custom_mapping.change((EV_KEY, 14), 'a', (None, None))
         self.assertEqual(self.window.selected_preset, 'new preset')
         self.window.on_save_preset_clicked(None)
         self.assertEqual(custom_mapping.get_character(EV_KEY, 14), 'a')
 
-        custom_mapping.change(EV_KEY, 14, 'b', None)
+        custom_mapping.change((EV_KEY, 14), 'b', (None, None))
         self.window.get('preset_name_input').set_text('asdf')
         self.window.on_save_preset_clicked(None)
         self.assertEqual(self.window.selected_preset, 'asdf')
@@ -385,7 +385,7 @@ class TestIntegration(unittest.TestCase):
         gtk_iteration()
         self.assertEqual(self.window.selected_preset, 'new preset')
         self.assertFalse(os.path.exists(f'{CONFIG}/device 1/abc 123.json'))
-        custom_mapping.change(EV_KEY, 10, '1', None)
+        custom_mapping.change((EV_KEY, 10), '1', (None, None))
         self.window.on_save_preset_clicked(None)
         gtk_iteration()
         self.assertEqual(self.window.selected_preset, 'abc 123')

tests/testcases/test_mapping.py

@@ -41,9 +41,9 @@ class TestMapping(unittest.TestCase):
 
     def test_clone(self):
         mapping1 = Mapping()
-        mapping1.change(EV_KEY, 1, 'a')
+        mapping1.change((EV_KEY, 1), 'a')
         mapping2 = mapping1.clone()
-        mapping1.change(EV_KEY, 2, 'b')
+        mapping1.change((EV_KEY, 2), 'b')
 
         self.assertEqual(mapping1.get_character(EV_KEY, 1), 'a')
         self.assertEqual(mapping1.get_character(EV_KEY, 2), 'b')
@@ -52,9 +52,9 @@ class TestMapping(unittest.TestCase):
         self.assertIsNone(mapping2.get_character(EV_KEY, 2))
 
     def test_save_load(self):
-        self.mapping.change(EV_KEY, 10, '1')
-        self.mapping.change(EV_KEY, 11, '2')
-        self.mapping.change(EV_KEY, 12, '3')
+        self.mapping.change((EV_KEY, 10), '1')
+        self.mapping.change((EV_KEY, 11), '2')
+        self.mapping.change((EV_KEY, 12), '3')
         self.mapping.config['foo'] = 'bar'
         self.mapping.save('device 1', 'test')
 
@@ -70,41 +70,42 @@ class TestMapping(unittest.TestCase):
 
     def test_change(self):
         # 1 is not assigned yet, ignore it
-        self.mapping.change(EV_KEY, 2, 'a', 1)
+        self.mapping.change((EV_KEY, 2), 'a', (EV_KEY, 1))
         self.assertTrue(self.mapping.changed)
         self.assertIsNone(self.mapping.get_character(EV_KEY, 1))
         self.assertEqual(self.mapping.get_character(EV_KEY, 2), 'a')
         self.assertEqual(len(self.mapping), 1)
 
         # change 2 to 3 and change a to b
-        self.mapping.change(EV_KEY, 3, 'b', 2)
+        self.mapping.change((EV_KEY, 3), 'b', (EV_KEY, 2))
         self.assertIsNone(self.mapping.get_character(EV_KEY, 2))
         self.assertEqual(self.mapping.get_character(EV_KEY, 3), 'b')
         self.assertEqual(len(self.mapping), 1)
 
         # add 4
-        self.mapping.change(EV_KEY, 4, 'c', None)
+        self.mapping.change((EV_KEY, 4), 'c', (None, None))
         self.assertEqual(self.mapping.get_character(EV_KEY, 3), 'b')
         self.assertEqual(self.mapping.get_character(EV_KEY, 4), 'c')
         self.assertEqual(len(self.mapping), 2)
 
         # change the mapping of 4 to d
-        self.mapping.change(EV_KEY, 4, 'd', None)
+        self.mapping.change((EV_KEY, 4), 'd', (None, None))
         self.assertEqual(self.mapping.get_character(EV_KEY, 4), 'd')
         self.assertEqual(len(self.mapping), 2)
 
         # this also works in the same way
-        self.mapping.change(EV_KEY, 4, 'e', 4)
+        self.mapping.change((EV_KEY, 4), 'e', (EV_KEY, 4))
         self.assertEqual(self.mapping.get_character(EV_KEY, 4), 'e')
         self.assertEqual(len(self.mapping), 2)
 
         # and this
-        self.mapping.change(EV_KEY, '4', 'f', '4')
+        self.mapping.change((EV_KEY, '4'), 'f', (str(EV_KEY), '4'))
         self.assertEqual(self.mapping.get_character(EV_KEY, 4), 'f')
         self.assertEqual(len(self.mapping), 2)
 
         # non-int keycodes are ignored
-        self.mapping.change(EV_KEY, 'b', 'c', 'a')
+        # TODO what about non-int types?
+        self.mapping.change((EV_KEY, 'b'), 'c', (EV_KEY, 'a'))
         self.assertEqual(len(self.mapping), 2)
 
     def test_clear(self):
@@ -119,10 +120,10 @@ class TestMapping(unittest.TestCase):
         self.assertEqual(len(self.mapping), 0)
         self.assertTrue(self.mapping.changed)
 
-        self.mapping.change(EV_KEY, 10, 'KP_1', None)
+        self.mapping.change((EV_KEY, 10), 'KP_1', (None, None))
         self.assertTrue(self.mapping.changed)
-        self.mapping.change(EV_KEY, 20, 'KP_2', None)
-        self.mapping.change(EV_KEY, 30, 'KP_3', None)
+        self.mapping.change((EV_KEY, 20), 'KP_2', (None, None))
+        self.mapping.change((EV_KEY, 30), 'KP_3', (None, None))
         self.assertEqual(len(self.mapping), 3)
         self.mapping.clear(EV_KEY, 20)
         self.assertEqual(len(self.mapping), 2)
@@ -131,9 +132,9 @@ class TestMapping(unittest.TestCase):
         self.assertEqual(self.mapping.get_character(EV_KEY, 30), 'KP_3')
 
     def test_empty(self):
-        self.mapping.change(EV_KEY, 10, '1')
-        self.mapping.change(EV_KEY, 11, '2')
-        self.mapping.change(EV_KEY, 12, '3')
+        self.mapping.change((EV_KEY, 10), '1')
+        self.mapping.change((EV_KEY, 11), '2')
+        self.mapping.change((EV_KEY, 12), '3')
         self.assertEqual(len(self.mapping), 3)
         self.mapping.empty()
         self.assertEqual(len(self.mapping), 0)