Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Moving child before/after self is a no-op. #2530

Merged
merged 4 commits into from
May 15, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
### Changed

- App `title` and `sub_title` attributes can be set to any type https://github.com/Textualize/textual/issues/2521
- Using `Widget.move_child` where the target and the child being moved are the same is now a no-op https://github.com/Textualize/textual/issues/1743

### Fixed

Expand Down
16 changes: 8 additions & 8 deletions src/textual/widget.py
Original file line number Diff line number Diff line change
Expand Up @@ -795,19 +795,15 @@ def move_child(

Args:
child: The child widget to move.
before: Optional location to move before. An `int` is the index
of the child to move before, a `str` is a `query_one` query to
find the widget to move before.
after: Optional location to move after. An `int` is the index
of the child to move after, a `str` is a `query_one` query to
find the widget to move after.
before: Child widget or location index to move before.
after: Child widget or location index to move after.

Raises:
WidgetError: If there is a problem with the child or target.

Note:
Only one of ``before`` or ``after`` can be provided. If neither
or both are provided a ``WidgetError`` will be raised.
Only one of `before` or `after` can be provided. If neither
or both are provided a `WidgetError` will be raised.
"""

# One or the other of before or after are required. Can't do
Expand All @@ -817,6 +813,10 @@ 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps a comment for future generations, to explain this is a null-op

return

def _to_widget(child: int | Widget, called: str) -> Widget:
"""Ensure a given child reference is a Widget."""
if isinstance(child, int):
Expand Down
8 changes: 2 additions & 6 deletions tests/test_widget_child_moving.py
Original file line number Diff line number Diff line change
Expand Up @@ -42,22 +42,18 @@ async def test_move_child_to_outside() -> None:
pilot.app.screen.move_child(child, before=Widget())


@pytest.mark.xfail(
strict=True, reason="https://github.com/Textualize/textual/issues/1743"
)
async def test_move_child_before_itself() -> None:
"""Test moving a widget before itself."""

async with App().run_test() as pilot:
child = Widget(Widget())
await pilot.app.mount(child)
pilot.app.screen.move_child(child, before=child)


@pytest.mark.xfail(
strict=True, reason="https://github.com/Textualize/textual/issues/1743"
)
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)
Expand Down