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

@@ -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.
+            before (int | str | Widget, optional): Optional location to mount before.
-            after (MountSpot, optional): Optional location to mount after.
+            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(
+        return self.screen.mount(*widgets, before=before, after=after)
-            self._register(self.screen, *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,
+        before: int | None = None,
-        after: MountSpot = 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.
+            before (int, optional): A location to mount before.
-            after (MountSpot, optional): Optional location to mount after.
+            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)

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))

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.
+            before (int | str | Widget, optional): Optional location to mount before.
-            after (MountSpot, optional): Optional location to mount after.
+            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:

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())