From 6dd73c21739e3610660c9a2c42744134c32240ab Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 18 Oct 2022 11:03:42 +0100 Subject: [PATCH 1/9] key bindings refactor --- sandbox/will/screen_actions.py | 24 +++++++++++++++ src/textual/app.py | 54 ++++++++++++++++++++-------------- src/textual/message_pump.py | 5 ++-- src/textual/widget.py | 7 +---- src/textual/widgets/_footer.py | 11 +++++-- src/textual/widgets/_input.py | 2 ++ 6 files changed, 71 insertions(+), 32 deletions(-) create mode 100644 sandbox/will/screen_actions.py diff --git a/sandbox/will/screen_actions.py b/sandbox/will/screen_actions.py new file mode 100644 index 000000000..b6957349d --- /dev/null +++ b/sandbox/will/screen_actions.py @@ -0,0 +1,24 @@ +from textual.app import App, ComposeResult +from textual.screen import Screen +from textual.widgets import Footer + + +class DefaultScreen(Screen): + + BINDINGS = [("f", "foo", "FOO")] + + def compose(self) -> ComposeResult: + yield Footer() + + def action_foo(self) -> None: + self.app.bell() + + +class ScreenApp(App): + def on_mount(self) -> None: + self.push_screen(DefaultScreen()) + + +app = ScreenApp() +if __name__ == "__main__": + app.run() diff --git a/src/textual/app.py b/src/textual/app.py index 20ae53db9..0263142dd 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -7,6 +7,7 @@ import os import platform import sys import warnings +from collections import defaultdict from contextlib import redirect_stderr, redirect_stdout from datetime import datetime from pathlib import Path, PurePath @@ -31,7 +32,7 @@ from ._callback import invoke from ._context import active_app from ._event_broker import NoHandler, extract_handler_actions from ._filter import LineFilter, Monochrome -from .binding import Bindings, NoBinding +from .binding import Binding, Bindings, NoBinding from .css.query import NoMatches from .css.stylesheet import Stylesheet from .design import ColorSystem @@ -320,16 +321,29 @@ class App(Generic[ReturnType], DOMNode): return self.screen.focused @property - def bindings(self) -> Bindings: + def namespace_bindings(self) -> dict[str, tuple[object, Binding]]: """Get current bindings. If no widget is focused, then the app-level bindings are returned. If a widget is focused, then any bindings present in the active screen and app are merged and returned.""" - if self.focused is None: - return Bindings.merge([self.screen._bindings, self._bindings]) + + focused = self.focused + namespace_bindings: list[tuple[object, Bindings]] + if focused is None: + namespace_bindings = [ + (self, self._bindings), + (self.screen, self.screen._bindings), + ] else: - return Bindings.merge( - node._bindings for node in reversed(self.focused.ancestors) - ) + namespace_bindings = [ + (node, node._bindings) for node in reversed(focused.ancestors) + ] + + namespace_binding_map: dict[str, tuple[object, Binding]] = {} + for namespace, bindings in namespace_bindings: + for key, binding in bindings.keys.items(): + namespace_binding_map[key] = (namespace, binding) + + return namespace_binding_map def _set_active(self) -> None: """Set this app to be the currently active app.""" @@ -1261,7 +1275,7 @@ class App(Generic[ReturnType], DOMNode): if not self.is_headless: self.console.bell() - async def press(self, key: str) -> bool: + async def check_bindings(self, key: str) -> bool: """Handle a key press. Args: @@ -1271,11 +1285,11 @@ class App(Generic[ReturnType], DOMNode): bool: True if the key was handled by a binding, otherwise False """ try: - binding = self.bindings.get_key(key) - except NoBinding: + namespace, binding = self.namespace_bindings[key] + except KeyError: return False else: - await self.action(binding.action) + await self.action(binding.action, default_namespace=namespace) return True async def on_event(self, event: events.Event) -> None: @@ -1291,16 +1305,12 @@ class App(Generic[ReturnType], DOMNode): if isinstance(event, events.MouseEvent): # Record current mouse position on App self.mouse_position = Offset(event.x, event.y) - if isinstance(event, events.Key) and self.focused is not None: - # Key events are sent direct to focused widget of the currently active screen - if self.bindings.allow_forward(event.key): - await self.focused._forward_event(event) - else: - # Key has allow_forward=False which disallows forward to focused widget - await super().on_event(event) + if isinstance(event, events.Key): + forward_target = self.focused or self.screen + await forward_target._forward_event(event) else: - # Forward the event to the currently active Screen await self.screen._forward_event(event) + elif isinstance(event, events.Paste): if self.focused is not None: await self.focused._forward_event(event) @@ -1404,7 +1414,7 @@ class App(Generic[ReturnType], DOMNode): elif event.key == "shift+tab": self.screen.focus_previous() else: - if not (await self.press(event.key)): + if not (await self.check_bindings(event.key)): await self.dispatch_key(event) async def _on_shutdown_request(self, event: events.ShutdownRequest) -> None: @@ -1430,8 +1440,8 @@ class App(Generic[ReturnType], DOMNode): if parent is not None: parent.refresh(layout=True) - async def action_press(self, key: str) -> None: - await self.press(key) + async def action_check_binding(self, key: str) -> None: + await self.check_bindings(key) async def action_quit(self) -> None: """Quit the app as soon as possible.""" diff --git a/src/textual/message_pump.py b/src/textual/message_pump.py index 6470b7e78..cdb8283ae 100644 --- a/src/textual/message_pump.py +++ b/src/textual/message_pump.py @@ -571,6 +571,7 @@ class MessagePump(metaclass=MessagePumpMeta): return public_handler or private_handler + handled = False invoked_method = None key_name = event.key_name if not key_name: @@ -583,10 +584,10 @@ class MessagePump(metaclass=MessagePumpMeta): _raise_duplicate_key_handlers_error( key_name, invoked_method.__name__, key_method.__name__ ) - await invoke(key_method, event) + handled = await invoke(key_method, event) invoked_method = key_method - return invoked_method is not None + return handled async def on_timer(self, event: events.Timer) -> None: event.prevent_default() diff --git a/src/textual/widget.py b/src/textual/widget.py index dab9e06d1..5b73d2063 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -1900,12 +1900,7 @@ class Widget(DOMNode): await self.handle_key(event) async def handle_key(self, event: events.Key) -> bool: - try: - binding = self._bindings.get_key(event.key) - except NoBinding: - return await self.dispatch_key(event) - await self.action(binding.action) - return True + return await self.dispatch_key(event) async def _on_compose(self, event: events.Compose) -> None: widgets = list(self.compose()) diff --git a/src/textual/widgets/_footer.py b/src/textual/widgets/_footer.py index 33e12375d..55c8d1696 100644 --- a/src/textual/widgets/_footer.py +++ b/src/textual/widgets/_footer.py @@ -87,7 +87,11 @@ class Footer(Widget): highlight_key_style = self.get_component_rich_style("footer--highlight-key") key_style = self.get_component_rich_style("footer--key") - bindings = self.app.bindings.shown_keys + bindings = [ + binding + for (_namespace, binding) in self.app.namespace_bindings.values() + if binding.show + ] action_to_bindings = defaultdict(list) for binding in bindings: @@ -107,7 +111,10 @@ class Footer(Widget): f" {binding.description} ", highlight_style if hovered else base_style, ), - meta={"@click": f"app.press('{binding.key}')", "key": binding.key}, + meta={ + "@click": f"app.check_binding('{binding.key}')", + "key": binding.key, + }, ) text.append_text(key_text) return text diff --git a/src/textual/widgets/_input.py b/src/textual/widgets/_input.py index 421cc2577..5f92b15ec 100644 --- a/src/textual/widgets/_input.py +++ b/src/textual/widgets/_input.py @@ -239,10 +239,12 @@ class Input(Widget, can_focus=True): # Do key bindings first if await self.handle_key(event): + print("HANDLE KEY STOPPED") event.prevent_default() event.stop() return elif event.is_printable: + print("PRINTABLE STOPPED") event.stop() assert event.char is not None self.insert_text_at_cursor(event.char) From 0b83170eed8fc652c8965110b894bca89243ee1b Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 18 Oct 2022 11:10:37 +0100 Subject: [PATCH 2/9] fix tests --- src/textual/message_pump.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/textual/message_pump.py b/src/textual/message_pump.py index cdb8283ae..7d7712a81 100644 --- a/src/textual/message_pump.py +++ b/src/textual/message_pump.py @@ -584,7 +584,9 @@ class MessagePump(metaclass=MessagePumpMeta): _raise_duplicate_key_handlers_error( key_name, invoked_method.__name__, key_method.__name__ ) - handled = await invoke(key_method, event) + # If key handlers return False, then they are not considered handled + # This allows key handlers to do some conditional logic + handled = (await invoke(key_method, event)) != False invoked_method = key_method return handled From a148ab4ddb64c71703989b1c92d1f6271c9193a1 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 18 Oct 2022 13:25:21 +0100 Subject: [PATCH 3/9] removed prints --- src/textual/widgets/_input.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/textual/widgets/_input.py b/src/textual/widgets/_input.py index 5f92b15ec..421cc2577 100644 --- a/src/textual/widgets/_input.py +++ b/src/textual/widgets/_input.py @@ -239,12 +239,10 @@ class Input(Widget, can_focus=True): # Do key bindings first if await self.handle_key(event): - print("HANDLE KEY STOPPED") event.prevent_default() event.stop() return elif event.is_printable: - print("PRINTABLE STOPPED") event.stop() assert event.char is not None self.insert_text_at_cursor(event.char) From 5af46995dde890832a410a1c8d2ec3b27a81fbda Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 18 Oct 2022 14:05:46 +0100 Subject: [PATCH 4/9] optimize check binding --- src/textual/app.py | 50 ++++++++++++++++++++-------------- src/textual/widgets/_footer.py | 2 +- 2 files changed, 30 insertions(+), 22 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 0263142dd..451054056 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -326,20 +326,8 @@ class App(Generic[ReturnType], DOMNode): are returned. If a widget is focused, then any bindings present in the active screen and app are merged and returned.""" - focused = self.focused - namespace_bindings: list[tuple[object, Bindings]] - if focused is None: - namespace_bindings = [ - (self, self._bindings), - (self.screen, self.screen._bindings), - ] - else: - namespace_bindings = [ - (node, node._bindings) for node in reversed(focused.ancestors) - ] - namespace_binding_map: dict[str, tuple[object, Binding]] = {} - for namespace, bindings in namespace_bindings: + for namespace, bindings in self._binding_chain: for key, binding in bindings.keys.items(): namespace_binding_map[key] = (namespace, binding) @@ -1275,6 +1263,26 @@ class App(Generic[ReturnType], DOMNode): if not self.is_headless: self.console.bell() + @property + def _binding_chain(self) -> list[tuple[DOMNode, Bindings]]: + """Get a chain of nodes and bindings to consider. + + Returns: + list[tuple[DOMNode, Bindings]]: List of DOM nodes and their bindings. + """ + focused = self.focused + namespace_bindings: list[tuple[DOMNode, Bindings]] + if focused is None: + namespace_bindings = [ + (self, self._bindings), + (self.screen, self.screen._bindings), + ] + else: + namespace_bindings = [ + (node, node._bindings) for node in reversed(focused.ancestors) + ] + return namespace_bindings + async def check_bindings(self, key: str) -> bool: """Handle a key press. @@ -1284,13 +1292,13 @@ class App(Generic[ReturnType], DOMNode): Returns: bool: True if the key was handled by a binding, otherwise False """ - try: - namespace, binding = self.namespace_bindings[key] - except KeyError: - return False - else: - await self.action(binding.action, default_namespace=namespace) - return True + + for namespace, bindings in reversed(self._binding_chain): + binding = bindings.keys.get(key) + if binding is not None: + await self.action(binding.action, default_namespace=namespace) + return True + return False async def on_event(self, event: events.Event) -> None: # Handle input events that haven't been forwarded @@ -1440,7 +1448,7 @@ class App(Generic[ReturnType], DOMNode): if parent is not None: parent.refresh(layout=True) - async def action_check_binding(self, key: str) -> None: + async def action_check_bindings(self, key: str) -> None: await self.check_bindings(key) async def action_quit(self) -> None: diff --git a/src/textual/widgets/_footer.py b/src/textual/widgets/_footer.py index 55c8d1696..63b2265aa 100644 --- a/src/textual/widgets/_footer.py +++ b/src/textual/widgets/_footer.py @@ -112,7 +112,7 @@ class Footer(Widget): highlight_style if hovered else base_style, ), meta={ - "@click": f"app.check_binding('{binding.key}')", + "@click": f"app.check_bindings('{binding.key}')", "key": binding.key, }, ) From cee9d17b76d82258165e2e5b05456af737534c1b Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 18 Oct 2022 14:08:33 +0100 Subject: [PATCH 5/9] better typing --- src/textual/app.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 451054056..153a2b98f 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -321,12 +321,12 @@ class App(Generic[ReturnType], DOMNode): return self.screen.focused @property - def namespace_bindings(self) -> dict[str, tuple[object, Binding]]: + def namespace_bindings(self) -> dict[str, tuple[DOMNode, Binding]]: """Get current bindings. If no widget is focused, then the app-level bindings are returned. If a widget is focused, then any bindings present in the active screen and app are merged and returned.""" - namespace_binding_map: dict[str, tuple[object, Binding]] = {} + namespace_binding_map: dict[str, tuple[DOMNode, Binding]] = {} for namespace, bindings in self._binding_chain: for key, binding in bindings.keys.items(): namespace_binding_map[key] = (namespace, binding) @@ -1265,7 +1265,8 @@ class App(Generic[ReturnType], DOMNode): @property def _binding_chain(self) -> list[tuple[DOMNode, Bindings]]: - """Get a chain of nodes and bindings to consider. + """Get a chain of nodes and bindings to consider. Goes from app to focused + widget, or app to screen. Returns: list[tuple[DOMNode, Bindings]]: List of DOM nodes and their bindings. From 32d18a148f8933c9efad4923cd8b1e3170fd20e9 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 18 Oct 2022 14:13:48 +0100 Subject: [PATCH 6/9] swap order --- src/textual/app.py | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 153a2b98f..ed22f3c54 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -327,7 +327,7 @@ class App(Generic[ReturnType], DOMNode): screen and app are merged and returned.""" namespace_binding_map: dict[str, tuple[DOMNode, Binding]] = {} - for namespace, bindings in self._binding_chain: + for namespace, bindings in reversed(self._binding_chain): for key, binding in bindings.keys.items(): namespace_binding_map[key] = (namespace, binding) @@ -1265,8 +1265,8 @@ class App(Generic[ReturnType], DOMNode): @property def _binding_chain(self) -> list[tuple[DOMNode, Bindings]]: - """Get a chain of nodes and bindings to consider. Goes from app to focused - widget, or app to screen. + """Get a chain of nodes and bindings to consider. Goes from focused widget to app, or + screen to app. Returns: list[tuple[DOMNode, Bindings]]: List of DOM nodes and their bindings. @@ -1275,13 +1275,11 @@ class App(Generic[ReturnType], DOMNode): namespace_bindings: list[tuple[DOMNode, Bindings]] if focused is None: namespace_bindings = [ - (self, self._bindings), (self.screen, self.screen._bindings), + (self, self._bindings), ] else: - namespace_bindings = [ - (node, node._bindings) for node in reversed(focused.ancestors) - ] + namespace_bindings = [(node, node._bindings) for node in focused.ancestors] return namespace_bindings async def check_bindings(self, key: str) -> bool: @@ -1294,7 +1292,7 @@ class App(Generic[ReturnType], DOMNode): bool: True if the key was handled by a binding, otherwise False """ - for namespace, bindings in reversed(self._binding_chain): + for namespace, bindings in self._binding_chain: binding = bindings.keys.get(key) if binding is not None: await self.action(binding.action, default_namespace=namespace) From f39e3f04208eedc386b81827cc6e340aa08431a4 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 18 Oct 2022 14:57:01 +0100 Subject: [PATCH 7/9] added universal keys --- src/textual/app.py | 15 ++++++++------- src/textual/binding.py | 10 +++++----- 2 files changed, 13 insertions(+), 12 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index ed22f3c54..2ada12f61 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -196,7 +196,7 @@ class App(Generic[ReturnType], DOMNode): self._logger = Logger(self._log) - self._bindings.bind("ctrl+c", "quit", show=False, allow_forward=False) + self._bindings.bind("ctrl+c", "quit", show=False, universal=True) self._refresh_required = False self.design = DEFAULT_COLORS @@ -1282,11 +1282,12 @@ class App(Generic[ReturnType], DOMNode): namespace_bindings = [(node, node._bindings) for node in focused.ancestors] return namespace_bindings - async def check_bindings(self, key: str) -> bool: + async def check_bindings(self, key: str, universal: bool = False) -> bool: """Handle a key press. Args: key (str): A key + universal (bool): Check universal keys if True, otherwise non-universal keys. Returns: bool: True if the key was handled by a binding, otherwise False @@ -1294,7 +1295,7 @@ class App(Generic[ReturnType], DOMNode): for namespace, bindings in self._binding_chain: binding = bindings.keys.get(key) - if binding is not None: + if binding is not None and binding.universal == universal: await self.action(binding.action, default_namespace=namespace) return True return False @@ -1312,11 +1313,11 @@ class App(Generic[ReturnType], DOMNode): if isinstance(event, events.MouseEvent): # Record current mouse position on App self.mouse_position = Offset(event.x, event.y) - if isinstance(event, events.Key): - forward_target = self.focused or self.screen - await forward_target._forward_event(event) - else: await self.screen._forward_event(event) + elif isinstance(event, events.Key): + if not await self.check_bindings(event.key, universal=True): + forward_target = self.focused or self.screen + await forward_target._forward_event(event) elif isinstance(event, events.Paste): if self.focused is not None: diff --git a/src/textual/binding.py b/src/textual/binding.py index 498201c76..39984f517 100644 --- a/src/textual/binding.py +++ b/src/textual/binding.py @@ -34,7 +34,7 @@ class Binding: """Show the action in Footer, or False to hide.""" key_display: str | None = None """How the key should be shown in footer.""" - allow_forward: bool = True + universal: bool = False """Allow forwarding from app to focused widget.""" @@ -55,7 +55,7 @@ class Bindings: description=binding.description, show=binding.show, key_display=binding.key_display, - allow_forward=binding.allow_forward, + universal=binding.universal, ) yield new_binding else: @@ -103,7 +103,7 @@ class Bindings: description: str = "", show: bool = True, key_display: str | None = None, - allow_forward: bool = True, + universal: bool = False, ) -> None: all_keys = [key.strip() for key in keys.split(",")] for key in all_keys: @@ -113,7 +113,7 @@ class Bindings: description, show=show, key_display=key_display, - allow_forward=allow_forward, + universal=universal, ) def get_key(self, key: str) -> Binding: @@ -126,4 +126,4 @@ class Bindings: binding = self.keys.get(key, None) if binding is None: return True - return binding.allow_forward + return not binding.universal From 45b5d4af0ad357447b6b1b1853951104d9ec04bb Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 18 Oct 2022 15:00:37 +0100 Subject: [PATCH 8/9] remove allow forward --- src/textual/binding.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/textual/binding.py b/src/textual/binding.py index 39984f517..3c54a1add 100644 --- a/src/textual/binding.py +++ b/src/textual/binding.py @@ -121,9 +121,3 @@ class Bindings: return self.keys[key] except KeyError: raise NoBinding(f"No binding for {key}") from None - - def allow_forward(self, key: str) -> bool: - binding = self.keys.get(key, None) - if binding is None: - return True - return not binding.universal From 87b6cbdc0a485f15fbdcc974de8649cda3dc66f2 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 18 Oct 2022 15:16:45 +0100 Subject: [PATCH 9/9] Update src/textual/app.py Co-authored-by: darrenburns --- src/textual/app.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 2ada12f61..6543e29fe 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1265,8 +1265,7 @@ class App(Generic[ReturnType], DOMNode): @property def _binding_chain(self) -> list[tuple[DOMNode, Bindings]]: - """Get a chain of nodes and bindings to consider. Goes from focused widget to app, or - screen to app. + """Get a chain of nodes and bindings to consider. If no widget is focused, returns the bindings from both the screen and the app level bindings. Otherwise, combines all the bindings from the currently focused node up the DOM to the root App. Returns: list[tuple[DOMNode, Bindings]]: List of DOM nodes and their bindings.