diff --git a/src/korvid/ui/widgets/resource_table.py b/src/korvid/ui/widgets/resource_table.py index 1bd5d5aa..df37d86d 100644 --- a/src/korvid/ui/widgets/resource_table.py +++ b/src/korvid/ui/widgets/resource_table.py @@ -2,15 +2,16 @@ from __future__ import annotations -from collections.abc import Callable +from collections.abc import Callable, Iterable +from collections.abc import Set as AbstractSet from datetime import UTC, datetime -from typing import Final, cast +from typing import Final, Self, cast from rich.cells import cell_len from rich.text import Text from textual.coordinate import Coordinate from textual.widgets import DataTable -from textual.widgets.data_table import RowDoesNotExist +from textual.widgets.data_table import Column, RowDoesNotExist, RowKey from korvid.core.config import ViewConfig from korvid.core.sorting import SortSpec, sort_rows @@ -347,6 +348,17 @@ class ResourceTable(DataTable[str | Text]): _last_all_namespaces: bool | None = None _last_sort: SortSpec | None = None _active_view: ViewConfig | None = None + #: Row keys whose widths this widget folded into the columns itself, + #: pending consumption by the next `_update_dimensions`. Created lazily so + #: the hook is safe before `on_mount` has run. + _absorbed_keys: set[str] | None = None + + @property + def _absorbed(self) -> set[str]: + keys = self._absorbed_keys + if keys is None: + keys = self._absorbed_keys = set() + return keys def on_mount(self) -> None: self.cursor_type = "row" @@ -459,6 +471,7 @@ def show( for key, cells in pending: self.add_row(*cells, key=key) self._emitted = dict(pending) + self._absorb_widths(pending) if restore is not None: # A background refresh must not scroll the cursor back into # view — the viewport restore below keeps the user's position. @@ -524,11 +537,15 @@ def _apply_in_place(self, pending: list[tuple[str, list[str | Text]]]) -> bool: settled by one identity check. `get_row` remains the fallback whenever `_emitted` has no record of a row. - Width updates are requested only for cells wider than their column: - `update_width=True` is not grow-only — a narrower replacement rescans - and *shrinks* the column, shifting the layout this path must keep - still. The trade-off is that a column stays at its widest-seen size - until the next rebuild. + Width updates are never handed to Textual's queue. `update_width=True` + is not grow-only: it defers the cell to `_update_column_widths`, which + runs *before* the dimension pass and re-measures every cell in the + column the moment the queued value looks narrower than the column — + both when the replacement genuinely shrank, and when another row in + the same repaint had already widened that column. Widths are absorbed + below instead, from the rows that actually changed. The trade-off is + unchanged: a column stays at its widest-seen size until the next + rebuild. """ plan = self._in_place_plan(pending) if plan is None: @@ -537,22 +554,116 @@ def _apply_in_place(self, pending: list[tuple[str, list[str | Text]]]) -> bool: for key in doomed: self.remove_row(key) columns = self.ordered_columns + touched: list[tuple[str, list[str | Text]]] = [] for key, cells in pending: if key not in current_set: self.add_row(*cells, key=key) - continue - old_cells = self._emitted.get(key) - if old_cells is cells: - continue # memo hit: same list object, nothing can have changed - if old_cells is None: - old_cells = self.get_row(key) - for column, old_cell, new_cell in zip(columns, old_cells, cells, strict=True): - if not _cells_equal(old_cell, new_cell): - grew = _cell_width(new_cell) > column.content_width - self.update_cell(key, column.key, new_cell, update_width=grew) + touched.append((key, cells)) + elif self._patch_row(key, cells, columns): + touched.append((key, cells)) self._emitted = dict(pending) + if touched: + self._absorb_widths(touched) return True + def _patch_row(self, key: str, cells: list[str | Text], columns: list[Column]) -> bool: + """Update an existing row's changed cells; True when any cell moved.""" + old_cells = self._emitted.get(key) + if old_cells is cells: + return False # memo hit: same list object, nothing can have changed + if old_cells is None: + old_cells = self.get_row(key) + changed = False + for column, old_cell, new_cell in zip(columns, old_cells, cells, strict=True): + if not _cells_equal(old_cell, new_cell): + self.update_cell(key, column.key, new_cell, update_width=False) + changed = True + return changed + + def _absorb_widths(self, rows: Iterable[tuple[str, list[str | Text]]]) -> None: + """Grow the column widths from cells this widget is holding anyway. + + `DataTable` derives column widths from `on_idle`, and to do so it + rebuilds every new row's renderables and measures each cell — 700,000 + renderable constructions and measurements to seed a 50,000-row view, + which is most of the freeze when a large kind first loads (issue + #210). The cells are already in hand here, so the same fourteen + integers are computed directly and `_update_dimensions` is told to + skip the rows they came from. + + The `len(raw) <= width and raw.isascii()` guard settles the + overwhelming majority of cells without measuring: an ASCII string's + display width is exactly its length, and both markup parsing and the + newline truncation `_cell_width` performs can only shorten it — so a + cell that short cannot widen its column no matter how it renders. + """ + columns = self.ordered_columns + widths = [column.content_width for column in columns] + limit = len(widths) + absorbed = self._absorbed + for key, cells in rows: + absorbed.add(key) + for index, cell in enumerate(cells): + if index >= limit: + break + raw = cell.plain if isinstance(cell, Text) else cell + if len(raw) <= widths[index] and raw.isascii(): + continue + width = _cell_width(cell) + if width > widths[index]: + widths[index] = width + for column, width in zip(columns, widths, strict=True): + if width > column.content_width: + column.content_width = width + # A wider column means a wider table; the superclass + # recomputes the virtual size from the dimension pass, which + # `update_cell(update_width=False)` does not schedule. + self._require_update_dimensions = True + + def remove_row(self, row_key: RowKey | str) -> None: + """Forget the absorption record for a row that is going away. + + A key removed and re-added before the next dimension pass carries + content this widget never measured, so it must not stay on the skip + list. + """ + self._absorbed.discard(row_key.value if isinstance(row_key, RowKey) else row_key) + super().remove_row(row_key) + + def clear(self, columns: bool = False) -> Self: + """Drop the absorption record along with the rows it described.""" + self._absorbed.clear() + return super().clear(columns=columns) + + def _update_dimensions(self, new_rows: Iterable[RowKey]) -> None: + """Measure only the rows whose widths were not absorbed above. + + The superclass still recomputes the virtual size and handles anything + this widget did not account for (a row added directly, or added after + the absorption and before this idle pass). Should a future Textual + rename the hook, this override simply stops being called and the + widget falls back to today's slower-but-correct measuring pass. + """ + absorbed = self._absorbed_keys + if absorbed: + new_rows = [key for key in new_rows if not self._is_absorbed(key, absorbed)] + absorbed.clear() + super()._update_dimensions(new_rows) + + def _is_absorbed(self, key: RowKey, absorbed: AbstractSet[str]) -> bool: + """Whether `_absorb_widths` fully accounted for the row behind *key*. + + Absorption only folds in cell *widths*. The superclass pass also sizes + the row-label column and computes auto-height rows, so a row carrying + either must still reach it — today this widget emits neither, but the + fast path must fail safe rather than silently drop them if that + changes. + """ + if key.value not in absorbed: + return False + row = self.rows.get(key) + return row is not None and row.label is None and not row.auto_height + def _prune_memo(self, pending: list[tuple[str, list[str | Text]]]) -> None: """Drop memo entries for rows no longer rendered. diff --git a/tests/ui/test_table_column_widths.py b/tests/ui/test_table_column_widths.py new file mode 100644 index 00000000..81a7408d --- /dev/null +++ b/tests/ui/test_table_column_widths.py @@ -0,0 +1,286 @@ +"""Column-width absorption (issue #210). + +Textual recomputes column widths from `on_idle` by re-deriving every new +row's renderables and measuring each cell — 700,000 measurements to seed a +50,000-row view, and ~74% of the freeze when a large kind first loads. + +`ResourceTable` already holds the exact cells it emitted, so it folds their +widths into the columns itself and hands the superclass only the rows it did +not account for. These tests pin both halves: that the measuring pass is +skipped, and that the widths it produces are indistinguishable from the ones +Textual would have measured. +""" + +from __future__ import annotations + +from typing import Any + +from rich.text import Text +from textual.app import App, ComposeResult +from textual.widgets import DataTable + +from korvid.core.store import Summary +from korvid.k8s.models import PodSummary +from korvid.ui.widgets.resource_table import ResourceTable + + +class _TableApp(App[None]): + """Bare host for a `ResourceTable`, so a test drives `show()` directly.""" + + def compose(self) -> ComposeResult: + yield ResourceTable() + + +def _pods(names: list[str]) -> list[Summary]: + return _pods_with_phase([(name, "Running") for name in names]) + + +def _pods_with_phase(rows: list[tuple[str, str]]) -> list[Summary]: + return [ + PodSummary( + name=name, + namespace="default", + phase=phase, + ready="1/1", + restarts=0, + node=None, + qos="-", + ) + for name, phase in rows + ] + + +def _spy_row_renderables(table: DataTable[Any]) -> list[int]: + """Record every row whose renderables are rebuilt. + + Textual also rebuilds renderables to *paint*, so only distinct data rows + matter: painting touches the handful on screen, measuring touches all of + them. Index -1 is the header row. + """ + seen: list[int] = [] + original = table._get_row_renderables + + def spy(row_index: int) -> Any: + seen.append(row_index) + return original(row_index) + + table._get_row_renderables = spy # type: ignore[method-assign] # test spy + return seen + + +def _widths(table: DataTable[Any]) -> list[int]: + return [column.content_width for column in table.ordered_columns] + + +async def test_seeding_does_not_rebuild_every_row_to_measure_it() -> None: + """Seeding a view must not cost one renderable rebuild per row.""" + app = _TableApp() + async with app.run_test(size=(120, 12)) as pilot: + table = app.query_one(ResourceTable) + await pilot.pause() + seen = _spy_row_renderables(table) + rows = _pods([f"pod-{i:04d}" for i in range(400)]) + table.show("pods", rows, all_namespaces=False, pattern="") + await pilot.pause() + assert table._require_update_dimensions is False, "dimension pass did not run" + touched = {index for index in seen if index >= 0} + assert len(touched) < 100, f"rebuilt {len(touched)} of 400 rows" + + +async def test_absorbed_widths_match_what_textual_would_measure() -> None: + """Widths must be identical to the superclass result for every cell shape.""" + names = [ + "a", + "pod-with-a-fairly-long-generated-name-0001", + "파드-매우-긴-한글-이름", + "[bold]not-markup[/bold]", + "trailing", + ] + app = _TableApp() + async with app.run_test(size=(120, 12)) as pilot: + table = app.query_one(ResourceTable) + await pilot.pause() + table.show("pods", _pods(names), all_namespaces=True, pattern="") + await pilot.pause() + + control: DataTable[Any] = DataTable() + await app.mount(control) + await pilot.pause() + control.add_columns(*[column.label for column in table.ordered_columns]) + for key, cells in table._emitted.items(): + control.add_row(*cells, key=key) + await pilot.pause() + + assert _widths(table) == _widths(control) + + +async def test_rows_added_outside_show_are_still_measured() -> None: + """A row this widget did not emit must fall through to the superclass.""" + app = _TableApp() + async with app.run_test(size=(120, 12)) as pilot: + table = app.query_one(ResourceTable) + await pilot.pause() + table.show("pods", _pods(["short"]), all_namespaces=False, pattern="") + await pilot.pause() + long_value = "a-value-added-without-going-through-show" + table.add_row(long_value, key="extra") + await pilot.pause() + assert table.ordered_columns[0].content_width >= len(long_value) + + +async def test_unabsorbed_row_added_before_the_dimension_pass_is_measured() -> None: + """Absorbing one batch must not suppress measurement of rows added after + it but before the idle pass runs.""" + app = _TableApp() + async with app.run_test(size=(120, 12)) as pilot: + table = app.query_one(ResourceTable) + await pilot.pause() + long_value = "a-value-added-between-show-and-idle-00000" + table.show("pods", _pods(["short"]), all_namespaces=False, pattern="") + table.add_row(long_value, key="extra") + await pilot.pause() + assert table.ordered_columns[0].content_width >= len(long_value) + + +async def test_row_appearing_in_place_widens_its_column() -> None: + """A new row appended by the in-place diff still grows the column.""" + app = _TableApp() + async with app.run_test(size=(120, 12)) as pilot: + table = app.query_one(ResourceTable) + await pilot.pause() + table.show("pods", _pods(["aaa"]), all_namespaces=False, pattern="") + await pilot.pause() + narrow = table.ordered_columns[0].content_width + appeared = "zzz-a-much-longer-pod-name-than-before" + table.show("pods", _pods(["aaa", appeared]), all_namespaces=False, pattern="") + await pilot.pause() + assert table.ordered_columns[0].content_width > narrow + assert table.ordered_columns[0].content_width >= len(appeared) + + +def _spy_column_rescan(table: DataTable[Any]) -> list[Any]: + """Record full-column width rescans. + + `_update_column_widths` reads a whole column back out — and measures every + cell in it — only when a queued cell looks *narrower* than the column it + sits in. `get_column` is that read, so any call means the O(total rows) + rescan this widget exists to avoid has just run. + """ + seen: list[Any] = [] + original = table.get_column + + def spy(column_key: Any) -> Any: + seen.append(column_key) + return original(column_key) + + table.get_column = spy # type: ignore[method-assign] # test spy + return seen + + +async def test_widening_new_row_does_not_trigger_a_full_column_rescan() -> None: + """A repaint that both widens a column and changes an existing cell must + not make Textual re-measure the whole column. + + Textual drains queued cell updates *before* the dimension pass. If the + column has already been widened by then, the queued cell reads as a + shrink and every cell in the column is measured again — reintroducing the + very cost this widget avoids. + """ + app = _TableApp() + async with app.run_test(size=(120, 12)) as pilot: + table = app.query_one(ResourceTable) + await pilot.pause() + table.show( + "pods", + _pods_with_phase([("alpha", "Running"), ("beta", "Running")]), + all_namespaces=False, + pattern="", + ) + await pilot.pause() + rescans = _spy_column_rescan(table) + table.show( + "pods", + _pods_with_phase( + [ + ("alpha", "Running"), + ("beta", "CrashLoop"), + ("zeta", "ContainerCreating"), + ] + ), + all_namespaces=False, + pattern="", + ) + await pilot.pause() + assert rescans == [], f"rescanned {len(rescans)} column(s)" + status = table.ordered_columns[2] + assert status.content_width >= len("ContainerCreating") + + +async def test_absorption_never_skips_a_labelled_or_auto_height_row() -> None: + """Width absorption must not swallow the rest of the dimension pass. + + Textual's `_update_dimensions` also assigns auto-height rows their height + and widens the row-label column. Absorption only accounts for cell + *widths*, so a row carrying either of those must still reach the + superclass — otherwise a future `height=None` row would render zero rows + tall and a labelled row would never size its label column. + """ + app = _TableApp() + async with app.run_test(size=(120, 12)) as pilot: + table = app.query_one(ResourceTable) + await pilot.pause() + table.show( + "pods", + _pods(["alpha", "beta"]), + all_namespaces=False, + pattern="", + ) + await pilot.pause() + + first, second = table.ordered_rows + first.auto_height = True + first.height = 0 + second.label = Text("a-very-long-row-label") + + table._absorbed.update(key for key in (first.key.value, second.key.value) if key) + table._update_dimensions([first.key, second.key]) + + assert first.height > 0, "auto-height row was skipped and stayed 0 tall" + assert table._label_column.content_width >= len("a-very-long-row-label") + + +async def test_a_repaint_that_widens_nothing_leaves_no_stale_skip() -> None: + """Absorption must not license skipping a row it never accounted for. + + When a repaint absorbs widths but grows no column, no dimension pass is + scheduled. If the "widths were absorbed" state survives to whatever pass + runs next, that pass filters out rows this widget merely *emitted* once — + including a row re-added since, whose wider content then never reaches + the column. + """ + app = _TableApp() + async with app.run_test(size=(120, 12)) as pilot: + table = app.query_one(ResourceTable) + await pilot.pause() + table.show("pods", _pods(["alpha", "beta"]), all_namespaces=False, pattern="") + await pilot.pause() + + # A repaint that changes a cell but widens nothing: absorption runs, + # no column grows, so nothing schedules a dimension pass. + table.show( + "pods", + _pods_with_phase([("alpha", "Pending"), ("beta", "Running")]), + all_namespaces=False, + pattern="", + ) + + name_column = table.ordered_columns[1] + wide = "a" * (name_column.content_width + 30) + key = next(iter(table._emitted)) + table.remove_row(key) + table.add_row(*([wide] * len(table.ordered_columns)), height=1, key=key) + await pilot.pause() + + assert name_column.content_width >= len(wide), ( + f"column stayed {name_column.content_width} wide, needed {len(wide)}" + )