From 8a782de6f11e60a43be80b2ab6d5aaf9a3ecf1c3 Mon Sep 17 00:00:00 2001 From: nelsonduarte Date: Sat, 11 Jul 2026 15:37:20 +0100 Subject: [PATCH] fix(viewer,editor): track connected window to avoid screenChanged disconnect warning PR-J's DPR change handler called disconnect+connect on every showEvent, which raised a RuntimeWarning ("Failed to disconnect from signal screenChanged") on the first show and any re-show where the signal was never actually connected. PySide6 6.11 emits a RuntimeWarning (not an exception) for the "signal was never connected" case, so contextlib.suppress(TypeError, RuntimeError) could not silence it. Now showEvent tracks the previously-connected window in _screen_signal_window and only disconnects when the window actually changes (e.g., widget was reparented to a different top-level). First show, same-window re-show, and reparent are all handled without the warning. Applied identically to _SelectCanvas (viewer) and PdfEditCanvas (editor). Co-Authored-By: Claude Opus 4.7 --- app/editor/canvas.py | 22 +++- app/viewer/canvas.py | 22 +++- tests/test_screen_changed_binding.py | 152 +++++++++++++++++++++++++++ 3 files changed, 188 insertions(+), 8 deletions(-) create mode 100644 tests/test_screen_changed_binding.py diff --git a/app/editor/canvas.py b/app/editor/canvas.py index 1afada3..11c626d 100644 --- a/app/editor/canvas.py +++ b/app/editor/canvas.py @@ -177,6 +177,12 @@ def __init__(self): self._current_stroke = None # list of (sx, sy) screen coords while drawing self._stroke_page = -1 self._open_note = None + # Tracks the QWindow whose screenChanged signal we're currently + # connected to (see showEvent). Avoids calling disconnect() on + # a signal that was never connected — which raises a PySide6 + # RuntimeWarning under 6.11 instead of an exception, so it + # can't be caught with contextlib.suppress. + self._screen_signal_window = None self.setMouseTracking(True) self.setCursor(Qt.CursorShape.CrossCursor) self.setMinimumSize(300, 400) @@ -536,12 +542,20 @@ def showEvent(self, event): pages blurry until the user changed zoom (R8/D1).""" super().showEvent(event) win = self.window().windowHandle() if self.window() else None - if win: - # Disconnect before reconnecting — re-show events can stack - # the same handler multiple times. + prev_win = self._screen_signal_window + # Same top-level QWindow as last showEvent → still connected, + # nothing to do. Prevents both double-connect and the PySide6 + # 6.11 RuntimeWarning that fires when disconnect() runs on a + # never-connected signal (RuntimeWarning bypasses + # contextlib.suppress, which only catches exceptions). + if win is prev_win: + return + if prev_win is not None: with contextlib.suppress(TypeError, RuntimeError): - win.screenChanged.disconnect(self._on_screen_changed) + prev_win.screenChanged.disconnect(self._on_screen_changed) + if win is not None: win.screenChanged.connect(self._on_screen_changed) + self._screen_signal_window = win def _on_screen_changed(self, _screen): """Drop cached pixmaps and re-queue visible pages at the new DPR.""" diff --git a/app/viewer/canvas.py b/app/viewer/canvas.py index 8bc9c30..b6d21fb 100644 --- a/app/viewer/canvas.py +++ b/app/viewer/canvas.py @@ -125,6 +125,12 @@ def __init__(self): self._open_note = None # (page_idx, annot_idx) of open balloon self._search_highlights: list[tuple[int, object]] = [] # [(page_idx, fitz_rect), ...] self._search_current = -1 # index of current match in _search_highlights + # Tracks the QWindow whose screenChanged signal we're currently + # connected to (see showEvent). Avoids calling disconnect() on + # a signal that was never connected — which raises a PySide6 + # RuntimeWarning under 6.11 instead of an exception, so it + # can't be caught with contextlib.suppress. + self._screen_signal_window = None self.setFocusPolicy(Qt.FocusPolicy.StrongFocus) self.setMouseTracking(True) self.setCursor(Qt.CursorShape.IBeamCursor) @@ -220,12 +226,20 @@ def showEvent(self, event): (R8/D1).""" super().showEvent(event) win = self.window().windowHandle() if self.window() else None - if win: - # Re-show events may fire after a tab switch — disconnect - # first to avoid stacking duplicate handlers. + prev_win = self._screen_signal_window + # Same top-level QWindow as last showEvent → still connected, + # nothing to do. Prevents both double-connect and the PySide6 + # 6.11 RuntimeWarning that fires when disconnect() runs on a + # never-connected signal (RuntimeWarning bypasses + # contextlib.suppress, which only catches exceptions). + if win is prev_win: + return + if prev_win is not None: with contextlib.suppress(TypeError, RuntimeError): - win.screenChanged.disconnect(self._on_screen_changed) + prev_win.screenChanged.disconnect(self._on_screen_changed) + if win is not None: win.screenChanged.connect(self._on_screen_changed) + self._screen_signal_window = win def _on_screen_changed(self, _screen): """Invalidate cached pixmaps and re-render at the new DPR.""" diff --git a/tests/test_screen_changed_binding.py b/tests/test_screen_changed_binding.py new file mode 100644 index 0000000..f2b189b --- /dev/null +++ b/tests/test_screen_changed_binding.py @@ -0,0 +1,152 @@ +"""Regression tests for the screenChanged signal binding pattern +in viewer/editor canvases. + +Prior to this fix, ``showEvent`` called ``disconnect`` before ``connect`` +in a ``contextlib.suppress(TypeError, RuntimeError)`` block. Under +PySide6 6.11 the "signal was never connected" case emits a +RuntimeWarning rather than raising, so the suppress didn't catch it +and the following warning showed up on the very first show:: + + libpyside: Failed to disconnect () from signal + "screenChanged(QScreen*)" + +The fix tracks the previously-connected QWindow in +``_screen_signal_window`` and only calls ``disconnect`` when the +top-level window actually changes (e.g. widget was reparented). +""" + +import sys +from pathlib import Path + +_REPO_ROOT = Path(__file__).resolve().parent.parent +sys.path.insert(0, str(_REPO_ROOT)) + +VIEWER_CANVAS = _REPO_ROOT / "app" / "viewer" / "canvas.py" +EDITOR_CANVAS = _REPO_ROOT / "app" / "editor" / "canvas.py" + + +def _read(path: Path) -> str: + return path.read_text(encoding="utf-8") + + +def _show_event_block(src: str) -> str: + """Return the source of the first ``def showEvent`` in src (best-effort).""" + idx = src.find("def showEvent(") + assert idx >= 0, "showEvent not found" + return src[idx:idx + 1500] + + +# ── Guard 1: tracker attribute exists on both canvases ───────────────────── + +def test_viewer_canvas_tracks_screen_signal_window(): + src = _read(VIEWER_CANVAS) + assert "_screen_signal_window" in src or "UniqueConnection" in src, ( + "_SelectCanvas.showEvent should either track the connected window " + "or use Qt.UniqueConnection to avoid the disconnect-before-connect " + "RuntimeWarning." + ) + + +def test_editor_canvas_tracks_screen_signal_window(): + src = _read(EDITOR_CANVAS) + assert "_screen_signal_window" in src or "UniqueConnection" in src, ( + "PdfEditCanvas.showEvent should either track the connected window " + "or use Qt.UniqueConnection to avoid the disconnect-before-connect " + "RuntimeWarning." + ) + + +# ── Guard 2: tracker attribute is initialised in __init__ ───────────────── + +def test_viewer_canvas_tracker_initialised_in_init(): + src = _read(VIEWER_CANVAS) + # UniqueConnection alternative would not need the attribute. + if "UniqueConnection" in src: + return + # The tracker must be initialised (assigned to None) somewhere in + # __init__, otherwise the first showEvent hits AttributeError. + assert "self._screen_signal_window = None" in src, ( + "_SelectCanvas.__init__ must initialise _screen_signal_window = None" + ) + + +def test_editor_canvas_tracker_initialised_in_init(): + src = _read(EDITOR_CANVAS) + if "UniqueConnection" in src: + return + assert "self._screen_signal_window = None" in src, ( + "PdfEditCanvas.__init__ must initialise _screen_signal_window = None" + ) + + +# ── Guard 3: no bare disconnect + connect in showEvent ──────────────────── + +def test_no_bare_disconnect_before_connect_viewer(): + """The old anti-pattern was ``disconnect`` on every showEvent even when + nothing was previously connected. Ensure any remaining ``disconnect`` + call is guarded by a prior-window check.""" + src = _read(VIEWER_CANVAS) + block = _show_event_block(src) + if "screenChanged.disconnect" not in block: + return # UniqueConnection-only path is fine too + assert any( + marker in block + for marker in ( + "prev_win", + "prev is not None", + "getattr", + "_screen_signal_window", + "UniqueConnection", + ) + ), ( + "viewer canvas showEvent: disconnect must be guarded by a " + "prior-window check, not called unconditionally on every show." + ) + + +def test_no_bare_disconnect_before_connect_editor(): + src = _read(EDITOR_CANVAS) + block = _show_event_block(src) + if "screenChanged.disconnect" not in block: + return + assert any( + marker in block + for marker in ( + "prev_win", + "prev is not None", + "getattr", + "_screen_signal_window", + "UniqueConnection", + ) + ), ( + "editor canvas showEvent: disconnect must be guarded by a " + "prior-window check, not called unconditionally on every show." + ) + + +# ── Guard 4: same-window re-show is a no-op ─────────────────────────────── + +def test_viewer_showevent_short_circuits_on_same_window(): + """When win is the same object as _screen_signal_window we must + return early. Otherwise we'd still call connect() and stack a + duplicate handler.""" + src = _read(VIEWER_CANVAS) + if "UniqueConnection" in src: + return + block = _show_event_block(src) + assert "win is prev_win" in block or "win == prev_win" in block, ( + "viewer canvas showEvent must short-circuit when the top-level " + "QWindow hasn't changed since the previous show." + ) + + +def test_editor_showevent_short_circuits_on_same_window(): + src = _read(EDITOR_CANVAS) + if "UniqueConnection" in src: + return + block = _show_event_block(src) + assert "win is prev_win" in block or "win == prev_win" in block, ( + "editor canvas showEvent must short-circuit when the top-level " + "QWindow hasn't changed since the previous show." + )