From c2f289e472e756a36736aeaffd5a5d860f9f0b31 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Thu, 12 Jan 2023 10:58:14 +0000 Subject: [PATCH 01/23] Add failing test. --- tests/css/test_text_style.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 tests/css/test_text_style.py diff --git a/tests/css/test_text_style.py b/tests/css/test_text_style.py new file mode 100644 index 000000000..ea4c7f1fa --- /dev/null +++ b/tests/css/test_text_style.py @@ -0,0 +1,20 @@ +from rich.style import Style + +from textual.css.styles import Styles + + +def test_text_style_none(): + styles = Styles() + styles.text_style = "none" + assert styles.text_style == Style() + + +def test_text_style_none_with_others(): + """Style "none" mixed with others should result in empty style.""" + styles = Styles() + styles.text_style = "bold none underline italic" + + none_styles = Styles() + styles.text_style = "none" + + assert styles.text_style == none_styles.text_style From 4a893b5169092336a626d3bc21a3446a64bcce64 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Thu, 12 Jan 2023 10:59:55 +0000 Subject: [PATCH 02/23] Short-circuit text style parsing when unnecessary. --- CHANGELOG.md | 1 + src/textual/css/_style_properties.py | 4 +++- 2 files changed, 4 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af9c30012..9d61abc6a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - The styles `scrollbar-background-active` and `scrollbar-color-hover` are no longer ignored https://github.com/Textualize/textual/pull/1480 - The widget `Placeholder` can now have its width set to `auto` https://github.com/Textualize/textual/pull/1508 +- The style `text-style` option `none` can also be mixed with other options https://github.com/Textualize/textual/issues/1420 ## [0.9.1] - 2022-12-30 diff --git a/src/textual/css/_style_properties.py b/src/textual/css/_style_properties.py index 266b1a5a1..edb3d68b3 100644 --- a/src/textual/css/_style_properties.py +++ b/src/textual/css/_style_properties.py @@ -891,6 +891,7 @@ class StyleFlagsProperty: Raises: StyleValueError: If the value is an invalid style flag """ + print(repr(style_flags)) _rich_traceback_omit = True if style_flags is None: if obj.clear_rule(self.name): @@ -909,7 +910,8 @@ class StyleFlagsProperty: self.name, word, context="inline" ), ) - style = Style.parse(style_flags) + # rich doesn't like "none" mixed with other styles, so short-circuit here. + style = Style() if "none" in words else Style.parse(style_flags) if obj.set_rule(self.name, style): obj.refresh() From 9d9726f85488c6851347e54cdb05b2b5d5830132 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Thu, 12 Jan 2023 11:28:28 +0000 Subject: [PATCH 03/23] Remove stray print statement. --- src/textual/css/_style_properties.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/textual/css/_style_properties.py b/src/textual/css/_style_properties.py index edb3d68b3..a7724ec94 100644 --- a/src/textual/css/_style_properties.py +++ b/src/textual/css/_style_properties.py @@ -891,7 +891,6 @@ class StyleFlagsProperty: Raises: StyleValueError: If the value is an invalid style flag """ - print(repr(style_flags)) _rich_traceback_omit = True if style_flags is None: if obj.clear_rule(self.name): From dc318b8c93965c23496e7a38beab758fa38dd144 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Thu, 12 Jan 2023 14:39:05 +0000 Subject: [PATCH 04/23] Update tests. --- tests/css/test_text_style.py | 20 -------------------- tests/test_style_properties.py | 13 +++++++++++++ 2 files changed, 13 insertions(+), 20 deletions(-) delete mode 100644 tests/css/test_text_style.py diff --git a/tests/css/test_text_style.py b/tests/css/test_text_style.py deleted file mode 100644 index ea4c7f1fa..000000000 --- a/tests/css/test_text_style.py +++ /dev/null @@ -1,20 +0,0 @@ -from rich.style import Style - -from textual.css.styles import Styles - - -def test_text_style_none(): - styles = Styles() - styles.text_style = "none" - assert styles.text_style == Style() - - -def test_text_style_none_with_others(): - """Style "none" mixed with others should result in empty style.""" - styles = Styles() - styles.text_style = "bold none underline italic" - - none_styles = Styles() - styles.text_style = "none" - - assert styles.text_style == none_styles.text_style diff --git a/tests/test_style_properties.py b/tests/test_style_properties.py index f8900c1b7..81abe8039 100644 --- a/tests/test_style_properties.py +++ b/tests/test_style_properties.py @@ -1,4 +1,8 @@ +import pytest +from rich.style import Style + from textual.color import Color +from textual.css.errors import StyleValueError from textual.css.styles import Styles @@ -7,3 +11,12 @@ def test_box_normalization(): styles = Styles() styles.border_left = ("none", "red") assert styles.border_left == ("", Color.parse("red")) + + +@pytest.mark.parametrize("style_attr", ["text_style", "link_style"]) +def test_text_style_none_with_others(style_attr): + """Style "none" mixed with others should give custom Textual exception.""" + styles = Styles() + + with pytest.raises(StyleValueError) as exc_info: + setattr(styles, style_attr, "bold none underline italic") From dd2a85d70b29380dd77fbdb21419a3fa1c2d8070 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Thu, 12 Jan 2023 14:40:25 +0000 Subject: [PATCH 05/23] Raise custom error when 'none' is mixed with other flags. --- src/textual/css/_style_properties.py | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/src/textual/css/_style_properties.py b/src/textual/css/_style_properties.py index a7724ec94..73d30f96d 100644 --- a/src/textual/css/_style_properties.py +++ b/src/textual/css/_style_properties.py @@ -12,6 +12,7 @@ from __future__ import annotations from operator import attrgetter from typing import TYPE_CHECKING, Generic, Iterable, NamedTuple, TypeVar, cast +import rich.errors import rich.repr from rich.style import Style @@ -909,8 +910,17 @@ class StyleFlagsProperty: self.name, word, context="inline" ), ) - # rich doesn't like "none" mixed with other styles, so short-circuit here. - style = Style() if "none" in words else Style.parse(style_flags) + try: + style = Style.parse(style_flags) + except rich.errors.StyleSyntaxError as exc: + if "none" in words and len(words) > 1: + raise StyleValueError( + "cannot mix 'none' with other style flags", + help_text=style_flags_property_help_text( + self.name, " ".join(words), context="inline" + ), + ) from None + raise exc from None if obj.set_rule(self.name, style): obj.refresh() From 003d98e01648bad988467c82d5ee9abac171467a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Thu, 12 Jan 2023 14:40:46 +0000 Subject: [PATCH 06/23] Update help text. --- src/textual/css/_help_text.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/textual/css/_help_text.py b/src/textual/css/_help_text.py index 7ac541645..8155dcd3c 100644 --- a/src/textual/css/_help_text.py +++ b/src/textual/css/_help_text.py @@ -729,6 +729,7 @@ def style_flags_property_help_text( f"Style flag values such as [i]{property_name}[/] expect space-separated values" ), Bullet(f"Permitted values are {friendly_list(VALID_STYLE_FLAGS)}"), + Bullet("The value 'none' cannot be mixed with others"), *ContextSpecificBullets( inline=[ Bullet( From 036c3e8651a4d49dc1ec7bd16eb05dfedc53c6ea Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Thu, 12 Jan 2023 14:42:04 +0000 Subject: [PATCH 07/23] Update documentation. --- docs/css_types/text_style.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/css_types/text_style.md b/docs/css_types/text_style.md index f41646e3c..40afea528 100644 --- a/docs/css_types/text_style.md +++ b/docs/css_types/text_style.md @@ -8,13 +8,13 @@ The `` CSS type represents styles that can be applied to text. ## Syntax -A [``](/css_types/text_style) can be any _space-separated_ combination of the following values: +A [``](/css_types/text_style) can be the value `none` for plain text with no styling, +or any _space-separated_ combination of the following values: | Value | Description | |-------------|-----------------------------------------------------------------| | `bold` | **Bold text.** | | `italic` | _Italic text._ | -| `none` | Plain text with no styling. | | `reverse` | Reverse video text (foreground and background colors reversed). | | `strike` | Strikethrough text. | | `underline` | Underline text. | @@ -42,5 +42,5 @@ A [``](/css_types/text_style) can be any _space-separated_ combinati widget.styles.text_style = "strike" # You can also combine multiple values -widget.styles.text_style = "bold underline italic" +widget.styles.text_style = "strike bold italic reverse ``` From 8f877d826f6c491cb8cbce0ac53c5349581e74fa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Thu, 12 Jan 2023 14:43:23 +0000 Subject: [PATCH 08/23] Update changelog. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9d61abc6a..439fd56f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,12 +22,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Fail-fast and print pretty tracebacks for Widget compose errors https://github.com/Textualize/textual/pull/1505 - Added Widget._refresh_scroll to avoid expensive layout when scrolling https://github.com/Textualize/textual/pull/1524 - `events.Paste` now bubbles https://github.com/Textualize/textual/issues/1434 +- Improved error message when style flag `none` is mixed with other flags (e.g., when setting `text-style`) https://github.com/Textualize/textual/issues/1420 ### Fixed - The styles `scrollbar-background-active` and `scrollbar-color-hover` are no longer ignored https://github.com/Textualize/textual/pull/1480 - The widget `Placeholder` can now have its width set to `auto` https://github.com/Textualize/textual/pull/1508 -- The style `text-style` option `none` can also be mixed with other options https://github.com/Textualize/textual/issues/1420 ## [0.9.1] - 2022-12-30 From 2b9cd81ca5cf9b1755d17a2183519d347f59c3ee Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 17 Jan 2023 10:29:19 +0000 Subject: [PATCH 09/23] Cleanup and new test. --- src/textual/css/_style_properties.py | 4 ++-- tests/test_style_properties.py | 12 +++++++++++- 2 files changed, 13 insertions(+), 3 deletions(-) diff --git a/src/textual/css/_style_properties.py b/src/textual/css/_style_properties.py index 73d30f96d..36a40f19b 100644 --- a/src/textual/css/_style_properties.py +++ b/src/textual/css/_style_properties.py @@ -912,7 +912,7 @@ class StyleFlagsProperty: ) try: style = Style.parse(style_flags) - except rich.errors.StyleSyntaxError as exc: + except rich.errors.StyleSyntaxError as error: if "none" in words and len(words) > 1: raise StyleValueError( "cannot mix 'none' with other style flags", @@ -920,7 +920,7 @@ class StyleFlagsProperty: self.name, " ".join(words), context="inline" ), ) from None - raise exc from None + raise error from None if obj.set_rule(self.name, style): obj.refresh() diff --git a/tests/test_style_properties.py b/tests/test_style_properties.py index 81abe8039..107aa2a22 100644 --- a/tests/test_style_properties.py +++ b/tests/test_style_properties.py @@ -18,5 +18,15 @@ def test_text_style_none_with_others(style_attr): """Style "none" mixed with others should give custom Textual exception.""" styles = Styles() - with pytest.raises(StyleValueError) as exc_info: + with pytest.raises(StyleValueError): setattr(styles, style_attr, "bold none underline italic") + + +@pytest.mark.parametrize("style_attr", ["text_style", "link_style"]) +def test_text_style_set_to_none(style_attr): + """Setting text style to "none" should clear the styles.""" + styles = Styles() + setattr(styles, style_attr, "bold underline italic") + assert getattr(styles, style_attr) != Style.null() + setattr(styles, style_attr, "none") + assert getattr(styles, style_attr) == Style.null() From 56f339db203b928eb18ebab3d7526e81021bc313 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 17 Jan 2023 12:26:03 +0000 Subject: [PATCH 10/23] Add failing tests. --- tests/test_focus.py | 133 ++++++++++++++++++++++++++++++++++++++------ 1 file changed, 117 insertions(+), 16 deletions(-) diff --git a/tests/test_focus.py b/tests/test_focus.py index 99a6d443d..7b96972e2 100644 --- a/tests/test_focus.py +++ b/tests/test_focus.py @@ -1,3 +1,5 @@ +import pytest + from textual.app import App from textual.screen import Screen from textual.widget import Widget @@ -15,6 +17,28 @@ class ChildrenFocusableOnly(Widget, can_focus=False, can_focus_children=True): pass +@pytest.fixture +def screen() -> Screen: + app = App() + app._set_active() + app.push_screen(Screen()) + + screen = app.screen + + # The classes even/odd alternate along the focus chain. + # The classes in/out identify nested widgets. + screen._add_children( + Focusable(id="foo", classes="a"), + NonFocusable(id="bar"), + Focusable(Focusable(id="Paul", classes="c"), id="container1", classes="b"), + NonFocusable(Focusable(id="Jessica", classes="a"), id="container2"), + Focusable(id="baz", classes="b"), + ChildrenFocusableOnly(Focusable(id="child", classes="c")), + ) + + return screen + + def test_focus_chain(): app = App() app._set_active() @@ -38,22 +62,7 @@ def test_focus_chain(): assert focus_chain == ["foo", "container1", "Paul", "baz", "child"] -def test_focus_next_and_previous(): - app = App() - app._set_active() - app.push_screen(Screen()) - - screen = app.screen - - screen._add_children( - Focusable(id="foo"), - NonFocusable(id="bar"), - Focusable(Focusable(id="Paul"), id="container1"), - NonFocusable(Focusable(id="Jessica"), id="container2"), - Focusable(id="baz"), - ChildrenFocusableOnly(Focusable(id="child")), - ) - +def test_focus_next_and_previous(screen: Screen): assert screen.focus_next().id == "foo" assert screen.focus_next().id == "container1" assert screen.focus_next().id == "Paul" @@ -64,3 +73,95 @@ def test_focus_next_and_previous(): assert screen.focus_previous().id == "Paul" assert screen.focus_previous().id == "container1" assert screen.focus_previous().id == "foo" + + +def test_focus_next_and_previous_with_type_selector(screen: Screen): + """Move focus with a selector that matches the currently focused node.""" + screen.set_focus(screen.query_one("#Paul")) + assert screen.focused.id == "Paul" + + assert screen.focus_next(Focusable).id == "Jessica" + assert screen.focus_next(Focusable).id == "baz" + assert screen.focus_next(Focusable).id == "child" + + assert screen.focus_previous(Focusable).id == "baz" + assert screen.focus_previous(Focusable).id == "Jessica" + assert screen.focus_previous(Focusable).id == "Paul" + assert screen.focus_previous(Focusable).id == "container1" + assert screen.focus_previous(Focusable).id == "foo" + + +def test_focus_next_and_previous_with_str_selector(screen: Screen): + """Move focus with a selector that matches the currently focused node.""" + screen.set_focus(screen.query_one("#foo")) + assert screen.focused.id == "foo" + + assert screen.focus_next(".a").id == "Jessica" + assert screen.focus_next("Focusable").id == "baz" + + assert screen.focus_previous(".b").id == "container1" + + assert screen.focus_next("Focusable").id == "Paul" + assert screen.focus_next(".c").id == "child" + + assert screen.focus_previous(".c").id == "Paul" + + +def test_focus_next_and_previous_with_type_selector_without_self(): + """Test moving the focus with a selector that does not match the currently focused node.""" + app = App() + app._set_active() + app.push_screen(Screen()) + + screen = app.screen + + from textual.containers import Horizontal, Vertical + from textual.widgets import Input, Label, Static + + screen._add_children( + Vertical( + Horizontal( + Input(id="w3"), + Label(id="w4"), + Input(id="w5"), + Static(id="w6"), + Label(id="w7"), + id="w2", + ), + Horizontal( + Static(id="w9"), + Label(id="w10"), + Static(id="w11"), + Input(id="w12"), + Input(id="w13"), + id="w8", + ), + id="w1", + ) + ) + + screen.set_focus(screen.query_one("#w3")) + assert screen.focused.id == "w3" + + assert screen.focus_next(Static).id == "w6" + assert screen.focus_next(Label).id == "w7" + assert screen.focus_next(Input).id == "w12" + + assert screen.focus_previous(Static).id == "w11" + assert screen.focus_previous(Label).id == "w10" + assert screen.focus_previous(Static).id == "w9" + assert screen.focus_previous(Input).id == "w5" + + +def test_focus_next_and_previous_with_str_selector_without_self(screen: Screen): + """Test moving the focus with a selector that does not match the currently focused node.""" + screen.set_focus(screen.query_one("#foo")) + assert screen.focused.id == "foo" + + assert screen.focus_next(".c").id == "Paul" + assert screen.focus_next(".b").id == "baz" + assert screen.focus_next(".c").id == "child" + + assert screen.focus_previous(".a").id == "Jessica" + assert screen.focus_previous(".b").id == "container1" + assert screen.focus_previous(".a").id == "foo" From 16f21ecfc817c874556d95851d92dd581f1a31e6 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 17 Jan 2023 14:22:32 +0000 Subject: [PATCH 11/23] Reraise compose TypeError from the error rather than None In its current form, if someone's `compose` results in an uncaught TypeError in their own code, the resulting error will hide the source. This small change ensures that the main problem (compose returned something that can't be turned into a list) is still reported, but also provides the developer a trackback they can look at to see what the ultimate source of the issue was. --- CHANGELOG.md | 1 + src/textual/app.py | 2 +- src/textual/widget.py | 2 +- 3 files changed, 3 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39e484376..d94233b4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,6 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `COMPONENT_CLASSES` are now inherited from base classes https://github.com/Textualize/textual/issues/1399 - Watch methods may now take no parameters - Added `compute` parameter to reactive +- A `KeyError` raised during `compose` now carries the full traceback. ### Fixed diff --git a/src/textual/app.py b/src/textual/app.py index 0db8c3b15..229bb0d04 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1569,7 +1569,7 @@ class App(Generic[ReturnType], DOMNode): except TypeError as error: raise TypeError( f"{self!r} compose() returned an invalid response; {error}" - ) from None + ) from error await self.mount_all(widgets) def _on_idle(self) -> None: diff --git a/src/textual/widget.py b/src/textual/widget.py index 9cd842261..1f5c18009 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -2395,7 +2395,7 @@ class Widget(DOMNode): except TypeError as error: raise TypeError( f"{self!r} compose() returned an invalid response; {error}" - ) from None + ) from error except Exception: self.app.panic(Traceback()) else: From 984fa41de57536441c935a2827c506db6f4eb0e7 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 17 Jan 2023 14:25:44 +0000 Subject: [PATCH 12/23] Fix typo/thinko in CHANGELOG --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d94233b4f..63a919a69 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -28,7 +28,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - `COMPONENT_CLASSES` are now inherited from base classes https://github.com/Textualize/textual/issues/1399 - Watch methods may now take no parameters - Added `compute` parameter to reactive -- A `KeyError` raised during `compose` now carries the full traceback. +- A `TypeError` raised during `compose` now carries the full traceback. ### Fixed From 12f272d50721bdb344a4e2f19a4aecac899fc72b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 17 Jan 2023 14:56:35 +0000 Subject: [PATCH 13/23] Fix tests. I misread the original app hierarchy and was trying to focus something that can't be focused. --- tests/test_focus.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/tests/test_focus.py b/tests/test_focus.py index 7b96972e2..e782e8961 100644 --- a/tests/test_focus.py +++ b/tests/test_focus.py @@ -80,12 +80,10 @@ def test_focus_next_and_previous_with_type_selector(screen: Screen): screen.set_focus(screen.query_one("#Paul")) assert screen.focused.id == "Paul" - assert screen.focus_next(Focusable).id == "Jessica" assert screen.focus_next(Focusable).id == "baz" assert screen.focus_next(Focusable).id == "child" assert screen.focus_previous(Focusable).id == "baz" - assert screen.focus_previous(Focusable).id == "Jessica" assert screen.focus_previous(Focusable).id == "Paul" assert screen.focus_previous(Focusable).id == "container1" assert screen.focus_previous(Focusable).id == "foo" @@ -96,15 +94,12 @@ def test_focus_next_and_previous_with_str_selector(screen: Screen): screen.set_focus(screen.query_one("#foo")) assert screen.focused.id == "foo" - assert screen.focus_next(".a").id == "Jessica" - assert screen.focus_next("Focusable").id == "baz" - - assert screen.focus_previous(".b").id == "container1" - - assert screen.focus_next("Focusable").id == "Paul" + assert screen.focus_next(".a").id == "foo" + assert screen.focus_next(".c").id == "Paul" assert screen.focus_next(".c").id == "child" assert screen.focus_previous(".c").id == "Paul" + assert screen.focus_previous(".a").id == "foo" def test_focus_next_and_previous_with_type_selector_without_self(): @@ -162,6 +157,6 @@ def test_focus_next_and_previous_with_str_selector_without_self(screen: Screen): assert screen.focus_next(".b").id == "baz" assert screen.focus_next(".c").id == "child" - assert screen.focus_previous(".a").id == "Jessica" - assert screen.focus_previous(".b").id == "container1" assert screen.focus_previous(".a").id == "foo" + assert screen.focus_previous(".a").id == "foo" + assert screen.focus_previous(".b").id == "baz" From ed1f7331d8f3d16f3cf5d5ae9ac6d76e8c99d286 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 17 Jan 2023 14:56:45 +0000 Subject: [PATCH 14/23] Add focus tests. --- tests/test_focus.py | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/tests/test_focus.py b/tests/test_focus.py index e782e8961..37a97f518 100644 --- a/tests/test_focus.py +++ b/tests/test_focus.py @@ -75,6 +75,47 @@ def test_focus_next_and_previous(screen: Screen): assert screen.focus_previous().id == "foo" +def test_focus_next_wrap_around(screen: Screen): + """Ensure focusing the next widget wraps around the focus chain.""" + screen.set_focus(screen.query_one("#child")) + assert screen.focused.id == "child" + + assert screen.focus_next().id == "foo" + + +def test_focus_previous_wrap_around(screen: Screen): + """Ensure focusing the previous widget wraps around the focus chain.""" + screen.set_focus(screen.query_one("#foo")) + assert screen.focused.id == "foo" + + assert screen.focus_previous().id == "child" + + +def test_wrap_around_selector(screen: Screen): + """Ensure moving focus in both directions wraps around the focus chain.""" + screen.set_focus(screen.query_one("#foo")) + assert screen.focused.id == "foo" + + assert screen.focus_previous("#Paul").id == "Paul" + assert screen.focus_next("#foo").id == "foo" + + +def test_no_focus_empty_selector(screen: Screen): + """Ensure focus is cleared when selector matches nothing.""" + assert screen.focus_next("#bananas") is None + assert screen.focus_previous("#bananas") is None + + screen.set_focus(screen.query_one("#foo")) + assert screen.focused is not None + assert screen.focus_next("bananas") is None + assert screen.focused is None + + screen.set_focus(screen.query_one("#foo")) + assert screen.focused is not None + assert screen.focus_previous("bananas") is None + assert screen.focused is None + + def test_focus_next_and_previous_with_type_selector(screen: Screen): """Move focus with a selector that matches the currently focused node.""" screen.set_focus(screen.query_one("#Paul")) From 4963f62893bd2bd78604afb277ba109125bfeef1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 17 Jan 2023 15:02:09 +0000 Subject: [PATCH 15/23] Use focusable widgets in tests. --- tests/test_focus.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/tests/test_focus.py b/tests/test_focus.py index 37a97f518..e11b14a8d 100644 --- a/tests/test_focus.py +++ b/tests/test_focus.py @@ -152,22 +152,22 @@ def test_focus_next_and_previous_with_type_selector_without_self(): screen = app.screen from textual.containers import Horizontal, Vertical - from textual.widgets import Input, Label, Static + from textual.widgets import Button, Checkbox, Input screen._add_children( Vertical( Horizontal( Input(id="w3"), - Label(id="w4"), + Checkbox(id="w4"), Input(id="w5"), - Static(id="w6"), - Label(id="w7"), + Button(id="w6"), + Checkbox(id="w7"), id="w2", ), Horizontal( - Static(id="w9"), - Label(id="w10"), - Static(id="w11"), + Button(id="w9"), + Checkbox(id="w10"), + Button(id="w11"), Input(id="w12"), Input(id="w13"), id="w8", @@ -179,13 +179,13 @@ def test_focus_next_and_previous_with_type_selector_without_self(): screen.set_focus(screen.query_one("#w3")) assert screen.focused.id == "w3" - assert screen.focus_next(Static).id == "w6" - assert screen.focus_next(Label).id == "w7" + assert screen.focus_next(Button).id == "w6" + assert screen.focus_next(Checkbox).id == "w7" assert screen.focus_next(Input).id == "w12" - assert screen.focus_previous(Static).id == "w11" - assert screen.focus_previous(Label).id == "w10" - assert screen.focus_previous(Static).id == "w9" + assert screen.focus_previous(Button).id == "w11" + assert screen.focus_previous(Checkbox).id == "w10" + assert screen.focus_previous(Button).id == "w9" assert screen.focus_previous(Input).id == "w5" From d2726d18f13e4b58b70ec3ee81a388a2af39d8c1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 17 Jan 2023 15:02:29 +0000 Subject: [PATCH 16/23] Add selector to focus next and previous. --- src/textual/screen.py | 76 ++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 71 insertions(+), 5 deletions(-) diff --git a/src/textual/screen.py b/src/textual/screen.py index 32ea3e7a2..d8d79d266 100644 --- a/src/textual/screen.py +++ b/src/textual/screen.py @@ -9,6 +9,9 @@ from rich.style import Style from . import errors, events, messages from ._callback import invoke from ._compositor import Compositor, MapGeometry +from .css.match import match +from .css.parse import parse_selectors +from .dom import DOMNode from .timer import Timer from ._types import CallbackType from .geometry import Offset, Region, Size @@ -178,7 +181,66 @@ class Screen(Widget): return widgets - def _move_focus(self, direction: int = 0) -> Widget | None: + def _move_focus( + self, direction: int = 0, selector: str | type[DOMNode.ExpectType] = "*" + ) -> Widget | None: + """Move the focus in the given direction. + + Args: + direction (int, optional): 1 to move forward, -1 to move backward, or + 0 to keep the current focus. + selector (str | type[DOMNode.ExpectType], optional): CSS selector to filter + what nodes can be focused. + + Returns: + Widget | None: Newly focused widget, or None for no focus. If the return + is not `None`, then it is guaranteed that the widget returned matches + the CSS selectors given in the argument. + """ + if not isinstance(selector, str): + selector = selector.__name__ + selector_set = parse_selectors(selector) + focus_chain = self.focus_chain + filtered_focus_chain = ( + node for node in focus_chain if match(selector_set, node) + ) + + if not focus_chain: + # Nothing focusable, so nothing to do + return self.focused + if self.focused is None: + # Nothing currently focused, so focus the first one. + to_focus = next(filtered_focus_chain, None) + self.set_focus(to_focus) + return self.focused + + # Ensure focus will be in a node that matches the selectors. + if not direction and not match(selector_set, self.focused): + direction = 1 + + try: + # Find the index of the currently focused widget + current_index = focus_chain.index(self.focused) + except ValueError: + # Focused widget was removed in the interim, start again + self.set_focus(next(filtered_focus_chain, None)) + else: + # Only move the focus if we are currently showing the focus + if direction: + to_focus: Widget | None = None + chain_length = len(focus_chain) + for step in range(1, len(focus_chain) + 1): + node = focus_chain[ + (current_index + direction * step) % chain_length + ] + if match(selector_set, node): + to_focus = node + break + self.set_focus(to_focus) + + return self.focused + + def __move_focus(self, direction: int = 0) -> Widget | None: """Move the focus in the given direction. Args: @@ -211,21 +273,25 @@ class Screen(Widget): return self.focused - def focus_next(self) -> Widget | None: + def focus_next( + self, selector: str | type[DOMNode.ExpectType] = "*" + ) -> Widget | None: """Focus the next widget. Returns: Widget | None: Newly focused widget, or None for no focus. """ - return self._move_focus(1) + return self._move_focus(1, selector) - def focus_previous(self) -> Widget | None: + def focus_previous( + self, selector: str | type[DOMNode.ExpectType] = "*" + ) -> Widget | None: """Focus the previous widget. Returns: Widget | None: Newly focused widget, or None for no focus. """ - return self._move_focus(-1) + return self._move_focus(-1, selector) def _reset_focus( self, widget: Widget, avoiding: list[Widget] | None = None From 4221febf44ecdb64df261ca73827d2d8156ccaaa Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 17 Jan 2023 15:04:15 +0000 Subject: [PATCH 17/23] Update changelog. --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 39e484376..df0e0f5c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Added a `Tree.NodeHighlighted` message, giving a `on_tree_node_highlighted` event handler https://github.com/Textualize/textual/issues/1400 - Added a `inherit_component_classes` subclassing parameter to control whether or not component classes are inherited from base classes https://github.com/Textualize/textual/issues/1399 - Added `diagnose` as a `textual` command https://github.com/Textualize/textual/issues/1542 +- Added an optional parameter `selector` to the methods `Screen.focus_next` and `Screen.focus_previous` that enable using a CSS selector to narrow down which widgets can get focus https://github.com/Textualize/textual/issues/1196 ### Changed From 1ebbf3c7fa832cdcb1c3d9d054a371c87c90fabd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 17 Jan 2023 15:05:25 +0000 Subject: [PATCH 18/23] Docstrings and remove dead code. --- src/textual/screen.py | 41 ++++++++--------------------------------- 1 file changed, 8 insertions(+), 33 deletions(-) diff --git a/src/textual/screen.py b/src/textual/screen.py index d8d79d266..273469bd5 100644 --- a/src/textual/screen.py +++ b/src/textual/screen.py @@ -240,44 +240,15 @@ class Screen(Widget): return self.focused - def __move_focus(self, direction: int = 0) -> Widget | None: - """Move the focus in the given direction. - - Args: - direction (int, optional): 1 to move forward, -1 to move backward, or - 0 to keep the current focus. - - Returns: - Widget | None: Newly focused widget, or None for no focus. - """ - focusable_widgets = self.focus_chain - - if not focusable_widgets: - # Nothing focusable, so nothing to do - return self.focused - if self.focused is None: - # Nothing currently focused, so focus the first one - self.set_focus(focusable_widgets[0]) - else: - try: - # Find the index of the currently focused widget - current_index = focusable_widgets.index(self.focused) - except ValueError: - # Focused widget was removed in the interim, start again - self.set_focus(focusable_widgets[0]) - else: - # Only move the focus if we are currently showing the focus - if direction: - current_index = (current_index + direction) % len(focusable_widgets) - self.set_focus(focusable_widgets[current_index]) - - return self.focused - def focus_next( self, selector: str | type[DOMNode.ExpectType] = "*" ) -> Widget | None: """Focus the next widget. + Args: + selector (str | type[DOMNode.ExpectType], optional): CSS selector to filter + what nodes can be focused. + Returns: Widget | None: Newly focused widget, or None for no focus. """ @@ -288,6 +259,10 @@ class Screen(Widget): ) -> Widget | None: """Focus the previous widget. + Args: + selector (str | type[DOMNode.ExpectType], optional): CSS selector to filter + what nodes can be focused. + Returns: Widget | None: Newly focused widget, or None for no focus. """ From 2bd1372c38daa442e7687f3de05ffcb689420ffd Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 17 Jan 2023 15:18:31 +0000 Subject: [PATCH 19/23] added minus key --- src/textual/cli/previews/keys.py | 14 +++++++++++--- src/textual/events.py | 2 +- src/textual/keys.py | 1 + 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/textual/cli/previews/keys.py b/src/textual/cli/previews/keys.py index a3b30e162..8b027bd94 100644 --- a/src/textual/cli/previews/keys.py +++ b/src/textual/cli/previews/keys.py @@ -1,15 +1,18 @@ from rich.panel import Panel +from rich.text import Text + from textual.app import App, ComposeResult +from textual.reactive import var, Reactive from textual import events from textual.containers import Horizontal from textual.widgets import Button, Header, TextLog INSTRUCTIONS = """\ -Press some keys! +[u]Press some keys![/] -Because we want to display all the keys, ctrl+C won't quit this example. Use the Quit button below to exit the app.\ +To quit the app press [b]ctrl+c[/b] [i]twice[i] or press the Quit button below.\ """ @@ -32,6 +35,8 @@ class KeysApp(App, inherit_bindings=False): } """ + last_key: Reactive[str | None] = var(None) + def compose(self) -> ComposeResult: yield Header() yield Horizontal( @@ -42,10 +47,13 @@ class KeysApp(App, inherit_bindings=False): yield KeyLog() def on_ready(self) -> None: - self.query_one(KeyLog).write(Panel(INSTRUCTIONS), expand=True) + self.query_one(KeyLog).write(Panel(Text.from_markup(INSTRUCTIONS)), expand=True) def on_key(self, event: events.Key) -> None: self.query_one(KeyLog).write(event) + if event.key == "ctrl+c" and self.last_key == "ctrl+c": + self.exit() + self.last_key = event.key def on_button_pressed(self, event: Button.Pressed) -> None: if event.button.id == "quit": diff --git a/src/textual/events.py b/src/textual/events.py index 376d17fe7..501dfac06 100644 --- a/src/textual/events.py +++ b/src/textual/events.py @@ -7,7 +7,7 @@ from rich.style import Style from ._types import MessageTarget from .geometry import Offset, Size -from .keys import _get_key_aliases +from .keys import _get_key_aliases, _get_key_display from .message import Message MouseEventT = TypeVar("MouseEventT", bound="MouseEvent") diff --git a/src/textual/keys.py b/src/textual/keys.py index f2fd93ad2..b4253c3ae 100644 --- a/src/textual/keys.py +++ b/src/textual/keys.py @@ -228,6 +228,7 @@ KEY_DISPLAY_ALIASES = { "backspace": "⌫", "escape": "ESC", "enter": "⏎", + "minus": "-", } From e32344615a9038c403af08cc7c532f3500d3f7d8 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 17 Jan 2023 15:19:40 +0000 Subject: [PATCH 20/23] Added test --- tests/test_keys.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/test_keys.py b/tests/test_keys.py index b0948757c..9f13e17d1 100644 --- a/tests/test_keys.py +++ b/tests/test_keys.py @@ -1,7 +1,7 @@ import pytest from textual.app import App -from textual.keys import _character_to_key +from textual.keys import _character_to_key, _get_key_display @pytest.mark.parametrize( @@ -48,3 +48,7 @@ async def test_character_bindings(): await pilot.press("x") await pilot.pause() assert counter == 3 + + +def test_get_key_display(): + assert _get_key_display("minus") == "-" From 88a1fe013cddeb8b8bdd122c1e0887a4dc098f4b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rodrigo=20Gir=C3=A3o=20Serr=C3=A3o?= <5621605+rodrigogiraoserrao@users.noreply.github.com> Date: Tue, 17 Jan 2023 15:18:32 +0000 Subject: [PATCH 21/23] Tweak docstrings. --- src/textual/screen.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/textual/screen.py b/src/textual/screen.py index 273469bd5..7db307b43 100644 --- a/src/textual/screen.py +++ b/src/textual/screen.py @@ -186,6 +186,9 @@ class Screen(Widget): ) -> Widget | None: """Move the focus in the given direction. + If no widget is currently focused, this will focus the first focusable widget. + If no focusable widget matches the given CSS selector, focus is set to `None`. + Args: direction (int, optional): 1 to move forward, -1 to move backward, or 0 to keep the current focus. @@ -243,28 +246,38 @@ class Screen(Widget): def focus_next( self, selector: str | type[DOMNode.ExpectType] = "*" ) -> Widget | None: - """Focus the next widget. + """Focus the next widget, optionally filtered by a CSS selector. + + If no widget is currently focused, this will focus the first focusable widget. + If no focusable widget matches the given CSS selector, focus is set to `None`. Args: selector (str | type[DOMNode.ExpectType], optional): CSS selector to filter what nodes can be focused. Returns: - Widget | None: Newly focused widget, or None for no focus. + Widget | None: Newly focused widget, or None for no focus. If the return + is not `None`, then it is guaranteed that the widget returned matches + the CSS selectors given in the argument. """ return self._move_focus(1, selector) def focus_previous( self, selector: str | type[DOMNode.ExpectType] = "*" ) -> Widget | None: - """Focus the previous widget. + """Focus the previous widget, optionally filtered by a CSS selector. + + If no widget is currently focused, this will focus the first focusable widget. + If no focusable widget matches the given CSS selector, focus is set to `None`. Args: selector (str | type[DOMNode.ExpectType], optional): CSS selector to filter what nodes can be focused. Returns: - Widget | None: Newly focused widget, or None for no focus. + Widget | None: Newly focused widget, or None for no focus. If the return + is not `None`, then it is guaranteed that the widget returned matches + the CSS selectors given in the argument. """ return self._move_focus(-1, selector) From 1bfc19f480c793d1161e5376b88ba4c647210307 Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 17 Jan 2023 15:24:31 +0000 Subject: [PATCH 22/23] help message --- src/textual/cli/previews/keys.py | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/textual/cli/previews/keys.py b/src/textual/cli/previews/keys.py index 8b027bd94..8dfedb9dd 100644 --- a/src/textual/cli/previews/keys.py +++ b/src/textual/cli/previews/keys.py @@ -51,8 +51,12 @@ class KeysApp(App, inherit_bindings=False): def on_key(self, event: events.Key) -> None: self.query_one(KeyLog).write(event) - if event.key == "ctrl+c" and self.last_key == "ctrl+c": - self.exit() + if event.key == "ctrl+c": + if self.last_key == "ctrl+c": + self.exit() + else: + self.query_one(KeyLog).write("Press Ctrl+C again to quit") + self.last_key = event.key def on_button_pressed(self, event: Button.Pressed) -> None: From 507413d473bab3006ba78c6565cb31559c88d9da Mon Sep 17 00:00:00 2001 From: Will McGugan Date: Tue, 17 Jan 2023 15:39:59 +0000 Subject: [PATCH 23/23] updated text --- src/textual/cli/previews/keys.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/textual/cli/previews/keys.py b/src/textual/cli/previews/keys.py index 8dfedb9dd..91ac5575a 100644 --- a/src/textual/cli/previews/keys.py +++ b/src/textual/cli/previews/keys.py @@ -12,7 +12,7 @@ from textual.widgets import Button, Header, TextLog INSTRUCTIONS = """\ [u]Press some keys![/] -To quit the app press [b]ctrl+c[/b] [i]twice[i] or press the Quit button below.\ +To quit the app press [b]ctrl+c[/b] [i]twice[/i] or press the Quit button below.\ """