Test#14
Conversation
WalkthroughAdds directional, prioritized prefetching with ICC transform caching and mmap-backed JPEG decode paths; UI gains a Jump-to-Image dialog and dialog-open/close hooks; app controller adds in-memory filtering, cache/ICC clearing, Helicon RAW/JPG selection refinements, expanded logging/timing instrumentation, and a version bump to 0.9.0. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as QML/UI
participant App as AppController
participant Pref as Prefetcher
participant Cache as ImageCache
participant Decode as Decoder
UI->>App: controller.dialog_opened() (Jump dialog or filter)
UI->>App: user requests image N
App->>Pref: update_prefetch(current=N, is_navigation=true, direction=+1)
Pref->>Pref: compute direction-biased prefetch window
Pref->>Cache: check for cached indices
alt cache miss (prioritized)
Pref->>Decode: submit_task(index, priority=high)
Decode->>Decode: mmap read -> turbo/pil decode -> apply ICC transform (cached)
Decode->>Cache: put(decoded_image)
Cache-->>App: image ready
else cache hit
Cache-->>App: return decoded image
end
App->>UI: display image N
UI->>App: controller.dialog_closed()
App->>Pref: cancel or preempt lower-priority tasks (generation-based)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
faststack/README.md (1)
3-3: Consider adding comma after year for style consistency.Some style guides recommend placing a comma after the year in month-day-year format: "November 20, 2025," for improved readability.
faststack/faststack/benchmark_decode.py (2)
8-8: Make the benchmark image path configurable.The hardcoded Windows path
C:\Users\alanr\Pictures\...is not portable and won't work on other systems. Consider making this configurable via a command-line argument or environment variable to allow other developers to run the benchmark with their own test images.Example using command-line argument:
+import sys import mmap import time from pathlib import Path from faststack.imaging.jpeg import decode_jpeg_resized, TURBO_AVAILABLE print(f"TurboJPEG available: {TURBO_AVAILABLE}") -test_image = Path(r"C:\Users\alanr\Pictures\Lightroom\2025\2025-11-14\20251114-PB140001-2.JPG") +if len(sys.argv) < 2: + print("Usage: python benchmark_decode.py <path_to_jpeg>") + sys.exit(1) +test_image = Path(sys.argv[1]) + +if not test_image.exists(): + print(f"Error: Image not found: {test_image}") + sys.exit(1)
11-18: Consider adding error handling for the benchmark loop.The benchmark assumes the image file exists and is a valid JPEG. Consider adding basic error handling to provide clearer feedback if the decoding fails.
faststack/faststack/app.py (3)
180-207: Resize/zoom cache invalidation behavior is sound, but the resize comment is staleThe new strategy of:
- Bumping
display_generationon resize and zoom changes, and- Calling
prefetcher.cancel_all()(which bumps generation and clears scheduled tasks) without clearingimage_cachecorrectly invalidates obsolete decode tasks while allowing the LRU to evict old entries naturally and potentially re‑use them when users resize/zoom back.
However, in
_handle_resize()the comment:“We don't clear the cache or cancel prefetch work here.”
is now inaccurate, since
self.prefetcher.cancel_all()is called. Consider updating this comment to avoid confusion for future maintainers.Also applies to: 218-231
305-362: Add index bounds check inget_decoded_imageto avoid backgroundIndexError
get_decoded_imageguards against an emptyimage_fileslist but doesn’t check thatindexis within bounds before callingself.prefetcher.submit_task(index, ...). Ifindexis ever out of range (e.g., transiently during a refresh),Prefetcher.submit_taskwill raiseIndexErrorin the worker thread.Consider an early guard:
def get_decoded_image(self, index: int) -> Optional[DecodedImage]: """Retrieves a decoded image, blocking until ready to ensure correct display.""" - if not self.image_files: # Handle empty image list + if not self.image_files or index < 0 or index >= len(self.image_files): log.warning("get_decoded_image called with empty image_files.") return NoneThis keeps all error handling on the UI thread and avoids noisy background tracebacks, while your new
last_displayed_imagefallback behavior remains unchanged.
909-921: Remove unused exception variable inempty_recycle_bin(Ruff F841)Ruff flags the
except OSError as e:here becauseeis never referenced in the block. Either drop the binding or log the error explicitly. For example:- except OSError as e: - log.exception("Failed to empty recycle bin") + except OSError: + log.exception("Failed to empty recycle bin")or
- except OSError as e: - log.exception("Failed to empty recycle bin") + except OSError as e: + log.exception("Failed to empty recycle bin: %s", e)This will clear the F841 lint and keep logging behavior explicit.
faststack/faststack/imaging/prefetch.py (2)
248-427: Decoding pipeline and timing instrumentation look good; drop unusedt_after_initThe reworked
_decode_and_cache:
- Uses early generation checks before and after the heavy decode work to avoid caching stale results.
- Switches all paths to mmap +
decode_jpeg_resized(...)(falling back todecode_jpeg_rgbwhen width/height are 0), which aligns with the zero‑copy JPEG goals.- Implements the ICC path as: TurboJPEG+resize → Pillow
Imagefor ICC → cached transform → in‑place transform → NumPy copy, with detailed timing logs gated onself.debug.- Cleanly falls back to a standard decode if ICC transform fails or no monitor profile is available.
- Keeps the saturation‑mode path compatible with the new decode flow and logs timing only in debug.
Minor issue:
t_after_init = time.perf_counter()is never used, and Ruff correctly flags this (F841). You can safely remove it, or include it in a timing log if you want to measure config lookup/initialization cost.- color_mode = config.get('color', 'mode', fallback="none").lower() - t_after_init = time.perf_counter() + color_mode = config.get('color', 'mode', fallback="none").lower()Otherwise the structure and error handling of this function look sound.
428-431: Remove unused exception variable in generic decode error handler (Ruff F841)In the final
except Exception as e:block,eisn’t referenced in the body;log.exceptionalready logs the stack trace and message without it. To satisfy Ruff and avoid dead assignments, either:- except Exception as e: - log.exception("Error decoding image %s at index %d", image_file.path, index) + except Exception: + log.exception("Error decoding image %s at index %d", image_file.path, index)or include
eexplicitly in the log message.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.gitignore(1 hunks)faststack/ChangeLog.md(1 hunks)faststack/README.md(1 hunks)faststack/faststack/app.py(23 hunks)faststack/faststack/benchmark_decode.py(1 hunks)faststack/faststack/imaging/prefetch.py(9 hunks)faststack/faststack/io/helicon.py(2 hunks)faststack/faststack/io/indexer.py(3 hunks)faststack/faststack/io/sidecar.py(3 hunks)faststack/faststack/ui/keystrokes.py(1 hunks)faststack/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
faststack/faststack/imaging/prefetch.py (4)
faststack/faststack/imaging/jpeg.py (2)
decode_jpeg_rgb(22-40)decode_jpeg_resized(99-149)faststack/faststack/models.py (1)
ImageFile(8-12)faststack/faststack/app.py (1)
get_display_info(175-178)faststack/faststack/config.py (1)
get(68-69)
faststack/faststack/io/sidecar.py (1)
faststack/faststack/io/watcher.py (1)
is_alive(72-74)
faststack/faststack/app.py (5)
faststack/faststack/imaging/prefetch.py (5)
Prefetcher(125-458)clear_icc_caches(42-48)cancel_all(445-452)update_prefetch(158-233)set_image_files(153-156)faststack/faststack/models.py (2)
ImageFile(8-12)DecodedImage(33-42)faststack/faststack/io/sidecar.py (2)
SidecarManager(13-98)load(28-60)faststack/faststack/io/indexer.py (1)
find_images(20-62)faststack/faststack/io/helicon.py (1)
launch_helicon_focus(16-91)
🪛 LanguageTool
faststack/README.md
[style] ~3-~3: Some style guides suggest that commas should set off the year in a month-day-year date.
Context: ...FastStack # Version 0.9 - November 20, 2025 # By Alan Rockefeller Ultra-fast, cach...
(MISSING_COMMA_AFTER_YEAR)
🪛 Ruff (0.14.5)
faststack/faststack/imaging/prefetch.py
262-262: Local variable t_after_init is assigned to but never used
Remove assignment to unused variable t_after_init
(F841)
faststack/faststack/app.py
355-355: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🔇 Additional comments (21)
.gitignore (1)
27-29: LGTM!Adding mypy cache directories to
.gitignoreis standard practice and aligns with the Python tooling improvements in this PR.faststack/pyproject.toml (1)
8-8: LGTM!Version bump to 0.9 is consistent with the changelog and appropriately reflects the performance improvements and new features in this release.
faststack/faststack/ui/keystrokes.py (1)
70-70: LGTM!Changing key press logging from
infotodebuglevel is appropriate for frequent events and reduces log noise while maintaining debugging capability.faststack/ChangeLog.md (1)
3-17: LGTM!The changelog entry for 0.9.0 is comprehensive and well-structured, clearly documenting the performance improvements with quantified metrics and new features introduced in this release.
faststack/faststack/io/helicon.py (2)
56-56: LGTM!Logging the input file list improves observability for debugging Helicon Focus integration issues.
73-74: LGTM!The improved log message formatting enhances readability and consistency.
faststack/faststack/io/indexer.py (3)
40-40: LGTM!Using
log.exceptioninstead oflog.erroris better for debugging as it automatically includes the full stack trace.
58-62: LGTM!Conditional logging based on DEBUG level is a good practice for reducing log noise in production while maintaining detailed timing information during development.
79-79: Verify the RAW pairing logic change is intentional.The condition changed from
dt < min_dt(strict less-than) todt <= min_dt(less-than-or-equal). This affects behavior when multiple RAW files have identical time differences from the JPG:
- Previous behavior: Only updated
best_matchif the new candidate was strictly better- New behavior: Updates
best_matcheven when time differences are equal, causing the last RAW file in the list with the minimum time difference to be selectedThis could change which RAW file gets paired when there are multiple candidates with identical time differences. Please confirm this change is intentional and aligns with the desired pairing strategy.
faststack/faststack/io/sidecar.py (1)
14-18: Debug flag wiring and watcher guard look correctConstructor-level
debugflag, conditional json timing log, and the additionalhasattr(self.watcher, "is_alive")guard insave()are consistent with the rest of the app’s debug plumbing and safely avoid AttributeError with non-standard watcher implementations.Also applies to: 39-40, 67-69
faststack/faststack/app.py (9)
74-108: Initialization of cached image list, sidecar debug, and prefetcher debug flag is consistentUsing
_all_imagesas the canonical on-disk list,image_filesas the filtered view, wiringSidecarManager(..., debug=_debug_mode), and passingget_display_infoanddebug=_debug_modeintoPrefetcherare all coherent with the rest of the PR’s debug and caching model; the added_last_image_lockis a reasonable safeguard aroundlast_displayed_imageeven though access is likely UI-thread-only.Also applies to: 96-105
136-152: In‑memory filtering over_all_imagesis a solid performance winSwitching
apply_filter/clear_filterto call_apply_filter_to_cached_list()over_all_images(populated byrefresh_image_list()) removes repeated disk scans on every keystroke while still:
- Respecting
_filter_enabledand_filter_string- Keeping
prefetcher.set_image_files()and metadata cache invalidation in syncNo functional issues spotted; behavior matches the documented expectations in
refresh_image_list()’s docstring.Also applies to: 160-172, 277-304
241-260: Directional prefetch integration with navigation and resize debounce looks correctThe extended
_do_prefetch(index, is_navigation=False, direction=None):
- Properly cancels a pending resize debounce and applies
_handle_resize()immediately when navigation happens during a resize, so prefetch uses up‑to‑date dimensions.- Defers prefetch until
display_readyis true, usingpending_prefetch_index.- For navigation (e.g.,
next_image/prev_image), passesis_navigation=Trueanddirection=±1, matching the newPrefetcher.update_prefetchAPI.The control flow aligns with the prefetcher’s directional bias design; no correctness issues evident.
Also applies to: 391-401
406-413: Additional guards around metadata and stack queries improve robustnessThe new checks in
get_current_metadata,toggle_current_flag,toggle_current_reject, andis_stackedthat return early whenimage_filesis empty orcurrent_indexis out of bounds will prevent index errors during edge cases (e.g., after deletes or refreshes). The_logged_empty_metadataflag also avoids repeated log spam.These are good defensive additions.
Also applies to: 435-453, 1091-1097
480-531: Helicon selection semantics and helper look correct, with RAW‑preferred/JPG‑fallbackThe new flow:
toggle_selection()selectingraw_pairwhen available, otherwisepath,launch_helicon()preferring explicit selections over stacks and only clearing selections/stacks on success, and_launch_helicon_with_files()deduplicating/sorting files, scheduling temp‑file deletion, and markingsidecarmetadata for anyImageFilewhose RAW or JPG matches a launched path,implements the advertised “RAW preferred, JPG fallback” behavior without obvious edge‑case regressions. The
_delete_temp_filehelper is also straightforward and safe under repeated calls.Also applies to: 538-567, 568-576
648-668: Callingclear_icc_caches()on color‑mode change is appropriateInvoking
clear_icc_caches()before clearing the image cache, cancelling prefetch, bumpingdisplay_generation, and restarting prefetch ensures:
- Monitor/profile and transform caches are flushed when ICC/saturation/none modes change.
- Subsequent decodes rebuild transforms consistently with the new mode.
This ties app behavior cleanly into the new ICC caching primitives.
810-821: Cancelling prefetch after delete/undo is the right trade‑offAfter delete and undo operations you now:
- Bump
display_generation,- Clear
image_cache,- Call
self.prefetcher.cancel_all()and thenupdate_prefetch(self.current_index).Given that the image list has structurally changed, cancelling outstanding tasks avoids wasted work and prevents caching decodes for indices that may now point at different files. This matches the generation semantics in the prefetcher.
Also applies to: 868-873
923-997: Photoshop launch error handling and logging look solidThe
edit_in_photoshopflow validates the executable path, safely parses additional args withshlex.split, avoidsshell=True, and cleanly handlesFileNotFoundErrorand otherOSError/SubprocessErrorcases with both user‑facing status messages andlog.exceptionstack traces. Nothing concerning here.
1070-1081: Stack info logging addition is harmless and usefulThe added debug log in
_get_stack_infoprovides clear insight into which stack state is reported for a given index, which is helpful when reasoning about stacking UI without affecting behavior.faststack/faststack/imaging/prefetch.py (2)
125-152: Prefetcher constructor changes and directional state are reasonableConstraining
max_workersviamin((os.cpu_count() or 1) * 2, 4)is a sensible default for I/O‑bound JPEG decoding, and the newdebugflag plus_last_navigation_direction/_direction_biasfields are cleanly initialized and ready for use inupdate_prefetch. No issues seen here.
158-235: Directional, generation‑aware prefetch window looks correct
update_prefetchnow:
- Cleans out
_scheduledentries for generations< self.generationto prevent unbounded growth.- Tracks
_last_navigation_directionfrom the optionaldirectionargument.- Expands the radius only after the user has navigated at least twice.
- Splits the effective radius into asymmetric
behind/aheadcounts based on_direction_biasand clamps them to at least 1.- Computes
[start, end)safely even whenimage_filesis empty.- Cancels futures outside
[start, end)and builds apriority_orderthat always includescurrent_indexand then the direction of travel before the opposite side.The logic matches how
_do_prefetchand navigation methods inapp.pycall this and should give the intended “biased” sequential browsing prefetch without obvious off‑by‑one errors.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
faststack/faststack/imaging/prefetch.py (1)
129-156: Stale task cancellation leaves indices “permanently scheduled” within a generation
update_prefetchmaintainsself._scheduled[generation]but, when canceling stale futures (indices outside[start, end)), it only removes them fromself.futures, not from thescheduledset. As a result, any index whose future was canceled as stale in the current generation will never be resubmitted by prefetch, even if it comes back into the window later in the same generation (e.g., rapid navigation back and forth).To keep behavior intuitive (allowing re‑prefetch after a cancellation) while still avoiding duplicate submissions, consider discarding stale indices from
scheduledat the same time:# Cancel stale futures stale_keys = [] - for index, future in self.futures.items(): - if index < start or index >= end: - future.cancel() - stale_keys.append(index) - for key in stale_keys: - del self.futures[key] - - # Submit new tasks - prioritize current image and direction of travel - scheduled = self._scheduled.setdefault(self.generation, set()) + scheduled = self._scheduled.setdefault(self.generation, set()) + for index, future in list(self.futures.items()): + if index < start or index >= end: + if future.cancel(): + stale_keys.append(index) + scheduled.discard(index) + for key in stale_keys: + del self.futures[key] + + # Submit new tasks - prioritize current image and direction of travelThis keeps
_scheduledconsistent with actual pending work and lets previously canceled indices be scheduled again when they re-enter the prefetch range.Also applies to: 162-238
faststack/faststack/qml/Main.qml (1)
420-423: Implementshow_jump_to_image_dialog()method to actually open the dialog.The keybinder integration is properly configured—Qt.Key_G maps to the method and keybinder calls it correctly. However, the method at
faststack/faststack/app.pylines 420–423 is non-functional (onlypass). The dialog never opens when the user presses G. The method needs to emit a signal or invoke the QMLshow_jump_to_image_dialog()function to openjumpToImageDialog.
♻️ Duplicate comments (1)
faststack/faststack/imaging/prefetch.py (1)
6-7: ICC profile and transform caching now use stable, reusable keysThe switch to
_icc_transform_cachekeyed by(src_profile_key, monitor_icc_path)withsrc_profile_keyderived from a SHA‑256 digest (or"srgb_builtin"for the built‑in sRGB profile) fixes the earlierid()‑based cache issue and gives true transform reuse without identity collisions.clear_icc_caches()correctly resets both profile and transform caches plus the warning flag, andget_monitor_profile()continues to cache by path with graceful handling of empty/misconfigured paths.Also applies to: 23-25, 27-52, 54-85
🧹 Nitpick comments (8)
faststack/faststack/qml/FilterDialog.qml (1)
10-11: Filter dialog wiring and keyboard UX look good; consider simplifying the text bindingClosing on Escape, updating
filterDialog.filterStringononTextChanged, and accepting on Enter/Return are all appropriate and improve usability. TheonOpened/onClosednotifications tocontrolleralso align with dialog lifecycle handling elsewhere.One small QML nit: you both declare
text: filterDialog.filterStringand imperatively assignfilterField.text = filterDialog.filterStringinonOpened, which breaks the original binding anyway. You could drop thetext:binding and rely solely on theonOpenedassignment plusonTextChangedto avoid redundant wiring.Also applies to: 26-61, 39-53, 65-79
faststack/faststack/qml/SettingsDialog.qml (1)
10-12: Settings dialog cache usage UI and lifecycle handling look goodThe dialog now:
- Closes on Escape and takes focus.
- Shows live cache usage via
cacheUsage+cacheUsageTimerpollinguiState.get_cache_usage_gb()only while visible.- Keeps
cacheSizeFieldtext synchronized withcacheSizeon first construction and on each open, with sensible [0.5, 16] validation and formatting.- Notifies
controller.dialog_opened()/controller.dialog_closed()based onvisible, which is consistent with other dialogs in the PR.Only minor nuance:
onVisibleChangedwill fire once withvisible == falseat creation time, socontroller.dialog_closed()will be called once before any open. If the controller treats that as idempotent, this is fine; otherwise you might gate the first call behind a simple “initialized” flag.Also applies to: 15-31, 99-124, 163-171
faststack/faststack/qml/JumpToImageDialog.qml (2)
20-23: Potential focus issue with selectAll().Calling
selectAll()immediately after clearing text (line 21) and setting focus (line 22) may not work reliably due to timing issues. The text field may not be ready to select empty text.Consider removing
selectAll()on line 23 since the text field is empty anyway:onOpened: { imageNumberField.text = "" imageNumberField.forceActiveFocus() - imageNumberField.selectAll() // Notify Python that a dialog is open controller.dialog_opened() }
63-63: Magic number: maximumLength should scale with maxImageCount.The hardcoded
maximumLength: 4assumes image counts never exceed 9999. If the image count is smaller (e.g., 50 images), users can still type 4 digits, leading to confusing validation failures.Consider dynamically calculating maximumLength based on maxImageCount:
-maximumLength: 4 +maximumLength: Math.max(1, Math.ceil(Math.log10(jumpDialog.maxImageCount + 1)))Or keep it simple with a reasonable upper bound like
Math.min(6, String(maxImageCount).length + 1).faststack/faststack/qml/Main.qml (1)
418-418: Minor: f-string without placeholders.Line 418 uses an f-string without any placeholders. This is a minor style issue flagged by static analysis.
-self.update_status_message(f"Invalid image number") +self.update_status_message("Invalid image number")faststack/faststack/app.py (3)
351-363: Note: Exception variable captured but not logged.At lines 352, 356, 360, exceptions are caught but the variable
eis not used in the log message. Thelog.exception()calls already include the exception info, so this is not a bug, but you could simplify by removing the exception variable:- except concurrent.futures.TimeoutError: + except concurrent.futures.TimeoutError: log.exception("Timeout decoding image at index %d", index) with self._last_image_lock: return self.last_displayed_image - except concurrent.futures.CancelledError: + except concurrent.futures.CancelledError: log.warning("Decode cancelled for index %d", index) with self._last_image_lock: return self.last_displayed_image - except Exception as e: + except Exception: log.exception("Error decoding image at index %d", index) with self._last_image_lock: return self.last_displayed_imageBased on learnings
407-419: Minor: Remove extraneous f-prefix.Line 418 uses an f-string without any placeholders. This is a minor style issue but worth cleaning up:
- self.update_status_message(f"Invalid image number") + self.update_status_message("Invalid image number")
966-967: Consider using log.exception for consistency.At lines 966-967 and 999-1000,
log.error()is used but could belog.exception()for consistency with error handling elsewhere in the file (e.g., lines 862, 915). However, the current implementation is acceptable if you want to differentiate between expected failures and unexpected exceptions.Also applies to: 999-1000
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
faststack/ChangeLog.md(1 hunks)faststack/faststack/app.py(25 hunks)faststack/faststack/imaging/cache.py(1 hunks)faststack/faststack/imaging/prefetch.py(8 hunks)faststack/faststack/qml/FilterDialog.qml(3 hunks)faststack/faststack/qml/JumpToImageDialog.qml(1 hunks)faststack/faststack/qml/Main.qml(5 hunks)faststack/faststack/qml/SettingsDialog.qml(3 hunks)faststack/faststack/ui/keystrokes.py(2 hunks)faststack/faststack/ui/provider.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- faststack/ChangeLog.md
- faststack/faststack/ui/keystrokes.py
🧰 Additional context used
🧬 Code graph analysis (3)
faststack/faststack/imaging/prefetch.py (4)
faststack/faststack/imaging/jpeg.py (2)
decode_jpeg_rgb(22-40)decode_jpeg_resized(99-149)faststack/faststack/models.py (1)
ImageFile(8-12)faststack/faststack/app.py (1)
get_display_info(178-181)faststack/faststack/config.py (1)
get(68-69)
faststack/faststack/ui/provider.py (1)
faststack/faststack/app.py (1)
get_cache_usage_gb(649-651)
faststack/faststack/app.py (7)
faststack/faststack/imaging/prefetch.py (5)
Prefetcher(129-484)clear_icc_caches(46-52)cancel_all(471-478)update_prefetch(162-237)submit_task(239-267)faststack/faststack/models.py (2)
ImageFile(8-12)DecodedImage(33-42)faststack/faststack/io/sidecar.py (4)
SidecarManager(13-98)load(28-60)get_metadata(93-95)save(62-91)faststack/faststack/ui/keystrokes.py (1)
handle_key_press(65-90)faststack/faststack/io/indexer.py (1)
find_images(20-62)faststack/faststack/ui/provider.py (3)
launch_helicon(243-244)clear_all_stacks(247-248)get_cache_usage_gb(279-280)faststack/faststack/io/helicon.py (1)
launch_helicon_focus(16-91)
🪛 Ruff (0.14.5)
faststack/faststack/app.py
359-359: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
418-418: f-string without any placeholders
Remove extraneous f prefix
(F541)
992-992: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (43)
faststack/faststack/imaging/cache.py (1)
36-44: Decoded image size computation looks robust across buffer typesUsing
nbyteswhen available, thenlen(buffer), with awidth * height * 3fallback, correctly covers numpy arrays, memoryviews, and generic buffers while preserving previous behavior for non-DecodedImageitems.faststack/faststack/imaging/prefetch.py (2)
269-456: ICC decode path, mmap usage, and timing instrumentation are consistent and guardedThe refactored
_decode_and_cache:
- Uses
mmapto feed JPEG bytes directly intodecode_jpeg_resized, avoiding an extra copy.- Implements an ICC pipeline that:
- Loads the monitor profile once via
get_monitor_profile().- Extracts embedded ICC bytes, builds a stable digest key, and falls back to the shared sRGB profile/key when absent.
- Reuses
get_icc_transform(...)and applies it in-place on the PIL image before converting back tonp.uint8.- Provides a clear fallback to standard decode when ICC conversion or monitor profile loading fails.
- Adds detailed, debug-gated timing logs for ICC, saturation, and standard paths.
- Rechecks
self.generationbefore caching to avoid populating the cache with obsolete decodes and logs any exceptions before returningNone.This structure looks correct and aligns well with the PR’s performance and robustness goals.
239-267: Priority flag is already properly integrated and actively used.Verification shows
priority=Trueis already passed at the appropriate call site inapp.py:335for the current-image decode path, with a comment confirming the intent. The prefetch operations correctly default topriority=False. The mechanism is functional and not unused complexity—no further action needed.faststack/faststack/ui/provider.py (1)
274-281: Cache usage slot wiring is straightforward and consistentExposing
get_cache_usage_gbas a@Slot(result=float)that delegates toapp_controller.get_cache_usage_gb()cleanly supports the new QML consumers without changing existing behavior.faststack/faststack/qml/Main.qml (16)
401-403: LGTM: Dialog integration is clean.The
show_jump_to_image_dialog()function provides a clean interface for external invocation. The dialog is properly instantiated with dynamicmaxImageCountbinding.
324-325: LGTM: Consistent dialog behavior.Adding
closePolicy: Popup.CloseOnEscapeandfocus: trueto all dialogs provides consistent escape-key dismissal and focus behavior across the UI.Also applies to: 368-369
42-42: LGTM: Debug mode propagation.Debug mode is now correctly propagated to
SidecarManagerandPrefetcher, enabling detailed timing logs in debug mode as mentioned in the PR objectives.Also applies to: 93-93, 104-104
107-107: LGTM: Thread safety and dialog coordination.Adding
_last_image_lockprevents race conditions when multiple threads accesslast_displayed_image. The_dialog_openflag cleanly disables global keybindings when dialogs are open.Also applies to: 135-136
77-78: Excellent performance optimization for filtering.Splitting
_all_images(full disk list) fromimage_files(filtered list) eliminates disk scans on every filter keystroke. The_apply_filter_to_cached_list()method performs fast in-memory filtering, achieving the 100-1000× speedup mentioned in the PR objectives.Also applies to: 280-306
222-228: Smart cache management avoids unnecessary clearing.Incrementing
display_generationwithout clearing the cache allows the LRU to naturally evict old entries while enabling instant reuse if the user toggles zoom repeatedly. This implements the "smart cache management" mentioned in the PR objectives.
698-699: ICC cache clearing on color mode change.Calling
clear_icc_caches()when color mode changes ensures stale ICC profiles and transforms are discarded, implementing the "ICC transform caching" feature from the PR objectives.
244-262: Directional prefetching with resize cancellation.The updated
_do_prefetch()signature supports directional context (forward/backward), enabling the asymmetric 70/30 prefetching mentioned in the PR objectives. Canceling the resize timer during navigation ensures prefetch uses correct dimensions.
308-365: Robust error handling with thread safety.The updated
get_decoded_image()properly handles cache misses by blocking with priority decode. Thread safety is correctly implemented with_last_image_lockaround all accesses tolast_displayed_image. Exception handling gracefully falls back to the last displayed image to avoid grey squares.Note: The static analysis hint about unused variable
eon line 359 is a false positive—log.exception()automatically captures and logs the exception context.
407-418: LGTM: Jump-to-image implementation.The
jump_to_image()implementation correctly handles bounds checking, direction calculation for prefetching, and provides appropriate user feedback.
425-435: LGTM: Dialog coordination prevents key event conflicts.The
dialog_opened()anddialog_closed()slots properly toggle_dialog_open, which is checked ineventFilter()to disable global keybindings when dialogs are open.
514-532: LGTM: RAW preference in selection.The updated
toggle_selection()correctly prefers RAW files when available, falling back to JPG. This implements the "JPG fallback for Helicon" feature mentioned in the PR objectives.
535-600: Excellent Helicon integration with RAW/JPG fallback.The updated
launch_helicon()and_launch_helicon_with_files()properly handle:
- RAW preference with JPG fallback for both selected files and stacks
- Success tracking to only clear stacks when launches succeed
- Metadata updates that match by both RAW pair and JPG path
This implements the "JPG fallback for Helicon" feature from the PR objectives.
441-443: LGTM: Comprehensive guard clauses.Guard clauses throughout the code prevent out-of-bounds access and null pointer issues when
image_filesis empty or the index is invalid. This improves robustness.Also applies to: 470-471, 480-481, 516-517, 1172-1172
926-1000: Enhanced recycle bin UX.The improved shutdown flow provides users with three clear options (Delete Permanently, Restore, Keep), and the
restore_all_from_recycle_bin()implementation safely restores files while avoiding overwrites.Note: The static analysis hint about line 992 is a false positive—line 992 is inside the try block, not an except handler where
logging.exception()should be used. The actuallog.exception()calls on lines 967 and 1000 are correctly placed in except blocks.
648-651: LGTM: Cache usage visibility.The new
get_cache_usage_gb()method provides visibility into current cache usage, supporting the cache management features.faststack/faststack/app.py (23)
42-42: LGTM: ICC cache clearing integrated correctly.The import of
clear_icc_cachesaligns with the PR's ICC transform caching feature and is properly utilized at line 699.
77-78: LGTM: Smart separation of filtered vs. cached image lists.Separating
image_files(filtered display list) from_all_images(full disk cache) enables the 100-1000× filter performance improvement mentioned in the PR objectives by eliminating disk scans on every keystroke.
107-107: LGTM: Thread-safety lock protects shared state.The
_last_image_lockproperly guardslast_displayed_imagefrom race conditions with concurrent prefetch threads, as used consistently at lines 325, 345, 353, 357, 361, 364.
135-136: LGTM: Dialog state tracking prevents keybinding conflicts.The
_dialog_openflag correctly disables global keybindings when dialogs are active (checked at line 235), preventing unintended actions when users interact with dialogs.
280-303: LGTM: Efficient image list caching eliminates disk I/O during filtering.The separation between
refresh_image_list()(disk scan) and_apply_filter_to_cached_list()(in-memory filtering) is well-designed. This directly achieves the 100-1000× filter performance improvement by caching the full image list in memory and avoiding repeated disk scans.
217-231: LGTM: Smart cache retention on zoom toggle.This implements the "smart cache management" from the PR objectives. By incrementing
display_generationwithout clearing the cache, old entries become naturally unreachable (LRU eviction) while allowing instant reuse if the user toggles zoom repeatedly. This is more efficient than aggressive cache clearing.
234-237: LGTM: Dialog-aware event filtering.Correctly prevents global keybindings from interfering with dialog interactions by returning early when
_dialog_openis True.
244-262: LGTM: Directional prefetching with navigation-aware resize handling.The enhanced signature supports directional prefetching (70% ahead, 30% behind per PR objectives). The navigation-during-resize handling (lines 254-256) ensures prefetch uses correct dimensions by immediately applying pending resizes, preventing cache misses from stale size information.
308-365: LGTM: Robust cache miss handling with proper thread-safety.The blocking decode on cache miss is acceptable for an image viewer where users expect immediate display. The
priority=Trueflag (line 335) correctly cancels pending prefetch tasks to free workers. Thread-safety is maintained consistently across all code paths with_last_image_lock.
395-405: LGTM: Direction-aware navigation.Passing explicit
directionvalues enables the directional prefetching optimization (70% ahead, 30% behind) mentioned in the PR objectives.
425-435: LGTM: Dialog state coordination.The
dialog_opened/dialog_closedslots properly toggle the_dialog_openflag, which is checked ineventFilter(line 235) to prevent keybinding conflicts.
440-445: LGTM: Defensive guards prevent crashes on invalid state.Consistent guard clauses across metadata methods prevent crashes when
image_filesis empty orcurrent_indexis out of bounds.Also applies to: 470-471, 480-481
514-532: LGTM: Selection prefers RAW with JPG fallback.This correctly implements the RAW-first selection strategy mentioned in the PR objectives for Helicon Focus workflows. The guard clause and selection logic are clean.
535-571: LGTM: Helicon integration with RAW/JPG fallback and success tracking.The improved error handling ensures selections and stacks are only cleared on successful launch. The RAW-preferred, JPG-fallback strategy (lines 552-553) aligns with the PR's Helicon Focus workflow improvements.
572-601: LGTM: Boolean return enables better error handling.The signature change to return
boolallows callers to track success and avoid clearing state on failure. The metadata update loop (lines 589-596) correctly matches files by checking bothraw_pairandpath.
602-609: LGTM: Improved temp file cleanup with documentation.The added docstring and logging make the cleanup behavior clear and observable.
649-651: LGTM: Cache usage monitoring.Provides visibility into current cache utilization, useful for diagnostics and UI display.
698-699: LGTM: ICC cache clearing on color mode change.Properly clears cached ICC transforms when the color mode changes, ensuring color transformations are rebuilt with the new settings. This integrates with the ICC transform caching optimization from the PR objectives.
856-862: LGTM: Consistent cache management and error logging.Using
prefetcher.cancel_all()aligns with the smart cache management strategy. Thelog.exception()calls provide better debugging context thanlog.error().Also applies to: 909-915
926-944: LGTM: Enhanced shutdown dialog with restore option.The improved UX provides three clear options for handling files in the recycle bin, with the safe "Keep in Recycle Bin" as the default. This prevents accidental data loss.
969-1000: LGTM: Safe restore-all implementation with collision detection.The implementation correctly:
- Skips files that already exist (prevents overwriting)
- Logs individual operations for auditability
- Clears delete history only on successful completion
- Handles errors gracefully with per-file error logging
1071-1076: LGTM: Enhanced error logging for Photoshop integration.Using
log.exception()provides full stack traces for debugging subprocess failures.
1172-1176: LGTM: Defensive guard in is_stacked.Consistent with guard clauses added throughout the file to prevent crashes on invalid state.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
faststack/faststack/qml/FilterDialog.qml (1)
26-64: Consider removing the Item wrapper for simpler structure.The
contentItem: Itemwrapping a singleColumnadds unnecessary nesting. You could set theColumnas thecontentItemdirectly for a cleaner structure.Apply this diff:
- contentItem: Item { - - Column { + contentItem: Column { spacing: 16 anchors.fill: parent anchors.margins: 20 - + Label { text: "Show only images whose filename contains:" wrapMode: Text.WordWrap width: parent.width } - + TextField { id: filterField placeholderText: "Enter text to filter (e.g., 'stacked', 'IMG_001')..." width: parent.width height: 50 selectByMouse: true focus: true font.pixelSize: 16 verticalAlignment: TextInput.AlignVCenter onTextChanged: { filterDialog.filterString = text } Keys.onReturnPressed: filterDialog.accept() Keys.onEnterPressed: filterDialog.accept() } - + Label { text: "Leave empty to show all images." font.italic: true opacity: 0.7 wrapMode: Text.WordWrap width: parent.width } - } }faststack/faststack/qml/JumpToImageDialog.qml (1)
17-18: Consider inheriting theme from parent for consistency.The dialog hardcodes
Material.theme: Material.Dark, which differs from FilterDialog's dynamic theme inheritance pattern (Material.theme: uiState && uiState.theme === 0 ? Material.Dark : Material.Light). For consistency with the app's theme switching, consider inheriting the theme from the parent or using the same dynamic pattern.Apply this diff:
// Inherit Material theme from parent - Material.theme: Material.Dark + Material.theme: uiState && uiState.theme === 0 ? Material.Dark : Material.Light Material.accent: "#4fb360"faststack/faststack/imaging/prefetch.py (1)
287-376: ICC decode path is well-implemented with good error handling.The implementation properly uses mmap for zero-copy reads, caches ICC transforms with stable keys, and gracefully falls back on errors. The timing instrumentation is appropriately gated by debug mode.
Optional optimization: Consider extracting ICC profile during initial decode.
Line 310 opens the image file again just to extract the ICC profile. If
decode_jpeg_resizedcould optionally return both the decoded array and ICC bytes in one pass, it would avoid the second file open.faststack/faststack/app.py (2)
395-426: Navigation and jump implementations are correct.The directional hints passed to prefetch will optimize cache hit rates for sequential browsing patterns.
Minor: Remove unnecessary f-string prefix on line 418.
Line 418 uses an f-string without any placeholders. This is harmless but can be simplified.
Apply this diff:
else: log.warning("Invalid image index: %d", index) - self.update_status_message(f"Invalid image number") + self.update_status_message("Invalid image number")
308-365: get_decoded_image properly handles cache misses with thread safety.The locking around
last_displayed_imageprevents race conditions, and the priority task submission reduces latency for synchronous decodes on cache misses.Optional: Remove unused exception variable on line 359.
The exception is captured as
ebut never used sincelog.exceptionautomatically logs the exception.Apply this diff:
except Exception as e: - log.exception("Error decoding image at index %d", index) + log.exception("Error decoding image at index %d", index) with self._last_image_lock: return self.last_displayed_imageOr change to:
- except Exception as e: + except Exception: log.exception("Error decoding image at index %d", index)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
faststack/faststack/app.py(25 hunks)faststack/faststack/imaging/prefetch.py(8 hunks)faststack/faststack/qml/FilterDialog.qml(3 hunks)faststack/faststack/qml/JumpToImageDialog.qml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
faststack/faststack/imaging/prefetch.py (4)
faststack/faststack/imaging/jpeg.py (2)
decode_jpeg_rgb(22-40)decode_jpeg_resized(99-149)faststack/faststack/models.py (1)
ImageFile(8-12)faststack/faststack/app.py (1)
get_display_info(178-181)faststack/faststack/config.py (1)
get(68-69)
faststack/faststack/app.py (7)
faststack/faststack/imaging/prefetch.py (5)
Prefetcher(129-487)clear_icc_caches(46-52)cancel_all(474-481)update_prefetch(162-240)submit_task(242-270)faststack/faststack/models.py (2)
ImageFile(8-12)DecodedImage(33-42)faststack/faststack/io/sidecar.py (4)
SidecarManager(13-98)load(28-60)get_metadata(93-95)save(62-91)faststack/faststack/ui/keystrokes.py (1)
handle_key_press(65-90)faststack/faststack/io/indexer.py (1)
find_images(20-62)faststack/faststack/ui/provider.py (3)
launch_helicon(243-244)clear_all_stacks(247-248)get_cache_usage_gb(279-280)faststack/faststack/io/helicon.py (1)
launch_helicon_focus(16-91)
🪛 Ruff (0.14.5)
faststack/faststack/app.py
359-359: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
418-418: f-string without any placeholders
Remove extraneous f prefix
(F541)
994-994: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (20)
faststack/faststack/qml/FilterDialog.qml (2)
10-12: LGTM!The closePolicy and increased dialog dimensions improve UX by allowing ESC key dismissal and providing more space for the enhanced TextField.
67-81: LGTM!The dialog lifecycle notifications properly integrate with the app-level keybinding suppression mechanism. This pattern is consistent with the JumpToImageDialog and prevents key event conflicts while dialogs are open.
faststack/faststack/qml/JumpToImageDialog.qml (3)
20-37: LGTM!The event handlers properly integrate with the dialog lifecycle management and input validation. The 1-based to 0-based index conversion is correct, and the validation logic ensures only valid image numbers are accepted.
54-72: LGTM!The input validation is well-designed with both
IntValidatorfor range checking and a computedmaximumLengththat correctly calculates the number of digits needed. The math handles edge cases properly (e.g., maxImageCount=0, boundary values).
39-78: LGTM!The content layout follows QML best practices with proper spacing, alignment, and dynamic sizing. The label clearly communicates the valid range to users.
faststack/faststack/imaging/prefetch.py (6)
27-52: LGTM!The ICC transform caching now uses stable keys (SHA-256 digest + profile path) instead of object IDs, addressing the previous review concern. This ensures correct cache reuse and prevents identity collision bugs.
130-156: LGTM!The initialization enhancements properly support debug mode and directional prefetching. The worker count cap at 4 is reasonable for I/O-bound operations to prevent thread thrashing.
162-240: LGTM!The directional prefetch implementation is well-designed. The asymmetric windowing (70% ahead / 30% behind) optimizes for sequential browsing patterns, and the generation handling fix (not incrementing on navigation) prevents unnecessary cache invalidation. The priority ordering ensures current image loads first, followed by images in the direction of travel.
242-270: LGTM!The priority task mechanism effectively reduces latency for on-demand decodes by canceling lower-priority prefetch tasks. This ensures the current image loads quickly even when the prefetch queue is full.
376-439: LGTM!The standard decode paths are consistent and well-instrumented. The mmap-based reads eliminate memory copies across all code paths, and the comprehensive timing logs support performance analysis when debug mode is enabled.
440-460: LGTM!The generation validation before caching and the exception handling with
log.exceptionprovide robust error handling and prevent stale cache entries.faststack/faststack/app.py (9)
42-42: LGTM!The new state additions support key performance improvements:
_all_imagescaching enables fast in-memory filtering (100-1000× speedup for large directories per PR objectives)_last_image_lockprevents race conditions on the last displayed image cache_dialog_openflag enables keybinding suppression during dialog interactionAlso applies to: 78-79, 107-107, 135-136
233-242: LGTM!The dialog state management properly disables global keybindings when dialogs are open, preventing conflicts between dialog and main window keyboard shortcuts. The implementation is straightforward and integrates cleanly with the QML dialog lifecycle.
Also applies to: 427-437
244-262: LGTM!The directional prefetch integration is well-designed. Canceling the resize debounce timer on navigation ensures prefetch uses accurate dimensions, preventing wasted work with incorrect display sizes.
442-490: LGTM!The enhanced bounds checking in metadata access methods prevents crashes when the image list is empty or indices are out of range. These defensive checks improve robustness.
516-535: LGTM!The RAW file preference in selection aligns with the Helicon Focus workflow objectives. The JPG fallback ensures the feature works even without RAW files, supporting the "JPG-only workflows" mentioned in the PR description.
537-612: LGTM!The Helicon integration properly supports both RAW and JPG workflows with appropriate fallbacks. The per-stack success tracking and metadata updates are well-implemented. The temp file cleanup and error handling are robust.
651-653: LGTM!The cache management enhancements properly integrate ICC cache clearing with color mode changes and provide cache usage monitoring for the UI. The generation increment ensures stale cached images with old color processing are not reused.
Also applies to: 700-720
280-306: LGTM!The separation of disk scanning (
refresh_image_list) and in-memory filtering (_apply_filter_to_cached_list) is well-designed. This enables the 100-1000× speedup for filter operations mentioned in the PR objectives by eliminating unnecessary disk scans on every keystroke.
968-970: LGTM!These exception handlers already use
log.exceptionappropriately for full traceback logging. The static analysis hint appears to be a false positive as the code follows the recommended pattern.Also applies to: 1001-1002
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
faststack/faststack/qml/FilterDialog.qml (2)
10-12: Escape-to-close and fixed dialog sizeUsing
closePolicy: Popup.CloseOnEscapeis good for keyboard UX and consistent with other Qt popups. The hard‑codedwidth: 500/height: 250is probably fine on desktop, but it can feel cramped or oversized on very small or very large windows; consider switching toimplicitWidth/implicitHeightbased onwindow.width/heightor adding min/max constraints for better responsiveness.
28-60: Column padding and repeated width formulaThe move to
Column { padding: 20 }pluswidth: parent.width - parent.padding * 2on the labels and text field gives clean, consistent horizontal margins and is functionally sound. The only downside is the repetition of the width expression across multiple children, which is slightly brittle if you ever tweak the padding.You could centralize this with a helper property on the
Columnand bind children to it, e.g.:contentItem: Column { - spacing: 16 - padding: 20 + spacing: 16 + padding: 20 + readonly property real contentWidth: width - padding * 2 @@ - Label { + Label { text: "Show only images whose filename contains:" wrapMode: Text.WordWrap - width: parent.width - parent.padding * 2 + width: parent.contentWidth } @@ - TextField { + TextField { id: filterField placeholderText: "Enter text to filter (e.g., 'stacked', 'IMG_001')..." - width: parent.width - parent.padding * 2 + width: parent.contentWidth height: 50 @@ - Label { + Label { text: "Leave empty to show all images." @@ - width: parent.width - parent.padding * 2 + width: parent.contentWidth }This keeps layout intent in one place and makes future tuning easier.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
faststack/faststack/app.py(25 hunks)faststack/faststack/qml/FilterDialog.qml(3 hunks)faststack/faststack/qml/JumpToImageDialog.qml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/faststack/qml/JumpToImageDialog.qml
🧰 Additional context used
🧬 Code graph analysis (1)
faststack/faststack/app.py (4)
faststack/faststack/imaging/prefetch.py (6)
Prefetcher(129-487)clear_icc_caches(46-52)cancel_all(474-481)update_prefetch(162-240)set_image_files(157-160)submit_task(242-270)faststack/faststack/io/sidecar.py (4)
SidecarManager(13-98)load(28-60)get_metadata(93-95)save(62-91)faststack/faststack/io/indexer.py (1)
find_images(20-62)faststack/faststack/io/helicon.py (1)
launch_helicon_focus(16-91)
🪛 Ruff (0.14.5)
faststack/faststack/app.py
359-359: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
994-994: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (13)
faststack/faststack/app.py (11)
42-42: LGTM! Initialization changes support the new features correctly.The additions properly support:
- ICC cache clearing via
clear_icc_cachesimport- In-memory filtering with
_all_imagescache- Thread-safe image access via
_last_image_lock- Dialog state tracking with
_dialog_open- Debug mode propagation to components
Also applies to: 77-78, 93-93, 103-107, 134-136
149-149: LGTM! In-memory filtering is a significant performance improvement.The refactored filtering logic correctly:
- Caches the full image list in
_all_imageson disk scan- Applies filters in memory via
_apply_filter_to_cached_list()(avoiding repeated disk I/O)- Provides clear documentation distinguishing full refresh from filtering operations
This achieves the PR's goal of 100-1000× faster filtering for large directories.
Also applies to: 169-169, 280-306
198-198: LGTM! Prefetch optimizations align well with PR objectives.The changes correctly implement:
- Generation-based cache invalidation (avoiding unnecessary cache clears)
- Cancellation of stale prefetch tasks on resize/zoom to prevent wasted work
- Directional prefetch hints (forward/backward) for asymmetric prefetching
- Smart resize debounce cancellation when navigation occurs
The comment at lines 224-227 clearly explains the cache strategy rationale.
Also applies to: 206-206, 222-228, 244-262, 398-398, 404-404
234-237: LGTM! Dialog handling is implemented correctly.The dialog state tracking properly:
- Disables global keybindings when dialogs are open
- Provides jump-to-image functionality with bounds validation and user-friendly messaging
- Uses defensive checks before invoking main_window methods
- Includes appropriate debug logging
Also applies to: 407-437
308-365: LGTM! Enhanced image decoding with proper thread safety and error handling.The refactored
get_decoded_imagecorrectly:
- Guards against empty lists and invalid indices
- Protects
last_displayed_imageaccess with_last_image_lockthroughout- Submits priority decode on cache miss to free up workers
- Provides graceful fallback to last image on timeout/cancellation/errors
- Includes clear documentation about blocking behavior
Note: The unused variable warning at line 359 is a static analysis false positive—
log.exceptionautomatically includes the exception info.
443-446: LGTM! Defensive guard checks prevent index errors.The bounds checks consistently guard against empty image lists and invalid indices across multiple methods, preventing potential crashes while maintaining expected behavior.
Also applies to: 472-473, 482-483, 518-519, 1174-1175
516-534: LGTM! Helicon integration correctly implements RAW/JPG fallback.The changes properly implement the PR's feature for JPG-only workflows:
toggle_selectionprefers RAW but falls back to JPGlaunch_heliconhandles both selected files and stacks with per-image RAW/JPG selection_launch_helicon_with_filesde-duplicates files and tracks success/failure- Temporary file cleanup is properly scheduled with error handling
This aligns with the PR objective: "Helicon Focus stacking now works with JPG-only workflows when RAW files are absent."
Also applies to: 537-602, 604-611
650-653: LGTM! Cache management enhancements are correct.The changes properly:
- Expose cache usage in GB for UI display (
get_cache_usage_gb)- Clear ICC transform caches when color mode changes to prevent stale transforms
- Cancel prefetch tasks after image list modifications (delete/undo) to avoid invalid indices
All align with the PR's performance and cache management objectives.
Also applies to: 700-701, 858-858, 911-911
928-946: LGTM! Enhanced recycle bin handling with improved UX.The changes provide:
- User-friendly shutdown dialog with three clear options (Delete, Restore, Keep)
- Robust
restore_all_from_recycle_binimplementation that handles per-file errors gracefully- Proper exception logging at both file and batch levels
Note: The static analysis hint at line 994 is overly pedantic—using
log.errorwith explicit exception formatting for per-file errors (where processing continues) is acceptable, while the outer exception at line 1002 correctly useslog.exception.Also applies to: 968-1002
864-864: LGTM! Improved exception logging for better diagnostics.The changes from
log.errortolog.exceptioncorrectly provide automatic traceback inclusion, improving debuggability for:
- Image deletion failures
- Undo restore failures
- Photoshop launch errors
Also applies to: 917-917, 1075-1075, 1078-1078
1162-1162: LGTM! Log level adjustment reduces verbosity appropriately.Changing
_get_stack_infologging to debug level is appropriate since this is called frequently during UI updates and the information is primarily useful for detailed diagnostics.faststack/faststack/qml/FilterDialog.qml (2)
46-52: Filter string propagation and Enter key handlingUpdating
filterDialog.filterStringinonTextChangedand wiring bothKeys.onReturnPressedandKeys.onEnterPressedtofilterDialog.accept()gives predictable, immediate behavior for typing and committing the filter. This looks correct and matches the PR’s goal of keystroke‑driven filtering.
63-77: Verification complete—controller methods are implementedThe concern about unguarded calls is addressed: both
dialog_opened()anddialog_closed()are implemented inapp.py(lines 428, 434), and this unguarded pattern is already established inSettingsDialog.qmlandJumpToImageDialog.qml. With a single, consistent controller serving all dialogs, no runtime errors will occur. Guarding the calls is not required.
1 participant
Performance Improvements
Features
Summary by CodeRabbit
New Features
Performance Improvements
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.