Version 0.8#13
Conversation
WalkthroughBumps project to 0.8.0; adds delete-with-undo (recycle bin) and keyboard shortcuts; introduces three color modes (none / saturation / icc) with saturation control and ICC handling in prefetch; prefers RAW files for Photoshop edits; adds debug/timing logging and CLI entrypoint. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as QML UI
participant Ctrl as AppController
participant FS as FileSystem
participant Cache as Prefetcher/Cache
User->>UI: Press Delete / Backspace
UI->>Ctrl: delete_current_image()
Ctrl->>FS: move current file -> recycle_bin_dir
Ctrl->>Ctrl: append to delete_history
Ctrl->>Cache: invalidate/refresh prefetch
Ctrl->>UI: update display & status
User->>UI: Press Ctrl+Z
UI->>Ctrl: undo_delete()
Ctrl->>FS: move file back from recycle_bin_dir
Ctrl->>Ctrl: update delete_history
Ctrl->>Cache: invalidate/refresh prefetch
Ctrl->>UI: update display & status
sequenceDiagram
participant User
participant UI as QML UI
participant Ctrl as AppController
participant Pref as Prefetcher
participant Config as Config
User->>UI: Select color mode
UI->>Ctrl: set_color_mode(mode)
Ctrl->>Config: persist color.mode
Ctrl->>Pref: reset prefetch / clear caches
Pref->>Pref: choose decode path based on mode
alt saturation
Pref->>Pref: apply_saturation_compensation(factor)
else icc
Pref->>Pref: load monitor profile & ImageCms conversion
end
Pref->>UI: supply updated image buffers
sequenceDiagram
participant User
participant UI as QML UI
participant Ctrl as AppController
participant FS as FileSystem
participant PS as Photoshop
User->>UI: Edit in Photoshop (E)
UI->>Ctrl: edit_in_photoshop()
Ctrl->>FS: check for RAW alongside image
alt RAW found
Ctrl->>Ctrl: choose RAW file
else
Ctrl->>Ctrl: fall back to JPEG
end
Ctrl->>Ctrl: validate Photoshop executable, build args safely
Ctrl->>PS: launch process (no shell)
Ctrl->>UI: show status or error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 0
🧹 Nitpick comments (8)
faststack/faststack/imaging/prefetch.py (4)
19-35: Cache monitor ICC profile instead of reloading it for every decode
get_monitor_profilere-reads the config and constructs a newImageCmsProfileon every ICC decode, and logs a warning on every call whenmonitor_icc_pathis unset. Caching the profile by path would reduce overhead and log spam, while still allowing the path to change at runtime.You can also narrow the bare
except Exceptionhere to the concrete error types you expect (e.g.,OSError,ImageCms.PyCMSError) so unexpected failures are not silently downgraded.-SRGB_PROFILE = ImageCms.createProfile("sRGB") - -def get_monitor_profile(): - """Dynamically load monitor ICC profile based on current config.""" - try: - monitor_icc_path = config.get('color', 'monitor_icc_path', fallback="").strip() - if monitor_icc_path: - profile = ImageCms.ImageCmsProfile(monitor_icc_path) - log.debug(f"Loaded monitor ICC profile: {monitor_icc_path}") - return profile - else: - log.warning("ICC mode enabled but no monitor_icc_path configured") - return None - except Exception as e: - log.warning(f"Failed to load monitor ICC profile: {e}") - return None +SRGB_PROFILE = ImageCms.createProfile("sRGB") +_MONITOR_PROFILE_CACHE: dict[str, Optional[ImageCms.ImageCmsProfile]] = {} + +def get_monitor_profile() -> Optional[ImageCms.ImageCmsProfile]: + """Dynamically load monitor ICC profile based on current config.""" + monitor_icc_path = config.get('color', 'monitor_icc_path', fallback="").strip() + if not monitor_icc_path: + log.warning("ICC mode enabled but no monitor_icc_path configured") + return None + + cached = _MONITOR_PROFILE_CACHE.get(monitor_icc_path) + if cached is not None: + return cached + + try: + profile = ImageCms.ImageCmsProfile(monitor_icc_path) + except OSError as e: + log.warning(f"Failed to load monitor ICC profile '{monitor_icc_path}': {e}") + profile = None + + _MONITOR_PROFILE_CACHE[monitor_icc_path] = profile + if profile is not None: + log.debug(f"Loaded monitor ICC profile: {monitor_icc_path}") + return profile
38-72: Add a cheap shape assertion to guard saturation compensation assumptionsThe saturation math looks good and is nicely localized, but it assumes
arr.size == height * bytes_per_line. If the upstream decode pipeline ever changes (e.g., padded rows), this will silently reshape incorrectly.A lightweight assertion here would make mismatches fail fast and easier to debug.
- # Treat the buffer as [height, bytes_per_line] - buf2d = arr.reshape((height, bytes_per_line)) + # Treat the buffer as [height, bytes_per_line] + assert arr.size == height * bytes_per_line, ( + f"Unexpected buffer size for saturation compensation: " + f"{arr.size} != {height} * {bytes_per_line}" + ) + buf2d = arr.reshape((height, bytes_per_line))
157-163: Consider narrowing bareExceptioncatches to expected error typesIn both the embedded ICC profile parsing (Lines 157–163) and the saturation compensation block (Lines 211–216), the code catches
Exceptionand continues. That’s safe for robustness, but it can also hide programming errors such asMemoryErrororKeyboardInterrupt.If possible, tighten these to the concrete exceptions you expect (e.g.,
OSError,ImageCms.PyCMSError,ValueError) and let truly unexpected failures surface.Also applies to: 211-216
168-175: Minor: drop unnecessary f-string in debug log
log.debug(f"Converting image from source to monitor profile")doesn’t interpolate any variables, so thefprefix is unnecessary and flagged by Ruff (F541).- log.debug(f"Converting image from source to monitor profile") + log.debug("Converting image from source to monitor profile")faststack/faststack/app.py (4)
523-579: Normalize color-mode casing and align saturation-factor semanticsTwo minor consistency points in the color management slots:
set_color_mode(Lines 529–537) expects exact lowercase'none' | 'saturation' | 'icc', but callers might pass'ICC'or'Saturation', andPrefetcheritself lowercasesconfig.get('color', 'mode', ...). Normalizing the input before validation will make this more robust.
apply_saturation_compensation’s docstring allowsfactor > 1.0for extra saturation, butset_saturation_factorclamps to[0.0, 1.0], so the UI can only reduce or keep saturation, not increase it. That matches the PR description (“allows reducing saturation”), but it’s slightly at odds with the helper’s docstring.@Slot(str) def set_color_mode(self, mode: str): """Sets color management mode and clears cache to force re-decode.""" - if mode not in ['none', 'saturation', 'icc']: + mode = mode.lower() + if mode not in ['none', 'saturation', 'icc']: log.error(f"Invalid color mode: {mode}") returnIf you intend saturation-only reduction in the UI, consider updating the saturation helper’s docstring to say
0.0–1.0rather than suggesting>1.0is supported.
641-697: Tighten delete/undo bookkeeping and exception loggingThe recycle-bin–based delete/undo flow is a nice UX improvement. A couple of edge cases are worth tightening:
delete_history.append((jpg_path, raw_path))is recorded even if neither file actually existed/moved (e.g., a race where the file was deleted externally).undo_deletewill then no-op while reporting “Restored: ”. Tracking only files that were actually moved, or storing that information alongside the paths, would keep history truthful.- On partial failures (e.g., JPG move succeeds but RAW move fails), the exception path does not add an entry to
delete_history, so the successfully moved file can’t be undone. Depending on how likely that is in your environment, you may want to record at least what was moved so the user can still undo that part.Also, for the
except OSError as e:blocks in bothdelete_current_imageandundo_delete, usinglog.exception(...)instead oflog.error(...)will capture a stack trace, which is helpful if filesystem errors show up in the wild.- except OSError as e: - self.update_status_message(f"Delete failed: {e}") - log.error(f"Failed to delete image: {e}") + except OSError as e: + self.update_status_message(f"Delete failed: {e}") + log.exception(f"Failed to delete image: {e}")- except OSError as e: - self.update_status_message(f"Undo failed: {e}") - log.error(f"Failed to restore image: {e}") - # Put it back in history if it failed - self.delete_history.append((jpg_path, raw_path)) + except OSError as e: + self.update_status_message(f"Undo failed: {e}") + log.exception(f"Failed to restore image: {e}") + # Put it back in history if it failed + self.delete_history.append((jpg_path, raw_path))Also applies to: 698-749
750-767: Keepdelete_historyin sync when emptying the recycle bin and log errors with tracebacksThe shutdown prompt +
empty_recycle_binbehavior is good, but currently:
empty_recycle_binremoves the directory withshutil.rmtreewithout clearingself.delete_history. After a successful empty,undo_deletecan still pop entries that no longer map to real files, leading to confusing “Restored: ” messages.- The
except OSErrorinempty_recycle_binuseslog.error, which drops the traceback (Ruff TRY400).Clearing the history on successful empty and switching to
log.exceptionwill keep internal state consistent and improve diagnosability.def empty_recycle_bin(self): """Permanently deletes all files in the recycle bin.""" if not self.recycle_bin_dir.exists(): return try: import shutil - shutil.rmtree(self.recycle_bin_dir) - log.info("Emptied recycle bin") - except OSError as e: - log.error(f"Failed to empty recycle bin: {e}") + shutil.rmtree(self.recycle_bin_dir) + self.delete_history.clear() + log.info("Emptied recycle bin and cleared delete history") + except OSError as e: + log.exception(f"Failed to empty recycle bin: {e}")Also applies to: 778-789
796-806: Simplify Photoshop error handling; avoid unreachableFileNotFoundErrorblockThe enhanced Photoshop integration looks solid: prefer RAW when available, validate the executable, build a safe argv list, and disable
shell=True. One small cleanup:
FileNotFoundErroris a subclass ofOSError, so the secondexcept FileNotFoundErrorblock (Lines 862–864) is currently unreachable because the earlierexcept (OSError, subprocess.SubprocessError)already catches it.You can either merge the messages into a single block or put the
FileNotFoundErrorhandler first if you want a special message.- except (OSError, subprocess.SubprocessError) as e: - self.update_status_message(f"Failed to open in Photoshop: {e}") - log.exception(f"Error launching Photoshop: {e}") - except FileNotFoundError as e: - self.update_status_message(f"Photoshop executable not found: {e}") - log.exception(f"Photoshop executable not found: {e}") + except FileNotFoundError as e: + self.update_status_message(f"Photoshop executable not found: {e}") + log.exception(f"Photoshop executable not found: {e}") + except (OSError, subprocess.SubprocessError) as e: + self.update_status_message(f"Failed to open in Photoshop: {e}") + log.exception(f"Error launching Photoshop: {e}")Also applies to: 832-842, 846-856, 859-864
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
faststack/ChangeLog.md(1 hunks)faststack/README.md(3 hunks)faststack/faststack.egg-info/PKG-INFO(2 hunks)faststack/faststack/app.py(5 hunks)faststack/faststack/config.py(1 hunks)faststack/faststack/imaging/prefetch.py(2 hunks)faststack/faststack/qml/Main.qml(3 hunks)faststack/faststack/ui/keystrokes.py(1 hunks)faststack/faststack/ui/provider.py(4 hunks)faststack/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
faststack/faststack/imaging/prefetch.py (3)
faststack/faststack/models.py (1)
DecodedImage(33-42)faststack/faststack/imaging/jpeg.py (2)
decode_jpeg_rgb(22-40)decode_jpeg_resized(99-149)faststack/faststack/config.py (1)
get(68-69)
faststack/faststack/ui/provider.py (2)
faststack/faststack/config.py (1)
get(68-69)faststack/faststack/app.py (2)
get_color_mode(524-526)get_saturation_factor(558-560)
faststack/faststack/app.py (3)
faststack/faststack/config.py (3)
get(68-69)save(58-66)getfloat(74-75)faststack/faststack/io/sidecar.py (1)
save(54-83)faststack/faststack/imaging/prefetch.py (3)
cancel_all(244-250)update_prefetch(95-115)shutdown(252-256)
🪛 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.8 - November 20, 2025 # By Alan Rockefeller Ultra-fast, cach...
(MISSING_COMMA_AFTER_YEAR)
🪛 Ruff (0.14.5)
faststack/faststack/imaging/prefetch.py
33-33: Do not catch blind exception: Exception
(BLE001)
161-161: Do not catch blind exception: Exception
(BLE001)
169-169: f-string without any placeholders
Remove extraneous f prefix
(F541)
215-215: Do not catch blind exception: Exception
(BLE001)
233-233: Consider moving this statement to an else block
(TRY300)
faststack/faststack/app.py
657-657: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
696-696: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
746-746: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
788-788: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (14)
faststack/pyproject.toml (1)
8-8: LGTM!The version bump to 0.8 is consistent with the PR objectives and aligns with changes documented in the ChangeLog.
faststack/ChangeLog.md (1)
3-8: LGTM!The changelog entries accurately document the new features introduced in version 0.8, including delete/undo functionality, enhanced Photoshop integration, and color management modes.
faststack/faststack.egg-info/PKG-INFO (1)
3-3: LGTM!The package metadata version updates are consistent with the version bump across all other files.
Also applies to: 24-24
faststack/faststack/ui/keystrokes.py (2)
42-43: LGTM!The addition of Delete and Backspace key bindings for image deletion follows the existing key mapping pattern and aligns with the PR objectives.
49-49: LGTM!The Ctrl+Z binding for undo functionality follows the established modifier key mapping pattern and provides standard undo behavior.
faststack/README.md (1)
3-3: LGTM!The README updates comprehensively document the new features in version 0.8, including the enhanced Photoshop integration with RAW file preference, delete/undo functionality, and additional keyboard shortcuts.
Also applies to: 20-20, 25-25, 50-54
faststack/faststack/config.py (1)
26-30: LGTM!The new color configuration section is well-structured with clear defaults and inline documentation. The saturation factor default of 0.85 provides a reasonable starting point for color compensation.
faststack/faststack/qml/Main.qml (3)
118-150: LGTM!The saturation slider implementation is well-integrated with conditional visibility based on color mode. The use of
onMovedrather thanonValueChangedis appropriate here as it only triggers when the user actively adjusts the slider, preventing feedback loops.
220-247: LGTM!The color mode menu implementation correctly uses
ActionGroupwithexclusive: trueto ensure only one color mode is active at a time. The checkable actions with bidirectional state binding (checked vs. uiState.colorMode) provide proper visual feedback.
323-357: LGTM!The Key Bindings dialog updates appropriately document the new file management features (Delete and Ctrl+Z) and the increased size (500x600) provides better readability for the expanded content.
faststack/faststack/ui/provider.py (2)
85-86: LGTM!The new signals and properties for color mode and saturation factor correctly expose the color management configuration to the QML UI layer, following the established pattern for other UI state properties.
Also applies to: 202-210
47-58: I found a potential API discrepancy. Let me verify the correct syntax for PySide6 6.10.QColorSpace API usage is correct.
PySide6 includes QColorSpace.NamedColorSpace for predefined color spaces, and the constructor accepts namedColorSpace parameters of type NamedColorSpace. The code correctly uses
QColorSpace(namedColorSpace)withQColorSpace.NamedColorSpace.SRgb. The ICC mode logic is sound: skipping sRGB application when pixels are already in monitor space. No changes needed.faststack/faststack/imaging/prefetch.py (1)
196-217: Color-mode decode pipeline and generation re-check look consistentThe branching on
color_mode(iccvs standard + optionalsaturation) and the generation re-check before caching integrate cleanly with the existing prefetcher design:
- ICC mode: resize before profile conversion, honor embedded profiles when present, and fall back to sRGB with clear logging.
- Non-ICC: reuse the existing
decode_jpeg_resizedpath, with an optional in-place saturation pass whencolor_mode == "saturation".- The second
if self.generation != local_generationguard beforecache_putcorrectly drops stale decodes.I don’t see functional issues here; this is a solid extension of the existing pipeline.
Also applies to: 218-233
faststack/faststack/app.py (1)
114-116: No exclusion logic needed—find_imagesdoes not recursively scan subdirectories.The original concern assumed
find_imagesperforms recursive directory traversal. However, the implementation usesos.scandir(directory)with only a top-level file check (if entry.is_file()), meaning it scans only immediate children of the provided directory. Subdirectories like"image recycle bin"are never entered, so files within it will not appear in the image list. The recycle bin implementation is safe as currently written.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
faststack/faststack/app.py (4)
246-291: Consider exception handling improvements and document blocking behavior.The blocking decode approach with fallback to
last_displayed_imageis a good UX improvement. However:
- Exception handling: Lines 282, 288, and 289 should use
logging.exception()instead oflogging.error()to include traceback information for debugging.- Broad exception catch: Line 287 catches bare
Exception- consider being more specific or add a comment justifying the broad catch.- Timeout value: The 5-second timeout may be insufficient for large RAW files in color management modes. Consider making this configurable or increasing it.
Apply this diff to improve exception logging:
except concurrent.futures.TimeoutError: - log.error(f"Timeout decoding image at index {index}") + log.exception(f"Timeout decoding image at index {index}") return self.last_displayed_image except concurrent.futures.CancelledError: log.warning(f"Decode cancelled for index {index}") return self.last_displayed_image except Exception as e: - log.error(f"Error decoding image at index {index}: {e}") + log.exception(f"Error decoding image at index {index}: {e}") return self.last_displayed_image
663-719: Improve exception logging in delete operation.The delete implementation is solid with proper directory creation, file moving, and state updates. However:
- Exception logging: Lines 679 and 718 should use
logging.exception()instead oflogging.error()to capture full traceback.- Partial failure handling: If JPG moves successfully but RAW fails, the operation is half-complete. Consider wrapping both moves in a transaction-like structure or documenting this behavior.
- Race condition: Between checking
jpg_path.exists()(line 685) and callingrename()(line 687), the file could be deleted externally. Consider catchingFileNotFoundErrorseparately.Apply this diff to improve exception logging:
except OSError as e: self.update_status_message(f"Delete failed: {e}") - log.error(f"Failed to delete image: {e}") + log.exception(f"Failed to delete image: {e}")
720-771: Improve exception logging and consider partial restore handling.The undo implementation correctly restores files and re-adds to history on failure. However:
- Exception logging: Line 768 should use
logging.exception()to capture full traceback.- Partial restore: If JPG restores successfully but RAW fails, the state is inconsistent. Consider handling this more explicitly.
Apply this diff:
except OSError as e: self.update_status_message(f"Undo failed: {e}") - log.error(f"Failed to restore image: {e}") + log.exception(f"Failed to restore image: {e}") # Put it back in history if it failed self.delete_history.append((jpg_path, raw_path))
800-811: Improve exception logging in empty_recycle_bin.The implementation correctly uses
shutil.rmtreefor recursive deletion. Line 810 should uselogging.exception()instead oflogging.error()to capture full traceback for debugging.Apply this diff:
except OSError as e: - log.error(f"Failed to empty recycle bin: {e}") + log.exception(f"Failed to empty recycle bin: {e}")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
faststack/faststack/app.py(13 hunks)faststack/faststack/io/indexer.py(3 hunks)faststack/faststack/io/sidecar.py(2 hunks)faststack/faststack/logging_setup.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
faststack/faststack/io/sidecar.py (1)
faststack/faststack/models.py (2)
Sidecar(25-30)EntryMetadata(15-21)
faststack/faststack/app.py (5)
faststack/faststack/models.py (1)
DecodedImage(33-42)faststack/faststack/imaging/prefetch.py (4)
submit_task(117-128)cancel_all(244-250)update_prefetch(95-115)shutdown(252-256)faststack/faststack/config.py (3)
get(68-69)save(58-66)getfloat(74-75)faststack/faststack/io/sidecar.py (1)
save(63-92)faststack/faststack/logging_setup.py (1)
setup_logging(15-46)
🪛 Ruff (0.14.5)
faststack/faststack/app.py
282-282: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
287-287: Do not catch blind exception: Exception
(BLE001)
288-288: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
679-679: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
718-718: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
768-768: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
810-810: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
989-989: Do not perform function call typer.Argument in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable
(B008)
🔇 Additional comments (9)
faststack/faststack/logging_setup.py (1)
15-45: LGTM! Clean debug mode implementation.The conditional logging configuration properly reduces noise in production while enabling detailed debugging when needed. The docstring clearly documents the new parameter.
faststack/faststack/io/sidecar.py (1)
33-42: LGTM! Consistent timing instrumentation.The timing measurement is appropriately scoped to the
json.load()operation. Same initialization order concern as noted inindexer.pyregarding_debug_mode.faststack/faststack/app.py (6)
64-66: Global debug flag is acceptable for this use case.The global
_debug_modeflag is set early inmain()and provides a simple mechanism for debug instrumentation across modules. Ensure other modules import this only after initialization (already addressed in other file reviews).
102-102: LGTM! Clean state management additions.The new state attributes support the delete/undo feature and image caching improvements. The
last_displayed_imagecache prevents grey squares on decode failures.Also applies to: 119-121
545-601: LGTM! Robust color management implementation.The color mode and saturation factor slots include proper validation, cache invalidation, and conditional refreshing. The optimization to only refresh images when in saturation mode (line 593) is well-considered.
818-828: LGTM! Clean RAW-first preference logic.The updated logic correctly prefers RAW files for Photoshop editing, falling back to JPG if RAW is unavailable. The logging clearly indicates which file type is being used.
854-864: Excellent security-conscious argument parsing.The use of
shlex.split()withposix=(os.name != 'nt')properly handles quoted arguments and platform differences. This is much safer than manual string splitting and pairs well withshell=Falseon line 873.
988-1060: Clean debug mode integration in main().The timing instrumentation provides useful performance insights during startup. The debug flag is properly initialized early (line 994) before any module imports that depend on
_debug_mode.Note: Static analysis flagged line 989 for performing
typer.Argument()call in function defaults. This is a minor style issue with typer but doesn't affect functionality.faststack/faststack/io/indexer.py (1)
55-59: The initialization concern is not valid -_debug_modehas a safe default.The variable
_debug_modeis initialized at module level infaststack/app.py:65with a default value ofFalse. This eliminates any risk ofAttributeErrorwhen the function-level import occurs, regardless of whenfind_images()is called. The pattern is defensive and correct: even if executed beforemain()sets_debug_mode = debug, the function will safely use theFalsedefault. No changes are required.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (9)
faststack/faststack/io/indexer.py (2)
40-40: Consider usinglogging.exceptionfor better debugging.While
logging.errorworks,logging.exceptionwould include the stack trace, which can be valuable for diagnosing directory scanning issues.Based on static analysis hints.
Apply this diff:
- log.error("Error scanning directory %s: %s", directory, e) + log.exception("Error scanning directory %s", directory)
55-61: Simplify the conditional logging logic.The code logs two similar messages when DEBUG is enabled (lines 60 and 61), which creates redundancy. Consider consolidating these into a single message that includes timing only when DEBUG is enabled.
Apply this diff:
elapsed = time.perf_counter() - t_start paired_count = sum(1 for im in image_files if im.raw_pair) - # Log timing info if DEBUG level is enabled - if log.isEnabledFor(logging.DEBUG): - log.info("find_images: found %d images in %.3fs", len(image_files), elapsed) - log.info("Found %d JPG files and paired %d with RAWs.", len(image_files), paired_count) + if log.isEnabledFor(logging.DEBUG): + log.info("Found %d JPG files and paired %d with RAWs in %.3fs", len(image_files), paired_count, elapsed) + else: + log.info("Found %d JPG files and paired %d with RAWs.", len(image_files), paired_count)faststack/faststack/imaging/prefetch.py (2)
19-57: LGTM: ICC profile management with cachingThe implementation properly caches monitor ICC profiles to minimize I/O overhead and log spam. Error handling is comprehensive.
Consider this minor refactor suggested by static analysis to improve code structure:
try: profile = ImageCms.ImageCmsProfile(monitor_icc_path) log.debug("Loaded monitor ICC profile: %s", monitor_icc_path) _monitor_profile_cache[monitor_icc_path] = profile - return profile except (OSError, ImageCms.PyCMSError) as e: log.warning("Failed to load monitor ICC profile from %s: %s", monitor_icc_path, e) _monitor_profile_cache[monitor_icc_path] = None - return None + + return _monitor_profile_cache[monitor_icc_path]
290-292: Consider usinglogging.exceptionfor better error diagnostics.While catching broad exceptions is appropriate for robustness in the decode path, using
logging.exceptionwould include the stack trace, which is valuable for diagnosing decode failures.Based on static analysis hints.
Apply this diff:
except Exception as e: - log.error("Error decoding image %s at index %d: %s", image_file.path, index, e) + log.exception("Error decoding image %s at index %d", image_file.path, index)faststack/faststack/app.py (5)
276-321: LGTM: Blocking decode prevents gray squaresThe refactored implementation correctly blocks on decode when a cache miss occurs, ensuring the correct image is displayed. The fallback to
last_displayed_imageon errors prevents gray squares during transient failures. The 5-second timeout is reasonable for JPEG decoding.Consider using
logging.exceptionfor lines 312 and 318 to include stack traces in error logs, which aids debugging:except concurrent.futures.TimeoutError: - log.error("Timeout decoding image at index %d", index) + log.exception("Timeout decoding image at index %d", index) return self.last_displayed_image except concurrent.futures.CancelledError: log.warning("Decode cancelled for index %d", index) return self.last_displayed_image except Exception as e: - log.error("Error decoding image at index %d: %s", index, e) + log.exception("Error decoding image at index %d", index) return self.last_displayed_imageBased on static analysis hints.
700-760: LGTM: Delete implementation with recycle binThe delete operation safely moves files to a recycle bin and properly updates application state. The implementation correctly handles missing files and provides clear status feedback.
Remove the redundant exception parameter from
logging.exception(line 759) as suggested by static analysis:except OSError as e: self.update_status_message(f"Delete failed: {e}") - log.exception(f"Failed to delete image: {e}") + log.exception("Failed to delete image")
762-815: LGTM: Undo implementation with history restorationThe undo operation correctly restores files and navigates to the restored image. The re-addition to history on failure (line 814) prevents data loss if the restoration fails partway through.
Remove the redundant exception parameter from
logging.exception(line 812) as suggested by static analysis:except OSError as e: self.update_status_message(f"Undo failed: {e}") - log.exception(f"Failed to restore image: {e}") + log.exception("Failed to restore image") # Put it back in history if it failed self.delete_history.append((jpg_path, raw_path))
816-856: LGTM: Shutdown with recycle bin promptThe shutdown sequence appropriately prompts users to empty the recycle bin, defaulting to "No" for safety. The implementation uses
shutil.rmtreefor efficient directory removal.Remove the redundant exception parameter from
logging.exception(line 855):except OSError as e: - log.exception(f"Failed to empty recycle bin: {e}") + log.exception("Failed to empty recycle bin")
857-932: LGTM: Enhanced Photoshop integration preferring RAW filesThe implementation correctly prefers RAW files when available (as per PR objectives) and demonstrates excellent security practices:
- Path validation before execution
- Safe argument parsing with
shlex- No shell execution (
shell=False)- Proper file descriptor management (
close_fds=True)Remove redundant exception parameters from
logging.exceptioncalls (lines 928, 931):except FileNotFoundError as e: self.update_status_message(f"Photoshop executable not found: {e}") - log.exception(f"Photoshop executable not found: {e}") + log.exception("Photoshop executable not found") except (OSError, subprocess.SubprocessError) as e: self.update_status_message(f"Failed to open in Photoshop: {e}") - log.exception(f"Error launching Photoshop: {e}") + log.exception("Error launching Photoshop")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
faststack/faststack/app.py(34 hunks)faststack/faststack/imaging/prefetch.py(4 hunks)faststack/faststack/io/indexer.py(5 hunks)faststack/faststack/qml/Main.qml(4 hunks)faststack/faststack/ui/provider.py(5 hunks)faststack/pyproject.toml(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/faststack/qml/Main.qml
🧰 Additional context used
🧬 Code graph analysis (3)
faststack/faststack/imaging/prefetch.py (3)
faststack/faststack/models.py (2)
ImageFile(8-12)DecodedImage(33-42)faststack/faststack/imaging/jpeg.py (2)
decode_jpeg_rgb(22-40)decode_jpeg_resized(99-149)faststack/faststack/config.py (1)
get(68-69)
faststack/faststack/ui/provider.py (2)
faststack/faststack/config.py (1)
get(68-69)faststack/faststack/app.py (3)
get_current_metadata(366-393)get_color_mode(582-584)get_saturation_factor(617-619)
faststack/faststack/app.py (7)
faststack/faststack/models.py (1)
DecodedImage(33-42)faststack/faststack/imaging/prefetch.py (4)
cancel_all(307-314)update_prefetch(129-170)submit_task(172-183)shutdown(316-320)faststack/faststack/ui/provider.py (10)
currentIndex(142-143)imageCount(146-147)currentFilename(154-157)isFlagged(160-163)isRejected(166-169)stackInfoText(184-187)preloadProgress(131-132)preloadProgress(135-138)isPreloading(121-122)isPreloading(125-128)faststack/faststack/io/watcher.py (1)
start(49-62)faststack/faststack/config.py (3)
save(58-66)get(68-69)getfloat(74-75)faststack/faststack/io/sidecar.py (1)
save(63-92)faststack/faststack/logging_setup.py (1)
setup_logging(15-46)
🪛 Ruff (0.14.5)
faststack/faststack/imaging/prefetch.py
52-52: Consider moving this statement to an else block
(TRY300)
288-288: Consider moving this statement to an else block
(TRY300)
290-290: Do not catch blind exception: Exception
(BLE001)
291-291: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/app.py
312-312: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
317-317: Do not catch blind exception: Exception
(BLE001)
318-318: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
513-513: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
716-716: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
759-759: Redundant exception object included in logging.exception call
(TRY401)
812-812: Redundant exception object included in logging.exception call
(TRY401)
855-855: Redundant exception object included in logging.exception call
(TRY401)
907-907: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
928-928: Redundant exception object included in logging.exception call
(TRY401)
931-931: Redundant exception object included in logging.exception call
(TRY401)
faststack/faststack/io/indexer.py
40-40: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (23)
faststack/pyproject.toml (2)
8-8: LGTM: Version bump to 0.8The version increment aligns with the PR objectives and changes introduced.
31-31: LGTM: CLI entrypoint updatedThe entrypoint change from
maintoclialigns with the new argument parsing structure introduced in app.py.faststack/faststack/io/indexer.py (1)
22-23: LGTM: Timing instrumentation addedThe timing setup follows a standard pattern for performance monitoring.
faststack/faststack/ui/provider.py (4)
47-58: LGTM: ICC mode color space handlingThe logic correctly skips Qt's sRGB color space assignment when ICC mode is active, since pixels are already in monitor space. The expanded exception handling for
ValueErrorimproves robustness.
85-86: LGTM: New signals for color managementThe new signals follow standard Qt patterns and support the color management UI bindings.
155-187: LGTM: Null safety guards addedThe defensive checks prevent crashes when
image_filesis empty, returning safe defaults. This improves robustness across all metadata properties.
214-222: LGTM: Color management properties exposed to QMLThe new properties correctly expose color mode and saturation factor settings to the UI layer, following standard Qt property patterns.
faststack/faststack/imaging/prefetch.py (6)
59-100: LGTM: Saturation compensation algorithmThe implementation efficiently applies saturation adjustment in-place using vectorized NumPy operations. The algorithm correctly interpolates between grayscale and original RGB values, with proper bounds checking and handling of buffer padding.
117-122: LGTM: Adaptive prefetch radius attributesThe attributes support a smart optimization: starting with a small prefetch radius during startup to reduce cache thrashing, then expanding once the user begins navigation.
129-170: LGTM: Enhanced prefetch with adaptive radius and deduplicationThe implementation intelligently manages prefetch radius expansion and prevents duplicate task submission. The deduplication logic correctly tracks scheduled indices per generation.
274-288: LGTM: Generation-aware cachingThe re-check of generation before caching correctly prevents stale results from being cached after the prefetch has moved on.
295-314: LGTM: Updated helper methodsThe optional radius parameter in
_is_in_prefetch_rangesupports the adaptive radius feature, and clearing_scheduledincancel_allensures proper cleanup.
194-272: Verify fallback value discrepancy for saturation_factorConfig defines saturation_factor default as "0.85" (config.py line 28), but prefetch.py line 268 uses fallback="1.0". Since 1.0 means no saturation compensation and 0.85 means 15% reduction, this discrepancy means if config.get fails, the code silently uses no compensation instead of the configured intent. Confirm this is intentional (safe fallback to no-op) or should match the configured default.
The exception handling for saturation compensation (lines 270-271) is appropriate: catching ValueError (invalid config value) and AssertionError (buffer dimension mismatch) are specific to the actual failure modes, and logging a warning while continuing is correct since the image remains usable without compensation applied.
faststack/faststack/app.py (10)
64-65: LGTM: Global debug flagThe module-level debug flag provides a simple mechanism for conditional debug instrumentation across the application.
87-88: LGTM: Display readiness trackingThese attributes support the deferred prefetch optimization, preventing cache invalidation before the display size stabilizes.
120-124: LGTM: Delete/undo state managementThe recycle bin and delete history structures properly support the delete-with-undo feature introduced in this version.
189-210: LGTM: Display resize handling with deferred prefetchThe implementation correctly defers prefetch until the display size stabilizes on first resize, then processes any pending prefetch request. This optimization reduces cache thrashing during startup.
231-242: LGTM: Centralized prefetch deferral logicThe
_do_prefetchhelper properly encapsulates the logic for deferring prefetch until display is ready, improving code consistency across navigation and filter operations.
351-361: LGTM: Navigation tracking for adaptive prefetchPassing
is_navigation=Trueto_do_prefetchenables the adaptive radius expansion feature in the prefetcher.
581-638: LGTM: Color mode and saturation factor controlsThe implementations properly validate inputs, persist configuration, and intelligently manage cache invalidation. The optimization to only refresh when in saturation mode (line 630) prevents unnecessary work when adjusting the slider in other modes.
413-486: LGTM: Parameterized logging improvementsThe conversion from f-strings to parameterized logging (using
%formatting) follows Python logging best practices and improves performance by deferring string interpolation until the message is actually logged.
1033-1104: LGTM: Enhanced main() with debug mode and timingThe addition of the
debugparameter and startup timing instrumentation provides valuable performance insights. The handling ofimage_diras a parameter supports the new CLI entrypoint while maintaining backward compatibility with default directory configuration.
1106-1115: LGTM: New CLI entrypoint with argparseThe new
cli()function provides a clean command-line interface usingargparse, aligning with the pyproject.toml entrypoint change and eliminating the typer dependency.
1 participant
images are over-saturated - or turn down the saturation in saturation mode. Test it out by loading an image in Faststack and Photoshop or another image viewer an
d make sure the colors look the same.
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.