Merge pull request #3896 from Textualize/move-child-before-after-self…

…-noop

Move child acts as noop if trying to move before/after itself

CHANGELOG.md

@@ -5,6 +5,16 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## Unreleased
+
+### Fixed
+
+- `Widget.move_child` would break if `before`/`after` is set to the index of the widget in `child` https://github.com/Textualize/textual/issues/1743
+
+### Changed
+
+- Breaking change: `Widget.move_child` parameters `before` and `after` are now keyword-only https://github.com/Textualize/textual/pull/3896
+
 ## [0.46.0] - 2023-12-17
 
 ### Fixed

src/textual/widget.py

@@ -892,9 +892,30 @@ def mount_all(
         await_mount = self.mount(*widgets, before=before, after=after)
         return await_mount
 
+    @overload
+    def move_child(
+        self,
+        child: int | Widget,
+        *,
+        before: int | Widget,
+        after: None = None,
+    ) -> None:
+        ...
+
+    @overload
+    def move_child(
+        self,
+        child: int | Widget,
+        *,
+        after: int | Widget,
+        before: None = None,
+    ) -> None:
+        ...
+
     def move_child(
         self,
         child: int | Widget,
+        *,
         before: int | Widget | None = None,
         after: int | Widget | None = None,
     ) -> None:
@@ -920,10 +941,6 @@ def move_child(
         elif before is not None and after is not None:
             raise WidgetError("Only one of `before` or `after` can be handled.")
 
-        # We short-circuit the no-op, otherwise it will error later down the road.
-        if child is before or child is after:
-            return
-
         def _to_widget(child: int | Widget, called: str) -> Widget:
             """Ensure a given child reference is a Widget."""
             if isinstance(child, int):
@@ -948,6 +965,9 @@ def _to_widget(child: int | Widget, called: str) -> Widget:
             cast("int | Widget", before if after is None else after), "move towards"
         )
 
+        if child is target:
+            return  # Nothing to be done.
+
         # At this point we should know what we're moving, and it should be a
         # child; where we're moving it to, which should be within the child
         # list; and how we're supposed to move it. All that's left is doing

tests/test_widget_child_moving.py

@@ -9,7 +9,7 @@
 async def test_move_child_no_direction() -> None:
     """Test moving a widget in a child list."""
     async with App().run_test() as pilot:
-        child = Widget(Widget())
+        child = Widget()
         await pilot.app.mount(child)
         with pytest.raises(WidgetError):
             pilot.app.screen.move_child(child)
@@ -18,7 +18,7 @@ async def test_move_child_no_direction() -> None:
 async def test_move_child_both_directions() -> None:
     """Test calling move_child with more than one direction."""
     async with App().run_test() as pilot:
-        child = Widget(Widget())
+        child = Widget()
         await pilot.app.mount(child)
         with pytest.raises(WidgetError):
             pilot.app.screen.move_child(child, before=1, after=2)
@@ -27,7 +27,7 @@ async def test_move_child_both_directions() -> None:
 async def test_move_child_not_our_child() -> None:
     """Test attempting to move a child that isn't ours."""
     async with App().run_test() as pilot:
-        child = Widget(Widget())
+        child = Widget()
         await pilot.app.mount(child)
         with pytest.raises(WidgetError):
             pilot.app.screen.move_child(Widget(), before=child)
@@ -36,28 +36,82 @@ async def test_move_child_not_our_child() -> None:
 async def test_move_child_to_outside() -> None:
     """Test attempting to move relative to a widget that isn't a child."""
     async with App().run_test() as pilot:
-        child = Widget(Widget())
+        child = Widget()
         await pilot.app.mount(child)
         with pytest.raises(WidgetError):
             pilot.app.screen.move_child(child, before=Widget())
 
 
-async def test_move_child_before_itself() -> None:
-    """Test moving a widget before itself."""
+@pytest.mark.parametrize(
+    "reference",
+    [
+        "before",
+        "after",
+    ],
+)
+async def test_move_child_index_in_relation_to_itself_index(reference: str) -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/1743"""
 
+    widget = Widget()
+    child = 0
+    kwargs = {reference: 0}
     async with App().run_test() as pilot:
-        child = Widget(Widget())
-        await pilot.app.mount(child)
-        pilot.app.screen.move_child(child, before=child)
-
-
-async def test_move_child_after_itself() -> None:
-    """Test moving a widget after itself."""
-    # Regression test for https://github.com/Textualize/textual/issues/1743
+        await pilot.app.screen.mount(widget)
+        pilot.app.screen.move_child(child, **kwargs)  # Shouldn't raise an error.
+
+
+@pytest.mark.parametrize(
+    "reference",
+    [
+        "before",
+        "after",
+    ],
+)
+async def test_move_child_index_in_relation_to_itself_widget(reference: str) -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/1743"""
+
+    widget = Widget()
+    child = 0
+    kwargs = {reference: widget}
     async with App().run_test() as pilot:
-        child = Widget(Widget())
-        await pilot.app.mount(child)
-        pilot.app.screen.move_child(child, after=child)
+        await pilot.app.screen.mount(widget)
+        pilot.app.screen.move_child(child, **kwargs)  # Shouldn't raise an error.
+
+
+@pytest.mark.parametrize(
+    "reference",
+    [
+        "before",
+        "after",
+    ],
+)
+async def test_move_child_widget_in_relation_to_itself_index(reference: str) -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/1743"""
+
+    widget = Widget()
+    child = widget
+    kwargs = {reference: 0}
+    async with App().run_test() as pilot:
+        await pilot.app.screen.mount(widget)
+        pilot.app.screen.move_child(child, **kwargs)  # Shouldn't raise an error.
+
+
+@pytest.mark.parametrize(
+    "reference",
+    [
+        "before",
+        "after",
+    ],
+)
+async def test_move_child_widget_in_relation_to_itself_widget(reference: str) -> None:
+    """Regression test for https://github.com/Textualize/textual/issues/1743"""
+
+    widget = Widget()
+    child = widget
+    kwargs = {reference: widget}
+    async with App().run_test() as pilot:
+        await pilot.app.screen.mount(widget)
+        pilot.app.screen.move_child(child, **kwargs)  # Shouldn't raise an error.
 
 
 async def test_move_past_end_of_child_list() -> None: