From a09773f436b3eff793a693922c528faa8151d477 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: Thu, 31 Aug 2023 14:03:44 +0100 Subject: [PATCH 01/14] DataTable new rows can have auto height. Related issue: #3122. --- src/textual/widgets/_data_table.py | 45 ++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 6 deletions(-) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index 6e7e3e543f..950467937d 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -187,6 +187,7 @@ class Row: key: RowKey height: int label: Text | None = None + auto_height: bool = False class RowRenderables(NamedTuple): @@ -1190,8 +1191,16 @@ def _update_column_widths(self, updated_cells: set[CellKey]) -> None: self._require_update_dimensions = True def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None: - """Called to recalculate the virtual (scrollable) size.""" + """Called to recalculate the virtual (scrollable) size. + + This recomputes column widths and then checks if any of the new rows need + to have their height computed. + + Args: + new_rows: The new rows that will affect the `DataTable` dimensions. + """ console = self.app.console + auto_height_rows: list[tuple[Row, list[RenderableType]]] = [] for row_key in new_rows: row_index = self._row_locations.get(row_key) @@ -1201,6 +1210,7 @@ def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None: continue row = self.rows.get(row_key) + assert row is not None if row.label is not None: self._labelled_row_exists = True @@ -1215,6 +1225,22 @@ def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None: content_width = measure(console, renderable, 1) column.content_width = max(column.content_width, content_width) + if row.auto_height: + auto_height_rows.append((row, cells_in_row)) + + for row, cells_in_row in auto_height_rows: + height = 0 + for column, renderable in zip(self.ordered_columns, cells_in_row): + height = max( + height, + len( + console.render_lines( + renderable, console.options.update_width(column.width) + ) + ), + ) + row.height = height + self._clear_caches() data_cells_width = sum(column.render_width for column in self.columns.values()) @@ -1373,7 +1399,7 @@ def add_column( def add_row( self, *cells: CellType, - height: int = 1, + height: int | None = 1, key: str | None = None, label: TextType | None = None, ) -> RowKey: @@ -1381,13 +1407,14 @@ def add_row( Args: *cells: Positional arguments should contain cell data. - height: The height of a row (in lines). + height: The height of a row (in lines). Use `None` to auto-detect the optimal + height. key: A key which uniquely identifies this row. If None, it will be generated for you and returned. label: The label for the row. Will be displayed to the left if supplied. Returns: - Uniquely identifies this row. Can be used to retrieve this row regardless + Unique identifier for this row. Can be used to retrieve this row regardless of its current location in the DataTable (it could have moved after being added due to sorting or insertion/deletion of other rows). """ @@ -1407,7 +1434,12 @@ def add_row( for column, cell in zip_longest(self.ordered_columns, cells) } label = Text.from_markup(label) if isinstance(label, str) else label - self.rows[row_key] = Row(row_key, height, label) + self.rows[row_key] = Row( + row_key, + height if height is not None else 1, + label, + height is None, + ) self._new_rows.add(row_key) self._require_update_dimensions = True self.cursor_coordinate = self.cursor_coordinate @@ -1546,7 +1578,8 @@ async def _on_idle(self, _: events.Idle) -> None: if self._require_update_dimensions: # Add the new rows *before* updating the column widths, since - # cells in a new row may influence the final width of a column + # cells in a new row may influence the final width of a column. + # Only then can we compute optimal height of rows with "auto" height. self._require_update_dimensions = False new_rows = self._new_rows.copy() self._new_rows.clear() From c3904e5c693cab73d759c6dbb1323d1a513aa3fc 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: Thu, 31 Aug 2023 14:05:35 +0100 Subject: [PATCH 02/14] Test auto height computation in DataTable.add_row --- CHANGELOG.md | 1 + tests/test_data_table.py | 60 +++++++++++++++++++++++++++------------- 2 files changed, 42 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c3c5ce003f..8939ba7d70 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Reactive callbacks are now scheduled on the message pump of the reactable that is watching instead of the owner of reactive attribute https://github.com/Textualize/textual/pull/3065 - Callbacks scheduled with `call_next` will now have the same prevented messages as when the callback was scheduled https://github.com/Textualize/textual/pull/3065 - Added `cursor_type` to the `DataTable` constructor. +- `DataTable.add_row` accepts `height=None` to automatically compute optimal height for a row https://github.com/Textualize/textual/pull/3213 ### Fixed diff --git a/tests/test_data_table.py b/tests/test_data_table.py index e00b9432a4..763f624a27 100644 --- a/tests/test_data_table.py +++ b/tests/test_data_table.py @@ -1,6 +1,7 @@ from __future__ import annotations import pytest +from rich.panel import Panel from rich.text import Text from textual._wait import wait_for_idle @@ -419,11 +420,11 @@ async def test_get_cell_coordinate_returns_coordinate(): table.add_row("ValR2C1", "ValR2C2", "ValR2C3", key="R2") table.add_row("ValR3C1", "ValR3C2", "ValR3C3", key="R3") - assert table.get_cell_coordinate('R1', 'C1') == Coordinate(0, 0) - assert table.get_cell_coordinate('R2', 'C2') == Coordinate(1, 1) - assert table.get_cell_coordinate('R1', 'C3') == Coordinate(0, 2) - assert table.get_cell_coordinate('R3', 'C1') == Coordinate(2, 0) - assert table.get_cell_coordinate('R3', 'C2') == Coordinate(2, 1) + assert table.get_cell_coordinate("R1", "C1") == Coordinate(0, 0) + assert table.get_cell_coordinate("R2", "C2") == Coordinate(1, 1) + assert table.get_cell_coordinate("R1", "C3") == Coordinate(0, 2) + assert table.get_cell_coordinate("R3", "C1") == Coordinate(2, 0) + assert table.get_cell_coordinate("R3", "C2") == Coordinate(2, 1) async def test_get_cell_coordinate_invalid_row_key(): @@ -434,7 +435,7 @@ async def test_get_cell_coordinate_invalid_row_key(): table.add_row("TargetValue", key="R1") with pytest.raises(CellDoesNotExist): - coordinate = table.get_cell_coordinate('INVALID_ROW', 'C1') + coordinate = table.get_cell_coordinate("INVALID_ROW", "C1") async def test_get_cell_coordinate_invalid_column_key(): @@ -445,7 +446,7 @@ async def test_get_cell_coordinate_invalid_column_key(): table.add_row("TargetValue", key="R1") with pytest.raises(CellDoesNotExist): - coordinate = table.get_cell_coordinate('R1', 'INVALID_COLUMN') + coordinate = table.get_cell_coordinate("R1", "INVALID_COLUMN") async def test_get_cell_at_returns_value_at_cell(): @@ -531,9 +532,9 @@ async def test_get_row_index_returns_index(): table.add_row("ValR2C1", "ValR2C2", key="R2") table.add_row("ValR3C1", "ValR3C2", key="R3") - assert table.get_row_index('R1') == 0 - assert table.get_row_index('R2') == 1 - assert table.get_row_index('R3') == 2 + assert table.get_row_index("R1") == 0 + assert table.get_row_index("R2") == 1 + assert table.get_row_index("R3") == 2 async def test_get_row_index_invalid_row_key(): @@ -544,7 +545,7 @@ async def test_get_row_index_invalid_row_key(): table.add_row("TargetValue", key="R1") with pytest.raises(RowDoesNotExist): - index = table.get_row_index('InvalidRow') + index = table.get_row_index("InvalidRow") async def test_get_column(): @@ -591,6 +592,7 @@ async def test_get_column_at_invalid_index(index): with pytest.raises(ColumnDoesNotExist): list(table.get_column_at(index)) + async def test_get_column_index_returns_index(): app = DataTableApp() async with app.run_test(): @@ -598,12 +600,12 @@ async def test_get_column_index_returns_index(): table.add_column("Column1", key="C1") table.add_column("Column2", key="C2") table.add_column("Column3", key="C3") - table.add_row("ValR1C1", "ValR1C2", "ValR1C3", key="R1") - table.add_row("ValR2C1", "ValR2C2", "ValR2C3", key="R2") + table.add_row("ValR1C1", "ValR1C2", "ValR1C3", key="R1") + table.add_row("ValR2C1", "ValR2C2", "ValR2C3", key="R2") - assert table.get_column_index('C1') == 0 - assert table.get_column_index('C2') == 1 - assert table.get_column_index('C3') == 2 + assert table.get_column_index("C1") == 0 + assert table.get_column_index("C2") == 1 + assert table.get_column_index("C3") == 2 async def test_get_column_index_invalid_column_key(): @@ -613,11 +615,10 @@ async def test_get_column_index_invalid_column_key(): table.add_column("Column1", key="C1") table.add_column("Column2", key="C2") table.add_column("Column3", key="C3") - table.add_row("TargetValue1", "TargetValue2", "TargetValue3", key="R1") + table.add_row("TargetValue1", "TargetValue2", "TargetValue3", key="R1") with pytest.raises(ColumnDoesNotExist): - index = table.get_column_index('InvalidCol') - + index = table.get_column_index("InvalidCol") async def test_update_cell_cell_exists(): @@ -1161,3 +1162,24 @@ async def test_unset_hover_highlight_when_no_table_cell_under_mouse(): # the widget, and the hover cursor is hidden await pilot.hover(DataTable, offset=Offset(42, 1)) assert not table._show_hover_cursor + + +@pytest.mark.parametrize( + ["cell", "height"], + [ + ("hey there", 1), + (Text("hey there"), 1), + (Text("long string", overflow="fold"), 2), + (Panel.fit("Hello\nworld"), 4), + ("1\n2\n3\n4\n5\n6\n7", 7), + ], +) +async def test_add_row_auto_height(cell: RenderableType, height: int): + app = DataTableApp() + async with app.run_test() as pilot: + table = app.query_one(DataTable) + table.add_column("C", width=10) + row_key = table.add_row(cell, height=None) + row = table.rows.get(row_key) + await pilot.pause() + assert row.height == height From 65aaf397a5aeab53e183c6ac567271825115ab6c 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: Thu, 31 Aug 2023 16:10:15 +0100 Subject: [PATCH 03/14] Add snapshot test for add_row height=None. --- .../__snapshots__/test_snapshots.ambr | 158 ++++++++++++++++++ .../data_table_add_row_auto_height.py | 21 +++ tests/snapshot_tests/test_snapshots.py | 5 + 3 files changed, 184 insertions(+) create mode 100644 tests/snapshot_tests/snapshot_apps/data_table_add_row_auto_height.py diff --git a/tests/snapshot_tests/__snapshots__/test_snapshots.ambr b/tests/snapshot_tests/__snapshots__/test_snapshots.ambr index bd2ffa863b..4e25c90d5d 100644 --- a/tests/snapshot_tests/__snapshots__/test_snapshots.ambr +++ b/tests/snapshot_tests/__snapshots__/test_snapshots.ambr @@ -12731,6 +12731,164 @@ ''' # --- +# name: test_datatable_add_row_auto_height + ''' + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + AutoHeightRowsApp + + + + + + + + + +  Column      +  hey there   +  hey there   +  long        +  string      +  ╭───────╮   +  │ Hello │   +  │ world │   +  ╰───────╯   +  1           +  2           +  3           +  4           +  5           +  6           +  7           + + + + + + + + + + + + + ''' +# --- # name: test_datatable_column_cursor_render ''' diff --git a/tests/snapshot_tests/snapshot_apps/data_table_add_row_auto_height.py b/tests/snapshot_tests/snapshot_apps/data_table_add_row_auto_height.py new file mode 100644 index 0000000000..a9b8c1c17b --- /dev/null +++ b/tests/snapshot_tests/snapshot_apps/data_table_add_row_auto_height.py @@ -0,0 +1,21 @@ +from rich.panel import Panel +from rich.text import Text + +from textual.app import App +from textual.widgets import DataTable + + +class AutoHeightRowsApp(App[None]): + def compose(self): + table = DataTable() + table.add_column("Column", width=10) + table.add_row("hey there", height=None) + table.add_row(Text("hey there"), height=None) + table.add_row(Text("long string", overflow="fold"), height=None) + table.add_row(Panel.fit("Hello\nworld"), height=None) + table.add_row("1\n2\n3\n4\n5\n6\n7", height=None) + yield table + + +if __name__ == "__main__": + AutoHeightRowsApp().run() diff --git a/tests/snapshot_tests/test_snapshots.py b/tests/snapshot_tests/test_snapshots.py index 36b003bb7e..34e0aeb40c 100644 --- a/tests/snapshot_tests/test_snapshots.py +++ b/tests/snapshot_tests/test_snapshots.py @@ -148,6 +148,11 @@ def test_datatable_add_column(snap_compare): assert snap_compare(SNAPSHOT_APPS_DIR / "data_table_add_column.py") +def test_datatable_add_row_auto_height(snap_compare): + # Check that rows added with auto height computation look right. + assert snap_compare(SNAPSHOT_APPS_DIR / "data_table_add_row_auto_height.py") + + def test_footer_render(snap_compare): assert snap_compare(WIDGET_EXAMPLES_DIR / "footer.py") From faf039255e8de483246ebde7e858e520d2d5ae57 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, 4 Sep 2023 14:40:19 +0100 Subject: [PATCH 04/14] Extract some styles logic into auxiliary methods. When adding a row with automatic height, I need to render the cells to compute their height. Instead of wasting that rendering, I want to do it well and then cache it, which means I need to reuse some of the logic of the other rendering methods. By extracting some logic, I'll be able to reuse it. --- src/textual/widgets/_data_table.py | 132 +++++++++++++++++++---------- 1 file changed, 85 insertions(+), 47 deletions(-) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index 950467937d..8785f20140 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -1800,7 +1800,6 @@ def _render_cell( if cell_cache_key not in self._cell_render_cache: base_style += Style.from_meta({"row": row_index, "column": column_index}) - height = self.header_height if is_header_cell else self.rows[row_key].height row_label, row_cells = self._get_row_renderables(row_index) if is_row_label_cell: @@ -1839,13 +1838,25 @@ def _render_cell( else Style.null() ) + if is_header_cell: + options = self.app.console.options.update_dimensions( + width, self.header_height + ) + else: + row = self.rows[row_key] + if row.auto_height: + options = self.app.console.options.update_width(width) + else: + options = self.app.console.options.update_dimensions( + width, row.height + ) lines = self.app.console.render_lines( Styled( Padding(cell, (0, 1)), pre_style=base_style + component_style, post_style=post_foreground + post_background, ), - self.app.console.options.update_dimensions(width, height), + options, ) self._cell_render_cache[cell_cache_key] = lines @@ -1892,29 +1903,9 @@ def _render_line_in_row( if cache_key in self._row_render_cache: return self._row_render_cache[cache_key] - def _should_highlight( - cursor: Coordinate, - target_cell: Coordinate, - type_of_cursor: CursorType, - ) -> bool: - """Determine whether we should highlight a cell given the location - of the cursor, the location of the cell, and the type of cursor that - is currently active.""" - if type_of_cursor == "cell": - return cursor == target_cell - elif type_of_cursor == "row": - cursor_row, _ = cursor - cell_row, _ = target_cell - return cursor_row == cell_row - elif type_of_cursor == "column": - _, cursor_column = cursor - _, cell_column = target_cell - return cursor_column == cell_column - else: - return False - - is_header_row = row_key is self._header_row_key + should_highlight = self._should_highlight render_cell = self._render_cell + header_style = self.get_component_styles("datatable--header").rich_style if row_key in self._row_locations: row_index = self._row_locations.get(row_key) @@ -1923,7 +1914,6 @@ def _should_highlight( # If the row has a label, add it to fixed_row here with correct style. fixed_row = [] - header_style = self.get_component_styles("datatable--header").rich_style if self._labelled_row_exists and self.show_row_labels: # The width of the row label is updated again on idle @@ -1933,14 +1923,17 @@ def _should_highlight( -1, header_style, width=self._row_label_column_width, - cursor=_should_highlight(cursor_location, cell_location, cursor_type), - hover=_should_highlight(hover_location, cell_location, cursor_type), + cursor=should_highlight(cursor_location, cell_location, cursor_type), + hover=should_highlight(hover_location, cell_location, cursor_type), )[line_no] fixed_row.append(label_cell_lines) if self.fixed_columns: - fixed_style = self.get_component_styles("datatable--fixed").rich_style - fixed_style += Style.from_meta({"fixed": True}) + if row_key is self._header_row_key: + fixed_style = header_style # We use the header style either way. + else: + fixed_style = self.get_component_styles("datatable--fixed").rich_style + fixed_style += Style.from_meta({"fixed": True}) for column_index, column in enumerate( self.ordered_columns[: self.fixed_columns] ): @@ -1948,28 +1941,16 @@ def _should_highlight( fixed_cell_lines = render_cell( row_index, column_index, - header_style if is_header_row else fixed_style, + fixed_style, column.render_width, - cursor=_should_highlight( + cursor=should_highlight( cursor_location, cell_location, cursor_type ), - hover=_should_highlight(hover_location, cell_location, cursor_type), + hover=should_highlight(hover_location, cell_location, cursor_type), )[line_no] fixed_row.append(fixed_cell_lines) - is_header_row = row_key is self._header_row_key - if is_header_row: - row_style = self.get_component_styles("datatable--header").rich_style - elif row_index < self.fixed_rows: - row_style = self.get_component_styles("datatable--fixed").rich_style - else: - if self.zebra_stripes: - component_row_style = ( - "datatable--odd-row" if row_index % 2 else "datatable--even-row" - ) - row_style = self.get_component_styles(component_row_style).rich_style - else: - row_style = base_style + row_style = self._get_row_style(row_index, base_style) scrollable_row = [] for column_index, column in enumerate(self.ordered_columns): @@ -1979,8 +1960,8 @@ def _should_highlight( column_index, row_style, column.render_width, - cursor=_should_highlight(cursor_location, cell_location, cursor_type), - hover=_should_highlight(hover_location, cell_location, cursor_type), + cursor=should_highlight(cursor_location, cell_location, cursor_type), + hover=should_highlight(hover_location, cell_location, cursor_type), )[line_no] scrollable_row.append(cell_lines) @@ -2108,6 +2089,63 @@ def render_line(self, y: int) -> Strip: return self._render_line(y, scroll_x, scroll_x + width, self.rich_style) + def _should_highlight( + self, + cursor: Coordinate, + target_cell: Coordinate, + type_of_cursor: CursorType, + ) -> bool: + """Determine if the given cell should be highlighted because of the cursor. + + This auxiliary method takes the cursor position and type into account when + determining whether the cell should be highlighted. + + Args: + cursor: The current position of the cursor. + target_cell: The cell we're checking for the need to highlight. + type_of_cursor: The type of cursor that is currently active. + + Returns: + Whether or not the given cell should be highlighted. + """ + if type_of_cursor == "cell": + return cursor == target_cell + elif type_of_cursor == "row": + cursor_row, _ = cursor + cell_row, _ = target_cell + return cursor_row == cell_row + elif type_of_cursor == "column": + _, cursor_column = cursor + _, cell_column = target_cell + return cursor_column == cell_column + else: + return False + + def _get_row_style(self, row_index: int, base_style: Style) -> Style: + """Gets the Style that should be applied to the row at the given index. + + Args: + row_index: The index of the row to style. + base_style: The base style to use by default. + + Returns: + The appropriate style. + """ + + if row_index == -1: + row_style = self.get_component_styles("datatable--header").rich_style + elif row_index < self.fixed_rows: + row_style = self.get_component_styles("datatable--fixed").rich_style + else: + if self.zebra_stripes: + component_row_style = ( + "datatable--odd-row" if row_index % 2 else "datatable--even-row" + ) + row_style = self.get_component_styles(component_row_style).rich_style + else: + row_style = base_style + return row_style + def _on_mouse_move(self, event: events.MouseMove): """If the hover cursor is visible, display it by extracting the row and column metadata from the segments present in the cells.""" From c9ae50d255db105e15d244fc002aae8f753a3f56 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, 4 Sep 2023 15:25:18 +0100 Subject: [PATCH 05/14] Cache auxiliary cell renderings. --- src/textual/widgets/_data_table.py | 55 ++++++++++++++++++++++-------- 1 file changed, 40 insertions(+), 15 deletions(-) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index 8785f20140..c0c0df31dd 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -1200,7 +1200,7 @@ def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None: new_rows: The new rows that will affect the `DataTable` dimensions. """ console = self.app.console - auto_height_rows: list[tuple[Row, list[RenderableType]]] = [] + auto_height_rows: list[tuple[int, Row, list[RenderableType]]] = [] for row_key in new_rows: row_index = self._row_locations.get(row_key) @@ -1226,23 +1226,48 @@ def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None: column.content_width = max(column.content_width, content_width) if row.auto_height: - auto_height_rows.append((row, cells_in_row)) - - for row, cells_in_row in auto_height_rows: - height = 0 - for column, renderable in zip(self.ordered_columns, cells_in_row): - height = max( - height, - len( - console.render_lines( - renderable, console.options.update_width(column.width) - ) - ), - ) - row.height = height + auto_height_rows.append((row_index, row, cells_in_row)) self._clear_caches() + # If there are rows that need to have their height computed, render them correctly + # so that we can cache this rendering for later. + if auto_height_rows: + render_cell = self._render_cell # This method renders & caches. + should_highlight = self._should_highlight + cursor_type = self.cursor_type + cursor_location = self.cursor_coordinate + hover_location = self.hover_coordinate + base_style = self.rich_style + fixed_style = self.get_component_styles( + "datatable--fixed" + ).rich_style + Style.from_meta({"fixed": True}) + ordered_columns = self.ordered_columns + fixed_columns = self.fixed_columns + + for row_index, row, cells_in_row in auto_height_rows: + height = 0 + row_style = self._get_row_style(row_index, base_style) + + for column_index, column in enumerate(ordered_columns): + style = fixed_style if column_index < fixed_columns else row_style + cell_location = Coordinate(row_index, column_index) + rendered_cell = render_cell( + row_index, + column_index, + style, + column.render_width, + cursor=should_highlight( + cursor_location, cell_location, cursor_type + ), + hover=should_highlight( + hover_location, cell_location, cursor_type + ), + ) + height = max(height, len(rendered_cell)) + + row.height = height + data_cells_width = sum(column.render_width for column in self.columns.values()) total_width = data_cells_width + self._row_label_column_width header_height = self.header_height if self.show_header else 0 From 81d94fa6d988079e6074a1644ac0967f96205a51 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, 4 Sep 2023 15:26:52 +0100 Subject: [PATCH 06/14] Fix test import. --- tests/test_data_table.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_data_table.py b/tests/test_data_table.py index 763f624a27..4433967c4b 100644 --- a/tests/test_data_table.py +++ b/tests/test_data_table.py @@ -6,7 +6,7 @@ from textual._wait import wait_for_idle from textual.actions import SkipAction -from textual.app import App +from textual.app import App, RenderableType from textual.coordinate import Coordinate from textual.geometry import Offset from textual.message import Message From 35ae591b688421ad7f8147ccc593e69523376d34 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: Tue, 5 Sep 2023 15:56:12 +0100 Subject: [PATCH 07/14] Set row height to 0 when adding auto-height row. --- src/textual/widgets/_data_table.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index 1d9aa02558..74ff07d9b9 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -1459,9 +1459,12 @@ def add_row( for column, cell in zip_longest(self.ordered_columns, cells) } label = Text.from_markup(label) if isinstance(label, str) else label + # Rows with auto-height get a height of 0 because 1) we need an integer height + # to do some intermediate computations and 2) because 0 doesn't impact the data + # table while we don't figure out how tall this row is. self.rows[row_key] = Row( row_key, - height if height is not None else 1, + height or 0, label, height is None, ) From 23367572471c955db47a2b6c47e0505ef4e5257a 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, 6 Sep 2023 11:56:37 +0100 Subject: [PATCH 08/14] Remove superfluous cache clear. --- src/textual/widgets/_data_table.py | 2 -- tests/test_data_table.py | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index 74ff07d9b9..ae8e88c6d4 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -1228,8 +1228,6 @@ def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None: if row.auto_height: auto_height_rows.append((row_index, row, cells_in_row)) - self._clear_caches() - # If there are rows that need to have their height computed, render them correctly # so that we can cache this rendering for later. if auto_height_rows: diff --git a/tests/test_data_table.py b/tests/test_data_table.py index 4433967c4b..8c10c38463 100644 --- a/tests/test_data_table.py +++ b/tests/test_data_table.py @@ -1183,3 +1183,22 @@ async def test_add_row_auto_height(cell: RenderableType, height: int): row = table.rows.get(row_key) await pilot.pause() assert row.height == height + + +async def test_add_row_expands_column_widths(): + """Regression test for https://github.com/Textualize/textual/issues/1026.""" + app = DataTableApp() + from textual.widgets._data_table import CELL_X_PADDING + + async with app.run_test() as pilot: + table = app.query_one(DataTable) + table.add_column("First") + table.add_column("Second", width=10) + await pilot.pause() + assert table.ordered_columns[0].render_width == 5 + CELL_X_PADDING + assert table.ordered_columns[1].render_width == 10 + CELL_X_PADDING + + table.add_row("a" * 20, "a" * 20) + await pilot.pause() + assert table.ordered_columns[0].render_width == 20 + CELL_X_PADDING + assert table.ordered_columns[1].render_width == 10 + CELL_X_PADDING From 2949dadf334411a8d4857b3e8f4046418c6bd642 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, 6 Sep 2023 14:26:16 +0100 Subject: [PATCH 09/14] Fix cache/typing issue. --- src/textual/widgets/_data_table.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index ae8e88c6d4..634722baab 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -33,7 +33,7 @@ from ..widget import PseudoClasses CellCacheKey: TypeAlias = ( - "tuple[RowKey, ColumnKey, Style, bool, bool, int, PseudoClasses]" + "tuple[RowKey, ColumnKey, Style, bool, bool, bool, int, PseudoClasses]" ) LineCacheKey: TypeAlias = "tuple[int, int, int, int, Coordinate, Coordinate, Style, CursorType, bool, int, PseudoClasses]" RowCacheKey: TypeAlias = "tuple[RowKey, int, Style, Coordinate, Coordinate, CursorType, bool, bool, int, PseudoClasses]" @@ -1813,7 +1813,7 @@ def _render_cell( row_key = self._row_locations.get_key(row_index) column_key = self._column_locations.get_key(column_index) - cell_cache_key = ( + cell_cache_key: CellCacheKey = ( row_key, column_key, base_style, From 6fe5f2fb6672b5663af73841c232013b04c0d736 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, 6 Sep 2023 14:35:07 +0100 Subject: [PATCH 10/14] Cache method to compute styles to render cell. We extract this logic into a method for two reasons. For one, having this as a method with an lru cache enables caching these auxiliary styles, which don't depend directly on the location of the cell, but instead depend on the values of 9 Boolean flags (making for a total of 512 possible combinations, versus the infinite number of different positions/states a cell can be in. Secondly, having this as a method allows me to compute these styles more easily from within _update_dimensions when trying to salvage the renderings of the cells of a new row that may have been pre-rendered with the wrong height. (See the following commits for more context.) --- src/textual/widgets/_data_table.py | 102 ++++++++++++++++++++--------- 1 file changed, 72 insertions(+), 30 deletions(-) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index 634722baab..8f3287a22d 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -1833,35 +1833,16 @@ def _render_cell( else: cell = row_cells[column_index] - get_component = self.get_component_rich_style - show_cursor = self.show_cursor - component_style = Style() - - if hover and show_cursor and self._show_hover_cursor: - component_style += get_component("datatable--hover") - if is_header_cell or is_row_label_cell: - # Apply subtle variation in style for the header/label (blue background by - # default) rows and columns affected by the cursor, to ensure we can - # still differentiate between the labels and the data. - component_style += get_component("datatable--header-hover") - - if cursor and show_cursor: - cursor_style = get_component("datatable--cursor") - component_style += cursor_style - if is_header_cell or is_row_label_cell: - component_style += get_component("datatable--header-cursor") - elif is_fixed_style_cell: - component_style += get_component("datatable--fixed-cursor") - - post_foreground = ( - Style.from_color(color=component_style.color) - if self.cursor_foreground_priority == "css" - else Style.null() - ) - post_background = ( - Style.from_color(bgcolor=component_style.bgcolor) - if self.cursor_background_priority == "css" - else Style.null() + component_style, post_style = self._get_styles_to_render_cell( + is_header_cell, + is_row_label_cell, + is_fixed_style_cell, + hover, + cursor, + self.show_cursor, + self._show_hover_cursor, + self.cursor_foreground_priority == "css", + self.cursor_background_priority == "css", ) if is_header_cell: @@ -1880,7 +1861,7 @@ def _render_cell( Styled( Padding(cell, (0, 1)), pre_style=base_style + component_style, - post_style=post_foreground + post_background, + post_style=post_style, ), options, ) @@ -1889,6 +1870,67 @@ def _render_cell( return self._cell_render_cache[cell_cache_key] + @functools.lru_cache( + maxsize=512 + ) # 9 Boolean arguments = 512 possible combinations. + def _get_styles_to_render_cell( + self, + is_header_cell: bool, + is_row_label_cell: bool, + is_fixed_style_cell: bool, + hover: bool, + cursor: bool, + show_cursor: bool, + show_hover_cursor: bool, + has_css_foreground_priority: bool, + has_css_background_priority: bool, + ) -> tuple[Style, Style]: + """Auxiliary method to compute styles used to render a given cell. + + Args: + is_header_cell: Is this a cell from a header? + is_row_label_cell: Is this the label of any given row? + is_fixed_style_cell: Should this cell be styled like a fixed cell? + hover: Does this cell have the hover pseudo class? + cursor: Is this cell covered by the cursor? + show_cursor: Do we want to show the cursor in the data table? + show_hover_cursor: Do we want to show the mouse hover when using the keyboard + to move the cursor? + has_css_foreground_priority: `self.cursor_foreground_priority == "css"`? + has_css_background_priority: `self.cursor_background_priority == "css"`? + """ + get_component = self.get_component_rich_style + component_style = Style() + + if hover and show_cursor and show_hover_cursor: + component_style += get_component("datatable--hover") + if is_header_cell or is_row_label_cell: + # Apply subtle variation in style for the header/label (blue background by + # default) rows and columns affected by the cursor, to ensure we can + # still differentiate between the labels and the data. + component_style += get_component("datatable--header-hover") + + if cursor and show_cursor: + cursor_style = get_component("datatable--cursor") + component_style += cursor_style + if is_header_cell or is_row_label_cell: + component_style += get_component("datatable--header-cursor") + elif is_fixed_style_cell: + component_style += get_component("datatable--fixed-cursor") + + post_foreground = ( + Style.from_color(color=component_style.color) + if has_css_foreground_priority + else Style.null() + ) + post_background = ( + Style.from_color(bgcolor=component_style.bgcolor) + if has_css_background_priority + else Style.null() + ) + + return component_style, post_foreground + post_background + def _render_line_in_row( self, row_key: RowKey, From cec81ee8aebad16508dc9eeb05aee2589ebe8afa 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, 11 Sep 2023 17:38:19 +0100 Subject: [PATCH 11/14] Perform surgery on the datatable cache. --- src/textual/widgets/_data_table.py | 25 +++++++++++++++++++++++-- 1 file changed, 23 insertions(+), 2 deletions(-) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index 8f3287a22d..f2ad83d1ac 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -1247,6 +1247,10 @@ def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None: height = 0 row_style = self._get_row_style(row_index, base_style) + # As we go through the cells, save their rendering, height, and + # column width. After we compute the height of the row, go over the cells + # that were rendered with the wrong height and append the missing padding. + rendered_cells: list[tuple[SegmentLines, int, int]] = [] for column_index, column in enumerate(ordered_columns): style = fixed_style if column_index < fixed_columns else row_style cell_location = Coordinate(row_index, column_index) @@ -1262,9 +1266,24 @@ def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None: hover_location, cell_location, cursor_type ), ) - height = max(height, len(rendered_cell)) + cell_height = len(rendered_cell) + rendered_cells.append( + (rendered_cell, cell_height, column.render_width) + ) + height = max(height, cell_height) row.height = height + # Do surgery on the cache for cells that were rendered with the incorrect + # height during the first pass. + for cell_renderable, cell_height, column_width in rendered_cells: + if cell_height < height: + first_line_space_style = cell_renderable[0][0].style + cell_renderable.extend( + [ + [Segment(" " * column_width, first_line_space_style)] + for _ in range(height - cell_height) + ] + ) data_cells_width = sum(column.render_width for column in self.columns.values()) total_width = data_cells_width + self._row_label_column_width @@ -1851,7 +1870,9 @@ def _render_cell( ) else: row = self.rows[row_key] - if row.auto_height: + # If an auto-height row hasn't had its height calculated, we don't fix + # the value for `height` so that we can measure the height of the cell. + if row.auto_height and row.height == 0: options = self.app.console.options.update_width(width) else: options = self.app.console.options.update_dimensions( From 12b2a0ab44744e9595c6fcd713bfa9d7b199cd66 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, 11 Sep 2023 17:44:48 +0100 Subject: [PATCH 12/14] Improve data table tests. --- .../__snapshots__/test_snapshots.ambr | 274 ++++++++++++++---- .../data_table_add_row_auto_height.py | 14 +- tests/snapshot_tests/test_snapshots.py | 7 + 3 files changed, 232 insertions(+), 63 deletions(-) diff --git a/tests/snapshot_tests/__snapshots__/test_snapshots.ambr b/tests/snapshot_tests/__snapshots__/test_snapshots.ambr index 4e25c90d5d..7241ac7e37 100644 --- a/tests/snapshot_tests/__snapshots__/test_snapshots.ambr +++ b/tests/snapshot_tests/__snapshots__/test_snapshots.ambr @@ -12754,134 +12754,292 @@ font-weight: 700; } - .terminal-1358738486-matrix { + .terminal-3912008695-matrix { font-family: Fira Code, monospace; font-size: 20px; line-height: 24.4px; font-variant-east-asian: full-width; } - .terminal-1358738486-title { + .terminal-3912008695-title { font-size: 18px; font-weight: bold; font-family: arial; } - .terminal-1358738486-r1 { fill: #dde6ed;font-weight: bold } - .terminal-1358738486-r2 { fill: #dde6ed } - .terminal-1358738486-r3 { fill: #c5c8c6 } - .terminal-1358738486-r4 { fill: #211505 } - .terminal-1358738486-r5 { fill: #e1e1e1 } + .terminal-3912008695-r1 { fill: #dde6ed;font-weight: bold } + .terminal-3912008695-r2 { fill: #dde6ed } + .terminal-3912008695-r3 { fill: #c5c8c6 } + .terminal-3912008695-r4 { fill: #211505 } + .terminal-3912008695-r5 { fill: #e1e1e1 } - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - + - AutoHeightRowsApp + AutoHeightRowsApp - - - -  Column      -  hey there   -  hey there   -  long        -  string      -  ╭───────╮   -  │ Hello │   -  │ world │   -  ╰───────╯   -  1           -  2           -  3           -  4           -  5           -  6           -  7           - - - - - - - + + + +  N  Column      +  3  hey there   +  1  hey there   +  5  long        +  string      +  2  ╭───────╮   +  │ Hello │   +  │ world │   +  ╰───────╯   +  4  1           +  2           +  3           +  4           +  5           +  6           +  7           + + + + + + + + + + + + + ''' +# --- +# name: test_datatable_add_row_auto_height_sorted + ''' + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + AutoHeightRowsApp + + + + + + + + + +  N  Column      +  1  hey there   +  2  ╭───────╮   +  │ Hello │   +  │ world │   +  ╰───────╯   +  3  hey there   +  4  1           +  2           +  3           +  4           +  5           +  6           +  7           +  5  long        +  string      + + + + + + + diff --git a/tests/snapshot_tests/snapshot_apps/data_table_add_row_auto_height.py b/tests/snapshot_tests/snapshot_apps/data_table_add_row_auto_height.py index a9b8c1c17b..23a224b4ff 100644 --- a/tests/snapshot_tests/snapshot_apps/data_table_add_row_auto_height.py +++ b/tests/snapshot_tests/snapshot_apps/data_table_add_row_auto_height.py @@ -8,14 +8,18 @@ class AutoHeightRowsApp(App[None]): def compose(self): table = DataTable() + self.column = table.add_column("N") table.add_column("Column", width=10) - table.add_row("hey there", height=None) - table.add_row(Text("hey there"), height=None) - table.add_row(Text("long string", overflow="fold"), height=None) - table.add_row(Panel.fit("Hello\nworld"), height=None) - table.add_row("1\n2\n3\n4\n5\n6\n7", height=None) + table.add_row(3, "hey there", height=None) + table.add_row(1, Text("hey there"), height=None) + table.add_row(5, Text("long string", overflow="fold"), height=None) + table.add_row(2, Panel.fit("Hello\nworld"), height=None) + table.add_row(4, "1\n2\n3\n4\n5\n6\n7", height=None) yield table + def key_s(self): + self.query_one(DataTable).sort(self.column) + if __name__ == "__main__": AutoHeightRowsApp().run() diff --git a/tests/snapshot_tests/test_snapshots.py b/tests/snapshot_tests/test_snapshots.py index 34e0aeb40c..1b49567a84 100644 --- a/tests/snapshot_tests/test_snapshots.py +++ b/tests/snapshot_tests/test_snapshots.py @@ -153,6 +153,13 @@ def test_datatable_add_row_auto_height(snap_compare): assert snap_compare(SNAPSHOT_APPS_DIR / "data_table_add_row_auto_height.py") +def test_datatable_add_row_auto_height_sorted(snap_compare): + # Check that rows added with auto height computation look right. + assert snap_compare( + SNAPSHOT_APPS_DIR / "data_table_add_row_auto_height.py", press=["s"] + ) + + def test_footer_render(snap_compare): assert snap_compare(WIDGET_EXAMPLES_DIR / "footer.py") From 007008bbb981442301bfd37eb1c8bd75e38c1953 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: Fri, 15 Sep 2023 15:19:50 +0100 Subject: [PATCH 13/14] Reduce cache size. The first five parameters (is_header_cell, is_row_label_cell, is_fixed_style_cell, hover, and cursor) are the ones that change more frequently, so it is reasonable to fix the size of the cache at 32. Related comment: https://github.com/Textualize/textual/pull/3213#discussion_r1326071862 --- src/textual/widgets/_data_table.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index f2ad83d1ac..7c001f0228 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -1891,9 +1891,7 @@ def _render_cell( return self._cell_render_cache[cell_cache_key] - @functools.lru_cache( - maxsize=512 - ) # 9 Boolean arguments = 512 possible combinations. + @functools.lru_cache(maxsize=32) def _get_styles_to_render_cell( self, is_header_cell: bool, From 2524af4abd23bad1bcc0b6b47dd9db7666e89f7b 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: Fri, 15 Sep 2023 15:23:03 +0100 Subject: [PATCH 14/14] Clear cache with other caches. Related comment: https://github.com/Textualize/textual/pull/3213#discussion_r1326071862. --- src/textual/widgets/_data_table.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/textual/widgets/_data_table.py b/src/textual/widgets/_data_table.py index 7c001f0228..4b81e9aee7 100644 --- a/src/textual/widgets/_data_table.py +++ b/src/textual/widgets/_data_table.py @@ -952,6 +952,7 @@ def _clear_caches(self) -> None: self._styles_cache.clear() self._offset_cache.clear() self._ordered_row_cache.clear() + self._get_styles_to_render_cell.cache_clear() def get_row_height(self, row_key: RowKey) -> int: """Given a row key, return the height of that row in terminal cells.