Change shortcuts _mark_conflicts logic to always compare between st…

…rings representations of shortcuts (napari#7124)

# References and relevant issues

Related to napari#7059 napari#6204 napari#7113

# Description

While checking PR napari#7113 I noticed that the mark conflicts logic was not detecting conflicts properly. From what I have checked seems like the action manager shortcuts lists ends up having a mix of `KeyBinding` instances and string representations of `KeyBinding`s.

Also, recalling some related work, I think some discussion about the `mark_conflicts` logic was done over PR napari#6204 (https://github.com/napari/napari/pull/6204/files/f2e39a83f64c6720b6b386b98cb999a86c9704fa#r1348933980) and a commit with some changes was also done there (napari@5ffb237).

This follows a similar implementation path and does some changes to the conflicts logic to always use a standard string representation to compare shortcuts when looking for conflicts. This also extends the current test that checks the `_mark_conflicts` logic to do the check using the different representations a shortcut could have which are encapsultated by the `KeyBindingLike` type (a `KeyBinding` instance itself, its `str` representation or its `int` representation).

Co-authored-by: Kira Evans <contact@kne42.me>

napari/_qt/widgets/_tests/test_shortcut_editor_widget.py

@@ -11,6 +11,7 @@
 from napari.settings import get_settings
 from napari.utils.action_manager import action_manager
 from napari.utils.interactions import KEY_SYMBOLS
+from napari.utils.key_bindings import KeyBinding
 
 META_CONTROL_KEY = Qt.KeyboardModifier.ControlModifier
 if sys.platform == 'darwin':
@@ -58,14 +59,42 @@ def test_layer_actions(shortcut_editor_widget):
 
 def test_mark_conflicts(shortcut_editor_widget, qtbot):
     widget = shortcut_editor_widget()
-    widget._table.item(0, widget._shortcut_col).setText('U')
+    ctrl_keybinding = KeyBinding.from_str('Ctrl')
+    u_keybinding = KeyBinding.from_str('U')
     act = widget._table.item(0, widget._action_col).text()
-    assert action_manager._shortcuts[act][0] == 'U'
+
+    # Add check for initial/default keybinding (first shortcuts column) and
+    # added one (second shortcuts column)
+    assert action_manager._shortcuts[act][0] == ctrl_keybinding
+    widget._table.item(0, widget._shortcut_col2).setText(str(u_keybinding))
+    assert action_manager._shortcuts[act][1] == str(u_keybinding)
+
+    # Check conflicts detection using `KeyBindingLike` params
+    # (`KeyBinding`, `str` and `int` representations of a shortcut)
+    with patch.object(WarnPopup, 'exec_') as mock:
+        assert not widget._mark_conflicts(ctrl_keybinding, 1)
+        assert mock.called
+    with patch.object(WarnPopup, 'exec_') as mock:
+        assert not widget._mark_conflicts(str(ctrl_keybinding), 1)
+        assert mock.called
     with patch.object(WarnPopup, 'exec_') as mock:
-        assert not widget._mark_conflicts(action_manager._shortcuts[act][0], 1)
+        assert not widget._mark_conflicts(int(ctrl_keybinding), 1)
         assert mock.called
-    assert widget._mark_conflicts('Y', 1)
+
+    with patch.object(WarnPopup, 'exec_') as mock:
+        assert not widget._mark_conflicts(u_keybinding, 1)
+        assert mock.called
+    with patch.object(WarnPopup, 'exec_') as mock:
+        assert not widget._mark_conflicts(str(u_keybinding), 1)
+        assert mock.called
+
+    # Check no conflicts are found using `KeyBindingLike` params
+    # (`KeyBinding`, `str` and `int` representations of a shortcut)
     # "Y" is arbitrary chosen and on conflict with existing shortcut should be changed
+    y_keybinding = KeyBinding.from_str('Y')
+    assert widget._mark_conflicts(y_keybinding, 1)
+    assert widget._mark_conflicts(str(y_keybinding), 1)
+    assert widget._mark_conflicts(int(y_keybinding), 1)
     qtbot.add_widget(widget._warn_dialog)
 
 

napari/_qt/widgets/qt_keyboard_settings.py

@@ -323,7 +323,9 @@ def _mark_conflicts(self, new_shortcut, row) -> bool:
         for row1, (action_name, action) in enumerate(actions_all.items()):
             shortcuts = action_manager._shortcuts.get(action_name, [])
 
-            if new_shortcut not in shortcuts:
+            if Shortcut(new_shortcut).qt not in [
+                Shortcut(shortcut).qt for shortcut in shortcuts
+            ]:
                 continue
             # Shortcut is here (either same action or not), don't replace in settings.
             if action_name != current_action: