Repository navigation
Add safe recovery choices after resolution cancellation - #1237
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #1237 +/- ##
===========================================
+ Coverage 67.00% 67.17% +0.17%
===========================================
Files 209 210 +1
Lines 30919 31068 +149
===========================================
+ Hits 20716 20869 +153
+ Misses 10203 10199 -4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Reuse analysis: extend the existing resolution request, stage movement, Stop Stage, model event queue, and PopUp lifecycles. Do not add a parallel stage executor, stop path, or public cancellation wrapper.
Reuse analysis: extend the existing resolution, stage movement, Stop Stage, model event queue, and PopUp lifecycles. Reject controller-only cancellation and a parallel emergency stage executor.
Reuse analysis: - Extended Microscope.move_stage, move_stage_offset, and stop_stage. - A second movement executor would duplicate stage mapping, limit enforcement, and device ownership. - Updated Numpydoc and focused movement tests.
Reuse analysis: - Kept Model.change_resolution and Model.stop_stage as canonical entry points. - Reused the model event queue and existing stage movement methods. - Added only private task state; no parallel public executor or stop API.
Reuse analysis: - ChangeResolution now participates in the model-owned resolution lifecycle. - No feature-specific stop path or duplicate acquisition cleanup API was added. - Focused tests verify acquisition preparation does not resume after cancellation.
Reuse analysis: - Refined the existing Controller.stop_stage retry to recognize only ObjectInSubprocess contention. - Kept hardware failures on the normal error path. - Extended Model.stop_stage best-effort handling so one microscope or readback failure cannot skip other stop attempts.
kevin-m-dean
force-pushed
the
kdean/issue-486-stop-resolution-change
branch
from
August 6, 2026 21:21
2573bfa to
ee747ae
Compare
Reuse analysis: exercises the existing StageController stop handler and Controller acquisition-bar binding with test-only headless button doubles. No public API or production lifecycle is added.
Reuse analysis: - Return movement uses Model.move_stage and the existing Stop Stage lifecycle. - Existing stage verification enforces the saved coordinates and configured limits. - No recovery-specific stage executor, limit validator, or stop API was added.
Reuse analysis: the dialog uses Navigate's existing PopUp, event queue, model thread pool, stage-control disablement, and Stop Stage lifecycle. It introduces no parallel movement or stop executor.
Reuse analysis: rejected and completed returns publish the existing resolution_return_complete event. Validation continues through each stage's existing strict verify_abs_position contract; no alternate limit or UI recovery path was added.
kevin-m-dean
force-pushed
the
kdean/issue-486-resolution-recovery
branch
from
August 6, 2026 22:07
64ae85d to
2159217
Compare
Reuse analysis: extends the existing private resolution task and Stop Stage lifecycle with one completion event. It preserves the canonical stop, readback, validation, event, and return paths without adding another stage executor or public API.
kevin-m-dean
marked this pull request as ready for review
August 6, 2026 22:27
Collaborator
Author
|
Hardware testing update: Stop Stage and Return to Previous Position appear to be working. It is not yet perfectly clear whether the microscope configuration is fully correct after stopping, so that aspect would benefit from review and additional verification. |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a post-cancellation recovery decision flow for resolution changes, allowing users to either keep the stopped stage position or (when proven safe) return to the pre-movement position, integrating with the existing Stop Stage lifecycle and model-owned task execution.
Changes:
- Introduces
ResolutionChangeCancelledPopupwith Keep/Return choices and safe default bindings (Close/Escape => Keep). - Extends model resolution-change tracking with a validated recovery snapshot and a cancellable “return to previous position” worker that emits terminal completion events.
- Updates controller event handling to show/deduplicate the popup, dispatch recovery commands, and disable/re-enable stage controls around return attempts.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/view/popups/test_resolution_change_popup.py | Adds unit tests for popup button labeling, default close behavior, and dismiss-before-callback ordering. |
| test/model/test_resolution_change.py | Adds model tests for return validation, cancellation snapshot behavior, stop/readback ordering, and return completion semantics. |
| test/controller/test_controller.py | Adds controller tests for popup deduplication, keep/return dispatch, control disabling, and event pump dispatch. |
| src/navigate/view/popups/resolution_change_popup.py | Implements the modal recovery popup UI and callback wiring. |
| src/navigate/model/resolution_change.py | Adds stop-completion synchronization to tasks and introduces an immutable recovery snapshot type. |
| src/navigate/model/model.py | Implements recovery command routing, snapshot validation/storage, cancellable return worker, and stop/readback synchronization. |
| src/navigate/controller/controller.py | Adds controller handlers for recovery popup display and stage-control restoration on completion. |
| docs/superpowers/specs/2026-08-04-issue-486-resolution-change-cancellation-design.md | Updates spec to require waiting for stop/readback before offering Return. |
| docs/superpowers/plans/2026-08-04-issue-486-resolution-change-cancellation.md | Marks plan steps complete and documents the stop/readback synchronization task. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
951
to
+955
| task = getattr(self, "_resolution_change_task", None) | ||
| if cancel_resolution_change and task is not None: | ||
| lock = getattr(self, "_resolution_change_lock", None) | ||
| if lock is None: | ||
| task.stop_complete_event.clear() |
annie-xd-wang
previously approved these changes
Sep 18, 2026
Resolve the independently added setting-update test module by retaining the channel-update regression tests and the resolution-cancellation regression. Remove the duplicate SimpleNamespace import introduced by automatic merging and apply the pinned formatting/lint requirements to the touched files. Reuse analysis: preserve the existing Model, Microscope, ChangeResolution, and Stop Stage APIs and the original PR's design/reuse records. This merge adds no new API or parallel execution path. Validation: 65 focused regression tests passed; pinned pre-commit hooks passed across all PR files; both parents' test/helper definitions were verified unchanged by AST comparison; git diff --check passed against develop.
Carry the develop integration and combined setting-update regression tests forward from PR #1236 while preserving all six recovery commits, including the reviewed acquisition-stop fixes. Initialize the autofocus progress queue in the recovery event-pump test fixture to match the current Controller initialization contract. Reuse analysis: preserve the existing task, recovery snapshot, event pump, stage movement, and Stop Stage lifecycles and their original design/reuse records. No application API or additional execution path is introduced. Validation: the event-pump regression failed before the fixture update and passed afterward; 85 focused regression tests and all pinned pre-commit hooks passed; git diff --check passed against the updated parent branch.
Record the squashed safety PR as an ancestor of the stacked recovery branch. The develop tree exactly matches the previously integrated #1236 parent, so retain the recovery branch's versions of the seven apparent conflicts. The resulting file tree is identical to d68f2be, which passed 2,091 tests in Windows CI. This commit changes history only, with no source, test, configuration, or documentation content changes. Validation: 85 focused tests and all pinned hooks passed again; the Git tree matches the previously validated head exactly; git diff --check passed.
kevin-m-dean
removed this pull request from stack #1238
September 19, 2026 14:01
kevin-m-dean
changed the base branch from
kdean/issue-486-stop-resolution-change
to
develop
September 19, 2026 14:01
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This stacked PR adds the explicit recovery decision requested for a cancelled resolution change:
Resolution Change Cancelleddialog only after the safety stop has quiesced the resolution worker;The core collision-prevention fix is intentionally isolated in draft PR #1236. This PR is based on that branch so reviewers can test and merge the safety stop independently from the optional recovery UX.
Related to #486.
Safety behavior
The saved position is copied into a private recovery snapshot tied to the cancelled task and selected microscope. A new resolution task invalidates any older snapshot. Return uses the existing cancellation-aware
Model.move_stage(..., wait_until_done=True)path and the existing Stop Stage lifecycle; it does not bypass device limit checks or create a second stage executor.The review pass also caught and fixed a stale-return edge case: if the saved position becomes unavailable between opening the dialog and selecting Return, the model refuses motion, emits a terminal failure event, and the controller re-enables ordinary stage controls.
Reuse analysis
Model.move_stage, strict stageverify_abs_position,PopUp, controller thread pool, and Stop Stage contracts.docs/superpowers/specs/2026-08-04-issue-486-resolution-change-cancellation-design.mddocs/superpowers/plans/2026-08-04-issue-486-resolution-change-cancellation.mdValidation
Popup behavior is tested without constructing Tk locally. Although Homebrew Xvfb is installed, Navigate current conda Tk links to macOS AppKit rather than X11, so Xvfb cannot isolate a real window in this environment. Native Windows or Linux X11 Tk under xvfb-run should perform the final constructed-window smoke check.
Stack