fix(gui): fix progress accounting and cancellation on window close - #498
Merged
Merged
Conversation
Three defects around the progress bar and the cancel button. The progress bar reached 100% halfway through a run. read_dataset() and export_metadataset() each incremented the counter on success, so a queue of N datasets stepped it up to 2N, while the GUI sets the bar's maximum to count_files_to_process() -- N. Datasets that failed to read, or already marked processed, incremented nothing at all, so a run holding failures could equally end short of the maximum. Count datasets instead: exactly one step per dataset, taken in process_datasets_in_queue() whatever the outcome. The per-phase messages stay, they are what makes the status line useful. Closing the window during a scan or an analysis froze it for three seconds and left the thread running: closeEvent() only called QThread.quit(), which asks a thread's event loop to return -- something a worker blocked inside its own run() never reaches. Request cancellation first, which the workers poll, then wait. The cancel button stayed visible after a successful PostGIS run: each terminal handler hid it on its own and that one had been forgotten. Hide it in _enable_processing_controls(), which every path out of a run already goes through, and drop the now-redundant calls. AI-Level: 7 (human specced, bots coded) AI-Model: claude-opus-5 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WABM4NMiD2DhEVreKLxWLq
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #498 +/- ##
==========================================
+ Coverage 75.54% 76.09% +0.55%
==========================================
Files 46 46
Lines 3410 3409 -1
Branches 414 415 +1
==========================================
+ Hits 2576 2594 +18
+ Misses 713 695 -18
+ Partials 121 120 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.



Description
Three defects around the progress bar and the cancel button, grouped as agreed.
1. The progress bar reached 100% halfway through a run
read_dataset()andexport_metadataset()each incremented the counter on success, so a queue of N datasets stepped it up to 2N — while the GUI sets the bar's maximum tocount_files_to_process(), i.e. N. Symmetrically, a dataset that failed to read (or was already marked processed) incremented nothing at all, so a run holding failures could end short of the maximum.Fixed by counting datasets rather than phases: exactly one step per dataset, taken in
process_datasets_in_queue()whatever the outcome. The per-phase messages ("Reading …", "Exporting …") stay — they are what makes the status line useful.This is the one change outside
mw_dicogis.py: the increments live in the pipeline, so that is where the accounting had to be fixed. The CLI passesprogress_reporter=Noneand is unaffected.2. Closing the window during a run froze it, then leaked the thread
closeEvent()only calledQThread.quit(), which asks a thread's event loop to return — something a worker blocked inside its ownrun()never reaches. Closing during a scan or an analysis blocked the window for the full three-secondwait()and still left the thread running. It now requests cancellation first, which the workers already poll, then waits.3. The cancel button stayed visible after a successful PostGIS run
Each terminal handler hid the button on its own, and
_on_db_processing_finishedhad been forgotten. Hiding it in_enable_processing_controls()— which every path out of a run already goes through — fixes it by construction rather than one handler at a time, so the redundant calls are dropped.Tests
Five added or reworked. On the previous code:
test_progress_advances_once_per_successful_datasettest_progress_reporter_receives_messages_but_no_incrementExpected 'increment' to not have been called. Called 1 times.test_main_window_close_requests_cancellationassert False is True where False = is_canceled()test_main_window_hides_cancel_button_when_a_run_endsassert True is False where True = btn_cancel.isEnabled()test_progress_advances_once_per_dataset_whatever_the_outcomepins the new behaviour for unreadable and already-processed datasets; it is a behaviour pin, not a regression guard — I first wrote the mixed queue as the regression test and it passed on the old code, its 2 successes × 2 increments plus 2 failures × 0 happening to total the same 4. Hence the split.Two notes
How this was verified. These modules import
osgeo, which will not build in my sandbox, so the runs above used a locally-injected fakeosgeo(a scratch file, not committed). Under it the whole suite reports the same 27 failures on this branch as onmaster— the georeader tests, which need real GDAL — with 285 passing here against 280 there. CI against real GDAL remains the real check.Lint.
ruff checkgoes from 38 to 49 violations on the four touched files: +11S101/ANN001from the tests added totests/ui/test_main_window_smoke.py. Nothing new in kind — every existing test in that file carries them — because[tool.ruff.lint.per-file-ignores]keys them on"tests/test_*.py", which does not match thetests/ui/subfolder. Changing that pattern to"tests/**/test_*.py"would zero them out repo-wide, but that is a tooling change and this is a bugfix PR, so I have left it alone. Happy to send it separately.Related issue(s)
None — found while reviewing the code ahead of the 4.0.0-beta13 release.
Type of change
fix/…branch)feature/…branch)docs/…branch or*.md/docs/**changes)packaging/…branch orbuilder/**changes)tooling/…branch or.github/**changes)tests/**changes)Checklist
pre-commit run --all-filespasses locally — its two ruff hooks, run as configured on the changed files:ruff-formatreports them already formatted andruff check --fix-onlyrewrites nothing. See the lint note above for whatruff checkstill reportspytestpasses locally — 221 passed on the plain non-GDAL modules, with the same 8 pre-existing sandbox failures as onmaster; the GDAL-importing modules were run under the fakeosgeodescribed aboveAI/LLM disclosure
🤖 Generated with Claude Code
https://claude.ai/code/session_01WABM4NMiD2DhEVreKLxWLq
Generated by Claude Code