diff --git a/CHANGELOG-INTERNAL.md b/CHANGELOG-INTERNAL.md index 86d1831..9ef8526 100644 --- a/CHANGELOG-INTERNAL.md +++ b/CHANGELOG-INTERNAL.md @@ -42,6 +42,48 @@ 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: 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 +".!toplevel2"` at `gui/windows.py::_save_geometry` and `TclError: invalid command name ".!label"` +at `gui/labels.py::_resize`. They share one cause: `App._build_ui` rebuilds the main UI by +destroying every child of the root window, and a `Toplevel` is a child of the root. + +- `App._build_ui` now skips the Toplevels the registry owns (new `WindowManager.toplevels()`). It + used to tear the open panel windows down behind the registry's back, so the `windows.rebuild()` + that follows a language switch ran `close()` on windows that no longer existed. That cost two + things, not one: the TclError above, and the geometry was never saved - the window reopened where + it had last been CLOSED, not where the user had just put it. +- `PanelWindow._save_geometry` returns early when the window is already gone (`winfo_exists`). + Nothing in the app should destroy a registry window behind its back now, but the root still takes + every Toplevel with it when the app quits. +- `bind_wraplength._resize` returns early when the label is gone. The `` binding lives on + the CONTAINER, which routinely outlives the label: the two banners built with + `wrapping_label(root, ...)` in `gui/app.py` hang off the root window itself, and every rebuild + destroys them and leaves their handler behind. Nothing unbinds it (it is added with `add="+"`, and + `Misc.unbind(seq, funcid)` still clears the WHOLE sequence on the oldest Python in the CI matrix - + verified only that 3.14 removes just the one binding). So this was never a one-off teardown race: + it is one dead handler per rebuild, each recording an entry on every resize from then on. +- `on_close` already closed the windows before persisting the UI state, so their geometry survives a + quit. That ORDER had no test; it has one now. +- `tests/fake_tk.py` made honest about destruction, or none of this could be caught: `destroy()` now + destroys children and leaves the widget DEAD, `winfo_exists()` is real, and `configure()` / + `geometry()` raise `TclError` afterwards, the way Tk does. `winfo_exists` HAD to be added rather + than inherited: `W.__getattr__` answers any unknown attribute with a no-op returning `None`, so a + `winfo_exists()` guard would have read as "destroyed" for every widget on the fake and silently + switched off the code it guards in every GUI test. +- New tests: `tests/test_windows.py::test_a_language_switch_keeps_the_window_alive_and_saves_its_geometry`, + `tests/test_windows.py::test_closing_a_window_that_is_already_gone_is_not_a_crash`, + `tests/test_windows.py::test_closing_the_app_saves_an_open_window_before_the_root_goes`, + `tests/test_gui_release_fixes.py::test_a_resize_after_the_label_is_gone_is_not_a_crash`. All four + 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. + ### Fixed: the connections "impaired?" column and its row highlight now use ONE signal - Field report: rows showed orange with the column reading "no" (and "yes" rows with no colour) - diff --git a/CHANGELOG.md b/CHANGELOG.md index 1dcc24b..a5f1c97 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,11 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol ### Fixed +- **A window you have moved now keeps its place when you switch language.** Changing the language + rebuilds the whole main window, and a smaller window open at the time - Settings, for example - + was torn down with it. It came back at the size and position it had the last time you closed it, + quietly throwing away wherever you had just dragged it. It now stays where you put it. + - **Setting a process target no longer makes the first start pause.** Working out which process owns each connection could take a second or two the first time, because it fell back to scanning every process on the system. It now resolves only what it needs, so starting - and typing a diff --git a/beantester/gui/app.py b/beantester/gui/app.py index 587b48d..4019750 100644 --- a/beantester/gui/app.py +++ b/beantester/gui/app.py @@ -308,7 +308,14 @@ def _build_ui(self): """Build (or rebuild on a language change) the whole UI.""" root = self.root set_language(self._lang) + # A rebuild owns the MAIN window's widgets - not the registry's windows. + # A Toplevel is a child of the root too, so destroying every child used to + # take the open panel windows with it, leaving WindowManager to close + # windows that were already gone (see WindowManager.toplevels). + owned = self.windows.toplevels() for widget in root.winfo_children(): + if widget in owned: + continue widget.destroy() pad = scaled(14) diff --git a/beantester/gui/labels.py b/beantester/gui/labels.py index 5a9784e..41922f1 100644 --- a/beantester/gui/labels.py +++ b/beantester/gui/labels.py @@ -20,6 +20,17 @@ def bind_wraplength(label, container=None, pad=16): def _resize(event=None): try: + # The binding lives on the CONTAINER, and the container routinely + # outlives the label: App._build_ui() destroys the labels on every + # rebuild while the root window they hang off stays. Nothing unbinds + # this (it is added with add="+", and unbinding by funcid still clears + # the whole sequence on the oldest Python in the CI matrix), so a dead + # label is not an edge case here - it is the steady state after the + # first language switch. Without this check every resize afterwards + # raised TclError("invalid command name") into the crash log, once per + # dead label, forever. + if not label.winfo_exists(): + return width = int(getattr(event, "width", 0) or holder.winfo_width() or 0) if width > scaled(80): label.config(wraplength=max(scaled(80), width - scaled(pad))) diff --git a/beantester/gui/windows.py b/beantester/gui/windows.py index bc11f5a..fdf67fb 100644 --- a/beantester/gui/windows.py +++ b/beantester/gui/windows.py @@ -193,10 +193,21 @@ def _restore_geometry(self): crashlog.note(_exc, "gui.windows") def _save_geometry(self): - if self.win is None: + """Remember where the user left this window - if it is still there to ask. + + A Toplevel is a CHILD OF ROOT, so anything that destroys the root takes + this window with it while ``self.win`` still points at it. Asking a + destroyed window for its geometry raises TclError ("bad window path + name"), which was then caught and RECORDED - a crash-log entry per close, + for a window that was simply already gone. + """ + win = self.win + if win is None: return try: - self.app.ui.set(self._state_key(), self.win.geometry()) + if not win.winfo_exists(): + return + self.app.ui.set(self._state_key(), win.geometry()) except Exception as _exc: crashlog.note(_exc, "gui.windows") @@ -255,3 +266,16 @@ def rebuild(self): def open_ids(self): return [wid for wid, panel in self._open.items() if panel.is_open()] + + def toplevels(self): + """The Toplevel widgets this registry currently owns. + + ``App._build_ui`` rebuilds the main UI by destroying every child of the + root window - and a Toplevel is a child of the root. That tore the open + windows down behind the registry's back, so ``rebuild()`` then ran + ``close()`` on windows that no longer existed: their geometry was never + saved (after a language switch the window came back where it used to be, + not where the user had put it) and reading it raised into the crash log. + The rebuild skips these; closing them is the registry's job. + """ + return {panel.win for panel in self._open.values() if panel.win is not None} diff --git a/tests/fake_tk.py b/tests/fake_tk.py index 38be310..13f99b2 100644 --- a/tests/fake_tk.py +++ b/tests/fake_tk.py @@ -46,13 +46,29 @@ def __init__(self, *args, **kw): self.grid_info = None self.bindings = {} self.states = set() # ttk widget state flags (active, focus, ...) + self.alive = True master = args[0] if args else kw.get("master") self.master = master if isinstance(master, W) else None if isinstance(master, W): master.children.append(self) + def _path(self): + return f".!{type(self).__name__.lower()}" + + def _alive_or_raise(self, reason="invalid command name"): + """Real Tk deletes the widget's command: touching it afterwards raises. + + Only the two calls that the real teardown crashes made - ``configure`` and + ``geometry`` - are modelled. The rest stay permissive on purpose: this is a + test double, not a second implementation of Tk, and every method that + starts raising is a way for an unrelated test to fail for a fake reason. + """ + if not self.alive: + raise TclError(f'{reason} "{self._path()}"') + # -- options ------------------------------------------------------------ # def configure(self, *a, **kw): + self._alive_or_raise() self.kw.update(kw) config = configure @@ -123,9 +139,31 @@ def unbind(self, sequence, funcid=None): unbind_all = unbind def destroy(self): + """Destroy the widget AND its children, and leave it DEAD. + + The old version only unlinked the widget from its parent, so a widget that + had been destroyed still answered every call - and a callback poking a dead + widget looked perfectly healthy here while raising TclError for the user. + Two shipped crashes hid behind exactly that (``gui/labels.py::_resize`` and + ``gui/windows.py::_save_geometry``): the handler lives on a CONTAINER that + outlives the widget it was given, so it keeps firing after the destroy. + """ + for child in list(self.children): + child.destroy() if self.master is not None and self in self.master.children: self.master.children.remove(self) self.children = [] + self.alive = False + + def winfo_exists(self): + """1 while the widget lives, 0 once it is destroyed. + + Explicit, because ``W.__getattr__`` answers any unknown attribute with a + no-op returning ``None``: a guard written as ``if not w.winfo_exists()`` + would then be TRUE for every widget on the fake, and silently turn the + code it guards off in every GUI test. + """ + return 1 if self.alive else 0 def winfo_children(self): return list(self.children) @@ -236,8 +274,12 @@ def state(self, spec=None): return "normal" def geometry(self, spec=None): + # Tk answers a wm call on a destroyed toplevel with "bad window path + # name", not "invalid command name" - and that is the exact message the + # recorded _save_geometry crash carried. + self._alive_or_raise("bad window path name") if spec is None: - return self.winfo_geometry() + return self.kw.get("geometry") or self.winfo_geometry() self.kw["geometry"] = spec return None diff --git a/tests/test_gui_release_fixes.py b/tests/test_gui_release_fixes.py index be41299..66e567c 100644 --- a/tests/test_gui_release_fixes.py +++ b/tests/test_gui_release_fixes.py @@ -262,6 +262,40 @@ def test_long_notes_wrap_instead_of_being_cut(): """) +def test_a_resize_after_the_label_is_gone_is_not_a_crash(): + """``wrapping_label`` binds on the CONTAINER, not on the label. + + The container routinely outlives the label - the two banners at app.py:395 and + :403 hang off the root window itself, and every ``_build_ui`` rebuild destroys + the labels and leaves their handlers behind. Nothing unbinds them, so the next + resize called ``configure`` on a destroyed widget: TclError("invalid command + name .!label"), recorded once per dead handler, and one more handler added per + rebuild. + """ + run_gui(""" + import tkinter as tk + from beantester import crashlog + from beantester.gui.labels import wrapping_label + + recorded = [] + crashlog.record = lambda exc, **kw: recorded.append( + (type(exc).__name__, kw.get("subsystem"))) + + holder = tk.Frame(app.root) + label = wrapping_label(holder, "a note long enough to wrap") + assert label.kw.get("wraplength"), "the label must wrap while it lives" + + handlers = holder.bindings.get("") or [] + assert handlers, "wrapping_label must listen on its container" + + label.destroy() + for handler in handlers: # the resize still arrives + handler(type("Event", (), {"width": 500})()) + + assert [r for r in recorded if r[1] == "gui.labels"] == [], recorded + """) + + def test_the_connection_table_has_no_stretch_columns(): """A stretch column is recomputed by ttk and snaps back after a drag.""" run_gui(""" diff --git a/tests/test_windows.py b/tests/test_windows.py index d30e84d..406ad6b 100644 --- a/tests/test_windows.py +++ b/tests/test_windows.py @@ -102,6 +102,107 @@ def build(self, body): """, lang="pl") +def test_a_language_switch_keeps_the_window_alive_and_saves_its_geometry(): + """A rebuild must not tear the registry's windows down behind its back. + + ``App._build_ui`` rebuilds by destroying every child of the root window, and a + Toplevel is one of those children - so the open windows died there, and the + ``WindowManager.rebuild()`` that came next ran ``close()`` on windows that no + longer existed. Two things followed: the geometry was never saved (the window + reopened where it had been LAST closed, not where the user had just put it), + and reading it raised TclError into the crash log - the recorded + "bad window path name .!toplevel2", once per language switch. + """ + run_gui(""" + from beantester import crashlog + from beantester.gui.windows import PanelWindow, register_window + + # Filtered by subsystem, not "nothing at all": a language switch logs a + # line, and the fake tkinter's log box answers index() with None, which + # records an unrelated gui.app entry on master too. + recorded = [] + crashlog.record = lambda exc, **kw: recorded.append( + (type(exc).__name__, kw.get("subsystem"))) + + class Probe(PanelWindow): + ID = "probe_geo" + TITLE = "app.tabs.connections" + def build(self, body): + pass + + register_window(Probe) + panel = app.open_window("probe_geo") + panel.win.geometry("640x480+11+22") # the user moved it there + + app.lang_var.set("English") + app._switch_language() + + assert [r for r in recorded if r[1] == "gui.windows"] == [], recorded + assert app.ui.get("window.probe_geo") == "640x480+11+22", app.ui.get("window.probe_geo") + assert app.windows.open_ids() == ["probe_geo"], "the window must come back" + """, lang="pl") + + +def test_closing_a_window_that_is_already_gone_is_not_a_crash(): + """Defence in depth for the recorded "bad window path name .!toplevel2". + + Nothing in the app should destroy a registry window behind its back now, but + the root window still takes every Toplevel with it whenever it goes - so + ``close()`` has to cope with a window that is already destroyed instead of + reading its geometry and recording the TclError. + """ + run_gui(""" + from beantester import crashlog + from beantester.gui.windows import PanelWindow, register_window + + recorded = [] + crashlog.record = lambda exc, **kw: recorded.append( + (type(exc).__name__, kw.get("subsystem"))) + + class Probe(PanelWindow): + ID = "probe_gone" + TITLE = "app.tabs.statistics" + def build(self, body): + pass + + register_window(Probe) + panel = app.open_window("probe_gone") + panel.win.destroy() # destroyed from under the panel + + panel.close() + + assert [r for r in recorded if r[1] == "gui.windows"] == [], recorded + assert not panel.is_open() + """) + + +def test_closing_the_app_saves_an_open_window_before_the_root_goes(): + """Quitting destroys the root, and a destroyed root takes every Toplevel with + it - so a window still open at that moment has nowhere left to save from. + + ``on_close`` closes them first for exactly that reason. It had no test, which + is how the ORDER (close the windows, then persist, then destroy) could have + been rearranged without anything noticing. + """ + run_gui(""" + from beantester.gui.windows import PanelWindow, register_window + + class Probe(PanelWindow): + ID = "probe_quit" + TITLE = "app.tabs.statistics" + def build(self, body): + pass + + register_window(Probe) + panel = app.open_window("probe_quit") + panel.win.geometry("500x400+7+8") + + app.on_close() + + assert app.ui.get("window.probe_quit") == "500x400+7+8", app.ui.get("window.probe_quit") + """) + + def test_a_broken_window_cannot_kill_the_tick(): """The tick drains the log and moves the statistics. One bad window must not take it down - that was the whole point of wrapping _tick in try/finally."""