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

fix refresh on remove #2008

Merged
merged 5 commits into from
Mar 10, 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
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,12 @@ 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

- Fixed container not resizing when a widget is removed https://github.com/Textualize/textual/issues/2007

## [0.14.0] - 2023-03-09

### Changed
Expand Down
5 changes: 3 additions & 2 deletions src/textual/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -2175,7 +2175,7 @@ def _walk_children(self, root: Widget) -> Iterable[list[Widget]]:
for child in widget._nodes:
push(child)

def _remove_nodes(self, widgets: list[Widget]) -> AwaitRemove:
def _remove_nodes(self, widgets: list[Widget], parent: DOMNode) -> AwaitRemove:
"""Remove nodes from DOM, and return an awaitable that awaits cleanup.

Args:
Expand All @@ -2198,7 +2198,8 @@ async def prune_widgets_task(
await self._prune_nodes(widgets)
finally:
finished_event.set()
self.refresh(layout=True)
if parent.styles.auto_dimensions:
parent.refresh(layout=True)
Comment on lines -2201 to +2202
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't we need to refresh the layout of the parent, regardless of whether they have auto dimensions or not?
Imagine the parent has fixed dimensions but the children have relative dimensions (e.g., fr units).
Don't we need the refresh to make sure the children make use of the space that was just made available?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

auto dimensions and relative units kind of cancel each other out.

Consider a container with auto height and a single child with height 1fr. The widget needs to know the height of the container, but the container needs to know the height of the child. So auto height considers all relative dimensions to be zero. It will still respect minimums, so essentially what it calculates is the minimal value which satisfies all constraints.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's interesting.
Up until now, I interpreted 1fr to mean that it would expand as much as possible while also respecting relative ratios to other widgets with fr dimensions.
Then, if the parent has auto and the child has 1fr, the child should look at the grandparent to find the maximum size it can occupy and the parent would expand to that size.
This is the behaviour I find more consistent with the interaction between absolute sizes and 1fr.
If a parent has a fixed size and the child has 1fr, the child will expand as much as possible. Hence, I have been taking the "expand as much as possible" as the interpretation of 1fr, which I don't think should fall apart when the parent is auto.


removed_widgets = self._detach_from_dom(widgets)

Expand Down
2 changes: 1 addition & 1 deletion src/textual/css/query.py
Original file line number Diff line number Diff line change
Expand Up @@ -355,7 +355,7 @@ def remove(self) -> AwaitRemove:
An awaitable object that waits for the widgets to be removed.
"""
app = active_app.get()
await_remove = app._remove_nodes(list(self))
await_remove = app._remove_nodes(list(self), self._node)
return await_remove

def set_styles(
Expand Down
2 changes: 1 addition & 1 deletion src/textual/widget.py
Original file line number Diff line number Diff line change
Expand Up @@ -2548,7 +2548,7 @@ def remove(self) -> AwaitRemove:
An awaitable object that waits for the widget to be removed.
"""

await_remove = self.app._remove_nodes([self])
await_remove = self.app._remove_nodes([self], self.parent)
return await_remove

def render(self) -> RenderableType:
Expand Down
160 changes: 160 additions & 0 deletions tests/snapshot_tests/__snapshots__/test_snapshots.ambr

Large diffs are not rendered by default.

38 changes: 38 additions & 0 deletions tests/snapshot_tests/snapshot_apps/remove_auto.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
from textual.app import App, ComposeResult
from textual.containers import Vertical
from textual.widgets import Header, Footer, Label


class VerticalRemoveApp(App[None]):
CSS = """
Vertical {
border: round green;
height: auto;
}

Label {
border: round yellow;
background: red;
color: yellow;
}
"""
BINDINGS = [
("a", "add", "Add"),
("d", "del", "Delete"),
]

def compose(self) -> ComposeResult:
yield Header()
yield Vertical()
yield Footer()

def action_add(self) -> None:
self.query_one(Vertical).mount(Label("This is a test label"))

def action_del(self) -> None:
if self.query_one(Vertical).children:
self.query_one(Vertical).children[-1].remove()


if __name__ == "__main__":
VerticalRemoveApp().run()
15 changes: 12 additions & 3 deletions tests/snapshot_tests/test_snapshots.py
Original file line number Diff line number Diff line change
Expand Up @@ -170,9 +170,11 @@ def test_content_switcher_example_initial(snap_compare):


def test_content_switcher_example_switch(snap_compare):
assert snap_compare(WIDGET_EXAMPLES_DIR / "content_switcher.py", press=[
"tab", "tab", "enter", "wait:500"
], terminal_size=(50, 50))
assert snap_compare(
WIDGET_EXAMPLES_DIR / "content_switcher.py",
press=["tab", "tab", "enter", "wait:500"],
terminal_size=(50, 50),
)


# --- CSS properties ---
Expand Down Expand Up @@ -234,6 +236,7 @@ def test_programmatic_scrollbar_gutter_change(snap_compare):
# --- CLI Preview Apps ---
# For our CLI previews e.g. `textual easing`, `textual colors` etc, we have snapshots


def test_borders_preview(snap_compare):
assert snap_compare(CLI_PREVIEWS_DIR / "borders.py", press=["tab", "enter"])

Expand Down Expand Up @@ -296,3 +299,9 @@ def test_focus_component_class(snap_compare):

def test_line_api_scrollbars(snap_compare):
assert snap_compare(SNAPSHOT_APPS_DIR / "line_api_scrollbars.py")


def test_remove_with_auto_height(snap_compare):
assert snap_compare(
SNAPSHOT_APPS_DIR / "remove_auto.py", press=["a", "a", "a", "d", "d"]
)