From 8a995fbcc131c1355a06e4d9b22c462c881407b9 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 19 Mar 2024 13:39:26 +0000 Subject: [PATCH 1/4] Fix a crash in run_action when an action is an empty tuple Fixes #4248. --- src/textual/app.py | 4 +++- tests/test_issue_4248.py | 46 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 1 deletion(-) create mode 100644 tests/test_issue_4248.py diff --git a/src/textual/app.py b/src/textual/app.py index aaf799a66c..b79674e713 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -2926,7 +2926,9 @@ async def run_action( if isinstance(action, str): target, params = actions.parse(action) else: - target, params = action + # `action` can end up coming in as (), so if that happens we'll + # ask the action parser for a correctly-formed empty action. + target, params = action or actions.parse("") implicit_destination = True if "." in target: destination, action_name = target.split(".", 1) diff --git a/tests/test_issue_4248.py b/tests/test_issue_4248.py new file mode 100644 index 0000000000..de3eee0928 --- /dev/null +++ b/tests/test_issue_4248.py @@ -0,0 +1,46 @@ +"""Test https://github.com/Textualize/textual/issues/4248""" + +from textual.app import App, ComposeResult +from textual.widgets import Label + + +async def test_issue_4248() -> None: + """Various forms of click parameters should be fine.""" + + bumps = 0 + + class ActionApp(App[None]): + + def compose(self) -> ComposeResult: + yield Label("[@click]click me and crash[/]", id="nothing") + yield Label("[@click=]click me and crash[/]", id="no-params") + yield Label("[@click=()]click me and crash[/]", id="empty-params") + yield Label("[@click=foobar]click me[/]", id="unknown-sans-parens") + yield Label("[@click=foobar()]click me[/]", id="unknown-with-parens") + yield Label("[@click=bump]click me[/]", id="known-sans-parens") + yield Label("[@click=bump()]click me[/]", id="known-empty-parens") + yield Label("[@click=bump(100)]click me[/]", id="known-with-param") + + def action_bump(self, by_value: int = 1) -> None: + nonlocal bumps + bumps += by_value + + app = ActionApp() + async with app.run_test() as pilot: + assert bumps == 0 + await pilot.click("#nothing") + assert bumps == 0 + await pilot.click("#no-params") + assert bumps == 0 + await pilot.click("#empty-params") + assert bumps == 0 + await pilot.click("#unknown-sans-parens") + assert bumps == 0 + await pilot.click("#unknown-with-parens") + assert bumps == 0 + await pilot.click("#known-sans-parens") + assert bumps == 1 + await pilot.click("#known-empty-parens") + assert bumps == 2 + await pilot.click("#known-with-param") + assert bumps == 102 From 90c1a8b7fc55e1028122c6adcdd53ecc226a898e Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 19 Mar 2024 14:23:01 +0000 Subject: [PATCH 2/4] Move responsibility for the empty action to App._broken_event --- src/textual/app.py | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index b79674e713..88bb5843de 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -2926,9 +2926,7 @@ async def run_action( if isinstance(action, str): target, params = actions.parse(action) else: - # `action` can end up coming in as (), so if that happens we'll - # ask the action parser for a correctly-formed empty action. - target, params = action or actions.parse("") + target, params = action implicit_destination = True if "." in target: destination, action_name = target.split(".", 1) @@ -3008,11 +3006,16 @@ async def _broker_event( return False else: event.stop() - if isinstance(action, (str, tuple)): + if isinstance(action, str) or (isinstance(action, tuple) and len(action) == 2): await self.run_action(action, default_namespace=default_namespace) # type: ignore[arg-type] elif callable(action): await action() else: + if isinstance(action, tuple) and self.debug: + # It's a tuple and made it this far, which means it'll be a + # malformed action. This is a no-op, but let's log that + # anyway. + log.warning(f"{event_name} event has an empty a action!") return False return True From 564010f273a2b12644fbd0216b9fd6187df575d2 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 19 Mar 2024 14:35:18 +0000 Subject: [PATCH 3/4] Improve the log text Also make it read something akin to English. O_o --- src/textual/app.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/app.py b/src/textual/app.py index 88bb5843de..942bf8b376 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -3015,7 +3015,7 @@ async def _broker_event( # It's a tuple and made it this far, which means it'll be a # malformed action. This is a no-op, but let's log that # anyway. - log.warning(f"{event_name} event has an empty a action!") + log.warning(f"{event_name} event has an empty action") return False return True From 0907da1e2dadeeb37785640115d284bd6343de34 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 21 Mar 2024 15:33:23 +0000 Subject: [PATCH 4/4] Final form of the warning --- src/textual/app.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/textual/app.py b/src/textual/app.py index 942bf8b376..15811232c9 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -3015,7 +3015,9 @@ async def _broker_event( # It's a tuple and made it this far, which means it'll be a # malformed action. This is a no-op, but let's log that # anyway. - log.warning(f"{event_name} event has an empty action") + log.warning( + f"Can't parse @{event_name} action from style meta; check your console markup syntax" + ) return False return True