Fix and refactor KeyBinding editor. (#1820)

* Clean up KeyBindings. This cleans up the KeyBindings classes to make the semantics a bit clearer and to fix some issues. Fixes #1805. * Fix and refactor KeyBinding editor. * Refactor tests. * Add news fragment. * Apply suggestions from code review Co-authored-by: Poruri Sai Rahul <[email protected]> Co-authored-by: Poruri Sai Rahul <[email protected]>
enthought · Mar 8, 2022 · 1f1a387 · 1f1a387
1 parent 7722b1b
commit 1f1a387
Show file tree

Hide file tree

Showing 6 changed files with 128 additions and 138 deletions.
diff --git a/docs/releases/upcoming/1820.bugfix.rst b/docs/releases/upcoming/1820.bugfix.rst
@@ -0,0 +1 @@
+Fix the KeyBindingEditor.
diff --git a/docs/source/traitsui_user_manual/examples/key_bindings.py b/docs/source/traitsui_user_manual/examples/key_bindings.py
@@ -28,6 +28,7 @@
     KeyBinding(
         binding1='Ctrl-k',
         description='Edit key bindings',
+        # special method name handled internally
         method_name='edit_bindings',
     ),
 )
@@ -42,10 +43,6 @@ def save_file(self, info):
     def run_script(self, info):
         info.object.status = "run script"
 
-    def edit_bindings(self, info):
-        info.object.status = "edit bindings"
-        key_bindings.edit_traits()
-
 
 class KBCodeExample(HasPrivateTraits):
 
@@ -70,7 +67,7 @@ class KBCodeExample(HasPrivateTraits):
 
     @observe('kb')
     def _edit_key_bindings(self, event):
-        key_bindings.edit_traits()
+        key_bindings.edit()
 
 
 if __name__ == '__main__':

diff --git a/traitsui/key_bindings.py b/traitsui/key_bindings.py
@@ -30,6 +30,7 @@
 from .editors.key_binding_editor import KeyBindingEditor
 from .editors.list_editor import ListEditor
 from .group import HGroup
+from .handler import ModelView
 from .item import Item
 from .toolkit import toolkit
 from .view import View
@@ -116,31 +117,10 @@ class KeyBindings(HasPrivateTraits):
     #: The child KeyBindings of this object (if any):
     children = List(transient=True)
 
-    #: Control that currently has the focus (if any)
-    focus_owner = Any(transient=True)
-
     # -------------------------------------------------------------------------
     #  Traits view definitions:
     # -------------------------------------------------------------------------
 
-    traits_view = View(
-        [
-            Item(
-                "bindings",
-                style="readonly",
-                show_label=False,
-                editor=ListEditor(style="custom"),
-            ),
-            "|{Click on an entry field, then press the key to "
-            "assign. Double-click a field to clear it.}<>",
-        ],
-        title="Update Key Bindings",
-        kind="livemodal",
-        resizable=True,
-        width=0.4,
-        height=0.4,
-    )
-
     def __init__(self, *bindings, **traits):
         # initialize bindings
         if len(bindings) == 1 and isinstance(bindings[0], SequenceTypes):
@@ -194,21 +174,12 @@ def dispose(self):
         del self.children
         del self.bindings
 
-        self.parent = self._root = self.focus_owner = None
+        self.parent = self._root = None
 
     def edit(self):
         """Edits a possibly hierarchical set of KeyBindings."""
-        bindings = list(set(self.root._get_bindings([])))
-        bindings.sort(key=lambda x: "%s%02d" % (x.binding1[-1:], x.binding1))
-        KeyBindings(bindings).edit_traits()
-
-    def key_binding_for(self, binding, key_name):
-        """Returns the current binding for a specified key (if any)."""
-        if key_name != "":
-            for a_binding in self._match_binding(key_name, skip={binding}):
-                return a_binding
-
-        return None
+        model_view = KeyBindingsHandler(model=self)
+        ui = model_view.edit_traits()
 
     # -- Property Implementations ---------------------------------------------
 
@@ -228,11 +199,6 @@ def _binding_updated(self, event):
             for a_binding in self._match_binding(event.new, skip={event.object}):
                 a_binding.clear_binding(event.new)
 
-    def _focus_owner_changed(self, old, new):
-        """Handles the focus owner being changed."""
-        if old is not None:
-            old.border_size = 0
-
     @observe("children.items")
     def _children_modified(self, event):
         """Handles child KeyBindings being added to the object."""
@@ -294,3 +260,48 @@ def _match_binding(self, binding, skip=frozenset()):
             for a_binding in self.bindings
             if a_binding not in skip and a_binding.match(binding)
         )
+
+
+class KeyBindingsHandler(ModelView):
+
+    bindings = List(Instance(KeyBinding))
+
+    def key_binding_for(self, binding, key_name):
+        """Returns the current binding for a specified key (if any)."""
+        if key_name != "":
+            for a_binding in self._match_binding(key_name, skip={binding}):
+                return a_binding
+
+        return None
+
+    def _match_binding(self, binding, skip=frozenset()):
+        """Return all KeyBinding instances that match the given binding.
+        """
+        return (
+            a_binding
+            for a_binding in self.bindings
+            if a_binding not in skip and a_binding.match(binding)
+        )
+
+    def _bindings_default(self):
+        bindings = list(set(self.model.root._get_bindings([])))
+        bindings.sort(key=lambda x: (x.binding1[-1:], x.binding1))
+        return bindings
+
+    traits_view = View(
+        [
+            Item(
+                "bindings",
+                style="readonly",
+                show_label=False,
+                editor=ListEditor(style="custom"),
+            ),
+            "|{Click on an entry field, then press the key to "
+            "assign. Double-click a field to clear it.}<>",
+        ],
+        title="Update Key Bindings",
+        kind="livemodal",
+        resizable=True,
+        width=0.4,
+        height=0.4,
+    )
diff --git a/traitsui/qt4/key_binding_editor.py b/traitsui/qt4/key_binding_editor.py
@@ -28,6 +28,7 @@
 
 from pyface.qt import QtCore, QtGui
 
+from pyface.api import YES, confirm
 from traits.api import Bool, Event
 
 from .editor import Editor
@@ -57,47 +58,34 @@ def init(self, parent):
         """
         self.control = KeyBindingCtrl(self)
 
-    def update_object(self, event):
-        """Handles the user entering input data in the edit control."""
-        try:
-            self.value = value = key_event_to_name(event)
-            self._binding.text = value
-        except:
-            pass
-
     def update_editor(self):
         """Updates the editor when the object trait changes externally to the
         editor.
         """
         self.control.setText(self.value)
 
-    def update_focus(self, has_focus):
-        """Updates the current focus setting of the control."""
-        if has_focus:
-            self._binding.border_size = 1
-            self.object.owner.focus_owner = self._binding
-
     def _key_changed(self, event):
         """Handles a keyboard event."""
         binding = self.object
         key_name = key_event_to_name(event)
-        cur_binding = binding.owner.key_binding_for(binding, key_name)
+        cur_binding = self.ui.parent.handler.key_binding_for(binding, key_name)
         if cur_binding is not None:
-            if (
-                QtGui.QMessageBox.question(
-                    self.control,
-                    "Duplicate Key Definition",
-                    "'%s' has already been assigned to '%s'.\n"
+            result = confirm(
+                parent=self.control,
+                message=(
+                    f"{key_name!r} has already been assigned to "
+                    f"'{cur_binding.description}'.\n"
                     "Do you wish to continue?"
-                    % (key_name, cur_binding.description),
-                    QtGui.QMessageBox.StandardButton.Yes | QtGui.QMessageBox.StandardButton.No,
-                    QtGui.QMessageBox.StandardButton.No,
-                )
-                != QtGui.QMessageBox.StandardButton.Yes
-            ):
+                ),
+                title="Duplicate Key Definition",
+            )
+            if result != YES:
                 return
 
         self.value = key_name
+        # Need to manually update editor because the update to the value
+        # won't get transferred to toolkit object due to loopback protection.
+        self.update_editor()
 
     def _clear_changed(self):
         """Handles a clear field event."""

diff --git a/traitsui/tests/test_key_bindings.py b/traitsui/tests/test_key_bindings.py
@@ -14,7 +14,7 @@
 from traits.testing.api import UnittestTools
 
 
-from ..key_bindings import KeyBinding, KeyBindings
+from ..key_bindings import KeyBinding, KeyBindings, KeyBindingsHandler
 
 
 class Controller1:
@@ -116,50 +116,6 @@ def test_merge(self):
         self.assertEqual(key_binding.binding1, "Ctrl-V")
         self.assertEqual(key_binding.binding2, "Ctrl-W")
 
-    def test_key_binding_for_match(self):
-        bindings = [
-            KeyBinding(binding1="Ctrl-S", binding2="Ctrl-T", method_name="m1"),
-            KeyBinding(binding1="Ctrl-U", binding2="", method_name="m2"),
-        ]
-        key_bindings = KeyBindings(bindings=bindings)
-
-        binding = key_bindings.key_binding_for(bindings[0], "Ctrl-U")
-
-        self.assertIs(binding, bindings[1])
-
-    def test_key_binding_for_no_match(self):
-        bindings = [
-            KeyBinding(binding1="Ctrl-S", binding2="Ctrl-T", method_name="m1"),
-            KeyBinding(binding1="Ctrl-U", binding2="", method_name="m2"),
-        ]
-        key_bindings = KeyBindings(bindings=bindings)
-
-        binding = key_bindings.key_binding_for(bindings[0], "Ctrl-V")
-
-        self.assertIsNone(binding)
-
-    def test_key_binding_for_match_self(self):
-        bindings = [
-            KeyBinding(binding1="Ctrl-S", binding2="Ctrl-T", method_name="m1"),
-            KeyBinding(binding1="Ctrl-U", binding2="", method_name="m2"),
-        ]
-        key_bindings = KeyBindings(bindings=bindings)
-
-        binding = key_bindings.key_binding_for(bindings[0], "Ctrl-S")
-
-        self.assertIsNone(binding)
-
-    def test_key_binding_for_match_empty(self):
-        bindings = [
-            KeyBinding(binding1="Ctrl-S", binding2="Ctrl-T", method_name="m1"),
-            KeyBinding(binding1="Ctrl-U", binding2="", method_name="m2"),
-        ]
-        key_bindings = KeyBindings(bindings=bindings)
-
-        binding = key_bindings.key_binding_for(bindings[0], "")
-
-        self.assertIsNone(binding)
-
     def test_clear_bindings_match(self):
         bindings = [
             KeyBinding(binding1="Ctrl-S", binding2="Ctrl-T", method_name="m1"),
@@ -269,3 +225,54 @@ def test_do_no_match_complete(self):
 
         self.assertFalse(result)
         self.assertIsNone(controller.called)
+
+
+class TestKeyBindingsHandler(unittest.TestCase):
+
+    def test_key_binding_for_match(self):
+        bindings = [
+            KeyBinding(binding1="Ctrl-S", binding2="Ctrl-T", method_name="m1"),
+            KeyBinding(binding1="Ctrl-U", binding2="", method_name="m2"),
+        ]
+        key_bindings = KeyBindings(bindings=bindings)
+        handler = KeyBindingsHandler(model=key_bindings)
+
+        binding = handler.key_binding_for(bindings[0], "Ctrl-U")
+
+        self.assertIs(binding, bindings[1])
+
+    def test_key_binding_for_no_match(self):
+        bindings = [
+            KeyBinding(binding1="Ctrl-S", binding2="Ctrl-T", method_name="m1"),
+            KeyBinding(binding1="Ctrl-U", binding2="", method_name="m2"),
+        ]
+        key_bindings = KeyBindings(bindings=bindings)
+        handler = KeyBindingsHandler(model=key_bindings)
+
+        binding = handler.key_binding_for(bindings[0], "Ctrl-V")
+
+        self.assertIsNone(binding)
+
+    def test_key_binding_for_match_self(self):
+        bindings = [
+            KeyBinding(binding1="Ctrl-S", binding2="Ctrl-T", method_name="m1"),
+            KeyBinding(binding1="Ctrl-U", binding2="", method_name="m2"),
+        ]
+        key_bindings = KeyBindings(bindings=bindings)
+        handler = KeyBindingsHandler(model=key_bindings)
+
+        binding = handler.key_binding_for(bindings[0], "Ctrl-S")
+
+        self.assertIsNone(binding)
+
+    def test_key_binding_for_match_empty(self):
+        bindings = [
+            KeyBinding(binding1="Ctrl-S", binding2="Ctrl-T", method_name="m1"),
+            KeyBinding(binding1="Ctrl-U", binding2="", method_name="m2"),
+        ]
+        key_bindings = KeyBindings(bindings=bindings)
+        handler = KeyBindingsHandler(model=key_bindings)
+
+        binding = handler.key_binding_for(bindings[0], "")
+
+        self.assertIsNone(binding)