From cee69fbc3288737b07c0b3d29c1f1d9564c590d1 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 1 Nov 2022 10:55:15 +0000 Subject: [PATCH 01/19] Add an _index method to NodeList This maps on to a normal List.index. --- src/textual/_node_list.py | 14 ++++++++++++++ tests/test_node_list.py | 11 +++++++++++ 2 files changed, 25 insertions(+) diff --git a/src/textual/_node_list.py b/src/textual/_node_list.py index fa5570fc4..8e763d3be 100644 --- a/src/textual/_node_list.py +++ b/src/textual/_node_list.py @@ -39,6 +39,20 @@ class NodeList(Sequence): def __contains__(self, widget: Widget) -> bool: return widget in self._nodes + def _index(self, widget: Widget) -> int: + """Return the index of the given widget. + + Args: + widget (Widget): The widget to find in the node list. + + Returns: + int: The index of the widget in the node list. + + Raises: + ValueError: If the widget is not in the node list. + """ + return self._nodes.index(widget) + def _append(self, widget: Widget) -> None: """Append a Widget. diff --git a/tests/test_node_list.py b/tests/test_node_list.py index 93093bb89..e6ed883b3 100644 --- a/tests/test_node_list.py +++ b/tests/test_node_list.py @@ -1,3 +1,5 @@ +import pytest + from textual.widget import Widget from textual._node_list import NodeList @@ -35,6 +37,15 @@ def test_contains(): assert widget in nodes assert Widget() not in nodes +def test_index(): + """Can we get the index of a widget in the list?""" + widget = Widget() + nodes = NodeList() + with pytest.raises(ValueError): + _ = nodes._index(widget) + nodes._append(widget) + assert nodes._index(widget) == 0 + def test_remove(): """Can we remove a widget we've added?""" widget = Widget() From 39b23e1f512b4fb50edb2e96947ccaff4b6747a2 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 1 Nov 2022 17:01:56 +0000 Subject: [PATCH 02/19] WIP: Starting work on adding before/after mount directions This is still a work-in-progress, but this feels like a good point to commit for safe keeping. This is a non-working WIP. --- src/textual/app.py | 29 +++++++++++++++++++---- src/textual/dom.py | 54 ++++++++++++++++++++++++++++++++++++++++++ src/textual/widget.py | 21 ++++++++++++---- tests/test_dom_spot.py | 22 +++++++++++++++++ 4 files changed, 118 insertions(+), 8 deletions(-) create mode 100644 tests/test_dom_spot.py diff --git a/src/textual/app.py b/src/textual/app.py index 7ea329ed4..32ba05d52 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -822,16 +822,29 @@ class App(Generic[ReturnType], DOMNode): self._require_stylesheet_update.add(self.screen if node is None else node) self.check_idle() - def mount(self, *widgets: Widget) -> AwaitMount: - """Mount the given widgets. + def mount( + self, *widgets: Widget, before: MountSpot = None, after: MountSpot = None + ) -> AwaitMount: + """Mount the given widgets relative to the app's screen. Args: *widgets (Widget): The widget(s) to mount. + before (MountSpot, optional): Optional location to mount before. + after (MountSpot, optional): Optional location to mount after. Returns: AwaitMount: An awaitable object that waits for widgets to be mounted. + + Raises: + MountError: If there is a problem with the mount request. + + Note: + Only one of ``before`` or ``after`` can be provided. If both are + provided a ``MountError`` will be raised. """ - return AwaitMount(self._register(self.screen, *widgets)) + return AwaitMount( + self._register(self.screen, *widgets, before=before, after=after) + ) def mount_all(self, widgets: Iterable[Widget]) -> AwaitMount: """Mount widgets from an iterable. @@ -1300,12 +1313,20 @@ class App(Generic[ReturnType], DOMNode): return True return False - def _register(self, parent: DOMNode, *widgets: Widget) -> list[Widget]: + def _register( + self, + parent: DOMNode, + *widgets: Widget, + before: MountSpot = None, + after: MountSpot = None, + ) -> list[Widget]: """Register widget(s) so they may receive events. Args: parent (DOMNode): Parent node. *widgets: The widget(s) to register. + before (MountSpot, optional): Optional location to mount before. + after (MountSpot, optional): Optional location to mount after. Returns: list[Widget]: List of modified widgets. diff --git a/src/textual/dom.py b/src/textual/dom.py index aa8f9a1b4..369879f5c 100644 --- a/src/textual/dom.py +++ b/src/textual/dom.py @@ -13,6 +13,7 @@ from typing import ( TypeVar, cast, overload, + Union, ) import rich.repr @@ -894,3 +895,56 @@ class DOMNode(MessagePump): def refresh(self, *, repaint: bool = True, layout: bool = False) -> None: pass + + DOMSpot = Union[int, str, "DOMQuery[Widget]", "Widget", None] + """The type of a relative location of a node in the DOM.""" + + def _find_spot(self, spot: DOMSpot) -> tuple["DOMNode", int]: + """Collapse a number of DOM location identifiers into a parent/child-index pair. + + Args: + spot (DOMSpot): The spot to find. + + Returns: + tuple[DOMNode, int]: The parent and the location in its child list. + + Raises: + ValueError: If a given node can't be located amongst children. + + The rules of this method are: + + - Given ``None``, parent is ``self`` and location is ``-1``. + - Given an integer, parent is ``self`` and location is the integer value. + - Given a DOMNode, parent is the node's parent and location is + where the widget is found in the parent's ``children``. If it + can't be found a ``ValueError`` will be raised. + - Given a query result, the ``first`` node is used. The code then + falls to acting as if a DOMNode were given. + - Given a string, it is used to perform a query and then the result + is used as if a query result were given. + """ + + from .widget import Widget + + # None pretty much means "at the end of our child list." + if spot is None: + return cast(Widget, self), -1 + + # A numeric location means at that point in our child list. + if isinstance(spot, int): + return cast(Widget, self), spot + + # We've got a widget that has a parent; let's look for that in our children. + if isinstance(spot, DOMNode): + try: + return cast(DOMNode, spot.parent), spot.parent.children._index(spot) + except ValueError: + raise DOMError(f"{spot!r} is not a child of {self!r}") from None + + # Do we have a string? If we do, cast that into a query. + if isinstance(spot, str): + spot = self.query(spot) + + # At this point, we should have a query of some description. So + # let's now descend into it and see. + return spot.first(DOMNode).parent._find_spot(spot.first(DOMNode)) diff --git a/src/textual/widget.py b/src/textual/widget.py index 9d1f50bb2..d617c7b60 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -51,7 +51,7 @@ from .reactive import Reactive from .render import measure if TYPE_CHECKING: - from .app import App, ComposeResult + from .app import App, ComposeResult, MountSpot from .scrollbar import ( ScrollBar, ScrollBarCorner, @@ -375,16 +375,29 @@ class Widget(DOMNode): if self._scrollbar_corner is not None: yield self._scrollbar_corner - def mount(self, *widgets: Widget) -> AwaitMount: - """Mount child widgets (making this widget a container). + def mount( + self, *widgets: Widget, before: MountSpot = None, after: MountSpot = None + ) -> AwaitMount: + """Mount widgets below this widget (making this widget a container). Args: *widgets (Widget): The widget(s) to mount. + before (MountSpot, optional): Optional location to mount before. + after (MountSpot, optional): Optional location to mount after. Returns: AwaitMount: An awaitable object that waits for widgets to be mounted. + + Raises: + MountError: If there is a problem with the mount request. + + Note: + Only one of ``before`` or ``after`` can be provided. If both are + provided a ``MountError`` will be raised. """ - return AwaitMount(self.app._register(self, *widgets)) + return AwaitMount( + self.app._register(self, *widgets, before=before, after=after) + ) def compose(self) -> ComposeResult: """Called by Textual to create child widgets. diff --git a/tests/test_dom_spot.py b/tests/test_dom_spot.py new file mode 100644 index 000000000..620c9718d --- /dev/null +++ b/tests/test_dom_spot.py @@ -0,0 +1,22 @@ +from textual.widget import Widget + +class Content(Widget): + pass + +class Body(Widget): + pass + +def test_find_dom_spot(): + screen = Widget(name="Screen") + header = Widget(name="Header", id="header") + body = Body(id="body") + content = [ Content(id=f"item{n}") for n in range(1000)] + body._add_children(*content) + footer = Widget(name="Footer", id="footer") + screen._add_children(header, body, footer) + assert list(screen.children) == [header,body,footer] + assert screen._find_spot(None) == (screen,-1) + assert screen._find_spot(1) == (screen, 1) + assert screen._find_spot(body) == screen._find_spot(1) + assert screen._find_spot("Body") == screen._find_spot(body) + assert screen._find_spot("#body") == screen._find_spot(1) From 3ed406f13ff973e99726c71d73dacbf9539d9ae8 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 1 Nov 2022 21:14:38 +0000 Subject: [PATCH 03/19] Tidy the flow through _find_spot While it was mostly working okay, I felt it needed a wee bit more protection, and also could stand to have some of the more extreme type-checking complaints handled. At this point I feel the code is about as tidy as it can get -- until I fully test it live and find other issues I guess. But this does pass the unit tests I've made for it so far. --- src/textual/dom.py | 21 ++++++++++++++++----- 1 file changed, 16 insertions(+), 5 deletions(-) diff --git a/src/textual/dom.py b/src/textual/dom.py index 369879f5c..3f9e3722d 100644 --- a/src/textual/dom.py +++ b/src/textual/dom.py @@ -924,6 +924,8 @@ class DOMNode(MessagePump): is used as if a query result were given. """ + # Due the the circular reference between DOMNode and Widget, we need + # to inline import Widget here. from .widget import Widget # None pretty much means "at the end of our child list." @@ -934,10 +936,18 @@ class DOMNode(MessagePump): if isinstance(spot, int): return cast(Widget, self), spot - # We've got a widget that has a parent; let's look for that in our children. + # We've got a DOM node... if isinstance(spot, DOMNode): + # ...it really should have a parent for any of this to make + # sense. So let's raise an exception if it doesn't have one. + if spot.parent is None: + raise DOMError( + f"Unable to find relative location of {spot!r} because it has no parent" + ) + # At this point it's safe to go looking for its numeric location + # amongst its siblings. try: - return cast(DOMNode, spot.parent), spot.parent.children._index(spot) + return spot.parent, spot.parent.children._index(spot) except ValueError: raise DOMError(f"{spot!r} is not a child of {self!r}") from None @@ -945,6 +955,7 @@ class DOMNode(MessagePump): if isinstance(spot, str): spot = self.query(spot) - # At this point, we should have a query of some description. So - # let's now descend into it and see. - return spot.first(DOMNode).parent._find_spot(spot.first(DOMNode)) + # At this point, we should have a query of some description. The + # query could have multiple hits; as a choice, let's take the first + # hit in the query and go around doing a Widget find. + return self._find_spot(spot.first(Widget)) From e35d146445c119cb794b75ea4794735c6af9bf3d Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 1 Nov 2022 21:33:25 +0000 Subject: [PATCH 04/19] Tidy up and extend the DOM spot tests --- tests/test_dom_spot.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/test_dom_spot.py b/tests/test_dom_spot.py index 620c9718d..c70da0b47 100644 --- a/tests/test_dom_spot.py +++ b/tests/test_dom_spot.py @@ -1,4 +1,7 @@ +import pytest + from textual.widget import Widget +from textual.dom import DOMError class Content(Widget): pass @@ -7,6 +10,8 @@ class Body(Widget): pass def test_find_dom_spot(): + + # Build up a "fake" DOM for an application. screen = Widget(name="Screen") header = Widget(name="Header", id="header") body = Body(id="body") @@ -14,9 +19,21 @@ def test_find_dom_spot(): body._add_children(*content) footer = Widget(name="Footer", id="footer") screen._add_children(header, body, footer) + + # Just as a quick double-check, make sure the main components are in + # their intended place. assert list(screen.children) == [header,body,footer] + + # Now check that we find what we're looking for in the places we expect + # to find them. assert screen._find_spot(None) == (screen,-1) assert screen._find_spot(1) == (screen, 1) assert screen._find_spot(body) == screen._find_spot(1) assert screen._find_spot("Body") == screen._find_spot(body) assert screen._find_spot("#body") == screen._find_spot(1) + + # Finally, let's be sure that we get an error if, for some odd reason, + # we go looking for a widget that isn't actually part of the DOM we're + # looking in. + with pytest.raises(DOMError): + _ = screen._find_spot(Widget()) From 016f7be83a36d2448c7aa90973a44c8e16f5788c Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Tue, 1 Nov 2022 22:26:38 +0000 Subject: [PATCH 05/19] Add an insert method to the NodeList --- src/textual/_node_list.py | 11 +++++++++++ tests/test_node_list.py | 10 ++++++++++ 2 files changed, 21 insertions(+) diff --git a/src/textual/_node_list.py b/src/textual/_node_list.py index 8e763d3be..a2a2d1d78 100644 --- a/src/textual/_node_list.py +++ b/src/textual/_node_list.py @@ -64,6 +64,17 @@ class NodeList(Sequence): self._nodes_set.add(widget) self._updates += 1 + def _insert(self, index: int, widget: Widget) -> None: + """Insert a Widget. + + Args: + widget (Widget): A widget. + """ + if widget not in self._nodes_set: + self._nodes.insert(index, widget) + self._nodes_set.add(widget) + self._updates += 1 + def _remove(self, widget: Widget) -> None: """Remove a widget from the list. diff --git a/tests/test_node_list.py b/tests/test_node_list.py index e6ed883b3..fec53dbb9 100644 --- a/tests/test_node_list.py +++ b/tests/test_node_list.py @@ -21,6 +21,16 @@ def test_repeat_add_one(): nodes._append(widget) assert len(nodes)==1 +def test_insert(): + nodes = NodeList() + widget1 = Widget() + widget2 = Widget() + widget3 = Widget() + nodes._append(widget1) + nodes._append(widget3) + nodes._insert(1,widget2) + assert list(nodes) == [widget1,widget2,widget3] + def test_truthy(): """Does a node list act as a truthy object?""" nodes = NodeList() From 627486a42a5f85efd8777ed6c774866b37b915d6 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 2 Nov 2022 11:35:22 +0000 Subject: [PATCH 06/19] Lots of reworking of the mount-before/after work Lots of things going on here, mainly narrowing in on the final form. --- src/textual/app.py | 80 ++++++++++++++++++++++++------ src/textual/dom.py | 64 ------------------------ src/textual/widget.py | 84 ++++++++++++++++++++++++++++++-- tests/test_widget_mount_point.py | 40 +++++++++++++++ 4 files changed, 183 insertions(+), 85 deletions(-) create mode 100644 tests/test_widget_mount_point.py diff --git a/src/textual/app.py b/src/textual/app.py index 32ba05d52..2211942f3 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -823,14 +823,17 @@ class App(Generic[ReturnType], DOMNode): self.check_idle() def mount( - self, *widgets: Widget, before: MountSpot = None, after: MountSpot = None + self, + *widgets: Widget, + before: int | str | Widget | None = None, + after: int | str | Widget | None = None, ) -> AwaitMount: """Mount the given widgets relative to the app's screen. Args: *widgets (Widget): The widget(s) to mount. - before (MountSpot, optional): Optional location to mount before. - after (MountSpot, optional): Optional location to mount after. + before (int | str | Widget, optional): Optional location to mount before. + after (int | str | Widget, optional): Optional location to mount after. Returns: AwaitMount: An awaitable object that waits for widgets to be mounted. @@ -842,20 +845,32 @@ class App(Generic[ReturnType], DOMNode): Only one of ``before`` or ``after`` can be provided. If both are provided a ``MountError`` will be raised. """ - return AwaitMount( - self._register(self.screen, *widgets, before=before, after=after) - ) + return self.screen.mount(*widgets, before=before, after=after) - def mount_all(self, widgets: Iterable[Widget]) -> AwaitMount: + def mount_all( + self, + widgets: Iterable[Widget], + before: int | str | Widget | None = None, + after: int | str | Widget | None = None, + ) -> AwaitMount: """Mount widgets from an iterable. Args: widgets (Iterable[Widget]): An iterable of widgets. + before (int | str | Widget, optional): Optional location to mount before. + after (int | str | Widget, optional): Optional location to mount after. Returns: AwaitMount: An awaitable object that waits for widgets to be mounted. + + Raises: + MountError: If there is a problem with the mount request. + + Note: + Only one of ``before`` or ``after`` can be provided. If both are + provided a ``MountError`` will be raised. """ - return self.mount(*widgets) + return self.mount(*widgets, before=before, after=after) def is_screen_installed(self, screen: Screen | str) -> bool: """Check if a given screen has been installed. @@ -1303,31 +1318,64 @@ class App(Generic[ReturnType], DOMNode): self._require_stylesheet_update.clear() self.stylesheet.update_nodes(nodes, animate=True) - def _register_child(self, parent: DOMNode, child: Widget) -> bool: + def _register_child( + self, parent: DOMNode, child: Widget, before: int | None, after: int | None + ) -> bool: + + # Let's be 100% sure that we've not been asked to do a before and an + # after at the same time. It's possible that we can remove this + # check later on, but for the purposes of development right now, + # it's likely a good idea to keep it here to check assumptions in + # the rest of the code. + if before is not None and after is not None: + raise AppError( + "A child can only be registered before or after, not before and after" + ) + + # If we don't already know about this widget... if child not in self._registry: - parent.children._append(child) + + # Now to figure out where to place it. If we've got a `before`... + if before is not None: + # ...it's safe to NodeList._insert before that location. + parent.children._insert(before, child) + elif after is not None and after != -1: + # In this case we've got an after. -1 holds the special + # position (for now) of meaning "okay really what I mean is + # do an append, like if I'd asked to add with no before or + # after". So... we insert before the next item in the node + # list, iff after isn't -1. + parent.children._insert(after + 1, child) + else: + # At this point we appear to not be adding before or after, + # or we've got a before/after value that really means + # "please append". So... + parent.children._append(child) + + # Now that the widget is in the NodeList of its parent, sort out + # the rest of the admin. self._registry.add(child) child._attach(parent) child._post_register(self) child._start_messages() return True + return False def _register( self, parent: DOMNode, *widgets: Widget, - before: MountSpot = None, - after: MountSpot = None, + before: int | None = None, + after: int | None = None, ) -> list[Widget]: """Register widget(s) so they may receive events. Args: parent (DOMNode): Parent node. *widgets: The widget(s) to register. - before (MountSpot, optional): Optional location to mount before. - after (MountSpot, optional): Optional location to mount after. - + before (int, optional): A location to mount before. + after (int, option): A location to mount after. Returns: list[Widget]: List of modified widgets. @@ -1342,7 +1390,7 @@ class App(Generic[ReturnType], DOMNode): if not isinstance(widget, Widget): raise AppError(f"Can't register {widget!r}; expected a Widget instance") if widget not in self._registry: - self._register_child(parent, widget) + self._register_child(parent, widget, before, after) if widget.children: self._register(widget, *widget.children) apply_stylesheet(widget) diff --git a/src/textual/dom.py b/src/textual/dom.py index 3f9e3722d..bcd3ab683 100644 --- a/src/textual/dom.py +++ b/src/textual/dom.py @@ -895,67 +895,3 @@ class DOMNode(MessagePump): def refresh(self, *, repaint: bool = True, layout: bool = False) -> None: pass - - DOMSpot = Union[int, str, "DOMQuery[Widget]", "Widget", None] - """The type of a relative location of a node in the DOM.""" - - def _find_spot(self, spot: DOMSpot) -> tuple["DOMNode", int]: - """Collapse a number of DOM location identifiers into a parent/child-index pair. - - Args: - spot (DOMSpot): The spot to find. - - Returns: - tuple[DOMNode, int]: The parent and the location in its child list. - - Raises: - ValueError: If a given node can't be located amongst children. - - The rules of this method are: - - - Given ``None``, parent is ``self`` and location is ``-1``. - - Given an integer, parent is ``self`` and location is the integer value. - - Given a DOMNode, parent is the node's parent and location is - where the widget is found in the parent's ``children``. If it - can't be found a ``ValueError`` will be raised. - - Given a query result, the ``first`` node is used. The code then - falls to acting as if a DOMNode were given. - - Given a string, it is used to perform a query and then the result - is used as if a query result were given. - """ - - # Due the the circular reference between DOMNode and Widget, we need - # to inline import Widget here. - from .widget import Widget - - # None pretty much means "at the end of our child list." - if spot is None: - return cast(Widget, self), -1 - - # A numeric location means at that point in our child list. - if isinstance(spot, int): - return cast(Widget, self), spot - - # We've got a DOM node... - if isinstance(spot, DOMNode): - # ...it really should have a parent for any of this to make - # sense. So let's raise an exception if it doesn't have one. - if spot.parent is None: - raise DOMError( - f"Unable to find relative location of {spot!r} because it has no parent" - ) - # At this point it's safe to go looking for its numeric location - # amongst its siblings. - try: - return spot.parent, spot.parent.children._index(spot) - except ValueError: - raise DOMError(f"{spot!r} is not a child of {self!r}") from None - - # Do we have a string? If we do, cast that into a query. - if isinstance(spot, str): - spot = self.query(spot) - - # At this point, we should have a query of some description. The - # query could have multiple hits; as a choice, let's take the first - # hit in the query and go around doing a Widget find. - return self._find_spot(spot.first(Widget)) diff --git a/src/textual/widget.py b/src/textual/widget.py index d617c7b60..dab686585 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -51,7 +51,7 @@ from .reactive import Reactive from .render import measure if TYPE_CHECKING: - from .app import App, ComposeResult, MountSpot + from .app import App, ComposeResult from .scrollbar import ( ScrollBar, ScrollBarCorner, @@ -375,15 +375,70 @@ class Widget(DOMNode): if self._scrollbar_corner is not None: yield self._scrollbar_corner + class MountError(Exception): + """Error raised when there was a problem with the mount request.""" + + def _find_mount_point(self, spot: int | str | "Widget") -> tuple["Widget", int]: + """Attempt to locate the point where the caller wants to mount something. + + Args: + spot (int | str | Widget): The spot to find. + + Returns: + tuple[Widget, int]: The parent and the location in its child list. + + Raises: + Widget.MountError: If there was an error finding where to mount a widget. + + The rules of this method are: + + - Given an ``int``, parent is ``self`` and location is the integer value. + - Given a ``Widget``, parent is the widget's parent and location is + where the widget is found in the parent's ``children``. If it + can't be found a ``MountError`` will be raised. + - Given a string, it is used to perform a ``query_one`` and then the + result is used as if a ``Widget`` had been given. + """ + + # A numeric location means at that point in our child list. + if isinstance(spot, int): + return self, spot + + # If we've got a string, that should be treated like a query that + # can be passed to query_one. So let's use that to get a widget to + # work on. + if isinstance(spot, str): + spot = self.query_one(spot, Widget) + + # At this point we should have a widget, either because we got given + # one, or because we pulled one out of the query. First off, does it + # have a parent? There's no way we can use it as a sibling to make + # mounting decisions if it doesn't have a parent. + if spot.parent is None: + raise self.MountError( + f"Unable to find relative location of {spot!r} because it has no parent" + ) + + # We've got a widget. It has a parent. It has (zero or more) + # children. We should be able to go looking for the widget's + # location amongst its parent's children. + try: + return spot.parent, spot.parent.children._index(spot) + except ValueError: + raise self.MountError(f"{spot!r} is not a child of {self!r}") from None + def mount( - self, *widgets: Widget, before: MountSpot = None, after: MountSpot = None + self, + *widgets: Widget, + before: int | str | Widget | None = None, + after: int | str | Widget | None = None, ) -> AwaitMount: """Mount widgets below this widget (making this widget a container). Args: *widgets (Widget): The widget(s) to mount. - before (MountSpot, optional): Optional location to mount before. - after (MountSpot, optional): Optional location to mount after. + before (int | str | Widget, optional): Optional location to mount before. + after (int | str | Widget, optional): Optional location to mount after. Returns: AwaitMount: An awaitable object that waits for widgets to be mounted. @@ -395,8 +450,27 @@ class Widget(DOMNode): Only one of ``before`` or ``after`` can be provided. If both are provided a ``MountError`` will be raised. """ + + # Saying you want to mount before *and* after something is an error. + if before is not None and after is not None: + raise self.MountError( + "Only one of `before` or `after` can be handled -- not both" + ) + + # Decide the final resting place depending on what we've been asked + # to do. + if before is not None: + parent, before = self._find_mount_point(before) + self.log.debug(f"MOUNT under {parent!r} before {before!r} ") + elif after is not None: + parent, after = self._find_mount_point(after) + self.log.debug(f"MOUNT under {parent!r} after {after!r} ") + else: + parent = self + self.log.debug(f"MOUNT under {self!r} at the end of the child list") + return AwaitMount( - self.app._register(self, *widgets, before=before, after=after) + self.app._register(parent, *widgets, before=before, after=after) ) def compose(self) -> ComposeResult: diff --git a/tests/test_widget_mount_point.py b/tests/test_widget_mount_point.py new file mode 100644 index 000000000..bcdc41dde --- /dev/null +++ b/tests/test_widget_mount_point.py @@ -0,0 +1,40 @@ +import pytest + +from textual.widget import Widget + + +class Content(Widget): + pass + + +class Body(Widget): + pass + + +def test_find_dom_spot(): + + # Build up a "fake" DOM for an application. + screen = Widget(name="Screen") + header = Widget(name="Header", id="header") + body = Body(id="body") + content = [Content(id=f"item{n}") for n in range(1000)] + body._add_children(*content) + footer = Widget(name="Footer", id="footer") + screen._add_children(header, body, footer) + + # Just as a quick double-check, make sure the main components are in + # their intended place. + assert list(screen.children) == [header, body, footer] + + # Now check that we find what we're looking for in the places we expect + # to find them. + assert screen._find_mount_point(1) == (screen, 1) + assert screen._find_mount_point(body) == screen._find_mount_point(1) + assert screen._find_mount_point("Body") == screen._find_mount_point(body) + assert screen._find_mount_point("#body") == screen._find_mount_point(1) + + # Finally, let's be sure that we get an error if, for some odd reason, + # we go looking for a widget that isn't actually part of the DOM we're + # looking in. + with pytest.raises(Widget.MountError): + _ = screen._find_mount_point(Widget()) From 2df01449028f1472dacb0d7281cd1a658dfb9dea Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 2 Nov 2022 11:39:58 +0000 Subject: [PATCH 07/19] Remove the old _find_dom_spot tests This set of tests got left over from the great renaming. --- tests/test_dom_spot.py | 39 --------------------------------------- 1 file changed, 39 deletions(-) delete mode 100644 tests/test_dom_spot.py diff --git a/tests/test_dom_spot.py b/tests/test_dom_spot.py deleted file mode 100644 index c70da0b47..000000000 --- a/tests/test_dom_spot.py +++ /dev/null @@ -1,39 +0,0 @@ -import pytest - -from textual.widget import Widget -from textual.dom import DOMError - -class Content(Widget): - pass - -class Body(Widget): - pass - -def test_find_dom_spot(): - - # Build up a "fake" DOM for an application. - screen = Widget(name="Screen") - header = Widget(name="Header", id="header") - body = Body(id="body") - content = [ Content(id=f"item{n}") for n in range(1000)] - body._add_children(*content) - footer = Widget(name="Footer", id="footer") - screen._add_children(header, body, footer) - - # Just as a quick double-check, make sure the main components are in - # their intended place. - assert list(screen.children) == [header,body,footer] - - # Now check that we find what we're looking for in the places we expect - # to find them. - assert screen._find_spot(None) == (screen,-1) - assert screen._find_spot(1) == (screen, 1) - assert screen._find_spot(body) == screen._find_spot(1) - assert screen._find_spot("Body") == screen._find_spot(body) - assert screen._find_spot("#body") == screen._find_spot(1) - - # Finally, let's be sure that we get an error if, for some odd reason, - # we go looking for a widget that isn't actually part of the DOM we're - # looking in. - with pytest.raises(DOMError): - _ = screen._find_spot(Widget()) From 8d815f6dde0b7b5d16366ec46c6904141916176f Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 2 Nov 2022 13:33:07 +0000 Subject: [PATCH 08/19] Don't the widgets parameter with a different type --- src/textual/app.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 2211942f3..61bfb5a4b 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1384,9 +1384,15 @@ class App(Generic[ReturnType], DOMNode): if not widgets: return [] - apply_stylesheet = self.stylesheet.apply + new_widgets = list(widgets) + if before is not None or after is not None: + # There's a before or after, which means there's going to be an + # insertion, so make it easier to get the new things in the + # correct order. + new_widgets = reversed(new_widgets) - for widget in widgets: + apply_stylesheet = self.stylesheet.apply + for widget in new_widgets: if not isinstance(widget, Widget): raise AppError(f"Can't register {widget!r}; expected a Widget instance") if widget not in self._registry: From 355ed008afbf258dcf97c61e7b0aa0c8b6397ad9 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 2 Nov 2022 14:23:21 +0000 Subject: [PATCH 09/19] Ensure the app is active before yielding the pilot --- src/textual/app.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/textual/app.py b/src/textual/app.py index 61bfb5a4b..577a66688 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -675,6 +675,9 @@ class App(Generic[ReturnType], DOMNode): # Wait until the app has performed all startup routines. await app_ready_event.wait() + # Get the app in an active state. + app._set_active() + # Context manager returns pilot object to manipulate the app yield Pilot(app) From 7bb39ef75bc68e2a540f5dbd0d2432b814955122 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 2 Nov 2022 14:35:21 +0000 Subject: [PATCH 10/19] Always clean up when coming out of the Pilot --- src/textual/app.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 577a66688..9659b399f 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -679,11 +679,12 @@ class App(Generic[ReturnType], DOMNode): app._set_active() # Context manager returns pilot object to manipulate the app - yield Pilot(app) - - # Shutdown the app cleanly - await app._shutdown() - await app_task + try: + yield Pilot(app) + finally: + # Shutdown the app cleanly + await app._shutdown() + await app_task async def run_async( self, From b925fb6b19df6717f9206e8599ab6ff08c2fdcda Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 2 Nov 2022 15:18:49 +0000 Subject: [PATCH 11/19] Start unit tests for mounting --- tests/test_widget_mounting.py | 81 +++++++++++++++++++++++++++++++++++ 1 file changed, 81 insertions(+) create mode 100644 tests/test_widget_mounting.py diff --git a/tests/test_widget_mounting.py b/tests/test_widget_mounting.py new file mode 100644 index 000000000..30e508bc6 --- /dev/null +++ b/tests/test_widget_mounting.py @@ -0,0 +1,81 @@ +import pytest + +from textual.app import App +from textual.widgets import Static + +async def test_mount_via_app() -> None: + """Perform mount tests via the app.""" + + # Make a background set of widgets. + widgets = [Static(id=f"starter-{n}") for n in range( 10 )] + + async with App().run_test() as pilot: + # Mount the first one and make sure it's there. + await pilot.app.mount(widgets[0]) + assert len(pilot.app.screen.children) == 1 + assert pilot.app.screen.children[0] == widgets[0] + + # Mount the next 2 widgets via mount. + await pilot.app.mount(*widgets[1:3]) + assert list(pilot.app.screen.children) == widgets[0:3] + + # Finally mount the rest of the widgets via mount_all. + await pilot.app.mount_all(widgets[3:]) + assert list(pilot.app.screen.children) == widgets + + async with App().run_test() as pilot: + # Mount a widget before -1, which is "before the end". + penultimate = Static(id="penultimate") + await pilot.app.mount_all(widgets) + await pilot.app.mount(penultimate, before=-1) + assert pilot.app.screen.children[-2] == penultimate + + async with App().run_test() as pilot: + # Mount a widget after -1, which is "at the end". + ultimate = Static(id="ultimate") + await pilot.app.mount_all(widgets) + await pilot.app.mount(ultimate, after=-1) + assert pilot.app.screen.children[-1] == ultimate + + async with App().run_test() as pilot: + # Mount a widget before 0, which is "at the start". + start = Static(id="start") + await pilot.app.mount_all(widgets) + await pilot.app.mount(start, before=0) + assert pilot.app.screen.children[0] == start + + async with App().run_test() as pilot: + # Mount a widget after 0. You get the idea... + second = Static(id="second") + await pilot.app.mount_all(widgets) + await pilot.app.mount(second, after=0) + assert pilot.app.screen.children[1] == second + + async with App().run_test() as pilot: + # Mount a widget relative to another via query. + queue_jumper = Static(id="queue-jumper") + await pilot.app.mount_all(widgets) + await pilot.app.mount(queue_jumper, after="#starter-5") + assert pilot.app.screen.children[6] == queue_jumper + + async with App().run_test() as pilot: + # Mount a widget relative to another via query. + queue_jumper = Static(id="queue-jumper") + await pilot.app.mount_all(widgets) + await pilot.app.mount(queue_jumper, after=widgets[5]) + assert pilot.app.screen.children[6] == queue_jumper + + async with App().run_test() as pilot: + # Make sure we get told off for trying to before and after. + await pilot.app.mount_all(widgets) + with pytest.raises(Static.MountError): + await pilot.app.mount(Static(), before=2, after=2) + + async with App().run_test() as pilot: + # Make sure we get told off trying to mount relative to something + # that isn't actually in the DOM. + await pilot.app.mount_all(widgets) + with pytest.raises(Static.MountError): + await pilot.app.mount(Static(), before=Static()) + with pytest.raises(Static.MountError): + await pilot.app.mount(Static(), after=Static()) From f29015f70a1173dd90981eec5ca3a819a5745a95 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 2 Nov 2022 15:57:09 +0000 Subject: [PATCH 12/19] Add a reminder to the mount tests about query_one See https://github.com/Textualize/textual/issues/1096 --- tests/test_widget_mounting.py | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/test_widget_mounting.py b/tests/test_widget_mounting.py index 30e508bc6..7b3d4769a 100644 --- a/tests/test_widget_mounting.py +++ b/tests/test_widget_mounting.py @@ -79,3 +79,21 @@ async def test_mount_via_app() -> None: await pilot.app.mount(Static(), before=Static()) with pytest.raises(Static.MountError): await pilot.app.mount(Static(), after=Static()) + + # TODO: At the moment query_one() simply takes a query and returns the + # .first() item. As such doing a query_one() that gets more than one + # thing isn't an error, it just skims off the first thing. OTOH the + # intention of before= and after= with a selector is that an exception + # will be thrown -- the exception being the own that should be thrown + # from query_one(). So, this test here is a TODO test because we'll wait + # for a change to query_one() and then its exception will just bubble + # up. + # + # See https://github.com/Textualize/textual/issues/1096 + # + # async with App().run_test() as pilot: + # # Make sure we get an error if we try and mount with a selector that + # # results in more than one hit. + # await pilot.app.mount_all(widgets) + # with pytest.raises( ?Something? ): + # await pilot.app.mount(Static(), before="Static") From 6c88c3c7f8bc6ec6379c3c2e99277cacf42efaf7 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 2 Nov 2022 16:07:09 +0000 Subject: [PATCH 13/19] Remove hangover import of Union --- src/textual/dom.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/textual/dom.py b/src/textual/dom.py index bcd3ab683..aa8f9a1b4 100644 --- a/src/textual/dom.py +++ b/src/textual/dom.py @@ -13,7 +13,6 @@ from typing import ( TypeVar, cast, overload, - Union, ) import rich.repr From abad0a311a860117b8a30f8737fcd4936cb97c5a Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Wed, 2 Nov 2022 16:10:34 +0000 Subject: [PATCH 14/19] Make NodeList._index "public" See https://github.com/Textualize/textual/pull/1095#discussion_r1011988160 --- src/textual/_node_list.py | 2 +- src/textual/widget.py | 2 +- tests/test_node_list.py | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/textual/_node_list.py b/src/textual/_node_list.py index a2a2d1d78..5a9a31486 100644 --- a/src/textual/_node_list.py +++ b/src/textual/_node_list.py @@ -39,7 +39,7 @@ class NodeList(Sequence): def __contains__(self, widget: Widget) -> bool: return widget in self._nodes - def _index(self, widget: Widget) -> int: + def index(self, widget: Widget) -> int: """Return the index of the given widget. Args: diff --git a/src/textual/widget.py b/src/textual/widget.py index dab686585..4d7aec881 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -423,7 +423,7 @@ class Widget(DOMNode): # children. We should be able to go looking for the widget's # location amongst its parent's children. try: - return spot.parent, spot.parent.children._index(spot) + return spot.parent, spot.parent.children.index(spot) except ValueError: raise self.MountError(f"{spot!r} is not a child of {self!r}") from None diff --git a/tests/test_node_list.py b/tests/test_node_list.py index fec53dbb9..23b16139d 100644 --- a/tests/test_node_list.py +++ b/tests/test_node_list.py @@ -52,9 +52,9 @@ def test_index(): widget = Widget() nodes = NodeList() with pytest.raises(ValueError): - _ = nodes._index(widget) + _ = nodes.index(widget) nodes._append(widget) - assert nodes._index(widget) == 0 + assert nodes.index(widget) == 0 def test_remove(): """Can we remove a widget we've added?""" From 0ff550c4818be00e62293338aafea495d7c78fa0 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 3 Nov 2022 15:17:56 +0000 Subject: [PATCH 15/19] Improve text of an exceptrion Co-authored-by: Will McGugan --- 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 9659b399f..50bdee3b3 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1333,7 +1333,7 @@ class App(Generic[ReturnType], DOMNode): # the rest of the code. if before is not None and after is not None: raise AppError( - "A child can only be registered before or after, not before and after" + "Only one of 'before' and 'after' may be specified." ) # If we don't already know about this widget... From 9d5d0e6f5d3b909f7daa7864eacd77fc068fe2dc Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 3 Nov 2022 15:20:50 +0000 Subject: [PATCH 16/19] Remove debug logging now we're close to happy with the new mount approach As per https://github.com/Textualize/textual/pull/1095#discussion_r1013042951 --- src/textual/widget.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/textual/widget.py b/src/textual/widget.py index 4d7aec881..ccd3336a3 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -461,13 +461,10 @@ class Widget(DOMNode): # to do. if before is not None: parent, before = self._find_mount_point(before) - self.log.debug(f"MOUNT under {parent!r} before {before!r} ") elif after is not None: parent, after = self._find_mount_point(after) - self.log.debug(f"MOUNT under {parent!r} after {after!r} ") else: parent = self - self.log.debug(f"MOUNT under {self!r} at the end of the child list") return AwaitMount( self.app._register(parent, *widgets, before=before, after=after) From c5d8911d7d928838c47af31e5494d2abb2fc4980 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 3 Nov 2022 15:38:23 +0000 Subject: [PATCH 17/19] Make black happy again --- src/textual/app.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index 9f9984359..d5c537926 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1350,9 +1350,7 @@ class App(Generic[ReturnType], DOMNode): # it's likely a good idea to keep it here to check assumptions in # the rest of the code. if before is not None and after is not None: - raise AppError( - "Only one of 'before' and 'after' may be specified." - ) + raise AppError("Only one of 'before' and 'after' may be specified.") # If we don't already know about this widget... if child not in self._registry: From 3a771fb3b123321633439be3047637998515bc9d Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 3 Nov 2022 15:43:57 +0000 Subject: [PATCH 18/19] Drop the return type of App._register_child Also add the docstring https://github.com/Textualize/textual/pull/1095#discussion_r1013034272 --- src/textual/app.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/textual/app.py b/src/textual/app.py index d5c537926..c93dd3eca 100644 --- a/src/textual/app.py +++ b/src/textual/app.py @@ -1342,7 +1342,16 @@ class App(Generic[ReturnType], DOMNode): def _register_child( self, parent: DOMNode, child: Widget, before: int | None, after: int | None - ) -> bool: + ) -> None: + """Register a widget as a child of another. + + Args: + parent (DOMNode): Parent node. + child (Widget): The child widget to register. + widgets: The widget to register. + before (int, optional): A location to mount before. + after (int, option): A location to mount after. + """ # Let's be 100% sure that we've not been asked to do a before and an # after at the same time. It's possible that we can remove this @@ -1378,9 +1387,6 @@ class App(Generic[ReturnType], DOMNode): child._attach(parent) child._post_register(self) child._start_messages() - return True - - return False def _register( self, From b0d2caf2219d037eec6efd8b64e29ddf1aa41730 Mon Sep 17 00:00:00 2001 From: Dave Pearson Date: Thu, 3 Nov 2022 15:49:15 +0000 Subject: [PATCH 19/19] Add unit tests for mounting relative to -2 To satisfy https://github.com/Textualize/textual/pull/1095#discussion_r1013041338 --- tests/test_widget_mounting.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/tests/test_widget_mounting.py b/tests/test_widget_mounting.py index 7b3d4769a..85d7d2da0 100644 --- a/tests/test_widget_mounting.py +++ b/tests/test_widget_mounting.py @@ -37,6 +37,20 @@ async def test_mount_via_app() -> None: await pilot.app.mount(ultimate, after=-1) assert pilot.app.screen.children[-1] == ultimate + async with App().run_test() as pilot: + # Mount a widget before -2, which is "before the penultimate". + penpenultimate = Static(id="penpenultimate") + await pilot.app.mount_all(widgets) + await pilot.app.mount(penpenultimate, before=-2) + assert pilot.app.screen.children[-3] == penpenultimate + + async with App().run_test() as pilot: + # Mount a widget after -2, which is "before the end". + penultimate = Static(id="penultimate") + await pilot.app.mount_all(widgets) + await pilot.app.mount(penultimate, after=-2) + assert pilot.app.screen.children[-2] == penultimate + async with App().run_test() as pilot: # Mount a widget before 0, which is "at the start". start = Static(id="start")