Conversation
The command-line client existed with no way to reach it from the interface most people will use. This adds the check, the notification and the install sequence, and the sequence is the interesting part. Replacing a desktop app underneath itself is the usual pattern, and it is not available here: the installer refuses maintenance while a transfer is in flight and never force-kills a running copy. So a running or paused job declines the update with the reason rather than queueing behind it or forcing it through. A paused job counts, because it is a partially written destination waiting to continue, and replacing the application under it is not a way to find out what happens. The queue is checked again immediately before the installer is launched, since a job can be started while the bytes are arriving. When nothing is running, the app closes itself so maintenance can proceed; that close is what makes the update possible, so it is announced rather than arriving as a side effect. The automatic check runs once a couple of seconds after the window opens and says nothing unless there is something to say, because an unrequested check that reports being up to date is noise. Help offers the same check with an answer either way, and Options turns the automatic one off. Both the network call and the signature probe run on a pool thread, so an unreachable network delays no paint and a failure lands in the status bar rather than a dialog. Writing the tests found an ordering fault worth keeping fixed: the check set the release before clearing its own busy flag, leaving a window in which a release was visible and prepare() still refused. In the app that is an update which announces itself and then quietly does nothing. The flag is now cleared before the result is published, and a test asserts that anything able to see a release can also start the download. Every step is injectable, so the whole sequence including each refusal is driven in tests without a network, a certificate or an installer.
34f8b75 to
65732f4
Compare
3e95b23 to
170f1a0
Compare
owenpkent
left a comment
owenpkent
left a comment
There was a problem hiding this comment.
Reviewed manual/automatic update checks, worker-to-GUI signaling, download progress, transfer refusal, and the install handoff. Three user-visible lifecycle defects are detailed inline: a failed check is reported as current, Cancel does not cancel, and the startup timer can suppress a manual result.
Validation: 18 targeted updater tests passed on this head and git diff --check passed. The injected raising-check case still emitted an uncaught worker traceback; a green test does not establish that the failed signal was delivered. Additional verification-path concerns inherited from #13 are included in the private updater notes rather than repeated publicly.
| # refuses, which reads as an update that quietly does nothing. | ||
| self._busy = False | ||
| self._release = release | ||
| if release is None: |
There was a problem hiding this comment.
[P2] Distinguish a failed update check from a successful current-version result.
The real offloader.update.check() catches fetch/TLS/feed-parsing exceptions and returns None. This branch therefore emits upToDate when the feed was unreachable or malformed, and a manual check tells the user that this is the newest release without having obtained a usable response. Automatic failures also never reach the documented failure-status path.
Return a distinct failure outcome, or allow a typed check error to reach an exception handler here. _run_check() itself needs an except path as well: an injected raising check currently produces a QRunnable traceback and no failed signal. Cover failed fetch, invalid feed, and a genuinely up-to-date response as separate controller outcomes.
| QMessageBox.Yes | QMessageBox.No, QMessageBox.Yes) | ||
| if answer != QMessageBox.Yes: | ||
| return | ||
| self._update_progress = QProgressDialog( |
There was a problem hiding this comment.
[P2] Wire Cancel to cancellation, or remove the button.
The dialog exposes Cancel, but its canceled signal is never connected and UpdateController has no cancellation state. Clicking it only hides the dialog; downloading and verification continue, and _on_update_ready() subsequently offers installation. The user's explicit cancellation therefore does not stop or suppress the operation.
Add cooperative cancellation through download/prepare, clean up only the owned incomplete download, and suppress ready for the canceled request. If cancellation is deferred, remove the Cancel button so the control does not promise behavior it cannot provide. Add a headless test that clicks Cancel during a blocked download and checks the terminal signal.
| """`announce` reports "you are up to date"; the automatic check does | ||
| not, because an unasked-for check should only ever speak up when there | ||
| is something to say.""" | ||
| self._announce_update = announce |
There was a problem hiding this comment.
[P2] Preserve an explicit check request when the launch timer fires.
_announce_update is overwritten before check_now() reports whether a new check actually started. If the user requests a manual check during the first 2.5 seconds and it is still running when the startup timer fires, the timer sets this flag to False, then the controller rejects the duplicate check. The original manual result is consequently handled as an automatic result, so the requested result/dialog disappears.
Associate announcement intent with the check that actually starts, or preserve an existing manual request when a duplicate call is refused. Add a controlled slow-check test that triggers the manual action first and the launch timer second, then asserts that the manual completion remains visible.
Three ways the update sequence told the user something that was not so. update.check() never raises and returns None for a failed fetch, a TLS error and an unparseable feed alike, and the controller read None as "you are the newest release". So a manual check reported that on the strength of a failed DNS lookup, and an automatic failure never reached the documented failure path at all. check_feed() is the form with somewhere to put the difference: None only when the feed answered and had nothing newer, FeedError otherwise. check() is unchanged for callers that have nowhere to put it. _run_check also had no except path, so an injected or future check that raised produced a bare QRunnable traceback and no signal - the window waited on a result that was never coming. The progress dialog offered Cancel and nothing was connected to it. Clicking it hid the dialog while the download and the signature check carried on, and the app then offered to install what the user had just declined. Cancellation is cooperative, because a blocking read loop on a pool thread has nowhere else to hand control back: the progress callback raises. What it guarantees is the part that was missing - the request is terminal, the incomplete download is removed, verification does not run, and ready is not emitted. Checked again after the download returns, so a file small enough to arrive between two callbacks cannot sail past the cancel. Only the file the release names is removed, since the download directory can be one the caller owns. Closing the dialog for a result that has arrived disconnects first, because QProgressDialog's own closeEvent emits canceled and that would be reported back as the user asking to stop. The announcement intent now belongs to the check that is running rather than to the window's last call. It was stamped before check_now reported whether a check had started, so a manual check requested during the first 2.5 seconds was downgraded by the launch timer firing behind it: the duplicate was refused, the intent was already overwritten, and the result the user asked for was handled silently. A refused duplicate now raises the intent and never lowers it, so it survives in both directions. Sixteen tests. Failed fetch, invalid feed and a genuine up-to-date answer as three separate outcomes at both the feed and the controller; a cancel during a blocked download and one arriving after the bytes have, each asserting the terminal signal, the absent installer and the removed file; a file beside it that is not removed; the refused-duplicate race in both directions and at the window; and the dialog's Cancel reaching the controller while closing it for a result does not.
1 participant
Summary
The client from #13 had no way to reach it from the interface most people will use. This adds the check, the notification and the install sequence. The sequence is the interesting part.
Everything here works without a signing certificate or a published release, which is why it was the next thing buildable.
The sequence cannot be the usual one
Replacing a desktop app underneath itself is the standard pattern and it is not available here: the installer refuses maintenance while a transfer is in flight and never force-kills a running copy. So:
Restraint in the check
One check, a couple of seconds after the window opens, and it says nothing unless there is something to say: an unrequested check reporting "you are up to date" is noise. A found release gets a line in the header rather than a dialog stealing attention from a running offload. Help, Check for updates now answers either way; Options, Check for updates on launch turns the automatic one off (on by default — a packaged copy that never learns a fix exists is the worse default for a tool people trust with original media).
Both the network call and the signature probe run on a pool thread, so an unreachable network delays no paint and a failure lands in the status bar, not a modal.
An ordering fault the tests found
Worth flagging because it was a real bug, not a test artefact.
_run_checksetself._releasebefore clearing its own busy flag, leaving a window in which a release was visible andprepare()still refused. In the app that is an update which announces itself and then quietly does nothing, on a timing-dependent basis.The flag is now cleared before the result is published, and
test_a_visible_release_can_always_be_preparedasserts the invariant: anything able to see a release can also start the download.Design notes for review
QRunnable, for the same reasondrives._ScanTaskdoes it: a runnable's Python wrapper can be collected as soon asstart()returns.refusal()takes the queue's states rather than the controller, so the rule is a pure function and testable without a window.Test plan
tests/test_gui_updates.py— 18 tests: the refusal for running and paused, idle and queued-only allowed, one active job among many still refusing; newer release reported, up-to-date reported separately, a second check refused while one runs, a raising check not escaping the pool thread, the ordering invariant, download/verify/ready with the streamed digest passed to both sides, a failed verification leaving no installer the window could run, progress throttled but finishing, and install refusing without a verified fileruff check src tests scripts build/windowscleanoffloader-gui.exelaunched and stayed up for 6s, so the launch-time check does not break startup