From be862376c6895dc50b6e238f127da379f3156926 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: Mon, 18 Dec 2023 15:44:23 +0000 Subject: [PATCH 1/5] Add overloads to move_child. This makes it so that typecheckers can warn users early about calling 'move_child' with no after/before or with both. We also make those two parameters keyword-only to increase readability. --- src/textual/widget.py | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/textual/widget.py b/src/textual/widget.py index 4de0709a38..2431aef011 100644 --- a/src/textual/widget.py +++ b/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: From a9444aadfd9876a4760fa70ef89e5f280fb44145 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: Mon, 18 Dec 2023 15:46:41 +0000 Subject: [PATCH 2/5] Fix move_child noop when target is child. This finishes the work done in #2530, which was incomplete as seen in https://github.com/Textualize/textual/issues/1743#issuecomment-1858132809. --- src/textual/widget.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/src/textual/widget.py b/src/textual/widget.py index 2431aef011..a4fa033e1c 100644 --- a/src/textual/widget.py +++ b/src/textual/widget.py @@ -941,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): @@ -969,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 From 5f85cf4bd4b146a3a15584fdda76af89fbeb0b32 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: Mon, 18 Dec 2023 15:46:56 +0000 Subject: [PATCH 3/5] Add tests. Co-authored-by: Jeff Epler --- CHANGELOG.md | 10 +++++ tests/test_widget_child_moving.py | 70 ++++++++++++++++++++++++------- 2 files changed, 64 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5303ec554f..e92ef7c57d 100644 --- a/CHANGELOG.md +++ b/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 + ## [0.46.0] - 2023-12-17 ### Fixed diff --git a/tests/test_widget_child_moving.py b/tests/test_widget_child_moving.py index f2d40a4aae..efb69e42e9 100644 --- a/tests/test_widget_child_moving.py +++ b/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,66 @@ 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( + ["type_of_child", "before"], + [ + ("widget", "widget"), + ("widget", "index"), + ("index", "widget"), + ("index", "index"), + ], +) +async def test_move_child_before_itself(type_of_child: str, before: str) -> None: + """Test moving a widget before itself. + + Regression test for https://github.com/Textualize/textual/issues/1743 + """ + + widget = Widget() + keys = { + "widget": widget, + "index": 0, + } + child = keys[type_of_child] + before = keys[before] async with App().run_test() as pilot: - child = Widget(Widget()) - await pilot.app.mount(child) - pilot.app.screen.move_child(child, before=child) - + await pilot.app.mount(widget) + pilot.app.screen.move_child(child, before=before) + + +@pytest.mark.parametrize( + ["type_of_child", "after"], + [ + ("widget", "widget"), + ("widget", "index"), + ("index", "widget"), + ("index", "index"), + ], +) +async def test_move_child_after_itself(type_of_child: str, after: str) -> None: + """Test moving a widget after itself. + + Regression test for https://github.com/Textualize/textual/issues/1743 + """ + + widget = Widget() + keys = { + "widget": widget, + "index": 0, + } + child = keys[type_of_child] + after = keys[after] -async def test_move_child_after_itself() -> None: - """Test moving a widget after itself.""" - # Regression test for https://github.com/Textualize/textual/issues/1743 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.mount(widget) + pilot.app.screen.move_child(child, after=after) async def test_move_past_end_of_child_list() -> None: From 40794371094602ecbe109f8529311d1b9e87b6ff 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: Mon, 18 Dec 2023 16:41:40 +0000 Subject: [PATCH 4/5] Link to PR in changelog. --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e92ef7c57d..1772024442 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,7 +13,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ### Changed -- Breaking change: `Widget.move_child` parameters `before` and `after` are now keyword-only +- 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 From e03de2765e2e495eddfaf1834e674863c3a27200 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: Wed, 20 Dec 2023 12:00:52 +0000 Subject: [PATCH 5/5] Refactor tests. Address review feedback in https://github.com/Textualize/textual/pull/3896#discussion_r1430711149. --- tests/test_widget_child_moving.py | 88 ++++++++++++++++++------------- 1 file changed, 52 insertions(+), 36 deletions(-) diff --git a/tests/test_widget_child_moving.py b/tests/test_widget_child_moving.py index efb69e42e9..f0ab58d303 100644 --- a/tests/test_widget_child_moving.py +++ b/tests/test_widget_child_moving.py @@ -43,59 +43,75 @@ async def test_move_child_to_outside() -> None: @pytest.mark.parametrize( - ["type_of_child", "before"], + "reference", [ - ("widget", "widget"), - ("widget", "index"), - ("index", "widget"), - ("index", "index"), + "before", + "after", ], ) -async def test_move_child_before_itself(type_of_child: str, before: str) -> None: - """Test moving a widget before itself. - - Regression test for https://github.com/Textualize/textual/issues/1743 - """ +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() - keys = { - "widget": widget, - "index": 0, - } - child = keys[type_of_child] - before = keys[before] - + child = 0 + kwargs = {reference: 0} async with App().run_test() as pilot: - await pilot.app.mount(widget) - pilot.app.screen.move_child(child, before=before) + await pilot.app.screen.mount(widget) + pilot.app.screen.move_child(child, **kwargs) # Shouldn't raise an error. @pytest.mark.parametrize( - ["type_of_child", "after"], + "reference", [ - ("widget", "widget"), - ("widget", "index"), - ("index", "widget"), - ("index", "index"), + "before", + "after", ], ) -async def test_move_child_after_itself(type_of_child: str, after: str) -> None: - """Test moving a widget after itself. +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: + await pilot.app.screen.mount(widget) + pilot.app.screen.move_child(child, **kwargs) # Shouldn't raise an error. - Regression test for https://github.com/Textualize/textual/issues/1743 - """ + +@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() - keys = { - "widget": widget, - "index": 0, - } - child = keys[type_of_child] - after = keys[after] + 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.mount(widget) - pilot.app.screen.move_child(child, after=after) + 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: