From 6bae7dc77df2beb79d6d6735a5af6e041636dff8 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 29 Sep 2026 01:16:05 +0200 Subject: [PATCH 1/3] fix(gui): reach every table row and choose rows from the keyboard The virtual table measured its viewport in slots. The window holds the rows on screen plus a partial row and a buffer, and the rows that fit were counted as height // rowheight, header and border included. At the bottom the last rows sat in slots nobody could see: in a 400 px tree Tk shows 17 rows in full, the table counted 18 and held 22 slots, so the last five rows of every table were unreachable, a 20-row table could not scroll, and Page Down jumped 22 rows. The rows shown in full are now read off Tk's own yview on every Configure (rows_shown_in_full), the count see() itself goes by, and the bottom, the page step and the scrollbar thumb use it. A thumb dragged to the end is rounded instead of truncated. ttk's own key and click handlers chose a slot and see()-d it, which moved the widget's own view under the window: after 40 presses of Down the first rows could not be scrolled back to. Tk also answers every selection the table writes with a queued <>, and the table rebuilt its selection from the rows on screen, so a selected row scrolled out of view was lost. Rows are now chosen by model position, with a cursor and an anchor kept as model keys: Up, Down, Page Up, Page Down, Home and End (with Shift for a range across pages), a click, a Shift+click and a Ctrl+click. The table's own selection echo is ignored. tools/ci_gui_render.py --tables measures all of this on real Tk in CI, because the fake Tk of the suite has no pixels. The two tests that pinned the old behaviour are rewritten on purpose. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 7 + README.md | 1 + beantester/gui/widgets/sortable_tree.py | 263 ++++++++++++++++++++++-- tests/test_mutation_registry.py | 93 +++++++++ tests/test_virtual_tables.py | 220 +++++++++++++++++++- tools/ci_gui_render.py | 188 ++++++++++++++++- 6 files changed, 747 insertions(+), 25 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ecfaf3..68bbb67 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,13 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol ### Fixed +- **Every row of a table can be scrolled into view, and tables work from the + keyboard.** The last rows of every table (Connections, the event log, Port check, + Sockets) could not be reached, a short table could not scroll at all, and Page Down + skipped rows. Up, Down, Page Up, Page Down, Home and End now move the selection, and + with Shift they select a range across pages. A selected row stays selected when it + scrolls out of view, so Ctrl+C and Shift+F10 keep working on it. + - **100% loss with "Losses in a row" set now loses every packet.** A run length used to let 11 to 33% of packets through, including during the full outage in the `mobile-lte-to-3g` scenario, and the log and the summary described runs of loss that diff --git a/README.md b/README.md index d369a00..c9d6faa 100644 --- a/README.md +++ b/README.md @@ -248,6 +248,7 @@ effect. | `Ctrl+S` / `Ctrl+O` | Save / Load config file | | `Ctrl+L` | Clear the log | | `Ctrl+F` | Search: the field search on Control, the table search on Connections, the socket search on Tools > Sockets | +| `Up` / `Down`, `Page Up` / `Page Down`, `Home` / `End` | In a table: move the selection. With `Shift`, select a range, which can run past one screen. `Ctrl+C` copies the selected rows and `Shift+F10` opens the row menu | **Finding a setting.** The box at the top of the Control page searches the settings by name - type part of a field or section name, or the command-line flag such as `--loss`. Every match is diff --git a/beantester/gui/widgets/sortable_tree.py b/beantester/gui/widgets/sortable_tree.py index b4bab69..ecfb347 100644 --- a/beantester/gui/widgets/sortable_tree.py +++ b/beantester/gui/widgets/sortable_tree.py @@ -32,7 +32,12 @@ row in the model, survive sorting, and are what selection is remembered by (the widget's item ids are recycled and meaningless); * :meth:`selection_values`, :meth:`selected_rows`, :meth:`selected_keys` and - :meth:`key_at` all answer from the model, so they survive a repaint; + :meth:`key_at` all answer from the model, so they survive a repaint - and a + scroll: the keyboard and a click choose rows by model position, with a cursor + and an anchor in model keys, so a range can run across any number of pages + (see ``_bind_navigation``); +* the viewport is measured in the rows Tk shows IN FULL (``rows_shown_in_full``), + never in slots: the window also holds a partial row and a buffer nobody sees; * everything else - sorting, header arrows, tooltips, tags, Ctrl+C, column-width clamping - behaves exactly as before. @@ -122,7 +127,45 @@ def fitted_widths(order, visible, natural, current, tree_width, out[col] += take left -= take return {c: w for c, w in out.items() if w != now[c]} -BUFFER_ROWS = 4 # slots kept past the viewport, hides scroll tearing + + +def rows_shown_in_full(yview, slots): + """How many slots Tk draws IN FULL, read off its own ``yview`` - or None. + + Pure, and tested without Tk, like ``fitted_widths``: the arithmetic is here, + the widget only answers the question it is asked. + + **Why Tk is asked instead of the height being divided.** Everything a table + promises - that the last row can be scrolled into view, how far a page goes, + how big the scrollbar thumb is - hangs on this one number, and the division + got it wrong: ``height // rowheight`` counts the header and the border as rows + (external review, P1-4). Measured 2026-09-29 on Tk 9.0.4 in a 400 px tree: 17 + rows fit, the division said 18, and with the buffer on top the last FIVE rows + of every table could never be shown. What Tk really fits is + ``(height - header - border) // rowheight`` - it held at all 440 heights tried + and at two row heights - but the header and the border belong to the theme + and the DPI, and ``yview`` already reports the result: the fraction of the + slots on screen. It is also the very count ``see`` uses to decide whether to + scroll, so a table sized by it cannot disagree with ttk about what is visible. + + The fraction only carries a count while there are MORE slots than fit, which + is what the buffer guarantees. When every slot fits, all of them are shown in + full and that is the answer. None when there is nothing to read - the caller + keeps what it had. + """ + try: + first, last = (float(value) for value in yview) + except (TypeError, ValueError): + return None + if slots <= 0: + return None + span = last - first + if span >= 1.0: + return slots + return max(1, round(span * slots)) + + +BUFFER_ROWS = 4 # slots past the viewport: a partial row + resize slack MIN_WINDOW = 12 # slots to keep even before the widget has a size @@ -179,9 +222,14 @@ def __init__(self, parent, columns, sort=None, on_sort=None, self._slots = [] # widget item ids, recycled forever self._slot_keys = [] # slot index -> model key currently shown self._selected = [] # selected MODEL KEYS (survive a repaint) + self._written = () # slot iids _restore_selection last wrote + self._cursor = None # model key the keyboard stands on + self._anchor = None # model key a Shift range is measured from self._painted = {} # slot iid -> (values, tags) last written self._show_row_menu = None # the page's menu, once bind_row_menu is called self._height = max(1, int(height)) + self._fits = self._height # rows Tk shows IN FULL - see rows_shown_in_full + self._multi = selectmode == "extended" if horizontal and stretch: # ttk re-stretches such a column back to fill the tree on the next @@ -225,7 +273,9 @@ def __init__(self, parent, columns, sort=None, on_sort=None, anchor="center") if empty_text else None self.refresh_headers() self._ensure_slots(self._height + BUFFER_ROWS) + self._bind_events() + def _bind_events(self): # Header tooltips only. The old code hung ONE tooltip on the whole tree, # so it popped up over the rows (covering them and the buttons below) and # said nothing useful about the column under the pointer. @@ -252,8 +302,41 @@ def __init__(self, parent, columns, sort=None, on_sort=None, self.tree.bind("", self._on_wheel, add="+") self.tree.bind("", self._on_wheel, add="+") self.tree.bind("", self._on_configure, add="+") - self.tree.bind("", lambda e: self.scroll_by(-self.window()), add="+") - self.tree.bind("", lambda e: self.scroll_by(self.window()), add="+") + self._bind_navigation() + + def _bind_navigation(self): + """Keys and clicks that choose rows, answered in the MODEL, never by ttk. + + ttk's own handlers pick a row by its SLOT and then ``see`` it, and under a + recycled window both halves are wrong (external review, P2-18): + + * ``see`` scrolls the widget's OWN view. The window holds a partial row and a + buffer below the rows on screen, so Down past the last full row, or a click + on the half row at the bottom, moved that view (measured on Tk 9.0.4: yview + 0 -> 0.045) under a window that knew nothing of it. After 40 presses of + Down, rows 0-4 could not be scrolled back to at all. + * a slot is not a row. Shift+click measured its range from ttk's focus item, + which is a slot, so after a scroll the range started at whatever row that + slot held by then. + + So every route that moves or extends the selection is taken over here and + ends in "break": the widget's own view never moves, and the selection is a + list of model keys with a cursor and an anchor that scrolling cannot touch. + Left and Right are swallowed because ttk's Right re-selects its focus slot + and ``see``s it. A click on a heading or a column separator stays with ttk - + that is sorting and resizing. + """ + for sequence, step in (("Up", "up"), ("Down", "down"), ("Prior", "page_up"), + ("Next", "page_down"), ("Home", "home"), ("End", "end")): + self.tree.bind(f"<{sequence}>", + lambda _e, s=step: self._on_key(s, False), add="+") + self.tree.bind(f"", + lambda _e, s=step: self._on_key(s, True), add="+") + for sequence in ("", ""): + self.tree.bind(sequence, lambda _e: "break", add="+") + self.tree.bind("", self._on_press, add="+") + self.tree.bind("", self._on_extend_press, add="+") + self.tree.bind("", self._on_toggle_press, add="+") # -- the viewport ---------------------------------------------------------- # def window(self): @@ -286,8 +369,15 @@ def _ensure_slots(self, count): except Exception as _exc: crashlog.note(_exc, "gui.widgets.sortable_tree") - def _visible_rows(self): - """Rows that fit in the widget right now (falls back to the built height).""" + def _rows_upper_bound(self): + """Height over row height: MORE rows than fit, which is all it is used for. + + It counts the header and the border as rows, so it is never the number on + screen (that is ``_fits``, see ``rows_shown_in_full``). It only sizes the + slots, and there it errs the right way: slots must outnumber the rows Tk + can show in full, or ``yview`` has nothing to report. Read from the height + Tk has already given the widget, so it is current inside ````. + """ try: height = int(self.tree.winfo_height() or 0) row_h = int(ttk.Style().lookup("Treeview", "rowheight") or 0) @@ -298,9 +388,22 @@ def _visible_rows(self): return self._height def _on_configure(self, _=None): - needed = self._visible_rows() + BUFFER_ROWS - if needed != self.window(): + needed = self._rows_upper_bound() + BUFFER_ROWS + resized = needed != self.window() + if resized: self._ensure_slots(needed) + # Asked on EVERY resize, not only when the slot count moves: the rows that + # fit can change while height // rowheight does not. Measured 2026-09-29 + # over 82 resizes: yview is already current inside this handler. + try: + fits = rows_shown_in_full(self.tree.yview(), self.window()) + except Exception as _exc: # a dying widget answers nothing + crashlog.note(_exc, "gui.widgets.sortable_tree") + fits = None + if fits is not None and fits != self._fits: + self._fits = fits + resized = True + if resized: self.offset = min(self.offset, self.max_offset()) self.repaint() # A wider widget means new slack to absorb. Guarded by the memo, so this @@ -308,7 +411,10 @@ def _on_configure(self, _=None): self.fit_columns() def max_offset(self): - return max(0, len(self.items) - self.window()) + # Measured in rows shown IN FULL, not in slots: at the bottom the last row + # has to sit in the last full slot, not in the partial row or the buffer + # below it, where nobody can see it (external review, P1-4). + return max(0, len(self.items) - self._fits) def scroll_by(self, lines): self.set_offset(self.offset + int(lines)) @@ -332,15 +438,19 @@ def _on_wheel(self, event): def _on_scrollbar(self, action, value, unit=None): total = max(1, len(self.items)) if action == "moveto": - self.set_offset(int(float(value) * total)) + # Rounded, not truncated: the thumb at the end stands on + # (total - fits) / total, and that float can land a hair under the last + # offset - truncated, it stopped one row short. Measured over 233 840 + # (total, fits) pairs: 2 227 lost a row that way, none once rounded. + self.set_offset(round(float(value) * total)) elif action == "scroll": - step = int(value) * (self.window() if str(unit) == "pages" else 1) + step = int(value) * (self._fits if str(unit) == "pages" else 1) self.scroll_by(step) def _sync_scrollbar(self): total = max(1, len(self.items)) first = min(1.0, self.offset / total) - last = min(1.0, (self.offset + self.window()) / total) + last = min(1.0, (self.offset + self._fits) / total) try: self.vsb.set(first, last) except Exception as _exc: @@ -480,9 +590,18 @@ def _slot_of(self, iid): def _on_select(self, _=None): try: - chosen = self.tree.selection() or () + chosen = tuple(self.tree.selection() or ()) except Exception: return + if chosen == self._written: + # Our own write coming back. <> is QUEUED, so it lands + # after _restore_selection has returned and no flag set around the write + # can recognise it (measured on Tk 9.0.4: every selection_set that + # leaves a selection behind fires one). Rebuilding from it kept only the + # rows on screen, so a selected row scrolled out of the window was + # dropped for good, and Ctrl+C then copied nothing (external review, + # P2-18). The model keys are the truth; the widget only mirrors them. + return keys, wanted = [], [] for iid in chosen: slot = self._slot_of(iid) @@ -497,7 +616,8 @@ def _on_select(self, _=None): # it, but it selects nothing. Drop those from the widget selection so an # empty row cannot sit there looking selected. Re-setting the selection # fires <> again, but with a clean set, so it settles at once. - if tuple(wanted) != tuple(chosen): + if tuple(wanted) != chosen: + self._written = tuple(wanted) try: if wanted: self.tree.selection_set(*wanted) @@ -508,10 +628,11 @@ def _on_select(self, _=None): def _restore_selection(self, selected=None): selected = set(self._selected) if selected is None else selected - wanted = [self._slots[i] for i, key in enumerate(self._slot_keys) - if key is not None and key in selected] + wanted = tuple(self._slots[i] for i, key in enumerate(self._slot_keys) + if key is not None and key in selected) try: - if tuple(self.tree.selection() or ()) != tuple(wanted): + if tuple(self.tree.selection() or ()) != wanted: + self._written = wanted # so _on_select knows its own echo self.tree.selection_set(*wanted) except Exception as _exc: crashlog.note(_exc, "gui.widgets.sortable_tree") @@ -524,8 +645,116 @@ def selected_keys(self): def select_keys(self, keys): index = self._ensure_index() self._selected = [str(k) for k in keys if str(k) in index] + if self._selected: # the keyboard continues from here + self._cursor = self._anchor = self._selected[0] self._restore_selection() + # -- choosing rows: keyboard and pointer, both in model positions ---------- # + def _position_of(self, key): + """Model position of a key, or None. On screen first, which is O(window).""" + if key is None: + return None + try: + return self.offset + self._slot_keys.index(key) + except ValueError: + return self._ensure_index().get(key) + + def _reveal(self, position): + """Scroll just far enough for ``position`` to be shown in full.""" + if position < self.offset: + self.set_offset(position) + elif position >= self.offset + self._fits: + self.set_offset(position - self._fits + 1) + + def _choose(self, position, extend=False, toggle=False): + """Put the cursor on ``position`` and select by the rules of the route. + + Alone, a range from the anchor (Shift), or toggled in and out (Ctrl). A + range is built from MODEL positions, so it may run across any number of + pages and survives every scroll in between. Without an anchor still in + the model, Shift selects the one row, like a plain press. + """ + key = str(self._key_of(self.items[position])) + anchor = self._position_of(self._anchor) if self._multi and extend else None + if anchor is not None: + low, high = sorted((anchor, position)) + self._selected = [str(self._key_of(item)) + for item in self.items[low:high + 1]] + elif self._multi and toggle: + self._selected = ([k for k in self._selected if k != key] + if key in self._selected else self._selected + [key]) + self._anchor = key + else: + self._selected = [key] + self._anchor = key + self._cursor = key + self._reveal(position) + self._restore_selection() + + def _on_key(self, step, extend): + """Up/Down, PageUp/PageDown and Home/End move the cursor through the MODEL. + + A page is the rows shown in full, so paging never skips one. With no + cursor yet, or one whose row has left the model, the keys start where the + reader is looking: the top row on screen. + """ + count = len(self.items) + if count: + here = self._position_of(self._cursor) + if step == "home": + target = 0 + elif step == "end": + target = count - 1 + elif here is None: + target = self.offset + else: + target = here + {"up": -1, "down": 1, "page_up": -self._fits, + "page_down": self._fits}[step] + self._choose(max(0, min(count - 1, target)), extend=extend) + return "break" + + def _row_under(self, event): + """``(True, position)`` over a row slot, with None for a blank one; + ``(False, None)`` over a heading, a separator or empty space.""" + try: + if self.tree.identify_region(event.x, event.y) not in ("cell", "tree"): + return False, None + slot = self._slot_of(self.tree.identify_row(event.y)) + except Exception: + return False, None + key = self._slot_keys[slot] if slot >= 0 else None + return True, (None if key is None else self.offset + slot) + + def _on_press(self, event, extend=False, toggle=False): + """A click on a row, chosen by model position - including the half row. + + The row half cut off at the bottom is the one ttk would ``see``, which + scrolled its own view by a row. Here it is chosen like any other and + ``_reveal`` scrolls the WINDOW, so it ends up in full where it can be read. + """ + # Tk runs one binding per tag and is the closer match, so the + # that hides a header tip no longer fires for button 1. + self._hide_tip() + is_row, position = self._row_under(event) + if not is_row: + return None # heading or separator: ttk sorts, resizes + try: + self.tree.focus_set() # what ttk's own press would have done + except Exception as _exc: + crashlog.note(_exc, "gui.widgets.sortable_tree") + if position is not None: + self._choose(position, extend=extend, toggle=toggle) + elif not (extend or toggle): + self._selected = [] # a blank slot below the rows selects nothing + self._restore_selection() + return "break" + + def _on_extend_press(self, event): + return self._on_press(event, extend=True) + + def _on_toggle_press(self, event): + return self._on_press(event, toggle=True) + def item_for_key(self, key): """The RAW model item behind a key (None when it is gone).""" pos = self._ensure_index().get(str(key)) diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 404ddf8..21f301d 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -3380,6 +3380,99 @@ "new": " self.tree.bind(\"<>\", lambda _event: \"break\")", "test": "test_the_table_is_reachable_and_readable_without_a_mouse", }, + { + # External review P1-4: the window holds a partial row and a buffer below + # the rows on screen, so a bottom measured in slots hid the last rows. + "label": "tables: the bottom is measured in slots again", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " return max(0, len(self.items) - self._fits)", + "new": " return max(0, len(self.items) - self.window())", + "test": "test_scrolling_moves_the_window_and_stays_in_range", + }, + { + "label": "tables: the scrollbar thumb is measured in slots again", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " last = min(1.0, (self.offset + self._fits) / total)", + "new": " last = min(1.0, (self.offset + self.window()) / total)", + "test": "test_scrolling_moves_the_window_and_stays_in_range", + }, + { + # The thumb dragged to the end asks for 1 - fits/total, a float a hair + # under the last offset: truncating it stops one row short. + "label": "tables: dragging the thumb to the end stops a row short", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " self.set_offset(round(float(value) * total))", + "new": " self.set_offset(int(float(value) * total))", + "test": "test_scrolling_moves_the_window_and_stays_in_range", + }, + { + # P2-18: Tk answers every selection the table writes with a QUEUED + # <>, and rebuilding from it kept only the rows on screen. + "label": "tables: the echo of our own selection wipes it again", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " if chosen == self._written:", + "new": " if False:", + "test": "test_selection_is_by_model_key_and_survives_sorting", + }, + { + # Without "break" ttk's class binding runs after ours and `see`s a slot, + # which scrolls the widget's own view under the window. + "label": "tables: a key lets ttk's own handler run after it", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " self._choose(max(0, min(count - 1, target)), extend=extend)\n" + " return \"break\"", + "new": " self._choose(max(0, min(count - 1, target)), extend=extend)\n" + " return None", + "test": "test_the_keyboard_moves_a_cursor_through_the_model", + }, + { + "label": "tables: PageDown skips the rows past the ones in full", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " \"page_down\": self._fits}[step]", + "new": " \"page_down\": self.window()}[step]", + "test": "test_the_keyboard_moves_a_cursor_through_the_model", + }, + { + "label": "tables: Shift ranges forget their anchor", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " anchor = self._position_of(self._anchor) if self._multi and extend else None", + "new": " anchor = None", + "test": "test_shift_selects_a_range_across_pages_and_scrolling_keeps_it", + }, + { + "label": "tables: a chosen row is left where it is, half cut off", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " self._cursor = key\n self._reveal(position)", + "new": " self._cursor = key", + "test": "test_a_click_chooses_by_model_row_and_brings_the_half_row_into_view", + }, + { + "label": "tables: a click lets ttk's own press run after it", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " self._restore_selection()\n return \"break\"\n\n" + " def _on_extend_press", + "new": " self._restore_selection()\n return None\n\n" + " def _on_extend_press", + "test": "test_a_click_chooses_by_model_row_and_brings_the_half_row_into_view", + }, + { + "label": "tables: the rows in full are taken to be every slot", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " return max(1, round(span * slots))", + "new": " return slots", + "test": "test_the_rows_in_full_are_read_off_tks_own_yview", + }, + { + # The pixel half of the table tests lives in the render check under Xvfb; + # dropping its pass from main would leave every fake-Tk test green. + "label": "render: CI stops measuring the table viewport", + "file": "tools/ci_gui_render.py", + "old": " rc = subprocess.run(\n" + " [sys.executable, os.path.abspath(__file__), \"--tables\"]).returncode\n" + " ok = (rc == 0) and ok\n", + "new": "", + "test": "test_the_render_check_measures_the_table_viewport_on_real_tk", + }, { # proc: on another table stops reading the PID: `proc:1234` finds nothing. "label": "search: another table's proc: judges the name alone", diff --git a/tests/test_virtual_tables.py b/tests/test_virtual_tables.py index e1b45f0..935cf19 100644 --- a/tests/test_virtual_tables.py +++ b/tests/test_virtual_tables.py @@ -18,10 +18,31 @@ 3. selection, the context menu and Ctrl+C work off MODEL KEYS, because the widget's item ids are recycled slots and mean nothing. """ -from beantester.gui.widgets.sortable_tree import MAX_WIDTH_FACTOR, fitted_widths -from fakes import check +import ast +import os + +from beantester.gui.widgets.sortable_tree import (MAX_WIDTH_FACTOR, fitted_widths, + rows_shown_in_full) +from fakes import ROOT, check from gui_harness import run_gui +# The fake Tk has no geometry, so Tk's answer to "how many rows fit" is injected +# the way a real table receives it: through yview, read at . Five slots +# short of the window is what a real table has - a partial row and the buffer. +INJECT_FITS = """ + slots = table.window() + fits = slots - 5 + table.tree._yview = (0.0, fits / slots) + table._on_configure() + assert table._fits == fits, (table._fits, fits) + + def key(sequence): + # The fake records bindings and never fires them, so they are run here. + # Every one has to answer "break", or ttk's class binding runs after it. + results = [handler(None) for handler in table.tree.bindings[sequence]] + assert "break" in results, (sequence, results) +""" + # -- hiding columns must not leave the table half empty ---------------------- # @@ -187,12 +208,40 @@ def render(item): """) +def test_the_rows_in_full_are_read_off_tks_own_yview(): + """Tk's fraction of the slots on screen IS the count (external review, P1-4). + + The height divided by the row height counted the header and the border as + rows. yview is what ``see`` itself goes by, so it cannot disagree with ttk. + """ + check("17 of 22 slots on screen", rows_shown_in_full((0.0, 17 / 22), 22) == 17, + f"({rows_shown_in_full((0.0, 17 / 22), 22)})") + check("Tk's own float, as it prints it", rows_shown_in_full( + (0.0, 0.7727272727272727), 22) == 17, "") + check("the span counts, not where it starts", + rows_shown_in_full((1 / 22, 18 / 22), 22) == 17, "") + check("every slot fits: all of them are shown in full", + rows_shown_in_full((0.0, 1.0), 14) == 14, "") + check("a widget shorter than a row still shows one", + rows_shown_in_full((0.0, 0.01), 22) == 1, "") + check("no slots: nothing to read", rows_shown_in_full((0.0, 0.5), 0) is None, "") + check("a dying widget's empty answer: nothing to read", + rows_shown_in_full("", 22) is None, "") + + def test_scrolling_moves_the_window_and_stays_in_range(): + """The bottom is measured in rows Tk shows IN FULL, not in slots. + + Until 2026-09-29 this test PINNED ``max_offset() == len - window()``. The window + holds a partial row and a buffer below the rows on screen, so at the bottom the + last rows sat in slots nobody could see: on real Tk the last five of every + table, and a table of 20 rows could not scroll at all (external review, P1-4). + """ run_gui(""" table = app.pages["connections"].table table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) for i in range(1000)]) - + """ + INJECT_FITS + """ assert table.offset == 0 table.scroll_by(50) assert table.offset == 50 @@ -200,8 +249,32 @@ def test_scrolling_moves_the_window_and_stays_in_range(): assert table.offset == 0, "must not scroll above the first row" table.set_offset(10 ** 9) assert table.offset == table.max_offset(), "must not scroll past the last row" - assert table.max_offset() == 1000 - table.window() + assert table.max_offset() == 1000 - fits, (table.max_offset(), fits) + + # the scrollbar: the thumb spans the rows in full and ends at the end + spans = [] + table.vsb.set = lambda first, last: spans.append((first, last)) + table.repaint() + assert spans[-1] == ((1000 - fits) / 1000, 1.0), spans[-1] + table._on_scrollbar("moveto", "0.0") + assert spans[-1] == (0.0, fits / 1000), spans[-1] + table._on_scrollbar("scroll", "1", "pages") + assert table.offset == fits, "a page is the rows shown in full" + # The thumb at the end stands on (total - fits) / total, and for about one + # model size in a hundred that float lands a hair under the last offset. + # A size where it does is picked, so truncating would stop one row short. + total = next(t for t in range(fits + 1, 5000) + if int(((t - fits) / t) * t) != t - fits) + table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) + for i in range(total)]) + table._on_scrollbar("moveto", repr((total - fits) / total)) + assert table.offset == total - fits, ("the end of the thumb is the end", + total, table.offset) + # a model a few rows longer than the view scrolls exactly that far + table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) for i in range(20)]) + table.set_offset(10 ** 9) + assert table.offset == 20 - fits, (table.offset, fits) # a model that fits entirely in the viewport cannot scroll at all table.sync([("only", ("1", "", "", "", "", "", "", "", ""))]) assert table.max_offset() == 0 @@ -210,7 +283,14 @@ def test_scrolling_moves_the_window_and_stays_in_range(): def test_selection_is_by_model_key_and_survives_sorting(): - """Item ids are recycled slots: a selection stored as an item id is a bug.""" + """Item ids are recycled slots: a selection stored as an item id is a bug. + + Until 2026-09-29 the scroll step below was green only because the fake never + fires <>. Real Tk answers every selection the table writes with + a QUEUED one, and the table rebuilt its selection from it - from the rows on + screen, which after the scroll were none (external review, P2-18). The echo is + now delivered by hand, as Tk delivers it. + """ run_gui(""" table = app.pages["connections"].table rows = [(f"k{i}", (f"p{i}", "TCP", "1.2.3.4", "443", @@ -228,9 +308,12 @@ def test_selection_is_by_model_key_and_survives_sorting(): assert table.selected_keys() == ["k7"], "selection lost when the order changed" assert table.selection_values()[0] == "p7" - # scrolling the selected row out of view does not deselect it + # scrolling the selected row out of view does not deselect it - not even + # once Tk's queued echo of that write arrives, naming no row at all table.set_offset(400) - assert table.selected_keys() == ["k7"] + table._on_select() + assert table.selected_keys() == ["k7"], table.selected_keys() + assert table.copy_text(), "Ctrl+C still has the row to copy" # a row that leaves the model does leave the selection table.sync(rows[:5]) @@ -238,6 +321,129 @@ def test_selection_is_by_model_key_and_survives_sorting(): """) +def test_the_keyboard_moves_a_cursor_through_the_model(): + """Up/Down, PageUp/PageDown and Home/End choose rows by MODEL position. + + ttk's own handlers chose a SLOT and ``see``-d it, which scrolled the widget's + own view under the window: after 40 presses of Down, rows 0-4 could not be + scrolled back to (external review, P2-18). Home and End did nothing at all. + """ + run_gui(""" + table = app.pages["connections"].table + table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) + for i in range(100)]) + """ + INJECT_FITS + """ + key("") # nothing chosen yet: the top row on screen + assert table.selected_keys() == ["k0"], table.selected_keys() + for _ in range(fits): + key("") + assert table.selected_keys() == [f"k{fits}"], table.selected_keys() + assert table.offset == 1, ("one row past the bottom moves the window one", table.offset) + key("") + assert table.selected_keys() == [f"k{2 * fits}"], "a page is the rows in full" + key("") + assert table.selected_keys() == ["k99"] and table.offset == 100 - fits + key("") + assert table.selected_keys() == ["k0"] and table.offset == 0 + key("") + assert table.selected_keys() == ["k0"], "Up on the first row stays on it" + key("") # swallowed: ttk's Right re-selects its slot + key("") + assert table.selected_keys() == ["k0"] + """) + + +def test_shift_selects_a_range_across_pages_and_scrolling_keeps_it(): + """A range is built from model positions, so it can run past a page. + + Shift+click used to measure from ttk's focus item, which is a slot: after a + scroll the range started at whatever row that slot held by then. + """ + run_gui(""" + table = app.pages["connections"].table + table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) + for i in range(100)]) + """ + INJECT_FITS + """ + key("") + for _ in range(30): + key("") + assert table.selected_keys() == [f"k{i}" for i in range(31)], table.selected_keys() + table.set_offset(60) # the whole range leaves the window + table._on_select() # and Tk's queued echo of that write arrives + assert len(table.selected_keys()) == 31, len(table.selected_keys()) + assert len(table.copy_text().splitlines()) == 31, "Ctrl+C copies all of it" + key("") + assert table.selected_keys() == [f"k{i}" for i in range(30)] + assert table.offset <= 29 < table.offset + fits, "the cursor is brought back" + """) + + +def test_a_click_chooses_by_model_row_and_brings_the_half_row_into_view(): + """The row half cut off at the bottom is the one ttk's press used to ``see``. + + That scrolled ttk's own view by a row (measured on Tk 9.0.4: yview 0 -> 0.045). + Now the click is answered by model position and the WINDOW scrolls, so the row + ends up in full; a heading or a separator is still ttk's, for sorting and + resizing. + """ + run_gui(""" + import types + table = app.pages["connections"].table + table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) + for i in range(100)]) + """ + INJECT_FITS + """ + table.tree.identify_region = lambda x, y: "cell" + + def click(slot, sequence=""): + table.tree.row_at = table._slots[slot] + event = types.SimpleNamespace(x=10, y=5) + return [handler(event) for handler in table.tree.bindings[sequence]] + + assert "break" in click(fits), "ttk's own press must not run after it" + assert table.selected_keys() == [f"k{fits}"], table.selected_keys() + assert table.offset == 1, ("the half row is scrolled into full view", table.offset) + click(0, "") # slot 0 holds k1 now + assert table.selected_keys() == [f"k{i}" for i in range(1, fits + 1)] + click(2, "") # k3 out of the range, the rest kept + assert "k3" not in table.selected_keys() + assert len(table.selected_keys()) == fits - 1, table.selected_keys() + + table.tree.identify_region = lambda x, y: "heading" + assert "break" not in click(0), "a heading click is ttk's: it sorts" + + table.tree.identify_region = lambda x, y: "cell" + table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) for i in range(3)]) + click(5) # a blank slot below the three rows + assert table.selected_keys() == [], table.selected_keys() + """) + + +def test_the_render_check_measures_the_table_viewport_on_real_tk(): + """The pixel half of the tests above runs in CI, not here. + + Which rows Tk draws in full and whether ttk's own view moved are pixel facts + the fake cannot show, so ``tools/ci_gui_render.py --tables`` measures them on + real Tk under Xvfb. This pins that CI still runs it: ``main`` sends + ``--tables`` to ``check_table_viewport`` and runs that pass on its own. Losing + the step would leave every test here green. + """ + path = os.path.join(ROOT, "tools", "ci_gui_render.py") + with open(path, encoding="utf-8") as handle: + module = ast.parse(handle.read()) + main = next(node for node in ast.walk(module) + if isinstance(node, ast.FunctionDef) and node.name == "main") + calls = [node for node in ast.walk(main) if isinstance(node, ast.Call)] + named = {call.func.id for call in calls if isinstance(call.func, ast.Name)} + check("--tables reaches check_table_viewport", "check_table_viewport" in named, + f"({sorted(named)})") + runs = [call for call in calls + if isinstance(call.func, ast.Attribute) and call.func.attr == "run" + and any(isinstance(leaf, ast.Constant) and leaf.value == "--tables" + for arg in call.args for leaf in ast.walk(arg))] + check("main runs the table pass as a process of its own", len(runs) == 1, + f"({len(runs)} subprocess run(s) with --tables)") + + def test_clicking_a_blank_slot_selects_nothing(): """Below the last real row the slots are empty; clicking one used to leave it highlighted. The widget selection must drop any blank slot.""" diff --git a/tools/ci_gui_render.py b/tools/ci_gui_render.py index 24d613c..08c3bb7 100644 --- a/tools/ci_gui_render.py +++ b/tools/ci_gui_render.py @@ -19,9 +19,14 @@ no CJK font unless one is installed, so a Chinese pass here would otherwise be a green nobody earned. +It also measures the virtual table on its own (``--tables``): every row reachable, +the widget's own view never moved, a selection kept across a scroll - pixel facts +the fake Tk of the test suite cannot show. + Usage: - python tools/ci_gui_render.py # all discovered languages + python tools/ci_gui_render.py # all discovered languages, then tables python tools/ci_gui_render.py --lang pl # one language + python tools/ci_gui_render.py --tables # the table viewport only Needs a display. In CI: xvfb-run -a --server-args="-screen 0 1366x768x24" python tools/ci_gui_render.py @@ -64,7 +69,9 @@ import bean_network_tester as n # noqa: E402 from beantester import winenv # noqa: E402 +from beantester.gui import scaling, theme # noqa: E402 from beantester.gui.pages import PAGES # noqa: E402 +from beantester.gui.widgets.sortable_tree import SortableTree # noqa: E402 from beantester.gui.windows import WINDOWS # noqa: E402 GEOMETRY = "1366x768" @@ -397,6 +404,178 @@ def scan(): return ok +# -- the virtual table, on real Tk --------------------------------------------- # +# What a table promises is a pixel fact - which rows Tk draws IN FULL, and whether +# the widget's own view moved under the window - and the fake Tk the suite runs on +# has no pixels. So it is measured here, on a bare SortableTree in the real theme. +# The suite pins the arithmetic and the wiring; this pins that the number the +# table works with is the one Tk draws (external review, P1-4 and P2-18). +# Run against the bug before it was trusted (2026-09-29): with `max_offset` put +# back on `len - window()` it reports the last rows out of sight, and with the +# echo guard in `_on_select` removed it reports the selection lost. + +def _boxes(table): + """``(key, box)`` for every slot Tk draws at all, and the rows' bottom edge. + + The edge is read off whichever slot has a box: when ttk's own view has moved + - the very fault measured here - the first slot has none. + """ + tree = table.tree + tree.update() + boxes = [(key, tree.bbox(iid)) for iid, key in zip(table._slots, table._slot_keys)] + boxes = [(key, box) for key, box in boxes if box] + if not boxes: + return [], 0 + # the bottom border is as wide as the side one, where the first column starts + return boxes, tree.winfo_height() - boxes[0][1][0] + + +def _drawn_in_full(table): + """Model keys of the rows Tk draws IN FULL, top to bottom.""" + boxes, bottom = _boxes(table) + return [key for key, box in boxes if key is not None and box[1] + box[3] <= bottom] + + +def _half_row(table): + """``(key, y)`` of a row Tk shows only in part at the bottom, or None.""" + boxes, bottom = _boxes(table) + for key, box in boxes: + if key is not None and box[1] + 3 < bottom < box[1] + box[3]: + return key, box[1] + 2 + return None + + +def _press(table, sequence, times=1): + """Generated keys go to the window that HAS the keyboard focus, or nowhere. + + Measured 2026-09-29 on a desktop: a window that lost the focus mid-check + dropped every key after it and the check blamed the table ("Down to row 63 + hides it"). So focus is asked for again when it is gone, and a focus that + cannot be had is reported as what it is - the keys were not measured. + """ + tree = table.tree + for _ in range(times): + if tree.focus_get() is not tree: + tree.focus_force() + tree.update() + if tree.focus_get() is not tree: + raise RuntimeError(f"the table lost the keyboard focus to " + f"{tree.focus_get()}, so {sequence} was not measured") + tree.event_generate(sequence) + tree.update() + + +def _own_view_moved(table): + return table.tree.yview()[0] > 0 + + +def _table_problems(table, rows): + """What is wrong with a table of ``rows`` rows at its current size.""" + tree = table.tree + keys = [f"k{i}" for i in range(rows)] + table.sync([(key, (key, str(i))) for i, key in enumerate(keys)]) + table.set_offset(0) + tree.update() + where = f"{rows} rows, {table._fits} in full" + problems = [] + table._on_scrollbar("moveto", "1.0") + if keys[-1] not in _drawn_in_full(table): + problems.append(f"{where}: the scrollbar's end leaves the last row out of sight") + _press(table, "") + _press(table, "") + if keys[-1] not in _drawn_in_full(table) or table.selected_keys() != keys[-1:]: + problems.append(f"{where}: End does not show and select the last row") + _press(table, "") + for i in range(1, rows): + _press(table, "") + if keys[i] not in _drawn_in_full(table) or _own_view_moved(table): + problems.append(f"{where}: Down to row {i} hides it or moves ttk's own view") + break + _press(table, "") + seen = set(_drawn_in_full(table)) + for _ in range(rows): + if table._cursor == keys[-1]: + break + _press(table, "") + seen.update(_drawn_in_full(table)) + if seen != set(keys): + problems.append(f"{where}: PageDown never shows {len(set(keys) - seen)} row(s)") + table.set_offset(0) + tree.update() + half = _half_row(table) + if half is not None: + tree.event_generate("", x=10, y=half[1]) + tree.event_generate("", x=10, y=half[1]) + tree.update() + if (table.selected_keys() != [half[0]] or half[0] not in _drawn_in_full(table) + or _own_view_moved(table)): + problems.append(f"{where}: a click on the half row does not select it in " + f"full, or moves ttk's own view") + table.select_keys(keys[:1]) + table._on_scrollbar("moveto", "1.0") + tree.update() # the queued <> lands here + table._on_scrollbar("moveto", "0.0") + tree.update() + if table.selected_keys() != keys[:1] or not table.copy_text(): + problems.append(f"{where}: a selected row scrolled away and back is not selected") + span = min(rows - 1, table._fits + 3) + _press(table, "") + _press(table, "", span) + if table.selected_keys() != keys[:span + 1]: + problems.append(f"{where}: Shift+Down across a page selected " + f"{len(table.selected_keys())} of {span + 1} rows") + return problems + + +def check_table_viewport(): + """Every row of a table reachable, and the selection kept, on real Tk.""" + root = tk.Tk() + scaling.init_scaling(root) + theme.init_style(root) + root.geometry("700x700") + frame = ttk.Frame(root, width=600, height=400) + frame.pack_propagate(False) + frame.pack(anchor="nw") + table = SortableTree(frame, {"a": "conns.remote_ip", "b": "conns.proto"}) + table.sync([(f"k{i}", (str(i), "")) for i in range(300)]) + root.update() + table.tree.focus_force() + root.update() + problems = [] + head = table.tree.bbox(table._slots[0]) + if root.focus_get() is not table.tree: + problems.append("the table never got the keyboard focus, so no key was measured") + elif not head: + problems.append("Tk gave the first row no box, so nothing here was measured") + else: + # Three sizes: a half row showing at the bottom (the row a click used to + # scroll ttk's own view by), and one where the rows fit exactly. + for in_full, extra in ((4, head[3] // 2), (10, 0), (17, head[3] // 2)): + frame.configure(height=head[1] + head[0] + in_full * head[3] + extra) + table.sync([(f"k{i}", (str(i), "")) for i in range(300)]) + table.set_offset(0) + root.update() + drawn = len(_drawn_in_full(table)) + if table._fits != drawn: + problems.append(f"the table counts {table._fits} rows in full where " + f"Tk draws {drawn}") + for rows in sorted({3, table._fits, table._fits + 1, 20, 200}): + try: + problems += _table_problems(table, rows) + except Exception as exc: # noqa: BLE001 - a crash is a finding + problems.append(f"{rows} rows: the check itself failed on what " + f"it found: {type(exc).__name__}: {exc}") + for problem in problems: + print(f" [tables] {problem}") + print(f" [tables] {'OK' if not problems else f'{len(problems)} problem(s)'}") + _cancel_afters(root) + try: + root.destroy() + except tk.TclError: + pass + return not problems + + def main(argv): # Before ANY Tk root, in the parent and in every per-language child alike: the # setting is per process, and a process that skips it is told 96 DPI by @@ -408,6 +587,8 @@ def main(argv): # the check has to measure the same window. Off Windows it is a no-op, so the # Linux runner's numbers are unchanged. dpi_mode = winenv.set_dpi_awareness() + if "--tables" in argv: + return 0 if check_table_viewport() else 1 if "--lang" in argv: i = argv.index("--lang") code = argv[i + 1] if i + 1 < len(argv) else "en" @@ -430,6 +611,11 @@ def main(argv): [sys.executable, os.path.abspath(__file__), "--lang", code] ).returncode ok = (rc == 0) and ok + # The table viewport once, in a process of its own: it is not a language + # question, and a table left broken here must not hide behind a green text pass. + rc = subprocess.run( + [sys.executable, os.path.abspath(__file__), "--tables"]).returncode + ok = (rc == 0) and ok print(f"GUI render: {'OK' if ok else 'FAIL'}") return 0 if ok else 1 From a30fa5396a04b038df8165a760f2cca2a2dcee10 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 29 Sep 2026 01:22:19 +0200 Subject: [PATCH 2/3] refactor(gui): find the clicked row without a new silent handler _row_under swallowed a failing identify call on its own, which the silent-handler inventory counts. It now goes through _region and key_at, which already answer a dying widget with None. The render check's zip states strict=True, as the lint gate asks. Co-Authored-By: Claude Opus 5.5 --- beantester/gui/widgets/sortable_tree.py | 15 +++++++-------- tools/ci_gui_render.py | 3 ++- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/beantester/gui/widgets/sortable_tree.py b/beantester/gui/widgets/sortable_tree.py index ecfb347..e769742 100644 --- a/beantester/gui/widgets/sortable_tree.py +++ b/beantester/gui/widgets/sortable_tree.py @@ -715,15 +715,14 @@ def _on_key(self, step, extend): def _row_under(self, event): """``(True, position)`` over a row slot, with None for a blank one; - ``(False, None)`` over a heading, a separator or empty space.""" - try: - if self.tree.identify_region(event.x, event.y) not in ("cell", "tree"): - return False, None - slot = self._slot_of(self.tree.identify_row(event.y)) - except Exception: + ``(False, None)`` over a heading, a separator or empty space. + + Built on ``_region`` and ``key_at``, which already answer a dying widget + with None - so no new place swallows an exception on its own. + """ + if self._region(event) not in ("cell", "tree"): return False, None - key = self._slot_keys[slot] if slot >= 0 else None - return True, (None if key is None else self.offset + slot) + return True, self._position_of(self.key_at(event.y)) def _on_press(self, event, extend=False, toggle=False): """A click on a row, chosen by model position - including the half row. diff --git a/tools/ci_gui_render.py b/tools/ci_gui_render.py index 08c3bb7..5daede3 100644 --- a/tools/ci_gui_render.py +++ b/tools/ci_gui_render.py @@ -422,7 +422,8 @@ def _boxes(table): """ tree = table.tree tree.update() - boxes = [(key, tree.bbox(iid)) for iid, key in zip(table._slots, table._slot_keys)] + boxes = [(key, tree.bbox(iid)) + for iid, key in zip(table._slots, table._slot_keys, strict=True)] boxes = [(key, box) for key, box in boxes if box] if not boxes: return [], 0 From 3d4c1f639581d587dd38d41664eef12f9022679e Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Tue, 29 Sep 2026 08:39:45 +0200 Subject: [PATCH 3/3] fix(gui): count table rows in full on Tk 8.6 and keep scrolls cheap The render check on the Linux runner (Tk 8.6.14) found the table counting 5 rows in full where Tk drew 4: Tk 8.6's yview counts the row cut off at the bottom as shown. rows_shown_in_full now asks identify_region where the row area ends, at an x inside the columns that are on screen, so a table scrolled sideways is measured too. _on_configure calls yview() first: Tk 8.6.14 lays a treeview out only when idle, and its bbox and identify read the old size inside . Measured right after 1340 of 1340 resizes on 8.6.14 and 9.0.4, plain and scrolled sideways. Review follow-ups: - select_keys([]) and a click on a blank slot clear the cursor and the anchor, so a Shift range cannot start from a cleared row. - The selection is an ordered dict read as it is. repaint no longer copies it: with 200 000 rows selected a one-row scroll took 9.9 ms. - The README names the right-click menu the same way in both places. The render check gains a wide table scrolled sideways, and reports a table with no row on screen as "nothing was measured". Co-Authored-By: Claude Opus 5.5 --- README.md | 2 +- beantester/gui/widgets/sortable_tree.py | 125 ++++++++++------- tests/test_mutation_registry.py | 64 ++++++++- tests/test_virtual_tables.py | 174 +++++++++++++++++++++--- tools/ci_gui_render.py | 61 ++++++++- 5 files changed, 347 insertions(+), 79 deletions(-) diff --git a/README.md b/README.md index c9d6faa..a66af94 100644 --- a/README.md +++ b/README.md @@ -248,7 +248,7 @@ effect. | `Ctrl+S` / `Ctrl+O` | Save / Load config file | | `Ctrl+L` | Clear the log | | `Ctrl+F` | Search: the field search on Control, the table search on Connections, the socket search on Tools > Sockets | -| `Up` / `Down`, `Page Up` / `Page Down`, `Home` / `End` | In a table: move the selection. With `Shift`, select a range, which can run past one screen. `Ctrl+C` copies the selected rows and `Shift+F10` opens the row menu | +| `Up` / `Down`, `Page Up` / `Page Down`, `Home` / `End` | In a table: move the selection. With `Shift`, select a range, which can run past one screen. `Ctrl+C` copies the selected rows and `Shift+F10` opens the right-click menu | **Finding a setting.** The box at the top of the Control page searches the settings by name - type part of a field or section name, or the command-line flag such as `--loss`. Every match is diff --git a/beantester/gui/widgets/sortable_tree.py b/beantester/gui/widgets/sortable_tree.py index e769742..027eb5e 100644 --- a/beantester/gui/widgets/sortable_tree.py +++ b/beantester/gui/widgets/sortable_tree.py @@ -129,40 +129,49 @@ def fitted_widths(order, visible, natural, current, tree_width, return {c: w for c, w in out.items() if w != now[c]} -def rows_shown_in_full(yview, slots): - """How many slots Tk draws IN FULL, read off its own ``yview`` - or None. +def rows_shown_in_full(first_box, width, height, region_at): + """How many rows Tk draws IN FULL, asked of Tk's own layout - or None. Pure, and tested without Tk, like ``fitted_widths``: the arithmetic is here, - the widget only answers the question it is asked. - - **Why Tk is asked instead of the height being divided.** Everything a table - promises - that the last row can be scrolled into view, how far a page goes, - how big the scrollbar thumb is - hangs on this one number, and the division - got it wrong: ``height // rowheight`` counts the header and the border as rows - (external review, P1-4). Measured 2026-09-29 on Tk 9.0.4 in a 400 px tree: 17 - rows fit, the division said 18, and with the buffer on top the last FIVE rows - of every table could never be shown. What Tk really fits is - ``(height - header - border) // rowheight`` - it held at all 440 heights tried - and at two row heights - but the header and the border belong to the theme - and the DPI, and ``yview`` already reports the result: the fraction of the - slots on screen. It is also the very count ``see`` uses to decide whether to - scroll, so a table sized by it cannot disagree with ttk about what is visible. - - The fraction only carries a count while there are MORE slots than fit, which - is what the buffer guarantees. When every slot fits, all of them are shown in - full and that is the answer. None when there is nothing to read - the caller - keeps what it had. + the widget only answers the questions it is asked. ``first_box`` is the first + slot's ``bbox`` (where the rows start, how tall one is), ``width`` and + ``height`` the widget's, and ``region_at`` is its ``identify_region``. + + Everything a table promises - that the last row can be scrolled into view, how + far a page goes, how big the scrollbar thumb is - hangs on this one number. + The count starts from the height as if nothing sat below the rows, which is + never too few, and steps back while the bottom pixel of the last row it counts + is not a row to Tk: the border under the rows is FOUND, never assumed. With a + border thinner than a row that is one or two questions. + + **Why not the three ways that came before** (measured 2026-09-29 over 470 + heights and 1340 resizes, on Tk 8.6.14 and 9.0.4): + + * ``height // rowheight`` counts the header and the border as rows (external + review, P1-4): the last five rows of every table could never be shown. + * ``yview``, which this read at first, holds only on Tk 9. Tk 8.6 counts the + row cut off at the bottom as shown, and the count ran one too high at 66 + heights of 470 - the render check caught it on the Linux runner. + * the height less the header less a bottom border as wide as the side one + holds until the table is scrolled sideways: the first slot's box then starts + left of the widget (x = -406 measured) and rows that are not there count. + + That last box is also why Tk is asked at an x in the middle of the part of it + that is on screen, never at its left edge. None when there is no box yet (or + a dying widget's empty one) - the caller keeps what it had. At least one row + otherwise, so a page always moves. """ try: - first, last = (float(value) for value in yview) + left, top, span, row_height = (int(value) for value in first_box) except (TypeError, ValueError): return None - if slots <= 0: + if row_height <= 0: return None - span = last - first - if span >= 1.0: - return slots - return max(1, round(span * slots)) + x = (max(left, 0) + min(left + span, int(width))) // 2 + rows = (int(height) - top) // row_height + while rows > 1 and region_at(x, top + rows * row_height - 1) not in ("cell", "tree"): + rows -= 1 + return max(1, rows) BUFFER_ROWS = 4 # slots past the viewport: a partial row + resize slack @@ -221,7 +230,8 @@ def __init__(self, parent, columns, sort=None, on_sort=None, self.offset = 0 # model row rendered in slot 0 self._slots = [] # widget item ids, recycled forever self._slot_keys = [] # slot index -> model key currently shown - self._selected = [] # selected MODEL KEYS (survive a repaint) + self._selected = {} # selected MODEL KEYS in order (a dict: see + # _restore_selection) self._written = () # slot iids _restore_selection last wrote self._cursor = None # model key the keyboard stands on self._anchor = None # model key a Shift range is measured from @@ -375,8 +385,9 @@ def _rows_upper_bound(self): It counts the header and the border as rows, so it is never the number on screen (that is ``_fits``, see ``rows_shown_in_full``). It only sizes the slots, and there it errs the right way: slots must outnumber the rows Tk - can show in full, or ``yview`` has nothing to report. Read from the height - Tk has already given the widget, so it is current inside ````. + can show in full, or there is no row at the bottom of the row area for + ``identify_region`` to find. Read from the height Tk has already given the + widget, so it is current inside ````. """ try: height = int(self.tree.winfo_height() or 0) @@ -393,10 +404,20 @@ def _on_configure(self, _=None): if resized: self._ensure_slots(needed) # Asked on EVERY resize, not only when the slot count moves: the rows that - # fit can change while height // rowheight does not. Measured 2026-09-29 - # over 82 resizes: yview is already current inside this handler. + # fit can change while height // rowheight does not. A treeview is laid out + # when Tk is next idle, and on Tk 8.6.14 (the Linux runner's) bbox and + # identify_region read the layout as it WAS - here, the old size: a table + # grown from 4 rows to 10 went on counting 4. yview first brings a pending + # layout up to date (ttkScroll.c, TtkUpdateScrollInfo); newer Tk does that + # inside bbox and identify too. With it, the count was right after every + # one of 1340 resizes on 8.6.14 and 9.0.4, in steps and in jumps, for a + # table as it is and for one scrolled sideways. try: - fits = rows_shown_in_full(self.tree.yview(), self.window()) + self.tree.yview() + fits = rows_shown_in_full(self.tree.bbox(self._slots[0]), + self.tree.winfo_width(), + self.tree.winfo_height(), + self.tree.identify_region) except Exception as _exc: # a dying widget answers nothing crashlog.note(_exc, "gui.widgets.sortable_tree") fits = None @@ -563,7 +584,6 @@ def repaint(self): blank = ("",) * len(self.columns) window = self.window() visible = self.items[self.offset:self.offset + window] - selected = set(self._selected) for i, iid in enumerate(self._slots): if i < len(visible): item = visible[i] @@ -578,7 +598,7 @@ def repaint(self): if self._painted.get(iid) != (values, tags): self.tree.item(iid, values=values, tags=tags) self._painted[iid] = (values, tags) - self._restore_selection(selected) + self._restore_selection() self._sync_scrollbar() # -- selection (by model key: item ids are recycled) ------------------------ # @@ -611,7 +631,7 @@ def _on_select(self, _=None): if key is not None: # a blank slot maps to no model row keys.append(key) wanted.append(iid) - self._selected = keys + self._selected = dict.fromkeys(keys) # A click can land on a blank slot below the last real row: ttk highlights # it, but it selects nothing. Drop those from the widget selection so an # empty row cannot sit there looking selected. Re-setting the selection @@ -626,8 +646,11 @@ def _on_select(self, _=None): except Exception as _exc: crashlog.note(_exc, "gui.widgets.sortable_tree") - def _restore_selection(self, selected=None): - selected = set(self._selected) if selected is None else selected + def _restore_selection(self): + # A dict, asked as it is and never copied: a range may hold every row of the + # model, and a set rebuilt on each repaint made a one-row scroll 140 times + # dearer with 200 000 rows selected (measured 2026-09-29: 0.07 -> 9.9 ms). + selected = self._selected wanted = tuple(self._slots[i] for i, key in enumerate(self._slot_keys) if key is not None and key in selected) try: @@ -644,9 +667,11 @@ def selected_keys(self): def select_keys(self, keys): index = self._ensure_index() - self._selected = [str(k) for k in keys if str(k) in index] - if self._selected: # the keyboard continues from here - self._cursor = self._anchor = self._selected[0] + self._selected = dict.fromkeys(str(k) for k in keys if str(k) in index) + # The keyboard continues from the first row, and an empty selection leaves + # no cursor and no anchor: a Shift range from a row cleared away would pick + # rows nobody chose. The keys then start from the top row on screen. + self._cursor = self._anchor = next(iter(self._selected), None) self._restore_selection() # -- choosing rows: keyboard and pointer, both in model positions ---------- # @@ -678,14 +703,16 @@ def _choose(self, position, extend=False, toggle=False): anchor = self._position_of(self._anchor) if self._multi and extend else None if anchor is not None: low, high = sorted((anchor, position)) - self._selected = [str(self._key_of(item)) - for item in self.items[low:high + 1]] + self._selected = dict.fromkeys(str(self._key_of(item)) + for item in self.items[low:high + 1]) elif self._multi and toggle: - self._selected = ([k for k in self._selected if k != key] - if key in self._selected else self._selected + [key]) + if key in self._selected: + del self._selected[key] + else: + self._selected[key] = None self._anchor = key else: - self._selected = [key] + self._selected = {key: None} self._anchor = key self._cursor = key self._reveal(position) @@ -744,7 +771,11 @@ def _on_press(self, event, extend=False, toggle=False): if position is not None: self._choose(position, extend=extend, toggle=toggle) elif not (extend or toggle): - self._selected = [] # a blank slot below the rows selects nothing + # A blank slot below the rows selects nothing and, like select_keys(()), + # leaves no cursor or anchor behind - written out, because select_keys + # builds the key index and a click should not cost O(rows). + self._selected = {} + self._cursor = self._anchor = None self._restore_selection() return "break" diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 3ea9d72..fd0d612 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -3466,11 +3466,67 @@ "test": "test_a_click_chooses_by_model_row_and_brings_the_half_row_into_view", }, { - "label": "tables: the rows in full are taken to be every slot", + # The count starts from the height as if nothing sat under the rows; the + # border is found by asking Tk. Without the step back it is a row too many. + "label": "tables: the border under the rows is counted as a row", "file": "beantester/gui/widgets/sortable_tree.py", - "old": " return max(1, round(span * slots))", - "new": " return slots", - "test": "test_the_rows_in_full_are_read_off_tks_own_yview", + "old": (" while rows > 1 and region_at(x, top + rows * row_height - 1) not in (\"cell\", \"tree\"):\n" + " rows -= 1\n"), + "new": "", + "test": "test_the_rows_in_full_are_asked_of_tks_own_layout", + }, + { + # Scrolled sideways, the first row's box starts left of the widget: asked + # at its left edge, Tk finds no row anywhere and the count collapses to one. + "label": "tables: the rows are asked about left of the widget", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " x = (max(left, 0) + min(left + span, int(width))) // 2", + "new": " x = left + 1", + "test": "test_the_rows_in_full_are_asked_of_tks_own_layout", + }, + { + # Columns narrower than the widget leave blank space right of them, and + # the middle of the widget is then no row at all. + "label": "tables: the rows are asked about right of the columns", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " x = (max(left, 0) + min(left + span, int(width))) // 2", + "new": " x = int(width) // 2", + "test": "test_the_rows_in_full_are_asked_of_tks_own_layout", + }, + { + # Tk 8.6.14 lays a treeview out when idle: without yview first, bbox and + # identify inside read the layout of the size before. + "label": "tables: a resize is measured on the layout before it", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " self.tree.yview()\n", + "new": "", + "test": "test_a_resize_is_measured_on_the_new_layout_not_the_old_one", + }, + { + # A set rebuilt from the selection on every repaint: a scroll paid for + # every selected row (200 000 selected: 0.07 -> 9.9 ms per row scrolled). + "label": "tables: every repaint copies the whole selection", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " selected = self._selected\n", + "new": " selected = set(self._selected)\n", + "test": "test_a_huge_selection_does_not_make_every_scroll_pay_for_it", + }, + { + # An emptied selection kept its anchor, and Shift ranged from a row that + # had been cleared away. + "label": "tables: select_keys([]) keeps the old anchor", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " self._cursor = self._anchor = next(iter(self._selected), None)", + "new": (" if self._selected:\n" + " self._cursor = self._anchor = next(iter(self._selected))"), + "test": "test_a_cleared_selection_leaves_no_anchor_behind", + }, + { + "label": "tables: a click on a blank slot keeps the old anchor", + "file": "beantester/gui/widgets/sortable_tree.py", + "old": " self._selected = {}\n self._cursor = self._anchor = None\n", + "new": " self._selected = {}\n", + "test": "test_a_cleared_selection_leaves_no_anchor_behind", }, { # The pixel half of the table tests lives in the render check under Xvfb; diff --git a/tests/test_virtual_tables.py b/tests/test_virtual_tables.py index 935cf19..18f4326 100644 --- a/tests/test_virtual_tables.py +++ b/tests/test_virtual_tables.py @@ -27,12 +27,19 @@ from gui_harness import run_gui # The fake Tk has no geometry, so Tk's answer to "how many rows fit" is injected -# the way a real table receives it: through yview, read at . Five slots -# short of the window is what a real table has - a partial row and the buffer. +# the way a real table receives it: the first row's box and identify_region, read +# at . Five slots short of the window is what a real table has - a +# partial row and the buffer. The row height leaves the division two rows too +# many, so the count has to be found by asking, as on real Tk. INJECT_FITS = """ slots = table.window() fits = slots - 5 - table.tree._yview = (0.0, fits / slots) + top = 25 + row_h = (table.tree.winfo_height() - top) // (fits + 2) + bottom = top + fits * row_h + row_h // 2 # half a row cut off below + table.tree.bbox = lambda item, column=None: (2, top, 600, row_h) + table.tree.identify_region = ( + lambda x, y: "cell" if 2 <= x < 602 and top <= y < bottom else "nothing") table._on_configure() assert table._fits == fits, (table._fits, fits) @@ -208,25 +215,86 @@ def render(item): """) -def test_the_rows_in_full_are_read_off_tks_own_yview(): - """Tk's fraction of the slots on screen IS the count (external review, P1-4). +def _tk_layout(left=2, top=25, columns=408, width=408, height=136, below=2): + """``rows_shown_in_full``'s arguments for a table, answered the way Tk answers. - The height divided by the row height counted the header and the border as - rows. yview is what ``see`` itself goes by, so it cannot disagree with ttk. + ``identify_region`` names a row only inside the row area: under the header, + above the ``below`` pixels of border, over the columns and inside the side + borders. The row height is 22. Returns the arguments and the questions asked. """ - check("17 of 22 slots on screen", rows_shown_in_full((0.0, 17 / 22), 22) == 17, - f"({rows_shown_in_full((0.0, 17 / 22), 22)})") - check("Tk's own float, as it prints it", rows_shown_in_full( - (0.0, 0.7727272727272727), 22) == 17, "") - check("the span counts, not where it starts", - rows_shown_in_full((1 / 22, 18 / 22), 22) == 17, "") - check("every slot fits: all of them are shown in full", - rows_shown_in_full((0.0, 1.0), 14) == 14, "") - check("a widget shorter than a row still shows one", - rows_shown_in_full((0.0, 0.01), 22) == 1, "") - check("no slots: nothing to read", rows_shown_in_full((0.0, 0.5), 0) is None, "") - check("a dying widget's empty answer: nothing to read", - rows_shown_in_full("", 22) is None, "") + asked = [] + + def region_at(x, y): + asked.append((x, y)) + over_rows = max(left, 2) <= x < min(left + columns, width - 2) + return "cell" if over_rows and top <= y < height - below else "nothing" + + return ((left, top, columns, 22), width, height, region_at), asked + + +def test_the_rows_in_full_are_asked_of_tks_own_layout(): + """The border under the rows is FOUND by asking Tk, never assumed. + + ``height // rowheight`` counted the header and the border as rows (external + review, P1-4). ``yview`` then held on Tk 9 only: Tk 8.6 counts the row cut off + at the bottom as shown, and the render check caught the count one too high on + the Linux runner. A bottom border taken to be as wide as the side one counts + rows that are not there once a table is scrolled sideways, because the first + row's box then starts left of the widget. The stand-in answers like Tk, so each + of those shortcuts fails one of the cases below. + """ + cases = ( + ("a half row at the bottom is not counted", {}, 4), + ("rows that fit exactly are all counted", {"height": 25 + 10 * 22 + 2}, 10), + ("a border thicker than a row is found too", + {"height": 25 + 5 * 22 + 30, "below": 30}, 5), + ("a table scrolled sideways is asked where it is on screen", + {"left": -406, "columns": 816}, 4), + ("columns narrower than the widget are asked over a column", + {"columns": 150}, 4), + ("a widget shorter than a row still shows one", {"height": 30}, 1), + ) + for name, layout, expected in cases: + args, asked = _tk_layout(**layout) + found = rows_shown_in_full(*args) + check(name, found == expected, f"({found}, expected {expected}; asked {asked})") + args, asked = _tk_layout() + rows_shown_in_full(*args) + check("a border thinner than a row takes two questions at most", len(asked) <= 2, + f"({asked})") + _, width, height, region_at = args + for box in ("", None, (2, 25, 408, 0)): + check(f"no row to measure ({box!r}): nothing to read", + rows_shown_in_full(box, width, height, region_at) is None, "") + + +def test_a_resize_is_measured_on_the_new_layout_not_the_old_one(): + """A treeview is laid out when Tk is next idle, after has run. + + On Tk 8.6.14, the Linux runner's, bbox and identify_region read the layout as + it was - the OLD size - and a table grown from 4 rows to 10 went on counting 4. + ``yview`` lays a pending layout out first, so the table calls it before it + measures. The stand-in answers like 8.6.14: from the old layout until yview. + """ + run_gui(""" + table = app.pages["connections"].table + tree = table.tree + top, row_h = 25, 20 + height = [top + 10 * row_h + 2] + laid_out = [top + 4 * row_h + 2] # the size before the resize + + def yview(*args): + laid_out[0] = height[0] + return (0.0, 1.0) + + tree.yview = yview + tree.winfo_height = lambda: height[0] + tree.bbox = lambda item, column=None: (2, top, 600, row_h) + tree.identify_region = ( + lambda x, y: "cell" if top <= y < laid_out[0] - 2 else "nothing") + table._on_configure() + assert table._fits == 10, ("measured on the layout before the resize", table._fits) + """) def test_scrolling_moves_the_window_and_stays_in_range(): @@ -418,6 +486,72 @@ def click(slot, sequence=""): """) +def test_a_cleared_selection_leaves_no_anchor_behind(): + """After the selection is cleared, Shift starts afresh from the top row on screen. + + ``select_keys([])`` and a click on a blank slot emptied the selection but kept + the cursor and the anchor, so the next Shift+Down selected a range from a row + that had been cleared away - rows nobody chose, and nothing on screen said + where the range came from. + """ + run_gui(""" + import types + table = app.pages["connections"].table + table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) + for i in range(100)]) + """ + INJECT_FITS + """ + key("") + for _ in range(3): + key("") + assert len(table.selected_keys()) == 4, table.selected_keys() + table.select_keys([]) + key("") + assert table.selected_keys() == ["k0"], ("select_keys([])", table.selected_keys()) + + table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) for i in range(3)]) + key("") + key("") + key("") + assert len(table.selected_keys()) == 3, table.selected_keys() + table.tree.identify_region = lambda x, y: "cell" + table.tree.row_at = table._slots[5] # a blank slot below the rows + for handler in table.tree.bindings[""]: + handler(types.SimpleNamespace(x=10, y=5)) + assert table.selected_keys() == [] + key("") + assert table.selected_keys() == ["k0"], ("blank click", table.selected_keys()) + """) + + +def test_a_huge_selection_does_not_make_every_scroll_pay_for_it(): + """A scroll costs the rows on screen, however many rows are selected. + + A Shift range can hold every row of the model, and each repaint rebuilt a set + of the selected keys: with 200 000 rows selected a one-row scroll went from + 0.07 to 9.9 ms (measured 2026-09-29 on Tk 9.0.4), and every live refresh paid + the same. Time is a noisy witness on a shared runner; memory is not - a set of + 100 000 keys is megabytes, the rows on screen are kilobytes. + """ + run_gui(""" + import tracemalloc + table = app.pages["connections"].table + table.sync([(f"k{i}", (str(i), "", "", "", "", "", "", "", "")) + for i in range(100_000)]) + """ + INJECT_FITS + """ + key("") + key("") + assert len(table.selected_keys()) == 100_000, len(table.selected_keys()) + table.set_offset(0) # End left it at the bottom + tracemalloc.start() + for _ in range(3): + table.scroll_by(1) + peak = tracemalloc.get_traced_memory()[1] + tracemalloc.stop() + assert table.offset == 3, table.offset + assert peak < 512 * 1024, f"three one-row scrolls allocated {peak} bytes" + """) + + def test_the_render_check_measures_the_table_viewport_on_real_tk(): """The pixel half of the tests above runs in CI, not here. diff --git a/tools/ci_gui_render.py b/tools/ci_gui_render.py index 5daede3..6b555c6 100644 --- a/tools/ci_gui_render.py +++ b/tools/ci_gui_render.py @@ -412,13 +412,19 @@ def scan(): # table works with is the one Tk draws (external review, P1-4 and P2-18). # Run against the bug before it was trusted (2026-09-29): with `max_offset` put # back on `len - window()` it reports the last rows out of sight, and with the -# echo guard in `_on_select` removed it reports the selection lost. - -def _boxes(table): +# echo guard in `_on_select` removed it reports the selection lost. Its first run +# on the Linux runner found a real one: Tk 8.6's yview counts the half row as +# shown, so the table counted 5 rows in full where Tk drew 4. +# The rows in full are judged here by geometry - the bottom border as wide as the +# side one - and the table asks identify_region: two ways, so one cannot hide a +# fault of the other. + +def _boxes(table, side=None): """``(key, box)`` for every slot Tk draws at all, and the rows' bottom edge. The edge is read off whichever slot has a box: when ttk's own view has moved - - the very fault measured here - the first slot has none. + - the very fault measured here - the first slot has none. ``side`` is the side + border, for a table scrolled sideways: its boxes start left of the widget. """ tree = table.tree tree.update() @@ -428,15 +434,55 @@ def _boxes(table): if not boxes: return [], 0 # the bottom border is as wide as the side one, where the first column starts - return boxes, tree.winfo_height() - boxes[0][1][0] + return boxes, tree.winfo_height() - (boxes[0][1][0] if side is None else side) -def _drawn_in_full(table): +def _drawn_in_full(table, side=None): """Model keys of the rows Tk draws IN FULL, top to bottom.""" - boxes, bottom = _boxes(table) + boxes, bottom = _boxes(table, side) return [key for key, box in boxes if key is not None and box[1] + box[3] <= bottom] +def _sideways_problems(root, before): + """A table scrolled sideways counts the rows it shows at every height. + + Its first row's box then starts left of the widget (x = -406 measured), so a + count asked at that box's left edge finds no row anywhere and collapses to + one. Every height across one row is tried, so a half row is on screen in most. + ``before`` is the first table's frame, taken away first: at 150% the two do + not fit in the window together, and a squeezed table shows no row at all. + """ + before.pack_forget() + frame = ttk.Frame(root, width=400, height=300) + frame.pack_propagate(False) + frame.pack(anchor="nw") + columns = {f"c{i}": "conns.remote_ip" for i in range(8)} + table = SortableTree(frame, columns, horizontal=True) + table.sync([(f"k{i}", tuple(str(i) for _ in columns)) for i in range(300)]) + root.update() + head = table.tree.bbox(table._slots[0]) + table.tree.xview_moveto(0.5) + root.update() + scrolled = table.tree.bbox(table._slots[0]) + problems = [] + if not head or not scrolled or scrolled[0] >= 0: + problems.append("the wide table did not scroll sideways, so nothing was measured") + else: + for extra in range(head[3]): + frame.configure(height=200 + extra) + root.update() + drawn = len(_drawn_in_full(table, side=head[0])) + if not drawn: + problems.append(f"the wide table shows no row in full at " + f"{table.tree.winfo_height()} px, so nothing was measured") + break + if table._fits != drawn: + problems.append(f"scrolled sideways, the table counts {table._fits} " + f"rows in full where Tk draws {drawn}") + frame.destroy() + return problems + + def _half_row(table): """``(key, y)`` of a row Tk shows only in part at the bottom, or None.""" boxes, bottom = _boxes(table) @@ -566,6 +612,7 @@ def check_table_viewport(): except Exception as exc: # noqa: BLE001 - a crash is a finding problems.append(f"{rows} rows: the check itself failed on what " f"it found: {type(exc).__name__}: {exc}") + problems += _sideways_problems(root, frame) for problem in problems: print(f" [tables] {problem}") print(f" [tables] {'OK' if not problems else f'{len(problems)} problem(s)'}")