From d1f45375dcaac87b5883e4b97e1d00c8e3f43f93 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Fri, 24 Jul 2026 18:43:45 +0200 Subject: [PATCH] fix(gui): bind the root handler once, not per rebuild App._build_ui runs on every language switch and the root window outlives it, so binding on the root inside _build_ui (add="+", nothing removing it) accumulated one handler per rebuild: measured 2 after the first build, 8 after three switches. The earlier "not fixed here" note named only _on_root_configure and undercounted - the two banners built with wrapping_label(root, ...) each bound their own on the same persistent root and multiplied identically. - _on_root_configure is bound once in __init__; the line is gone from _build_ui. It reaches its widgets through self, so one binding always drives the freshly rebuilt ones - no unbind (which is not safe on the oldest Python in the CI matrix anyway). - The two banners are plain ttk.Labels wrapped by that same handler instead of wrapping_label; same look, no leak. labels.py and its other callers untouched. - Not user-visible (identical look/wrapping), so CHANGELOG.md is left alone. New test asserts exactly one handler on the root across rebuilds and that the banner still wraps to the width-derived value; both halves verified by mutation. Full suite green (660 passed). Co-Authored-By: Claude Opus 4.8 --- CHANGELOG-INTERNAL.md | 37 ++++++++++++++++++++++++--- beantester/gui/app.py | 44 ++++++++++++++++++++++++++++----- tests/test_gui_release_fixes.py | 36 +++++++++++++++++++++++++++ 3 files changed, 107 insertions(+), 10 deletions(-) diff --git a/CHANGELOG-INTERNAL.md b/CHANGELOG-INTERNAL.md index 9ef8526..2f0ec8f 100644 --- a/CHANGELOG-INTERNAL.md +++ b/CHANGELOG-INTERNAL.md @@ -42,6 +42,36 @@ a `### BREAKING` section placed FIRST in that version, and each such line is pre - Help text and the flag tables in both READMEs now state that the flag is valid on its own. - Version bump deliberately NOT taken (convention 34): the owner closes it in `VERSION.txt`. +### Fixed: the UI rebuild no longer piles up `` handlers on the root + +Follow-up to the teardown-crash fix. `App._build_ui` runs on every language switch, and the root +window outlives it, so binding `` on the root inside `_build_ui` (with `add="+"`, nothing +removing it) accumulated one handler per rebuild. Measured on the fake tkinter: 2 after the first +build, 8 after three switches, linear. Each is cheap - and after the teardown fix the dead ones are +no-ops, not crash records - but it is O(rebuilds) work on every resize for the life of the process. + +- The earlier "not fixed here" note named only `_on_root_configure` and undercounted (convention 5): + the two banners built with `wrapping_label(root, ...)` (`engine_warning` always, `admin_warning` + when non-admin) each bound their OWN `` on the same persistent root and multiplied + identically - half the handlers. A `wrapping_label` on a SHORT-LIVED container does not leak (its + binding dies with the container); only the two whose container is the root do. +- `_on_root_configure` is now bound ONCE in `__init__`, before the first `_build_ui`, and the line is + gone from `_build_ui`. It reaches its widgets through `self`, so a single binding always drives the + freshly rebuilt ones - no unbind needed, and none was safe (`Misc.unbind(seq, funcid)` still clears + the whole sequence on the oldest Python in the CI matrix, and the root carries other `` + bindings). +- The two banners are now plain `ttk.Label`s wrapped by that same single handler, not `wrapping_label`. + Same look (left/anchored, an initial `wraplength` refined on the first resize). `gui/labels.py` and + its short-lived-container callers are untouched; `wrapping_label` is no longer imported by + `gui/app.py`. +- Not user-visible (identical look and wrapping, no behaviour change), so CHANGELOG.md is deliberately + left alone - a user-facing line for an imperceptible change would be the filler convention 4 forbids. +- New test: `tests/test_gui_release_fixes.py::test_the_ui_rebuild_does_not_pile_up_configure_handlers_on_the_root` - + asserts exactly one `` handler on the root after several rebuilds, and that the banner + still wraps to the width-derived value (not just its build-time default, which would pass even with + the fold gone). Both halves verified by MUTATION: reintroducing the per-rebuild bind, and dropping + the banner wrap, each turns it red. + ### Fixed: two swallowed GUI teardown crashes, and the window geometry they were losing From a field `crashes.ndjson`: two `swallowed`/`debug` records, `TclError: bad window path name @@ -79,10 +109,9 @@ destroying every child of the root window, and a `Toplevel` is a child of the ro verified by MUTATION (convention 5): each guard was removed in turn and the test that claims to catch it went red. The `on_close` mutation is what showed the fourth test was originally guarding a duplicate call added in this chunk rather than the real one; the duplicate was removed. -- Not fixed here: `App._build_ui` also adds a fresh `root.bind("", self._on_root_configure, - add="+")` on every rebuild and never removes the old one, so that handler multiplies the same way. - It holds the App rather than a dead widget, so it costs duplicated work per resize, not crash - records. Left alone deliberately, not overlooked. +- Left for a follow-up (now DONE, see the entry above): `App._build_ui` also re-bound `` + on the root every rebuild, so that handler multiplied too - as did the banners, which this note + missed. ### Fixed: the connections "impaired?" column and its row highlight now use ONE signal diff --git a/beantester/gui/app.py b/beantester/gui/app.py index 4019750..176c9f4 100644 --- a/beantester/gui/app.py +++ b/beantester/gui/app.py @@ -54,7 +54,6 @@ from .pages import PAGES from .profiles import ProfileStore from .rates import PeakWindow -from .labels import wrapping_label from .scaling import (geometry_fits, init_scaling, initial_geometry, max_window_size, min_window_size, scaled) from .scrollable import WheelDispatcher @@ -207,6 +206,12 @@ def __init__(self, root): self.admin_warning = None self._init_vars() + # Bind ONCE, for the App's lifetime - not inside _build_ui. The handler + # reads the widgets it wraps through self, so a single binding always sees + # the freshly rebuilt ones; re-adding it on every rebuild (with add="+", on + # the root, which outlives the rebuild) left one dead-weight copy per + # language switch. See _on_root_configure. + root.bind("", self._on_root_configure, add="+") self._build_ui() self._restore_last_profile() # one-time, only if the preference is on self._wheel = WheelDispatcher(root) @@ -398,18 +403,27 @@ def _build_ui(self): # it up front, as a banner, instead of only a line in the log strip at the # bottom (easy to miss) and a dialog after the click. Same treatment as the # other "your run is not doing what you think" warnings. + # These two banners are the ONLY wrapping labels whose container is the + # persistent root, so - unlike a field note, whose container dies with it - + # wrapping_label's per-label binding would pile up on the root, + # one per rebuild. They are plain labels instead, wrapped by the single + # _on_root_configure below (which already does exactly this for the + # summary). Same look as wrapping_label: left/anchored, an initial + # wraplength refined on the first resize. if not self._is_admin: - self.admin_warning = wrapping_label(root, T("warn.not_admin"), - style="Bad.TLabel") + self.admin_warning = ttk.Label( + root, text=T("warn.not_admin"), style="Bad.TLabel", + justify="left", anchor="w", wraplength=scaled(600)) self.admin_warning.pack(side="top", fill="x", padx=pad, pady=(0, scaled(4))) # A queue overflow means the TOOL is dropping the user's packets - packets # they did not ask to lose - so their measured loss is partly ours. That was # a number in a table on a page they might never open. It is now a banner: # a run whose numbers are WRONG must not look like a run that went fine. - self.engine_warning = wrapping_label(root, "", style="Bad.TLabel") + self.engine_warning = ttk.Label( + root, text="", style="Bad.TLabel", + justify="left", anchor="w", wraplength=scaled(600)) self._shown_engine_warning = None - root.bind("", self._on_root_configure, add="+") # The bottom strip (START/STOP + log) is packed FIRST, against the bottom # edge, so the notebook can never take its space. It used to share a @@ -511,13 +525,31 @@ def _bind_shortcuts(self): crashlog.note(_exc, "gui.app") def _on_root_configure(self, event=None): - """Keep the summary wrapping to the real window width (not a fixed 620 px).""" + """Keep the root-level labels wrapping to the real window width. + + Bound ONCE for the App's lifetime (in __init__), never inside _build_ui: + it reaches its widgets through self, so one binding always drives the + freshly rebuilt ones. Binding it per rebuild - on the root, which the + rebuild does not destroy - left a dead-weight copy behind on every language + switch. The two root banners wrap here for the same reason: as + wrapping_label they each bound their own on that same + persistent root and multiplied identically. + """ try: width = self.summary_holder.winfo_width() if width and width > scaled(80): self.summary.config(wraplength=width - scaled(8)) except Exception as _exc: crashlog.note(_exc, "gui.app") + try: + root_width = self.root.winfo_width() + if root_width and root_width > scaled(80): + wrap = max(scaled(80), root_width - scaled(28)) # padx both sides + margin + for banner in (self.admin_warning, self.engine_warning): + if banner is not None: + banner.config(wraplength=wrap) + except Exception as _exc: + crashlog.note(_exc, "gui.app") # -- language --------------------------------------------------------------- # def _switch_language(self): diff --git a/tests/test_gui_release_fixes.py b/tests/test_gui_release_fixes.py index 66e567c..43c4f4b 100644 --- a/tests/test_gui_release_fixes.py +++ b/tests/test_gui_release_fixes.py @@ -262,6 +262,42 @@ def test_long_notes_wrap_instead_of_being_cut(): """) +def test_the_ui_rebuild_does_not_pile_up_configure_handlers_on_the_root(): + """`_build_ui` runs on every language switch, and the root window outlives it. + + It used to bind `` on the root each time - once for + `_on_root_configure`, and once more for every banner built with + `wrapping_label(root, ...)` - all with `add="+"` and nothing to remove them. So + the handlers multiplied one per rebuild: measured 2 after the first build, 8 + after three language switches. Each is cheap, but it is O(rebuilds) work on + every resize, forever. The handler is now bound once in __init__, and the + banners wrap through it instead of self-binding, so the count is fixed at one. + """ + run_gui(""" + def configure_handlers(): + return root.bindings.get("", []) + + assert len(configure_handlers()) == 1, configure_handlers() + + for _ in range(4): # what four language switches do + app._lang = "en" if app._lang == "pl" else "pl" + app._build_ui() + + assert len(configure_handlers()) == 1, ( + "the rebuild leaked a handler onto the root", configure_handlers()) + + # ...and the banner still wraps: the leak was closed by folding it into the + # single handler, not by dropping the wrapping. Assert the width-derived + # value, not just "truthy" - a banner keeps its initial build-time + # wraplength, so only checking for one would pass even if the fold were gone. + from beantester.gui.scaling import scaled + app.engine_warning.config(text="a long overflow warning that has to wrap") + app._on_root_configure() + assert app.engine_warning.kw.get("wraplength") == root.winfo_width() - scaled(28), ( + app.engine_warning.kw.get("wraplength"), root.winfo_width()) + """) + + def test_a_resize_after_the_label_is_gone_is_not_a_crash(): """``wrapping_label`` binds on the CONTAINER, not on the label.