Merge pull request #1346 from davep/bug/1342/inherited-movement-keys

Add support for a `PRIORITY_BINDINGS` classvar to work alongside `BINDINGS`

Author: willmcgugan

CHANGELOG.md

@@ -7,6 +7,17 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 
 ## Unreleased
 
+### Added
+
+- Added `PRIORITY_BINDINGS` class variable, which can be used to control if a widget's bindings have priority by default. https://github.com/Textualize/textual/issues/1343
+
+### Changed
+
+- Renamed the `Binding` argument `universal` to `priority`. https://github.com/Textualize/textual/issues/1343
+- When looking for bindings that have priority, they are now looked from `App` downwards. https://github.com/Textualize/textual/issues/1343
+- `BINDINGS` on an `App`-derived class have priority by default. https://github.com/Textualize/textual/issues/1343
+- `BINDINGS` on a `Screen`-derived class have priority by default. https://github.com/Textualize/textual/issues/1343
+
 ### Fixed
 
 - Fixed validator not running on first reactive set https://github.com/Textualize/textual/pull/1359

docs/guide/input.md

@@ -127,6 +127,15 @@ Note how the footer displays bindings and makes them clickable.
     Multiple keys can be bound to a single action by comma-separating them.
     For example, `("r,t", "add_bar('red')", "Add Red")` means both ++r++ and ++t++ are bound to `add_bar('red')`.
 
+
+!!! note
+
+    Ordinarily a binding on a focused widget has precedence over the same key binding at a higher level. However, bindings at the `App` or `Screen` level always have priority.
+
+    The priority of a single binding can be controlled with the `priority` parameter of a `Binding` instance. Set it to `True` to give it priority, or `False` to not.
+
+    The default priority of all bindings on a class can be controlled with the `PRIORITY_BINDINGS` class variable. Set it to `True` or `False` to set the default priroty for all `BINDINGS`.
+
 ### Binding class
 
 The tuple of three strings may be enough for simple bindings, but you can also replace the tuple with a [Binding][textual.binding.Binding] instance which exposes a few more options.

examples/five_by_five.py

@@ -166,10 +166,10 @@ class Game(Screen):
         Binding("n", "new_game", "New Game"),
         Binding("question_mark", "push_screen('help')", "Help", key_display="?"),
         Binding("q", "quit", "Quit"),
-        Binding("up,w,k", "navigate(-1,0)", "Move Up", False, universal=True),
-        Binding("down,s,j", "navigate(1,0)", "Move Down", False, universal=True),
-        Binding("left,a,h", "navigate(0,-1)", "Move Left", False, universal=True),
-        Binding("right,d,l", "navigate(0,1)", "Move Right", False, universal=True),
+        Binding("up,w,k", "navigate(-1,0)", "Move Up", False),
+        Binding("down,s,j", "navigate(1,0)", "Move Down", False),
+        Binding("left,a,h", "navigate(0,-1)", "Move Left", False),
+        Binding("right,d,l", "navigate(0,1)", "Move Right", False),
         Binding("space", "move", "Toggle", False),
     ]
     """The bindings for the main game grid."""

src/textual/app.py

@@ -231,6 +231,8 @@ class App(Generic[ReturnType], DOMNode):
     }
     """
 
+    PRIORITY_BINDINGS = True
+
     SCREENS: dict[str, Screen | Callable[[], Screen]] = {}
     _BASE_PATH: str | None = None
     CSS_PATH: CSSPathType = None
@@ -298,7 +300,7 @@ def __init__(
 
         self._logger = Logger(self._log)
 
-        self._bindings.bind("ctrl+c", "quit", show=False, universal=True)
+        self._bindings.bind("ctrl+c", "quit", show=False, priority=True)
         self._refresh_required = False
 
         self.design = DEFAULT_COLORS
@@ -1729,20 +1731,22 @@ def _binding_chain(self) -> list[tuple[DOMNode, Bindings]]:
             ]
         return namespace_bindings
 
-    async def check_bindings(self, key: str, universal: bool = False) -> bool:
+    async def check_bindings(self, key: str, priority: bool = False) -> bool:
         """Handle a key press.
 
         Args:
             key (str): A key
-            universal (bool): Check universal keys if True, otherwise non-universal keys.
+            priority (bool): If `True` check from `App` down, otherwise from focused up.
 
         Returns:
             bool: True if the key was handled by a binding, otherwise False
         """
 
-        for namespace, bindings in self._binding_chain:
+        for namespace, bindings in (
+            reversed(self._binding_chain) if priority else self._binding_chain
+        ):
             binding = bindings.keys.get(key)
-            if binding is not None and binding.universal == universal:
+            if binding is not None and binding.priority == priority:
                 await self.action(binding.action, default_namespace=namespace)
                 return True
         return False
@@ -1762,7 +1766,7 @@ async def on_event(self, event: events.Event) -> None:
                 self.mouse_position = Offset(event.x, event.y)
                 await self.screen._forward_event(event)
             elif isinstance(event, events.Key):
-                if not await self.check_bindings(event.key, universal=True):
+                if not await self.check_bindings(event.key, priority=True):
                     forward_target = self.focused or self.screen
                     await forward_target._forward_event(event)
             else:

src/textual/binding.py

@@ -20,6 +20,8 @@ class NoBinding(Exception):
 
 @dataclass(frozen=True)
 class Binding:
+    """The configuration of a key binding."""
+
     key: str
     """str: Key to bind. This can also be a comma-separated list of keys to map multiple keys to a single action."""
     action: str
@@ -30,15 +32,31 @@ class Binding:
     """bool: Show the action in Footer, or False to hide."""
     key_display: str | None = None
     """str | None: How the key should be shown in footer."""
-    universal: bool = False
-    """bool: Allow forwarding from app to focused widget."""
+    priority: bool | None = None
+    """bool | None: Is this a priority binding, checked form app down to focused widget?"""
 
 
 @rich.repr.auto
 class Bindings:
     """Manage a set of bindings."""
 
-    def __init__(self, bindings: Iterable[BindingType] | None = None) -> None:
+    def __init__(
+        self,
+        bindings: Iterable[BindingType] | None = None,
+        default_priority: bool | None = None,
+    ) -> None:
+        """Initialise a collection of bindings.
+
+        Args:
+            bindings (Iterable[BindingType] | None, optional): An optional set of initial bindings.
+            default_priority (bool | None, optional): The default priority of the bindings.
+
+        Note:
+            The iterable of bindings can contain either a `Binding`
+            instance, or a tuple of 3 values mapping to the first three
+            properties of a `Binding`.
+        """
+
         def make_bindings(bindings: Iterable[BindingType]) -> Iterable[Binding]:
             for binding in bindings:
                 # If it's a tuple of length 3, convert into a Binding first
@@ -49,20 +67,20 @@ def make_bindings(bindings: Iterable[BindingType]) -> Iterable[Binding]:
                         )
                     binding = Binding(*binding)
 
-                binding_keys = binding.key.split(",")
-                if len(binding_keys) > 1:
-                    for key in binding_keys:
-                        new_binding = Binding(
-                            key=key,
-                            action=binding.action,
-                            description=binding.description,
-                            show=binding.show,
-                            key_display=binding.key_display,
-                            universal=binding.universal,
-                        )
-                        yield new_binding
-                else:
-                    yield binding
+                # At this point we have a Binding instance, but the key may
+                # be a list of keys, so now we unroll that single Binding
+                # into a (potential) collection of Binding instances.
+                for key in binding.key.split(","):
+                    yield Binding(
+                        key=key.strip(),
+                        action=binding.action,
+                        description=binding.description,
+                        show=binding.show,
+                        key_display=binding.key_display,
+                        priority=default_priority
+                        if binding.priority is None
+                        else binding.priority,
+                    )
 
         self.keys: MutableMapping[str, Binding] = (
             {binding.key: binding for binding in make_bindings(bindings)}
@@ -105,7 +123,7 @@ def bind(
         description: str = "",
         show: bool = True,
         key_display: str | None = None,
-        universal: bool = False,
+        priority: bool = False,
     ) -> None:
         """Bind keys to an action.
 
@@ -115,7 +133,7 @@ def bind(
             description (str, optional): An optional description for the binding.
             show (bool, optional): A flag to say if the binding should appear in the footer.
             key_display (str | None, optional): Optional string to display in the footer for the key.
-            universal (bool, optional): Allow forwarding from the app to the focused widget.
+            priority (bool, optional): Is this a priority binding, checked form app down to focused widget?
         """
         all_keys = [key.strip() for key in keys.split(",")]
         for key in all_keys:
@@ -125,7 +143,7 @@ def bind(
                 description,
                 show=show,
                 key_display=key_display,
-                universal=universal,
+                priority=priority,
             )
 
     def get_key(self, key: str) -> Binding:

src/textual/dom.py

@@ -92,6 +92,9 @@ class DOMNode(MessagePump):
     # Virtual DOM nodes
     COMPONENT_CLASSES: ClassVar[set[str]] = set()
 
+    # Should the content of BINDINGS be treated as priority bindings?
+    PRIORITY_BINDINGS: ClassVar[bool] = False
+
     # Mapping of key bindings
     BINDINGS: ClassVar[list[BindingType]] = []
 
@@ -225,11 +228,18 @@ def _merge_bindings(cls) -> Bindings:
         """
         bindings: list[Bindings] = []
 
+        # To start with, assume that bindings won't be priority bindings.
+        priority = False
+
         for base in reversed(cls.__mro__):
             if issubclass(base, DOMNode):
+                # See if the current class wants to set the bindings as
+                # priority bindings. If it doesn't have that property on the
+                # class, go with what we saw last.
+                priority = base.__dict__.get("PRIORITY_BINDINGS", priority)
                 if not base._inherit_bindings:
                     bindings.clear()
-                bindings.append(Bindings(base.__dict__.get("BINDINGS", [])))
+                bindings.append(Bindings(base.__dict__.get("BINDINGS", []), priority))
         keys = {}
         for bindings_ in bindings:
             keys.update(bindings_.keys)

src/textual/screen.py

@@ -26,6 +26,11 @@
 class Screen(Widget):
     """A widget for the root of the app."""
 
+    # The screen is a special case and unless a class that inherits from us
+    # says otherwise, all screen-level bindings should be treated as having
+    # priority.
+    PRIORITY_BINDINGS = True
+
     DEFAULT_CSS = """
     Screen {
         layout: vertical;

tests/test_binding.py

@@ -6,19 +6,26 @@
 
 BINDING1 = Binding("a,b", action="action1", description="description1")
 BINDING2 = Binding("c", action="action2", description="description2")
+BINDING3 = Binding(" d   , e ", action="action3", description="description3")
 
 
 @pytest.fixture
 def bindings():
     yield Bindings([BINDING1, BINDING2])
 
+@pytest.fixture
+def more_bindings():
+    yield Bindings([BINDING1, BINDING2, BINDING3])
 
 def test_bindings_get_key(bindings):
     assert bindings.get_key("b") == Binding("b", action="action1", description="description1")
     assert bindings.get_key("c") == BINDING2
     with pytest.raises(NoBinding):
         bindings.get_key("control+meta+alt+shift+super+hyper+t")
 
+def test_bindings_get_key_spaced_list(more_bindings):
+    assert more_bindings.get_key("d").action == more_bindings.get_key("e").action
+
 def test_bindings_merge_simple(bindings):
     left = Bindings([BINDING1])
     right = Bindings([BINDING2])