feat(scan): single-shot preview window for devices with no frame adapter - #779
feat(scan): single-shot preview window for devices with no frame adapter#779cymbal221 wants to merge 2 commits into
Conversation
| @@ -0,0 +1,183 @@ | |||
| """Modal pop-up: a single low-res preview scan and a crop window, for devices with | |||
There was a problem hiding this comment.
I would reuse strip preview dialog (limiting it to 1 frame when applicable) rather than building completely separate dialog
|
Good call on avoiding a second dialog class outright — I looked at merging into StripPreviewDialog directly (gated to 1 frame), but two things there are hard-wired for the multi-frame/feed-axis case rather than just extra UI to hide:
Making those conditional would mean StripPreviewDialog carrying two real UX modes rather than being "limited" to fewer frames. So I kept the second, thinner dialog, but pulled out what actually was duplicated: the four scan_roll_preview_*/scan_error/scan_cancelled signal wiring and teardown, now shared via I am putting this to draft until I test changes with the scanner. |
The scan-window preview (StripPreviewDialog) was gated entirely on caps.adapter_frame_capacity — a SANE `frame` option only a motorized strip/roll feeder (Coolscan's SF-210, autoloader) exposes. Plustek and similar scanners have one manual film holder with no such option, so the whole scan-window row was hidden for them: no way to preview a frame and crop before the real (full-resolution) scan. QuickScanPreviewDialog is a lighter sibling of StripPreviewDialog for this case — one preview, one crop window, no offset/drift sliders or tile grid (those correct feed-axis drift along a strip, meaningless for a single manual holder). Extracted the shared preview_positive() negative->positive display helper out of strip_preview_dialog.py so both dialogs use it. PerFrameRollSession.preview() previously always requested `frame=slot`, which fails loud on a device with no `frame` option (frame 1 is not "the first frame" there, it's simply not a settable option). Now omits `frame` when the device has no adapter capacity, making the existing roll-preview worker/signal plumbing (RollPreviewRequest -> ScanWorker.run_roll_preview -> roll_preview_ready) reusable for single-slot devices with no other backend changes. Building this surfaced a real bug in the 'source' IR strategy: the scan window (tl_x/tl_y/br_x/br_y) was only ever applied once, before the RGB pass's dev.start(). The IR pass's own dev.start() (after switching `source`) never re-applied it — and a source switch can reset or re-range those geometry options on real hardware. Left alone, that means a windowed IR-enabled scan on this class of device gets RGB and IR passes back at *different pixel sizes*, and preview_manager's loader-side guard silently drops the "mismatched" IR plane — which surfaces as the IR Removal toggle just being greyed out after loading, no error. Fixed by extracting _apply_scan_window() and calling it before both dev.start() calls, not just the first. Verified with a fake device that reproduces the reset (test fails without the fix — ir-pass snapshot shows the window wiped — and passes with it restored). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Responds to review: reuse StripPreviewDialog instead of a separate dialog. Doing that literally would give StripPreviewDialog a second UX mode, not just a smaller one: - The -90° preview rotation is unconditional and specific to the LS-50's vertical feed axis. Applied to a Plustek/genesys flatbed as-is, it would show the preview and crop box sideways. - Offset/Drift correct feed-axis drift *between* frames on a strip — with one frame they'd render but do nothing. Both would need new conditionals threaded through the same class, not just fewer tiles. What actually was duplicated: both dialogs wire the same four scan_roll_preview_ready/_finished/scan_error/scan_cancelled signals onto their handlers and tear them down in closeEvent, near-verbatim. Extracted that into RollPreviewSignalsMixin (scan_preview_common.py) — both dialogs now call self._connect_preview_signals() and no longer define their own closeEvent. Added direct tests for QuickScanPreviewDialog (previously only exercised via a monkeypatched fake in test_scan_sidebar.py), including one proving close() actually disconnects its signals. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
9d1632a to
63e8705
Compare
Stacked on #778 — this diff will show both commits until that one merges; review only the second (
feat(scan): single-shot preview window...) commit.The scan-window preview (StripPreviewDialog) was gated entirely oncaps.adapter_frame_capacity — a SANE
frameoption only a motorizedstrip/roll feeder (Coolscan's SF-210, autoloader) exposes. Plustek and
similar scanners have one manual film holder with no such option, so the
whole scan-window row was hidden for them: no way to preview a frame and
crop before the real (full-resolution) scan.
QuickScanPreviewDialog is a lighter sibling of StripPreviewDialog for this
case — one preview, one crop window, no offset/drift sliders or tile grid
(those correct feed-axis drift along a strip, meaningless for a single
manual holder). Extracted the shared preview_positive() negative->positive
display helper out of strip_preview_dialog.py so both dialogs use it.
PerFrameRollSession.preview() previously always requested
frame=slot,which fails loud on a device with no
frameoption (frame 1 is not "thefirst frame" there, it's simply not a settable option). Now omits
framewhen the device has no adapter capacity, making the existing roll-preview
worker/signal plumbing (RollPreviewRequest -> ScanWorker.run_roll_preview
-> roll_preview_ready) reusable for single-slot devices with no other
backend changes.
Building this surfaced a real bug in the 'source' IR strategy: the scan
window (tl_x/tl_y/br_x/br_y) was only ever applied once, before the RGB
pass's dev.start(). The IR pass's own dev.start() (after switching
source) never re-applied it — and a source switch can reset or re-rangethose geometry options on real hardware. Left alone, that means a windowed
IR-enabled scan on this class of device gets RGB and IR passes back at
different pixel sizes, and preview_manager's loader-side guard silently
drops the "mismatched" IR plane — which surfaces as the IR Removal toggle
just being greyed out after loading, no error. Fixed by extracting
_apply_scan_window() and calling it before both dev.start() calls, not
just the first. Verified with a fake device that reproduces the reset
(test fails without the fix — ir-pass snapshot shows the window wiped —
and passes with it restored).
Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com