Skip to content

refactor(window): extract auto-update into UpdateController (R4) #159

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nelsonduarte merged 2 commits into from
Aug 11, 2026
Merged

refactor(window): extract auto-update into UpdateController (R4) #159

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5ca922c
refactor(window): extract auto-update subsystem into UpdateController
nelsonduarte Aug 11, 2026
70fb82e
docs(updater): point stale comments to update_controller after R4
nelsonduarte Aug 11, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 177 additions & 0 deletions app/update_controller.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
"""PDFApps – UpdateController: the MainWindow auto-update subsystem.

Extracted verbatim from ``app/window.py`` (R4 refactor). Owns the
background update-check worker (a ``QObject`` moved onto a ``QThread``),
the notify/decision step and the update dialog. Behaviour, threading and
worker lifecycle are unchanged — only the code's home moved.

Threading note (preserved deliberately): the controller is a ``QObject``
whose parent is the main window, so it keeps main-thread affinity. The
worker's ``done`` signal therefore delivers ``_on_update_found`` back on
the main thread via Qt's automatic queued connection — exactly as it did
when these were ``MainWindow`` methods. No lambda crosses the thread
boundary (a bound QObject method is used), which is the Py3.14-safe
pattern this project relies on.
"""

import contextlib
import os

from PySide6.QtCore import QObject

from app.i18n import t


class UpdateController(QObject):
"""Encapsulates the MainWindow auto-update check, worker and dialog.

Composed by ``MainWindow`` as ``self._update_controller``. The window
still owns the toolbar update button (part of its layout) and passes
it in so the controller can reveal it when an update is found and wire
its click to :meth:`show_update_dialog`.
"""

def __init__(self, window, update_button):
# Parent to the window → main-thread affinity + lifetime tied to
# the window. This is what makes the worker's cross-thread ``done``
# signal resume on the main thread.
super().__init__(window)
self._window = window
self._update_btn = update_button
self._update_release = None
self._update_thread = None
self._update_worker = None
# Holds the in-flight urlopen response of the background update
# check so release_worker can abort a blocked network read from
# closeEvent instead of waiting out the socket timeout.
self._update_cancel = None

def check_async(self):
# Skip auto-update inside Flatpak/Snap — the host package
# manager handles updates. MSIX/Microsoft Store installs are
# short-circuited inside check_for_update() itself (the Store
# updates packaged apps automatically), so they never reach the
# NSIS download path.
if os.environ.get("FLATPAK_ID") or os.environ.get("SNAP"):
return
# Pre-import the updater module on the main thread BEFORE the
# worker thread starts. The worker would otherwise lazy-import
# `app.updater`, which transitively pulls in `urllib.request`
# → `http.client` → `ssl`. On Python 3.14, importing those
# heavy modules from a non-main thread while the main thread
# is still busy creating Qt widgets races against each other
# (CPython's import machinery + Qt's widget construction +
# garbage collection on the worker side) and produces a
# Windows access-violation crash. Doing the import here keeps
# all that import work on the main thread; the worker only
# calls the already-imported function.
from app.updater import check_for_update
from PySide6.QtCore import QThread, QObject, Signal as _Sig

# Shared holder so the main thread can close the worker's urlopen
# response and unblock its network read on shutdown (M4).
self._update_cancel = {"resp": None}

class _Worker(QObject):
done = _Sig()
def __init__(self, cancel_holder):
super().__init__()
self.release = None
self._cancel = cancel_holder
def run(self):
self.release = check_for_update(self._cancel)
if self.release:
self.done.emit()
self.thread().quit()

self._update_thread = QThread()
self._update_worker = _Worker(self._update_cancel)
self._update_worker.moveToThread(self._update_thread)
self._update_thread.started.connect(self._update_worker.run)
self._update_worker.done.connect(self._on_update_found)
self._update_thread.finished.connect(self._update_thread.deleteLater)
self._update_thread.start()

def _on_update_found(self):
if self._update_worker is not None:
self._update_release = self._update_worker.release
self._notify_update()
# R8-H2: the worker QObject lived for the lifetime of the
# application before this — only the QThread was scheduled for
# deleteLater. Drop the worker after the check completes so the
# closure (and its captured release dict) is freed.
self.release_worker()

def release_worker(self):
"""Tear down the update worker/thread defensively.

Safe to call from both ``_on_update_found`` (the happy path) and
``MainWindow.closeEvent`` (in case the worker never emitted
``done`` — e.g. no update available, network failure).

The check worker runs ``check_for_update()`` which blocks in
``urllib.request.urlopen(..., timeout=10)``. ``quit()`` only signals
the worker's event loop AFTER ``run()`` returns, so it cannot
interrupt a network read that is still parked inside urlopen. If the
user closes the window during the ~10 s window right after launch
the old code's ``wait(1000)`` returned False and closeEvent went on
to destroy the MainWindow with the QThread still running ("QThread:
Destroyed while thread is still running" + possible abort()).

Root-cause fix: (1) close the in-flight urlopen response so the
blocked read raises and ``run()`` returns immediately; (2) wait long
enough to cover the urlopen timeout; (3) ``terminate()`` as a last
resort so a running QThread is never left to be destroyed."""
# (1) Abort the blocked network read, if any.
holder = getattr(self, "_update_cancel", None)
if holder is not None:
resp = holder.get("resp")
if resp is not None:
with contextlib.suppress(Exception):
resp.close()
worker = getattr(self, "_update_worker", None)
if worker is not None:
with contextlib.suppress(RuntimeError):
worker.deleteLater()
self._update_worker = None
thread = getattr(self, "_update_thread", None)
if thread is not None:
with contextlib.suppress(RuntimeError):
if thread.isRunning():
thread.quit()
# (2) Cover the urlopen timeout; the aborted read above
# should return well before this elapses.
if not thread.wait(12000):
# (3) Last resort — never destroy a running QThread.
thread.terminate()
thread.wait(2000)
self._update_thread = None
self._update_cancel = None

def _notify_update(self):
"""Show update notification dialog automatically."""
# Guard against a race with closeEvent -> release_worker,
# which nulls _update_worker (and thus leaves _update_release
# unset/None) between the worker's done.emit() on the worker thread
# and this queued slot running on the main thread. Without this
# guard the .get() below raises AttributeError on None.
if not self._update_release:
return
self._update_btn.setVisible(True)
tag = self._update_release.get("tag_name", "?")
from PySide6.QtWidgets import QMessageBox
msg = t("update.available").format(version=tag)
msg += "\n\n" + t("update.installer_info")
msg += "\n\n" + t("update.install") + "?"
reply = QMessageBox.question(
self._window, "PDFApps", msg,
QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No,
)
if reply == QMessageBox.StandardButton.Yes:
self.show_update_dialog()

def show_update_dialog(self):
if self._update_release:
from app.updater import UpdateDialog
dlg = UpdateDialog(self._update_release, parent=self._window)
dlg.exec()
4 changes: 2 additions & 2 deletions app/updater.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,13 +572,13 @@ def run(self):
self._signals.cancelled.connect(self._dl_thread.quit)
# Explicit cleanup so retries within the same dialog (e.g. after
# an error toast) don't leak QThread and _Worker objects. Mirrors
# the pattern in window.py:_update_thread.finished -> deleteLater.
# the pattern in update_controller.py:_update_thread.finished -> deleteLater.
self._dl_thread.finished.connect(self._dl_thread.deleteLater)
# Root-cause fix for the dangling-wrapper bug: once the thread is
# done (success, error OR cancel) drop our Python reference so the
# pending deleteLater can free the C++ object without leaving
# reject()/closeEvent holding a stale wrapper. Mirrors the
# _release_update_worker pattern in window.py.
# release_worker pattern in update_controller.py.
self._dl_thread.finished.connect(self._on_dl_thread_finished)
self._signals.finished.connect(self._dl_worker.deleteLater)
self._signals.error.connect(self._dl_worker.deleteLater)
Expand Down
149 changes: 8 additions & 141 deletions app/window.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from app.utils import resource_path, _make_palette
from app.widgets import DropFileEdit
from app.single_instance import SingleInstanceServer
from app.update_controller import UpdateController
from app.viewer.panel import PdfViewerPanel
from app.tools.split import TabDividir
from app.tools.merge import TabJuntar
Expand Down Expand Up @@ -265,22 +266,19 @@ def _a11y(btn, tip):
f"QPushButton {{ border: 1.5px solid {ACCENT}; border-radius: 6px; }}"
f"QPushButton:hover {{ background: rgba(20,184,166,0.15); }}"
)
self._update_btn.clicked.connect(self._show_update_dialog)
# Auto-update subsystem lives in a dedicated controller. The window
# keeps the toolbar button (part of this layout) and hands it over
# so the controller can reveal it and drive its click.
self._update_controller = UpdateController(self, self._update_btn)
self._update_btn.clicked.connect(self._update_controller.show_update_dialog)
wb_h.addWidget(self._update_btn)
self._update_release = None
self._update_thread = None
self._update_worker = None
# Holds the in-flight urlopen response of the background update
# check so _release_update_worker can abort a blocked network
# read from closeEvent instead of waiting out the socket timeout.
self._update_cancel = None
# R11-M7: defer the update check 2s past __init__ so the main
# window can finish painting + showMaximized before any network
# I/O races the UI. Previously fired before the window was even
# visible, occasionally stalling first paint on slow networks.
QTimer.singleShot(
2000,
lambda: self._check_for_updates_async() if isValid(self) else None,
lambda: self._update_controller.check_async() if isValid(self) else None,
)

root_v.addWidget(self._workspace_bar)
Expand Down Expand Up @@ -1319,7 +1317,7 @@ def closeEvent(self, event):
# the QObject worker to release its closure / release dict
# if the user closes the window before the check completes
# (R8-H2 defensive path).
self._release_update_worker()
self._update_controller.release_worker()
try:
from app.i18n import _update_config
sizes = self._splitter.sizes()
Expand Down Expand Up @@ -1457,134 +1455,3 @@ def _apply_theme(self):
if _is_valid(pres):
pres.update_theme(self._dark_mode)

# ── Auto-update ───────────────────────────────────────────────────────

def _check_for_updates_async(self):
# Skip auto-update inside Flatpak/Snap — the host package
# manager handles updates. MSIX/Microsoft Store installs are
# short-circuited inside check_for_update() itself (the Store
# updates packaged apps automatically), so they never reach the
# NSIS download path.
if os.environ.get("FLATPAK_ID") or os.environ.get("SNAP"):
return
# Pre-import the updater module on the main thread BEFORE the
# worker thread starts. The worker would otherwise lazy-import
# `app.updater`, which transitively pulls in `urllib.request`
# → `http.client` → `ssl`. On Python 3.14, importing those
# heavy modules from a non-main thread while the main thread
# is still busy creating Qt widgets races against each other
# (CPython's import machinery + Qt's widget construction +
# garbage collection on the worker side) and produces a
# Windows access-violation crash. Doing the import here keeps
# all that import work on the main thread; the worker only
# calls the already-imported function.
from app.updater import check_for_update
from PySide6.QtCore import QThread, QObject, Signal as _Sig

# Shared holder so the main thread can close the worker's urlopen
# response and unblock its network read on shutdown (M4).
self._update_cancel = {"resp": None}

class _Worker(QObject):
done = _Sig()
def __init__(self, cancel_holder):
super().__init__()
self.release = None
self._cancel = cancel_holder
def run(self):
self.release = check_for_update(self._cancel)
if self.release:
self.done.emit()
self.thread().quit()

self._update_thread = QThread()
self._update_worker = _Worker(self._update_cancel)
self._update_worker.moveToThread(self._update_thread)
self._update_thread.started.connect(self._update_worker.run)
self._update_worker.done.connect(self._on_update_found)
self._update_thread.finished.connect(self._update_thread.deleteLater)
self._update_thread.start()

def _on_update_found(self):
if self._update_worker is not None:
self._update_release = self._update_worker.release
self._notify_update()
# R8-H2: the worker QObject lived for the lifetime of the
# application before this — only the QThread was scheduled for
# deleteLater. Drop the worker after the check completes so the
# closure (and its captured release dict) is freed.
self._release_update_worker()

def _release_update_worker(self):
"""Tear down the update worker/thread defensively.

Safe to call from both ``_on_update_found`` (the happy path) and
``closeEvent`` (in case the worker never emitted ``done`` — e.g.
no update available, network failure).

The check worker runs ``check_for_update()`` which blocks in
``urllib.request.urlopen(..., timeout=10)``. ``quit()`` only signals
the worker's event loop AFTER ``run()`` returns, so it cannot
interrupt a network read that is still parked inside urlopen. If the
user closes the window during the ~10 s window right after launch
the old code's ``wait(1000)`` returned False and closeEvent went on
to destroy the MainWindow with the QThread still running ("QThread:
Destroyed while thread is still running" + possible abort()).

Root-cause fix: (1) close the in-flight urlopen response so the
blocked read raises and ``run()`` returns immediately; (2) wait long
enough to cover the urlopen timeout; (3) ``terminate()`` as a last
resort so a running QThread is never left to be destroyed."""
# (1) Abort the blocked network read, if any.
holder = getattr(self, "_update_cancel", None)
if holder is not None:
resp = holder.get("resp")
if resp is not None:
with contextlib.suppress(Exception):
resp.close()
worker = getattr(self, "_update_worker", None)
if worker is not None:
with contextlib.suppress(RuntimeError):
worker.deleteLater()
self._update_worker = None
thread = getattr(self, "_update_thread", None)
if thread is not None:
with contextlib.suppress(RuntimeError):
if thread.isRunning():
thread.quit()
# (2) Cover the urlopen timeout; the aborted read above
# should return well before this elapses.
if not thread.wait(12000):
# (3) Last resort — never destroy a running QThread.
thread.terminate()
thread.wait(2000)
self._update_thread = None
self._update_cancel = None

def _notify_update(self):
"""Show update notification dialog automatically."""
# Guard against a race with closeEvent -> _release_update_worker,
# which nulls _update_worker (and thus leaves _update_release
# unset/None) between the worker's done.emit() on the worker thread
# and this queued slot running on the main thread. Without this
# guard the .get() below raises AttributeError on None.
if not self._update_release:
return
self._update_btn.setVisible(True)
tag = self._update_release.get("tag_name", "?")
from PySide6.QtWidgets import QMessageBox
msg = t("update.available").format(version=tag)
msg += "\n\n" + t("update.installer_info")
msg += "\n\n" + t("update.install") + "?"
reply = QMessageBox.question(
self, "PDFApps", msg,
QMessageBox.StandardButton.Yes | QMessageBox.StandardButton.No,
)
if reply == QMessageBox.StandardButton.Yes:
self._show_update_dialog()

def _show_update_dialog(self):
if self._update_release:
from app.updater import UpdateDialog
dlg = UpdateDialog(self._update_release, parent=self)
dlg.exec()
10 changes: 7 additions & 3 deletions tests/test_audit_mediums_lows.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,12 +141,16 @@ def test_pipeline_save_logs_symlink_destination():

def test_update_check_deferred_post_init():
src = _read("app/window.py")
# The bare self._check_for_updates_async() call inside __init__ is
# replaced by a QTimer.singleShot.
init_block = src.split("self._update_release = None")[1].split("# ── Viewer property")[0]
# The bare update check inside __init__ is deferred via QTimer.singleShot.
# R4: the check moved behind self._update_controller.check_async(); the
# deferral wiring stays in MainWindow.__init__.
init_block = src.split("self._update_controller = UpdateController")[1].split("# ── Viewer property")[0]
assert "QTimer.singleShot" in init_block, (
"Update check must be deferred via QTimer.singleShot from __init__."
)
assert "self._update_controller.check_async()" in init_block, (
"The deferred call must go through the UpdateController."
)
# And guarded with isValid so a quick close won't crash.
assert "isValid(self)" in init_block

Expand Down
Loading