Test#20
Conversation
WalkthroughAdds an in-app cropping workflow (QML UI, keyboard handlers, execute/cancel), backup-aware crop/save with undo, an RGB histogram window, UI-state normalization and histogram integration, imaging backup/save refactor, directory open teardown/reinit improvements, various QML/UI tweaks and removals, and a version bump to 1.3.0. Changes
Sequence Diagram(s)sequenceDiagram
participant QML as Components.qml (UI)
participant App as AppController
participant Editor as imaging.editor
Note over QML,App: Enter crop mode (O) and draw crop rect
QML->>App: toggle_crop_mode()
QML->>App: updateCropBox(normalized_rect)
Note over QML,App: User confirms crop (Enter)
QML->>App: execute_crop()
App->>App: validate & convert normalized -> pixel rect
App->>Editor: request crop & save
Editor->>Editor: create_backup_file(original_path)
Editor-->>App: save result (success/failure)
App->>App: record undo entry (backup_path), invalidate caches
App->>QML: refresh UI/state, reset zoom/pan, update histogram
sequenceDiagram
participant UI as Main UI
participant App as AppController
participant Watcher as DirWatcher
participant Loader as ImageLoader
UI->>App: open_folder(new_dir)
App->>Watcher: stop()
App->>App: clear histories/stacks/batches/caches/prefetcher/editor
App->>App: reset last_displayed_image (under lock)
App->>Watcher: create/start for new_dir
App->>Loader: load(new_dir)
Loader-->>App: images list
App->>UI: rebind UI with new images
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: 0
🧹 Nitpick comments (2)
faststack/faststack/app.py (2)
178-205: Consider also clearinglast_displayed_imageto prevent stale references.The refactoring comprehensively handles directory switching by stopping the watcher, reinitializing components, and clearing caches. However,
self.last_displayed_image(defined at line 111) may still reference an image from the old directory.Consider adding this line after clearing the caches (around line 1202):
self._metadata_cache = {} self._metadata_cache_index = (-1, -1) + +# Clear last displayed image since it references old directory +with self._last_image_lock: + self.last_displayed_image = None
1782-1784: Use underscore pattern for intentionally unusedbackup_path.The
backup_pathvariable is unpacked but never used in this function. This differs fromquick_auto_white_balance()(line 1856) which tracks the backup for undo. The lack of undo tracking here appears intentional for manual saves.Apply this diff to indicate the variable is intentionally unused:
- save_result = self.image_editor.save_image() - if save_result: - saved_path, backup_path = save_result + save_result = self.image_editor.save_image() + if save_result: + saved_path, _ = save_resultBased on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
faststack/faststack/app.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
faststack/faststack/app.py (4)
faststack/faststack/io/watcher.py (2)
stop(64-70)Watcher(41-74)faststack/faststack/io/sidecar.py (1)
SidecarManager(13-98)faststack/faststack/imaging/prefetch.py (1)
cancel_all(474-481)faststack/faststack/imaging/editor.py (1)
save_image(245-302)
🪛 Ruff (0.14.5)
faststack/faststack/app.py
1784-1784: Unpacked variable backup_path is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🔇 Additional comments (1)
faststack/faststack/app.py (1)
619-619: LGTM: Docstring accurately reflects behavior.The updated docstring correctly documents that both batches and stacks are cleared, aligning with the implementation in
clear_all_stacks()(lines 955-969).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/faststack/app.py (1)
181-208: Consider additional state clearing for directory switch.The directory switching logic is comprehensive but missing a few items:
Missing
last_displayed_imageclearing: Based on lines 133-135's comment,last_displayed_imageshould be cleared here with the lock since it references the old directory's images:with self._last_image_lock: self.last_displayed_image = NoneMissing editor state clearing: If the image editor is open when switching directories, its state (
original_image,current_filepath,_preview_image) should be cleared to prevent it from holding references to the old directory's images.Consider incrementing
display_generation: Since we're switching to a completely new directory, incrementingdisplay_generationwould invalidate any stale display info and ensure clean cache keys.Apply this diff:
# Clear caches since they reference old directory's images + with self._last_image_lock: + self.last_displayed_image = None self.image_cache.clear() self.prefetcher.cancel_all() + self.display_generation += 1 self._metadata_cache = {} self._metadata_cache_index = (-1, -1) + + # Clear editor state if open + self.image_editor.original_image = None + self.image_editor.current_filepath = None + self.image_editor._preview_image = None # Load images from new directory self.load()
🧹 Nitpick comments (1)
faststack/faststack/app.py (1)
1785-1818: Add error feedback for save failures.The code correctly handles the two-tuple return from
save_image(), but whensave_resultisNone(save failed), no error message is shown to the user. They'll see no feedback if the save fails, which could be confusing.Apply this diff:
def save_edited_image(self): """Saves the edited image.""" save_result = self.image_editor.save_image() if save_result: saved_path, _ = save_result # Clear the image editor state so it will reload fresh next time ... QMessageBox.information( None, "Save Successful", f"Image saved to: {saved_path}. Original backed up.", QMessageBox.Ok ) + else: + self.update_status_message("Failed to save image") + log.error("Failed to save edited image")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
faststack/faststack/app.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
faststack/faststack/app.py (4)
faststack/faststack/io/watcher.py (2)
stop(64-70)Watcher(41-74)faststack/faststack/io/sidecar.py (1)
SidecarManager(13-98)faststack/faststack/imaging/prefetch.py (1)
cancel_all(474-481)faststack/faststack/imaging/editor.py (1)
save_image(245-302)
🔇 Additional comments (1)
faststack/faststack/app.py (1)
622-623: LGTM: Docstring accurately reflects behavior.The updated docstring correctly indicates that this method clears both batches and stacks, which matches the implementation that delegates to
clear_all_stacks().
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
faststack/README.md (1)
3-16: Document theOcrop hotkey in Keyboard Shortcuts (plus small style tweaks)The crop feature bullet mentions the cr(O)p hotkey, but the
## Keyboard Shortcutslist doesn’t documentO, which can confuse users scanning that section.Also, static analysis suggested a couple of minor style fixes:
- Optional comma after the year in the version line.
- Hyphenate “high-performance decoding”.
You could address all of this with something like:
-# Version 1.3 - November 23, 2025 +# Version 1.3 - November 23, 2025 @@ -- **Crop:** Added the ability to crop images, via the cr(O)p hotkey. It can be a freeform crop, or constrained to several popular aspect ratios. +- **Crop:** Added the ability to crop images via the cr(O)p hotkey. It can be a freeform crop, or constrained to several popular aspect ratios. @@ -- **Instant Navigation:** Sub-10ms next/previous image switching, high performance decoding via `PyTurboJPEG`. +- **Instant Navigation:** Sub-10ms next/previous image switching, high‑performance decoding via `PyTurboJPEG`. @@ - `P`: Edit in Photoshop (uses RAW file if available) +- `O`: Toggle crop mode (cr(O)p hotkey; Enter to crop, Esc to cancel) - `Delete` / `Backspace`: Move image to recycle bin(Feel free to keep the exact wording you prefer; the key point is documenting
Oalongside the other shortcuts.)Also applies to: 44-65
faststack/faststack/app.py (1)
1840-1876: Surface failures fromsave_image()to the user insave_edited_image
save_edited_imagenow correctly unpacks(saved_path, backup_path)fromImageEditor.save_image(), but ifsave_image()returnsNone(e.g., backup creation failed or saving failed), the method silently does nothing—no status message or dialog. By contrast,quick_auto_white_balanceshows an explicit “Failed to save image” message on failure.To avoid confusing UX when the user clicks “Save” and nothing appears to happen, consider explicitly handling the failure case, e.g.:
@Slot() def save_edited_image(self): """Saves the edited image.""" - save_result = self.image_editor.save_image() - if save_result: - saved_path, _ = save_result + save_result = self.image_editor.save_image() + if not save_result: + QMessageBox.warning( + None, + "Save Failed", + "Failed to save edited image. Please check the log for details.", + QMessageBox.Ok, + ) + return + + saved_path, _ = save_result # Clear the image editor state so it will reload fresh next time self.image_editor.original_image = None ...This keeps the happy path unchanged while making failures explicit to the user.
♻️ Duplicate comments (1)
faststack/faststack/app.py (1)
131-136: Fixlast_displayed_imageclearing location/comment (belongs on directory switch, not in__init__)The comment at Line 133 says:
Clear last displayed image since it references old directory
…but this code runs in
__init__, where there is no old directory yet, andlast_displayed_imagehas just been initialized toNone. The real “old directory” case is inopen_folder, where you reassignimage_dirand reload.This was already called out in a previous review and is still misleading, and it also means
last_displayed_imageis not cleared when switching folders – so any error path inget_decoded_imagethat falls back tolast_displayed_imagecan still show an image from the previous directory.A minimal fix is to:
- Simplify the constructor (no need to clear here), and
- Explicitly clear
last_displayed_imageduringopen_folder, after tearing down directory‑bound state.For example:
@@ - self._metadata_cache = {} - self._metadata_cache_index = (-1, -1) - # Clear last displayed image since it references old directory - with self._last_image_lock: - self.last_displayed_image = None + self._metadata_cache = {} + self._metadata_cache_index = (-1, -1) @@ def open_folder(self): @@ if path: @@ # Clear caches since they reference old directory's images self.image_cache.clear() self.prefetcher.cancel_all() self._metadata_cache = {} self._metadata_cache_index = (-1, -1) + # Clear last displayed image since it references the old directory + with self._last_image_lock: + self.last_displayed_image = None @@ # Load images from new directory self.load()This aligns the comment with the behavior and avoids stale fallback images after directory changes.
Also applies to: 1186-1218
🧹 Nitpick comments (8)
faststack/faststack/ui/provider.py (1)
587-615: Consider logging/validating when normalizingcurrentCropBoxfailsThe new normalization logic makes the setter resilient to QJSValue and list inputs, which is great. Right now, though, any
TypeError/IndexError/AttributeErrorduring conversion is silently swallowed, and an arbitrarily shaped value can end up in_current_crop_box.Two small, non‑blocking improvements:
- Log failures so odd values coming from QML are traceable.
- Optionally validate that the final value looks like a 4‑tuple before accepting it.
For example:
- @currentCropBox.setter - def currentCropBox(self, new_value): - # Convert QJSValue or list to tuple if needed - try: - if hasattr(new_value, 'toVariant'): + @currentCropBox.setter + def currentCropBox(self, new_value): + # Convert QJSValue or list to tuple if needed + original_value = new_value + try: + if hasattr(new_value, 'toVariant'): @@ - except (TypeError, IndexError, AttributeError): - # If conversion fails, try to use as-is or log error - pass + except (TypeError, IndexError, AttributeError) as e: + log.warning( + "UIState.currentCropBox: failed to normalize value %r (type %s): %s", + original_value, + type(original_value), + e, + ) + + # Optional: only accept 4‑element tuples + if not isinstance(new_value, tuple) or len(new_value) != 4: + log.warning("UIState.currentCropBox: ignoring invalid crop box %r", new_value) + returnThis would make weird crop values easier to diagnose without changing the happy path.
faststack/faststack/imaging/editor.py (1)
23-55: Backup helper behavior is good; narrow the exception type increate_backup_fileThe new
create_backup_filenicely centralizes the*-backup,*-backup2, … naming and is consistent with the RAW pairing regex inedit_in_photoshop. The only real nit is the blindexcept Exception, which Ruff flagged and which is broader than necessary here.You can keep behavior identical while satisfying the linter by catching
OSError(coversFileNotFoundError,PermissionError, etc.):- try: - # Perform the backup - shutil.copy2(original_path, backup_path) - return backup_path - except Exception as e: - print(f"Failed to create backup: {e}") - return None + try: + # Perform the backup + shutil.copy2(original_path, backup_path) + return backup_path + except OSError as e: + print(f"Failed to create backup: {e}") + return NoneNo other behavior changes are required.
faststack/faststack/app.py (3)
1186-1218: Directory switch teardown and re‑init are thorough and correctThe new
open_folderflow:
- Stops the old
Watcher.- Updates
image_dir.- Re‑creates
WatcherandSidecarManagerfor the new directory.- Resets per‑directory state (
delete_history,undo_history, stacks/batches, markers).- Clears caches and metadata indices.
- Calls
load()to rescan and start the new watcher.This matches the
WatcherandSidecarManagerpatterns (new observer per directory, old one fully stopped) and avoids leaking old directory state into the new one. Once you movelast_displayed_imageclearing here (as suggested above), the directory switch story will be very clean. Based on relevant code snippets forWatcherandSidecarManager.
1394-1470: Crop undo logic is correct but duplicates the auto‑white‑balance undo pathThe new
"crop"branch inundo_deletemirrors the"auto_white_balance"branch (restore backup → refresh image list → locate image → reset cache/prefetch → sync UI → update status), which is functionally correct and gives crops proper undo support.To keep this maintainable, consider extracting the common restore‑from‑backup flow into a small helper, e.g.:
def _restore_backup_action(self, action_label: str, saved_path: str | Path, backup_path: str | Path, timestamp: float, action_type: str): filepath_obj = Path(saved_path) try: backup_path_obj = Path(backup_path) if backup_path_obj.exists(): filepath_obj.unlink() backup_path_obj.rename(filepath_obj) # refresh image list, find index, reset cache/prefetch, sync UI... self.update_status_message(f"Undid {action_label}") else: self.update_status_message("Backup not found") log.warning("Backup %s disappeared before it could be restored.", backup_path) self.undo_history.append((action_type, (saved_path, backup_path), timestamp)) except OSError as e: self.update_status_message(f"Undo failed: {e}") log.exception("Failed to undo %s", action_label) self.undo_history.append((action_type, (saved_path, backup_path), timestamp))…and then call it from both branches with
action_label="auto white balance"/"crop"andaction_type="auto_white_balance"/"crop"as appropriate.Not urgent, but it would reduce copy‑paste and keep future fixes in one place.
1893-2043: Crop mode implementation is robust; only minor logging/exception‑handling nitsThe new crop workflow (
toggle_crop_mode,cancel_crop_mode,execute_crop) hangs together well:
- Crop mode state is tracked in
ui_state.isCroppingwith clear status messages.- Default crop box is initialized to
(0, 0, 1000, 1000)and aspect ratios are wired fromASPECT_RATIOS.execute_cropvalidates the crop box, converts normalized coords to pixels with sensible clamping, preserves format/EXIF where possible, creates a backup viacreate_backup_file, records undo history, refreshes image lists/caches, and resets zoom/pan and crop state.A couple of minor, optional refinements:
In the crop‑box parsing block (Lines 1929‑1947), you already log an error on failure; switching to
log.exceptionwould include the traceback for easier debugging:except (TypeError, IndexError, AttributeError) as e:self.update_status_message("Invalid crop box")log.error("Failed to parse crop box (type: %s): %s", type(crop_box_raw), e)
except (TypeError, IndexError, AttributeError):self.update_status_message("Invalid crop box")log.exception("Failed to parse crop box (type: %s)", type(crop_box_raw))
- The outer
except Exception as eat the end ofexecute_cropensures the UI doesn’t crash on unexpected errors, which is reasonable for a user‑facing tool. If you ever want to tighten this up for linting, you could narrow it toOSError/Pillow errors while still keeping the status message and log.Overall, the crop flow is a solid addition.
faststack/faststack/qml/Components.qml (3)
239-260: Complex aspect ratio constraint logic with potential edge cases.The aspect ratio application during drag operations involves multiple steps: (1) adjust crop box based on drag mode, (2) recalculate dimensions to match aspect ratio, (3) clamp to bounds. When the crop box is near image edges, the clamping may prevent the exact aspect ratio from being maintained, which could be confusing to users.
Consider one of these approaches:
- Lock the aspect ratio more strictly by constraining drag operations to only allow movements that maintain the ratio
- Add visual feedback when aspect ratio cannot be perfectly maintained due to boundary constraints
- Simplify by applying aspect ratio only on drag completion rather than continuously during drag
341-382: Duplicate aspect ratio constraint logic.This aspect ratio constraint implementation is very similar to lines 239-260. The duplication increases maintenance burden and risk of inconsistency.
Extract the aspect ratio constraint logic into a shared function:
+ function applyAspectRatioConstraint(left, top, right, bottom, ratioName) { + var ratio = getAspectRatio(ratioName) + if (!ratio) return {left: left, top: top, right: right, bottom: bottom} + + var currentWidth = right - left + var currentHeight = bottom - top + var targetAspect = ratio[0] / ratio[1] + var currentAspect = currentWidth / currentHeight + + if (currentAspect > targetAspect) { + // Adjust height + var newHeight = currentWidth / targetAspect + var centerY = (top + bottom) / 2 + top = centerY - newHeight / 2 + bottom = centerY + newHeight / 2 + if (top < 0) { bottom += -top; top = 0 } + if (bottom > 1000) { top -= (bottom - 1000); bottom = 1000 } + } else { + // Adjust width + var newWidth = currentHeight * targetAspect + var centerX = (left + right) / 2 + left = centerX - newWidth / 2 + right = centerX + newWidth / 2 + if (left < 0) { right += -left; left = 0 } + if (right > 1000) { left -= (right - 1000); right = 1000 } + } + + return {left: left, top: top, right: right, bottom: bottom} + }Then call this function from both locations.
387-394: Hardcoded aspect ratios should come from backend.The aspect ratio mapping is hardcoded in QML but the aspect ratio names come from
uiState.aspectRatioNames. If the backend changes or adds aspect ratios, this mapping must be manually updated.Consider exposing the aspect ratio values (not just names) from the backend through
uiState, or compute ratios by parsing the names (e.g., "1:1" → [1, 1]).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
faststack/ChangeLog.md(1 hunks)faststack/README.md(2 hunks)faststack/faststack/app.py(9 hunks)faststack/faststack/imaging/editor.py(3 hunks)faststack/faststack/qml/Components.qml(4 hunks)faststack/faststack/qml/Main.qml(1 hunks)faststack/faststack/ui/keystrokes.py(1 hunks)faststack/faststack/ui/provider.py(1 hunks)faststack/pyproject.toml(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- faststack/ChangeLog.md
🧰 Additional context used
🧬 Code graph analysis (1)
faststack/faststack/app.py (4)
faststack/faststack/imaging/editor.py (2)
create_backup_file(23-54)save_image(279-324)faststack/faststack/ui/provider.py (2)
isCropping(491-492)isCropping(495-498)faststack/faststack/io/watcher.py (2)
stop(64-70)Watcher(41-74)faststack/faststack/io/sidecar.py (1)
save(62-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 1.3 - November 23, 2025 # By Alan Rockefeller Ultra-fast, cach...
(MISSING_COMMA_AFTER_YEAR)
[grammar] ~16-~16: Use a hyphen to join words.
Context: ...10ms next/previous image switching, high performance decoding via PyTurboJPEG. ...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.5)
faststack/faststack/app.py
1947-1947: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
2005-2005: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/imaging/editor.py
51-51: Consider moving this statement to an else block
(TRY300)
52-52: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (10)
faststack/faststack/ui/keystrokes.py (1)
44-51: Crop toggle keystroke wiring looks goodMapping
Qt.Key_Ototoggle_crop_modecleanly hooks the new crop workflow into the existing keybinding infrastructure and AppController API.faststack/pyproject.toml (1)
6-9: Version bump to 1.3 is consistentProject version aligns with the README and new crop feature set; no further changes needed here.
faststack/faststack/app.py (1)
248-263: Crop‑mode key handling ineventFilteris correctly integratedIntercepting Enter/Return and Esc when
ui_state.isCroppingis true cleanly routes:
- Enter/Return →
execute_crop()- Esc →
cancel_crop_mode()before the keybinder sees the event, so you don’t accidentally launch Helicon while in crop mode. Outside crop mode, existing keybindings still work unchanged. This wiring looks solid.
faststack/faststack/qml/Components.qml (5)
89-117: LGTM! Crop state management looks well-structured.The properties for tracking crop drag state are appropriate for the interactive crop workflow.
165-180: LGTM! Coordinate conversion logic is correct.The function properly accounts for image letterboxing when converting from normalized [0,1000] space to display coordinates.
396-405: LGTM! Aspect ratio re-application logic is correct.This function properly re-applies the selected aspect ratio constraint when the user changes their selection.
409-496: LGTM! Crop overlay rendering is well-implemented.The four-rectangle approach for dimming the area outside the crop box is a standard and effective technique. The z-index of 100 should keep it above the main image.
531-576: LGTM! Aspect ratio selector UI is well-implemented.The repeater-based selector properly highlights the active aspect ratio and triggers constraint re-application on selection change.
faststack/faststack/qml/Main.qml (2)
810-810: LGTM! Documentation accurately reflects crop undo support.The help text correctly indicates that Ctrl+Z can now undo crop operations in addition to delete and auto white balance.
815-815: LGTM! Crop mode key binding documented clearly.The help text accurately describes the crop mode workflow: O to toggle, Enter to execute, ESC to cancel.
modified: faststack/README.md modified: faststack/faststack/app.py modified: faststack/faststack/imaging/editor.py modified: faststack/faststack/qml/Components.qml modified: faststack/faststack/ui/provider.py new file: faststack/z.patch
modified: faststack/faststack/app.py new file: faststack/faststack/qml/HistogramWindow.qml modified: faststack/faststack/qml/Main.qml modified: faststack/faststack/ui/keystrokes.py modified: faststack/faststack/ui/provider.py
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (10)
faststack/README.md (1)
3-3: Docs look consistent with new features; only minor style nits if you careThe new version info, crop feature description, expanded feature bullets, and
Ocrop shortcut all read clearly and match the new app behavior (crop, histogram, settings, undo types). If you want to be pedantic about wording:
- Line 16: consider “high‑performance decoding” with a hyphen.
- Header line 3: some style guides would write “Version 1.3 – November 23, 2025” with commas around the year, but this is optional.
Functionally this is all good to go.
Also applies to: 12-28, 63-63
faststack/faststack/imaging/editor.py (1)
4-4: Backup helper is sound; consider centralizing regex and using loggingThe new
create_backup_filehelper correctly:
- Normalizes any existing
-backup,-backup2,-backup-1, etc. suffix to a base stem.- Allocates
base-stem-backup.jpg, then-backup2,-backup3, … avoiding collisions.- Returns
Noneif the original doesn’t exist or the copy fails, whichsave_imagenow propagates by aborting the save.
save_image’s switch tobackup_path = create_backup_file(original_path)and early‑exit onNonecleanly reuses this logic and keeps the EXIF/format handling unchanged.Two small improvements you might want:
- Replace the
print(f"Failed to create backup: {e}")increate_backup_filewith module‑level logging for consistency.- The
-backup(-?\d+)?$pattern is also used inAppController.edit_in_photoshop; consider moving this regex to a shared constant or small helper to avoid the two drifting apart in future edits.Overall, the backup behavior is correct and integrates cleanly with the new undo flows.
Also applies to: 23-55, 293-297
faststack/faststack/ui/provider.py (2)
98-99: Histogram UI state wiring is coherent; minor duplication with controller logicThe added pieces:
- Signals:
is_histogram_visible_changed,histogram_data_changed.- State:
_is_histogram_visible,_histogram_data.- Properties:
isHistogramVisible(emits and triggersapp_controller.update_histogram()when set to True) andhistogramData(dict withr/g/blists, emittinghistogram_data_changed).These give QML everything it needs (
uiState.isHistogramVisible,uiState.histogramData, and the implicitonHistogramDataChangedhandler used inHistogramWindow.qml).One thing to be aware of:
AppController.toggle_histogramalso callsupdate_histogram()when turning the histogram on, so you currently callupdate_histogram()twice on open (once from the property setter, once from the slot). That’s harmless but redundant; you could rely on just the setter’s side effect and drop the extra call intoggle_histogramif you’d like to trim work slightly.Also applies to: 131-133, 504-527
621-649:currentCropBoxnormalization is robust; just watch duplication and a tiny encoding nitThe new
currentCropBoxsetter does a good job of:
- Accepting QJSValue, list, tuple, or other iterables from QML/Python.
- Normalizing to a 4‑element tuple, with detailed log warnings if normalization fails.
- Ignoring invalid values rather than corrupting
_current_crop_box.Two minor points:
- Since
UIState.currentCropBoxnow guarantees a 4‑tuple,AppController.execute_cropno longer needs its own QJSValue/sequence normalization logic; it could just expect a tuple and simplify its error paths. That would de‑duplicate logic and reduce the chance of the two code paths diverging.- The comment
# only accept 4‑element tuplesuses a non‑breaking hyphen; replacing it with a normal ASCII-avoids subtle encoding/linters issues.Behavior‑wise this setter looks solid.
faststack/faststack/app.py (5)
133-134: Avoid double resettinglast_displayed_imageYou now clear
last_displayed_image:
- Once in
__init__, under_last_image_lock.- Twice in
open_folder: inside the “Clear caches…” block and again a couple of lines later.Functionally it’s harmless, but the second clear in
open_folderis redundant:- # Clear caches since they reference old directory's images - with self._last_image_lock: - self.last_displayed_image = None + # Clear caches since they reference old directory's images + with self._last_image_lock: + self.last_displayed_image = None self.image_cache.clear() ... - # Clear last displayed image since it references the old directory - with self._last_image_lock: - self.last_displayed_image = NoneRemoving the second block makes the intent clearer without changing behavior.
Also applies to: 1219-1229
253-261: Crop workflow and undo integration look correct, with one opportunity to de‑duplicate parsingThe new crop flow is nicely integrated:
eventFilterhandles Enter/Return and ESC only whenuiState.isCroppingis true, routing toexecute_crop()/cancel_crop_mode()and preventing the keybinder from double‑handling those keys.toggle_crop_modeandcancel_crop_modecorrectly manageuiState.isCropping, resetcurrentCropBoxto full‑frame, and populateaspectRatioNames/currentAspectRatioIndexfromASPECT_RATIOS.execute_crop:
- Validates that a non‑default crop box exists.
- Converts normalized [0‑1000] coords into pixel space and clamps to at least 1×1 pixels inside the image.
- Uses
create_backup_file(original_path)before overwriting, and bails cleanly if backup creation fails.- Saves with original format/EXIF when possible, falling back sensibly.
- Appends a
"crop"entry toundo_historywith bothsaved_pathandbackup_path.- Refreshes the image list, re‑locates the current image, invalidates caches/generation, triggers prefetch, syncs UI, resets zoom/pan, and refreshes the histogram when visible.
- The new
"crop"branch inundo_deletemirrors the auto‑white‑balance undo logic and correctly renames the backup back over the edited file and refreshes all the UI/cache state.One place you could simplify:
- Because
UIState.currentCropBoxalready normalizes and enforces a 4‑tuple, the extra QJSValue/sequence parsing inexecute_cropis likely redundant; you can readcrop_box = list(self.ui_state.currentCropBox)and rely on the UIState setter for validation, logging only iflen(crop_box) != 4. That centralizes type‑handling in one place.Otherwise the crop pipeline and undo behavior look solid.
Also applies to: 1975-2129, 1451-1487
423-454: Histogram integration is correct; consider trimming duplicate updates and improving loggingThe histogram feature appears well‑wired end‑to‑end:
- Navigation (
next_image,prev_image,jump_to_image) callsupdate_histogram()only whenuiState.isHistogramVisibleis true.set_edit_parameterandquick_auto_white_balancealso refresh the histogram when visible, so the histogram reflects edited previews.toggle_histogramflipsuiState.isHistogramVisibleand logs open/close events, andupdate_histogram:
- Fetches the current
DecodedImage(or editor preview when editing).- Uses NumPy to build per‑channel 256‑bin histograms.
- Publishes a
{'r': [...], 'g': [...], 'b': [...]}dict viaui_state.histogramData, whichHistogramWindow.qmlconsumes.Two small refinements to consider:
- As noted in the UIState review,
UIState.isHistogramVisible’s setter already callsupdate_histogram()when enabling the window, so the extraif self.ui_state.isHistogramVisible: self.update_histogram()intoggle_histogramcan be removed without losing behavior.- In the general exception handler of
update_histogram, you already uselog.exception("Failed to compute histogram: %s", e); you can drop the%s, eargument and just loglog.exception("Failed to compute histogram")to avoid double‑printing the exception object.Functionally this is correct and gated well enough that the extra work only happens when users have the histogram open.
Also applies to: 1837-1841, 1918-1973, 2181-2184
1199-1234: Directory switch teardown/reinit flow looks comprehensiveThe
open_folderrework does a thorough job when the user picks a new directory:
- Stops the existing
Watcherif present.- Updates
self.image_dirand constructs a freshWatcherandSidecarManagerbound to the new directory.- Updates
recycle_bin_dirto the new location.- Clears directory‑specific state: delete/undo history, stacks/batches, and their start indices.
- Clears caches and bumps
display_generationso stale decoded images are invalidated, and resets metadata cache.- Clears editor state (
original_image,current_filepath,_preview_image) so the next edit reloads from the new directory.- Finally calls
load(), which rescans, starts the new watcher, restores stacks from the new sidecar, and syncs UI.Aside from the small duplicate
last_displayed_imagereset noted earlier, this looks like a clean, all‑in‑one place to enforce “new directory = fresh state”.
1864-1901: Backup‑aware save and undo for AWB are consistent with the new helperThe changes around saving and undoing edits line up with
ImageEditor.save_image:
save_edited_imagenow:
- Treats
save_result = self.image_editor.save_image()as(saved_path, backup_path)and only proceeds on success.- Clears the editor’s in‑memory state, resets UI edit parameters, refreshes the image list, re‑locates the edited image, and fully invalidates caches/generation before syncing UI.
- Surfaces failure via a status message + log if
save_imagereturnsNone.
quick_auto_white_balance:
- Calls
save_image()and, on success, records("auto_white_balance", (saved_path, backup_path), timestamp)toundo_history.- Clears the editor state, refreshes the list, re‑indexes, and invalidates caches/display similarly, plus refreshes the histogram if visible.
The
"auto_white_balance"branch inundo_deletenow uses the storedbackup_pathinstead of globbing, and correctly:
- Unlinks the edited file, renames the backup back to the original filename, refreshes the list, re‑locates the image, invalidates caches/generation, and syncs the UI.
- Restores the undo entry on failure.
This is a much cleaner, deterministic pairing between backups and undo operations. If in the future you decide to add full undo for generic “Save Edited Image” (not just AWB and crop), you already have all the information you need (the backup path from
save_image).Also applies to: 2151-2179, 1417-1450
faststack/faststack/qml/Components.qml (1)
387-394: Consider using a map for aspect ratio lookup.The hardcoded if-chain works but could be more maintainable as a mapping object, especially if more ratios are added later.
Example:
+ property var aspectRatioMap: ({ + "1:1 (Square)": [1, 1], + "4:5 (Portrait)": [4, 5], + "1.91:1 (Landscape)": [191, 100], + "9:16 (Story)": [9, 16] + }) + function getAspectRatio(name) { - // Map aspect ratio names to ratios - if (name === "1:1 (Square)") return [1, 1] - if (name === "4:5 (Portrait)") return [4, 5] - if (name === "1.91:1 (Landscape)") return [191, 100] - if (name === "9:16 (Story)") return [9, 16] - return null + return aspectRatioMap[name] || null }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
faststack/README.md(3 hunks)faststack/faststack/app.py(14 hunks)faststack/faststack/imaging/editor.py(3 hunks)faststack/faststack/qml/Components.qml(4 hunks)faststack/faststack/qml/HistogramWindow.qml(1 hunks)faststack/faststack/qml/Main.qml(4 hunks)faststack/faststack/ui/keystrokes.py(2 hunks)faststack/faststack/ui/provider.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/faststack/ui/keystrokes.py
🧰 Additional context used
🧬 Code graph analysis (2)
faststack/faststack/app.py (3)
faststack/faststack/imaging/editor.py (2)
create_backup_file(23-54)save_image(279-324)faststack/faststack/ui/provider.py (15)
isCropping(495-496)isCropping(499-502)isHistogramVisible(505-506)isHistogramVisible(509-515)isEditorOpen(471-472)isEditorOpen(475-478)histogramData(518-520)histogramData(523-526)currentCropBox(616-618)currentCropBox(621-652)aspectRatioNames(596-597)aspectRatioNames(600-603)currentAspectRatioIndex(606-607)currentAspectRatioIndex(610-613)resetZoomPan(464-466)faststack/faststack/io/watcher.py (2)
stop(64-70)Watcher(41-74)
faststack/faststack/ui/provider.py (1)
faststack/faststack/app.py (1)
update_histogram(1929-1973)
🪛 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 1.3 - November 23, 2025 # By Alan Rockefeller Ultra-fast, cach...
(MISSING_COMMA_AFTER_YEAR)
[grammar] ~16-~16: Use a hyphen to join words.
Context: ...10ms next/previous image switching, high performance decoding via PyTurboJPEG. ...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.5)
faststack/faststack/imaging/editor.py
51-51: Consider moving this statement to an else block
(TRY300)
faststack/faststack/app.py
1969-1969: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1972-1972: Redundant exception object included in logging.exception call
(TRY401)
2029-2029: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
2087-2087: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/ui/provider.py
646-646: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
🔇 Additional comments (9)
faststack/faststack/qml/Main.qml (3)
630-643: Editor toggle semantics are now clearly documented and match behaviorThe comment and logic for the
Ekey correctly describe the behavior: togglinguiState.isEditorOpenand closing without saving if it’s already open, with explicit saves via Ctrl+S. This matches the help text and AppController’ssave_edited_imageflow, so users won’t be surprised by implicit saves on close.
837-846: Key‑bindings help text matches implemented shortcutsThe updated “Key Bindings” text:
- Extends Ctrl+Z to undo delete, auto white balance, and crop.
- Documents Ctrl+Shift+B for quick AWB, O for crop mode (Enter/ESC semantics), H for histogram, and E for editor toggle.
These align with the new AppController slots (
quick_auto_white_balance, crop undo,toggle_histogram,toggle_crop_mode) and key handling, so the help dialog is accurate.
895-898: Histogram window wiring is straightforward; relies on HistogramWindow.qml internalsInstantiating
HistogramWindow { id: histogramWindow }here and letting it bindvisibletouiState.isHistogramVisiblekeeps the main file clean: AppController togglesuiState.isHistogramVisible, and the window manages its own visibility and updates viaonVisibleChangedanduiStateconnections. No additional wiring is needed inMain.qml.faststack/faststack/qml/Components.qml (6)
89-116: LGTM! Crop state properties are well-structured.The cursor shape logic appropriately switches to a cross cursor during crop mode, and the new properties effectively track crop dragging state and initial crop box coordinates.
118-163: LGTM! DPI-aware edge detection implemented.The crop interaction logic correctly handles both new crop creation and existing crop box manipulation. The DPI-aware edge threshold (line 128) addresses the previous review concern about high-DPI displays.
165-180: LGTM! Coordinate conversion correctly handles image layout.The function properly accounts for
PreserveAspectFitcentering and converts normalized crop coordinates to screen space.
296-302: LGTM! Proper cleanup of crop drag state.
396-405: LGTM! Correctly reapplies aspect ratio to existing crop box.
409-496: LGTM! Crop overlay rendering is well-implemented.The overlay correctly renders dimmed areas around the crop region and responds to both crop box changes and image dimension changes. The four-rectangle approach for dimming is standard and efficient.
| // Apply aspect ratio if needed | ||
| if (uiState.currentAspectRatioIndex > 0 && uiState.aspectRatioNames && uiState.aspectRatioNames.length > uiState.currentAspectRatioIndex) { | ||
| var ratioName = uiState.aspectRatioNames[uiState.currentAspectRatioIndex] | ||
| var ratio = getAspectRatio(ratioName) | ||
| if (ratio) { | ||
| var targetAspect = ratio[0] / ratio[1] | ||
| var currentWidth = right - left | ||
| var currentHeight = bottom - top | ||
| var currentAspect = currentWidth / currentHeight | ||
|
|
||
| if (currentAspect > targetAspect) { | ||
| var newHeight = currentWidth / targetAspect | ||
| var centerY = (top + bottom) / 2 | ||
| top = Math.max(0, centerY - newHeight / 2) | ||
| bottom = Math.min(1000, top + newHeight) | ||
| } else { | ||
| var newWidth = currentHeight * targetAspect | ||
| var centerX = (left + right) / 2 | ||
| left = Math.max(0, centerX - newWidth / 2) | ||
| right = Math.min(1000, left + newWidth) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Aspect ratio enforcement may feel unnatural during edge dragging.
When dragging an edge with an aspect ratio constraint active, the adjustment centers the change (lines 250-252, 255-257), moving both the dragged edge and the opposite edge. Users typically expect the opposite edge to remain fixed while only the dragged edge moves.
Consider anchoring the stationary edge based on cropDragMode to provide more intuitive feedback.
🤖 Prompt for AI Agents
In faststack/faststack/qml/Components.qml around lines 238 to 260, the
aspect-ratio adjustment recenters the crop (changing both dragged and opposite
edges) which feels wrong during edge dragging; instead anchor the opposite edge
according to cropDragMode so only the dragged edge moves. Update the branch that
computes newHeight/newWidth to check cropDragMode (e.g., draggingTop,
draggingBottom, draggingLeft, draggingRight) and, when fixing height, compute
top or bottom from the fixed edge (do not recenter): if top is fixed set bottom
= Math.min(1000, top + newHeight) or if bottom is fixed set top = Math.max(0,
bottom - newHeight); similarly for width: if left is fixed set right =
Math.min(1000, left + newWidth) or if right is fixed set left = Math.max(0,
right - newWidth). Keep the same clamp bounds and fallback to the existing
centering logic only when no drag edge is detected.
| Component.onCompleted: { | ||
| // Update colors based on theme | ||
| color = isDark ? "#333333" : "#f0f0f0" | ||
| border.color = isDark ? "#666666" : "#cccccc" | ||
| } |
There was a problem hiding this comment.
Theme colors won't update if theme changes dynamically.
The Component.onCompleted handler sets colors once based on isDark, but if the theme changes at runtime, the colors remain stale. The isDark property binding doesn't trigger updates to color and border.color properties.
Bind colors directly to the theme state:
- color: "#333333"
- border.color: "#666666"
+ color: isDark ? "#333333" : "#f0f0f0"
+ border.color: isDark ? "#666666" : "#cccccc"
border.width: 1
radius: 4
z: 1000
-
- // Try to get root from parent hierarchy
- property bool isDark: root.isDarkTheme
-
- Component.onCompleted: {
- // Update colors based on theme
- color = isDark ? "#333333" : "#f0f0f0"
- border.color = isDark ? "#666666" : "#cccccc"
- }
+
+ property bool isDark: (typeof root !== 'undefined' && root.hasOwnProperty('isDarkTheme')) ? root.isDarkTheme : trueCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In faststack/faststack/qml/Components.qml around lines 516 to 520, the
Component.onCompleted block assigns color and border.color once using isDark,
which prevents updates when the theme changes; remove those imperative
assignments and instead use property bindings so the values react to isDark
(e.g., set color: isDark ? "#333333" : "#f0f0f0" and border.color: isDark ?
"#666666" : "#cccccc"), ensuring colors update automatically at runtime.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
faststack/faststack/qml/Components.qml (1)
491-558: Make aspect-ratio window fully theme-aware (background and text colors)The aspect-ratio window currently:
- Sets
colorandborder.coloronce inComponent.onCompletedbased onisDark, so runtime theme changes won’t update it.- Uses hard-coded
"white"for the title and option text, which can have poor contrast in light theme.You can fix both by binding colors directly to
isDarkand using that for text as well, e.g.:- width: 200 - height: Math.max(150, aspectRatioColumn.implicitHeight + 20) - color: "#333333" - border.color: "#666666" + width: 200 + height: Math.max(150, aspectRatioColumn.implicitHeight + 20) + color: isDark ? "#333333" : "#f0f0f0" + border.color: isDark ? "#666666" : "#cccccc" @@ - Component.onCompleted: { - // Update colors based on theme - color = isDark ? "#333333" : "#f0f0f0" - border.color = isDark ? "#666666" : "#cccccc" - } + // Colors now bound directly to isDark; no Component.onCompleted needed @@ - Text { - text: "Aspect Ratio" - font.bold: true - color: "white" - font.pixelSize: 12 - } + Text { + text: "Aspect Ratio" + font.bold: true + color: aspectRatioWindow.isDark ? "white" : "black" + font.pixelSize: 12 + } @@ - color: "white" + color: aspectRatioWindow.isDark ? "white" : "black"This keeps the behavior compatible with your existing
isDarkproperty while ensuring the widget reacts to theme changes and maintains good contrast in both themes.
🧹 Nitpick comments (7)
faststack/faststack/imaging/cache.py (1)
35-47: Tighten fallback logic inget_decoded_image_sizeThe new branches are generally sound, but you can make the fallback a bit more robust and predictable:
- The
hasattr(item.buffer, '__sizeof__')check is mostly redundant;sys.getsizeofalready works for arbitrary Python objects.- If you ever pass a
memoryviewdirectly asbuffer, it might be clearer to treat it alongsidebytes/bytearray.One possible refinement:
- elif isinstance(item.buffer, (bytes, bytearray)): - return len(item.buffer) - else: - # Fallback: estimate using sys.getsizeof or compute from dimensions - # Assuming 4 bytes/pixel for RGBA or 3 for RGB - check item.channels if available - import sys - return sys.getsizeof(item.buffer) if hasattr(item.buffer, '__sizeof__') else item.width * item.height * 4 + elif isinstance(item.buffer, (bytes, bytearray, memoryview)): + return len(item.buffer) + else: + // Fallback: use sys.getsizeof; if that fails, approximate from dimensions + import sys + try { + return sys.getsizeof(item.buffer) + } catch (TypeError) { + return item.width * item.height * 4 + }Also double‑check that anything you store in the cache as a
DecodedImagereally does havewidth/heightattributes so the fallback remains safe.faststack/faststack/imaging/editor.py (1)
298-358: Robust save workflow with appropriate fallbacks.The three-tier fallback strategy (EXIF + format → format only → inferred) handles PIL's varied format-specific exceptions gracefully. The informative print statements aid debugging.
The static analysis warnings about broad
Exceptioncatching (lines 322, 339, 356) are flagged, but this is a reasonable trade-off given PIL's diverse exception types across formats. If you want to be more specific, consider catching(OSError, ValueError, KeyError, struct.error)which covers most PIL save failures.faststack/faststack/app.py (5)
1214-1223: Remove duplicate clearing of last_displayed_image.The
last_displayed_imageis cleared twice under the same lock (lines 1214-1216 and lines 1222-1223). Remove one of these duplicate blocks.Apply this diff:
# Clear caches since they reference old directory's images with self._last_image_lock: self.last_displayed_image = None self.image_cache.clear() self.prefetcher.cancel_all() self.display_generation += 1 self._metadata_cache = {} self._metadata_cache_index = (-1, -1) - # Clear last displayed image since it references the old directory - with self._last_image_lock: - self.last_displayed_image = None # Clear editor state if open
2000-2128: Comprehensive crop implementation with good error handling.The crop workflow correctly handles QJSValue conversion, pixel coordinate mapping, backup creation, and view refresh. The error handling throughout is appropriate.
Minor: The save logic (lines 2071-2089) duplicates some error handling from
ImageEditor.save_image. Consider consolidating this into a shared helper if the pattern repeats elsewhere, though the current implementation is acceptable for a specialized crop operation.
1404-1479: Consider refactoring duplicate undo logic.The undo handlers for
"auto_white_balance"(lines 1404-1441) and"crop"(lines 1442-1479) are nearly identical. Both restore a backup file, refresh the image list, find the restored image, and update the cache.Extract a shared helper method like
_undo_image_edit(action_type, saved_path, backup_path, timestamp)to reduce duplication and improve maintainability.
1968-1973: Fix logging in exception handlers.The error handling should use
logging.exceptionwithout explicitly passing the exception object, aslogging.exceptionautomatically includes the traceback.Apply this diff:
except ImportError: - log.error("NumPy not available for histogram computation") + log.exception("NumPy not available for histogram computation") self.update_status_message("Histogram requires NumPy") except Exception as e: - log.exception("Failed to compute histogram: %s", e) + log.exception("Failed to compute histogram") self.update_status_message(f"Histogram error: {e}")
2010-2030: QJSValue conversion is thorough but verbose.The QJSValue-to-Python list conversion handles multiple cases appropriately with good error handling. If this pattern is needed elsewhere (e.g., in
set_crop_box), consider extracting it to a utility function like_normalize_qjs_list(value, expected_length).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
faststack/ChangeLog.md(2 hunks)faststack/faststack.egg-info/PKG-INFO(2 hunks)faststack/faststack/app.py(18 hunks)faststack/faststack/benchmark_decode.py(0 hunks)faststack/faststack/config.py(1 hunks)faststack/faststack/imaging/cache.py(1 hunks)faststack/faststack/imaging/editor.py(6 hunks)faststack/faststack/imaging/prefetch.py(4 hunks)faststack/faststack/io/sidecar.py(1 hunks)faststack/faststack/qml/Components.qml(4 hunks)faststack/faststack/qml/FilterDialog.qml(1 hunks)faststack/faststack/qml/ImageEditorDialog.qml.new(0 hunks)faststack/faststack/qml/SettingsDialog.qml(1 hunks)faststack/faststack/ui/keystrokes.py.bak(0 hunks)faststack/faststack/ui/provider.py.bak(0 hunks)
💤 Files with no reviewable changes (4)
- faststack/faststack/ui/provider.py.bak
- faststack/faststack/qml/ImageEditorDialog.qml.new
- faststack/faststack/ui/keystrokes.py.bak
- faststack/faststack/benchmark_decode.py
✅ Files skipped from review due to trivial changes (1)
- faststack/faststack/io/sidecar.py
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/ChangeLog.md
🧰 Additional context used
🧬 Code graph analysis (1)
faststack/faststack/app.py (4)
faststack/faststack/imaging/editor.py (2)
create_backup_file(23-54)get_preview_data(254-271)faststack/faststack/ui/provider.py (4)
isHistogramVisible(505-506)isHistogramVisible(509-515)currentCropBox(616-618)currentCropBox(621-652)faststack/faststack/io/sidecar.py (2)
save(61-90)SidecarManager(13-97)faststack/faststack/io/watcher.py (2)
stop(64-70)Watcher(41-74)
🪛 Ruff (0.14.5)
faststack/faststack/imaging/editor.py
51-51: Consider moving this statement to an else block
(TRY300)
322-322: Do not catch blind exception: Exception
(BLE001)
339-339: Do not catch blind exception: Exception
(BLE001)
355-355: Consider moving this statement to an else block
(TRY300)
356-356: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/app.py
1969-1969: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1972-1972: Redundant exception object included in logging.exception call
(TRY401)
2029-2029: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
2087-2087: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (15)
faststack/faststack/qml/FilterDialog.qml (1)
24-24: LGTM – Fixed border color is consistent with the hardcoded theme.The border color is now fixed at
#404040, which aligns with the commented-outMaterial.themeline (Line 20) and the hardcodedbackgroundColor. This simplifies the UI by removing dynamic theming.faststack/faststack/imaging/prefetch.py (5)
20-20: LGTM!The threading import is appropriately placed and necessary for the ICC cache synchronization.
32-33: LGTM!The global lock is correctly defined to protect concurrent access to ICC caches from the thread pool.
44-50: Thread-safe cache access correctly implemented.The locking properly prevents race conditions during lazy initialization of ICC transforms. Note that the lock is held during the expensive
buildTransformcall, which could briefly block other threads. However, this is mitigated by caching—the build only occurs once per unique profile pair.
55-59: LGTM!Cache clearing is correctly synchronized. All ICC state (profiles, transforms, warning flag) is atomically cleared under the lock, preventing concurrent access issues.
70-92: Thread-safe monitor profile caching with correct locking.Verification confirms the return statement at line 92 is properly indented inside the
with _icc_cache_lock:block (8 spaces, matching all other statements in the block). The thread-safe lazy loading of the monitor ICC profile is correctly implemented:
- Cache check happens first under lock
- Disk I/O protected from race conditions
- Appropriate error handling for missing/invalid profiles
- Performance impact from lock-held I/O is acceptable given caching mitigates repeated loads
faststack/faststack/config.py (1)
27-38: Config comment tweaks align with UI defaultsThe updated comments for
saturation_factor,monitor_icc_path, andawb.modecorrectly document the mode-specific behavior and stay consistent with the QML defaults (awbMode: "lab"). No functional issues from these changes.faststack/faststack/qml/SettingsDialog.qml (1)
214-219: Safer AWB ComboBox index handlingClamping
currentIndexwithMath.max(0, model.indexOf(settingsDialog.awbMode))avoids invalid-1selections if the savedawbModeisn’t in the model. That will gracefully reset unknown modes to the first option ("lab"); confirm that this is the intended fallback.faststack/faststack.egg-info/PKG-INFO (1)
3-5: Release metadata update looks consistentVersion, summary, and release header updates to 1.3 align with the new feature set described in the PR. Just ensure
pyproject.toml,ChangeLog.md, andREADME.mdall carry the same version and headline description when you cut the release.Also applies to: 23-23
faststack/faststack/qml/Components.qml (1)
89-163: Crop mouse handling and DPI-aware edge detection look solidGating crop interactions behind
uiState.isCroppingwhile leaving pan/drag-outside behavior untouched is a clean separation, and usingedgeThreshold = 10 * Screen.devicePixelRatioshould make edge/corner grabs much more usable on high‑DPI displays. I don’t see correctness issues in this part of the interaction logic.Also applies to: 269-275
faststack/faststack/imaging/editor.py (3)
23-54: LGTM: Backup creation is well-designed.The sequential backup naming and sanitization of existing
-backupsuffixes prevents filename pollution. Usingshutil.copy2preserves file metadata.Minor: Consider logging instead of printing errors for consistency with the rest of the codebase, though
158-161: Good defensive programming.The division-by-zero guard prevents potential crashes when blacks and whites adjustments result in identical black_point and white_point values.
223-228: LGTM: White balance formula revised.The updated formula applies temperature primarily to the red-blue axis and tint primarily to green, with partial compensation on R/B. The comment clearly documents the intent. This appears to be a deliberate algorithmic improvement.
faststack/faststack/app.py (2)
1854-1899: LGTM: Properly handles updated save_image return type.The refactored
save_edited_imagecorrectly unpacks the tuple return fromImageEditor.save_image, handles failures gracefully with user feedback, and performs a complete refresh of the view and caches.
429-431: LGTM: Histogram updates are properly integrated.The histogram updates are consistently triggered on image navigation, edit parameter changes, crop operations, and auto white balance. This ensures the histogram stays in sync with the displayed image.
Also applies to: 438-440, 450-452, 1830-1832, 2119-2121, 2181-2183
| onPositionChanged: function(mouse) { | ||
| if (uiState && uiState.isCropping && isCropDragging) { | ||
| if (cropDragMode === "new") { | ||
| // Update crop rectangle while dragging | ||
| updateCropBox(cropStartX, cropStartY, mouse.x, mouse.y) | ||
| } else if (cropDragMode !== "none") { | ||
| // Refine existing crop box | ||
| var cropRect = getCropRect() | ||
| var imgWidth = mainImage.paintedWidth | ||
| var imgHeight = mainImage.paintedHeight | ||
| var imgX = (mainImage.width - imgWidth) / 2 | ||
| var imgY = (mainImage.height - imgHeight) / 2 | ||
|
|
||
| // Convert mouse position to normalized coordinates | ||
| var mouseX = (mouse.x - imgX) / imgWidth | ||
| var mouseY = (mouse.y - imgY) / imgHeight | ||
| mouseX = Math.max(0, Math.min(1, mouseX)) * 1000 | ||
| mouseY = Math.max(0, Math.min(1, mouseY)) * 1000 | ||
|
|
||
| var left = cropBoxStartLeft | ||
| var top = cropBoxStartTop | ||
| var right = cropBoxStartRight | ||
| var bottom = cropBoxStartBottom | ||
|
|
||
| // Adjust based on drag mode | ||
| if (cropDragMode === "move") { | ||
| var dx = mouseX - (cropBoxStartLeft + cropBoxStartRight) / 2 | ||
| var dy = mouseY - (cropBoxStartTop + cropBoxStartBottom) / 2 | ||
| var width = cropBoxStartRight - cropBoxStartLeft | ||
| var height = cropBoxStartBottom - cropBoxStartTop | ||
| left = Math.max(0, Math.min(1000 - width, cropBoxStartLeft + dx)) | ||
| top = Math.max(0, Math.min(1000 - height, cropBoxStartTop + dy)) | ||
| right = left + width | ||
| bottom = top + height | ||
| } else if (cropDragMode === "left") { | ||
| left = Math.max(0, Math.min(right - 10, mouseX)) | ||
| } else if (cropDragMode === "right") { | ||
| right = Math.max(left + 10, Math.min(1000, mouseX)) | ||
| } else if (cropDragMode === "top") { | ||
| top = Math.max(0, Math.min(bottom - 10, mouseY)) | ||
| } else if (cropDragMode === "bottom") { | ||
| bottom = Math.max(top + 10, Math.min(1000, mouseY)) | ||
| } else if (cropDragMode === "topleft") { | ||
| left = Math.max(0, Math.min(right - 10, mouseX)) | ||
| top = Math.max(0, Math.min(bottom - 10, mouseY)) | ||
| } else if (cropDragMode === "topright") { | ||
| right = Math.max(left + 10, Math.min(1000, mouseX)) | ||
| top = Math.max(0, Math.min(bottom - 10, mouseY)) | ||
| } else if (cropDragMode === "bottomleft") { | ||
| left = Math.max(0, Math.min(right - 10, mouseX)) | ||
| bottom = Math.max(top + 10, Math.min(1000, mouseY)) | ||
| } else if (cropDragMode === "bottomright") { | ||
| right = Math.max(left + 10, Math.min(1000, mouseX)) | ||
| bottom = Math.max(top + 10, Math.min(1000, mouseY)) | ||
| } | ||
|
|
||
|
|
||
| var constrainedBox = applyAspectRatioConstraint(left, top, right, bottom) | ||
| left = constrainedBox[0] | ||
| top = constrainedBox[1] | ||
| right = constrainedBox[2] | ||
| bottom = constrainedBox[3] | ||
|
|
||
|
|
||
| uiState.currentCropBox = [Math.round(left), Math.round(top), Math.round(right), Math.round(bottom)] | ||
| } | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
Aspect-ratio constraint can yield out-of-bounds crop boxes for wide ratios
In applyAspectRatioConstraint, the “too tall, adjust width” branch derives newWidth = currentHeight * targetAspect. For wide ratios like 1.91:1, newWidth can exceed 1000 when the crop is tall, and the subsequent clamping can leave left negative after the left/right adjustments (e.g., tall crop near center with large currentHeight).
That means uiState.currentCropBox can end up with values outside [0, 1000], which may:
- Draw the crop overlay partially outside the image area, and
- Break any Python-side logic that assumes normalized coordinates lie within
[0, 1000].
Consider normalizing the final box after enforcing the aspect ratio so that you always guarantee:
0 <= left < right <= 10000 <= top < bottom <= 1000
while preserving the aspect ratio as much as possible. One approach is:
- Compute the target width/height from the aspect ratio around the current center.
- If either dimension would exceed the
[0,1000]bounds, shrink both dimensions proportionally to keep the ratio while fitting entirely inside the normalized space. - Finally, translate (without resizing) to keep the box inside
[0,1000].
If you’d like, I can help sketch a concrete implementation of that normalization step.
Also applies to: 296-332, 343-387
I'll verify this review comment by examining the applyAspectRatioConstraint function and tracing through the logic for the concern raised.
🏁 Script executed:
#!/bin/bash
# Find the applyAspectRatioConstraint function definition
rg -n "applyAspectRatioConstraint" faststack/faststack/qml/Components.qml -A 50 -B 2Length of output: 5829
🏁 Script executed:
#!/bin/bash
# Also search for the function in case it's defined elsewhere
fd "\.qml$" -x rg -l "applyAspectRatioConstraint" {}Length of output: 40
🏁 Script executed:
#!/bin/bash
# Get the full context of the onPositionChanged handler and surrounding code
cat faststack/faststack/qml/Components.qml | sed -n '170,250p'Length of output: 4436
🏁 Script executed:
#!/bin/bash
# Check lines 296-332 and 343-387 mentioned in the review
cat faststack/faststack/qml/Components.qml | sed -n '290,340p'Length of output: 2290
🏁 Script executed:
#!/bin/bash
# Check the third location
cat faststack/faststack/qml/Components.qml | sed -n '337,390p'Length of output: 2509
Final bounds check required in applyAspectRatioConstraint
The verification confirms the review concern. Tracing through the "too tall, adjust width" branch with a 1.91:1 ratio and a tall crop box (e.g., width=200, height=800):
newWidth = 800 * 1.91 = 1528- Initial calculation places
left = -264,right = 1264 - First clamp sets
left = 0,right = 1528 - Second clamp computes
left = 0 - 528 = -528,right = 1000 - Result: out-of-bounds
leftvalue stored inuiState.currentCropBox
The same sequential clamping flaw affects the symmetrical "too wide" branch. All three call sites (lines 238, 324, 331) are affected because they all use the same broken constraint function.
The suggested fix is valid: after applying aspect ratio constraints, normalize the box to guarantee 0 ≤ left < right ≤ 1000 and 0 ≤ top < bottom ≤ 1000 by shrinking dimensions proportionally if needed, then translating to fit within bounds.
🤖 Prompt for AI Agents
faststack/faststack/qml/Components.qml lines 181-247 (and call sites at ~238,
324, 331): the applyAspectRatioConstraint function can produce boxes that remain
out-of-bounds after sequential clamping (e.g., left < 0 or right > 1000); update
applyAspectRatioConstraint so that after computing an aspect-correct box you (1)
enforce width/height are at least the minimum and at most 1000, (2) if either
side exceeds the 0–1000 range shrink the offending dimension proportionally
(maintaining the aspect ratio) until it fits, and (3) translate the box to fully
lie inside [0,1000] (prefer shrinking first, then translating) so the returned
[left,top,right,bottom] always satisfies 0 ≤ left < right ≤ 1000 and 0 ≤ top <
bottom ≤ 1000.
modified: faststack/faststack/app.py modified: faststack/faststack/imaging/cache.py modified: faststack/faststack/logging_setup.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
faststack/faststack/imaging/editor.py (1)
23-54: Consider using logging instead of print for error messages.The error message on line 53 uses
print()for error reporting, which is inconsistent with the logging approach used throughout the codebase.Apply this diff to use proper logging:
+import logging + +log = logging.getLogger(__name__) + def create_backup_file(original_path: Path) -> Optional[Path]: """ Creates a backup of the original file with naming pattern: filename-backup.jpg, filename-backup2.jpg, etc. Returns: Path to the backup file on success, None on failure. """ if not original_path.exists(): return None # Extract base name without any existing -backup suffix stem = original_path.stem # Remove any existing -backup, -backup2, -backup-1, etc. (handles both old and new formats) base_stem = re.sub(r'-backup(-?\d+)?$', '', stem) # Try filename-backup.jpg first backup_path = original_path.parent / f"{base_stem}-backup{original_path.suffix}" # If that exists, try filename-backup2.jpg, filename-backup3.jpg, etc. i = 2 while backup_path.exists(): backup_path = original_path.parent / f"{base_stem}-backup{i}{original_path.suffix}" i += 1 try: # Perform the backup shutil.copy2(original_path, backup_path) return backup_path except OSError as e: - print(f"Failed to create backup: {e}") + log.error("Failed to create backup for %s: %s", original_path, e) return Nonefaststack/faststack/app.py (1)
1204-1239: Remove duplicate clearing of last_displayed_image.Lines 1225-1226 and 1232-1234 both clear
last_displayed_imageunder the same lock. The second clearing (lines 1232-1234) is redundant.Apply this diff to remove the duplication:
# Clear caches since they reference old directory's images with self._last_image_lock: self.last_displayed_image = None self.image_cache.clear() self.prefetcher.cancel_all() self.display_generation += 1 self._metadata_cache = {} self._metadata_cache_index = (-1, -1) - # Clear last displayed image since it references the old directory - with self._last_image_lock: - self.last_displayed_image = None # Clear editor state if open self.image_editor.clear()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
faststack.json(1 hunks)faststack/faststack/app.py(19 hunks)faststack/faststack/config.py(1 hunks)faststack/faststack/imaging/cache.py(1 hunks)faststack/faststack/imaging/editor.py(7 hunks)faststack/faststack/logging_setup.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- faststack.json
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/faststack/imaging/cache.py
🧰 Additional context used
🪛 Ruff (0.14.5)
faststack/faststack/imaging/editor.py
51-51: Consider moving this statement to an else block
(TRY300)
330-330: Do not catch blind exception: Exception
(BLE001)
347-347: Do not catch blind exception: Exception
(BLE001)
363-363: Consider moving this statement to an else block
(TRY300)
364-364: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/app.py
1133-1133: Undefined name self
(F821)
1134-1134: Undefined name self
(F821)
1135-1135: Undefined name self
(F821)
1136-1136: Undefined name self
(F821)
1137-1137: Undefined name self
(F821)
1137-1137: Undefined name self
(F821)
1138-1138: Undefined name self
(F821)
1972-1972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1975-1975: Redundant exception object included in logging.exception call
(TRY401)
2032-2032: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
2090-2090: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (13)
faststack/faststack/config.py (1)
28-29: LGTM! Clear mode-specific documentation.The updated comments effectively clarify which color mode each parameter applies to, improving configuration documentation.
faststack/faststack/logging_setup.py (1)
36-36: LGTM! Prevents duplicate log handlers.Clearing existing handlers before adding the new
RotatingFileHandlerensures a clean logging setup and prevents duplicate log entries.faststack/faststack/imaging/editor.py (4)
67-73: LGTM! Clean state reset method.The
clear()method properly resets the editor state, which supports the directory-switching teardown improvements mentioned in the PR objectives.
166-169: LGTM! Good defensive programming.The division-by-zero guard prevents a potential crash when
blacksandwhitesadjustments would create equal black and white points.
231-236: LGTM! Clearer white balance implementation.The separated R, G, B adjustments with explicit temperature (by_shift) and tint (mg_shift) logic make the white balance implementation more readable and maintainable.
292-366: LGTM! Robust save implementation with multiple fallbacks.The refactored
save_imagemethod now uses the centralizedcreate_backup_filehelper and implements a robust multi-tier fallback strategy:
- First attempt with original format and EXIF
- Retry without EXIF if EXIF was the issue
- Final fallback letting Pillow infer settings
This ensures maximum compatibility even with corrupted EXIF data or format issues.
Note: The static analysis hints about catching
Exceptionare false positives here—broad exception handling is appropriate for a save operation with multiple fallback strategies.faststack/faststack/app.py (7)
252-260: LGTM! Clean keyboard controls for crop mode.The Enter/Escape key bindings for executing and canceling crop operations provide an intuitive workflow that aligns with the in-app cropping workflow described in the PR objectives.
429-431: LGTM! Histogram stays synchronized with navigation.The histogram updates after image navigation (also at lines 438-440 and 453-455) ensure the histogram window displays data for the currently viewed image.
1448-1484: LGTM! Crop undo implementation mirrors existing patterns.The undo logic for crop operations follows the same pattern as the auto white balance undo, ensuring consistency and integrating well with the backup-aware crop workflow mentioned in the PR objectives.
1859-1902: LGTM! Improved save workflow with better error handling.The updated
save_edited_imagemethod now properly handles the tuple return from the new backup-aware save implementation and provides clear user feedback through message boxes.
1921-1976: LGTM! RGB histogram computation looks solid.The histogram implementation:
- Toggles visibility with proper logging
- Computes separate R, G, B channel histograms using NumPy
- Intelligently uses editor preview when available
- Handles errors gracefully
This aligns well with the RGB histogram window mentioned in the PR objectives.
Note: The static analysis hints about logging style are minor and acceptable in this context.
1978-2132: LGTM! Comprehensive crop workflow implementation.The crop mode implementation provides:
- Clean state management (toggle, cancel, execute)
- Robust QJSValue-to-list conversion for QML integration (lines 2014-2033)
- Thorough validation of crop box coordinates
- Backup creation and undo tracking
- Complete cache invalidation and view refresh
This successfully implements the in-app cropping workflow with execute/cancel as described in the PR objectives.
Note: The static analysis hint about catching
Exceptionat line 2090 is acceptable here—this is a save operation with a fallback, and broad exception handling is appropriate.
1834-1837: LGTM! Histogram updates with parameter changes.Updating the histogram when edit parameters change ensures the histogram window reflects the current state of the edited image, providing real-time feedback during adjustments.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
faststack/faststack/app.py (2)
2030-2032: Consider using logging.exception for consistency.For consistency with other error handlers in this file (e.g., line 1443, 1482), consider using
logging.exceptioninstead oflogging.errorwhen logging exceptions.Apply this diff:
except (TypeError, IndexError, AttributeError) as e: self.update_status_message("Invalid crop box") - log.error("Failed to parse crop box (type: %s): %s", type(crop_box_raw), e) + log.exception("Failed to parse crop box (type: %s)", type(crop_box_raw)) returnAs per static analysis hints.
642-643: Consider renaming for clarity.The method
clear_all_batchesactually clears both batches and stacks (via delegation toclear_all_stacks). The docstring is accurate, but the method name is potentially misleading.Consider either:
- Renaming to
clear_all_batches_and_stacksfor clarity, or- Having
clear_all_batchesonly clear batches and callingclear_all_stacksseparately where neededThis is a minor naming clarity issue and not functionally incorrect.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
faststack/faststack/app.py(19 hunks)
🧰 Additional context used
🪛 Ruff (0.14.5)
faststack/faststack/app.py
1972-1972: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1975-1975: Redundant exception object included in logging.exception call
(TRY401)
2032-2032: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
2090-2090: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (17)
faststack/faststack/app.py (17)
33-33: LGTM: Import and PIL configuration changes support new crop workflow.The increased
MAX_IMAGE_PIXELSlimit accommodates modern high-resolution cameras, and thecreate_backup_fileimport is used appropriately in the new crop functionality.Also applies to: 47-47
132-133: LGTM: Thread-safe initialization improvements.The lock-protected initialization of
last_displayed_imageprevents race conditions, and the type annotation fordelete_historyimproves code clarity.Also applies to: 138-138
252-260: LGTM: Standard crop mode keyboard handlers.The Enter/Escape key handlers follow standard UX patterns and are properly guarded by the
isCroppingstate check.
429-431: LGTM: Consistent histogram updates after navigation.Histogram updates are properly conditional (only when visible) and consistently applied across all navigation methods.
Also applies to: 438-440, 453-455
446-448: Good optimization: Skip redundant jump_to_image calls.The early return when already at the target index prevents unnecessary status messages and work.
1131-1138: LGTM: AWB strength refresh logic properly implemented.The cache invalidation and prefetch restart when changing AWB strength is appropriate and consistent with similar operations elsewhere in the codebase (e.g.,
set_color_mode).
1448-1484: LGTM: Crop undo implementation follows established pattern.The crop undo logic mirrors the auto white balance undo implementation and includes proper error handling, state refresh, and restoration on failure.
1861-1902: LGTM: save_edited_image properly handles tuple return and provides user feedback.The refactored method correctly unpacks the tuple return from
save_image(), clears editor state, refreshes the view, and provides appropriate success/error dialogs.
1921-1929: LGTM: Histogram toggle properly manages visibility and updates.The toggle correctly updates visibility state and refreshes histogram data when opening.
1932-1974: LGTM: Histogram computation is efficient and handles edge cases.The histogram implementation properly uses the editor preview when available, handles missing NumPy gracefully, and efficiently computes per-channel histograms.
1978-2001: LGTM: Crop mode toggle and cancel provide clear UX.Both methods properly manage crop state, reset UI elements, and provide helpful status messages to guide users.
2004-2041: LGTM: Robust crop box parsing and validation.The crop box parsing handles multiple UI data formats (QJSValue, list, tuple) and validates bounds appropriately.
2046-2092: LGTM: Crop execution with backup and EXIF preservation.The implementation properly:
- Converts normalized coordinates to pixels
- Creates backup before modifying
- Preserves EXIF metadata for JPEG files
- Falls back to basic save if EXIF preservation fails
2095-2131: LGTM: Complete post-crop cleanup and state refresh.The method properly:
- Tracks operation for undo
- Exits crop mode
- Refreshes image list and display
- Resets zoom/pan
- Updates histogram if visible
2162-2162: LGTM: Proper editor state cleanup and histogram refresh after quick AWB.Clearing the editor state ensures a fresh reload, and the histogram update maintains consistency with other image modification operations.
Also applies to: 2182-2184
1834-1837: LGTM: Real-time histogram updates during editing.The conditional histogram update when parameters change provides immediate visual feedback to users while editing.
683-716: LGTM: Stack removal properly persists changes.The sidecar save after modifying stacks ensures changes are persisted to disk, consistent with other stack modification operations.
| with self._last_image_lock: | ||
| self.last_displayed_image = None | ||
| self.image_cache.clear() | ||
| self.prefetcher.cancel_all() | ||
| self.display_generation += 1 | ||
| self._metadata_cache = {} | ||
| self._metadata_cache_index = (-1, -1) | ||
| # Clear last displayed image since it references the old directory | ||
| with self._last_image_lock: | ||
| self.last_displayed_image = None |
There was a problem hiding this comment.
Remove duplicate clearing of last_displayed_image.
Lines 1225-1226 and 1233-1234 both clear last_displayed_image under the same lock. The second block is redundant.
Apply this diff to remove the duplication:
# Clear caches since they reference old directory's images
with self._last_image_lock:
self.last_displayed_image = None
self.image_cache.clear()
self.prefetcher.cancel_all()
self.display_generation += 1
self._metadata_cache = {}
self._metadata_cache_index = (-1, -1)
- # Clear last displayed image since it references the old directory
- with self._last_image_lock:
- self.last_displayed_image = None
# Clear editor state if open
self.image_editor.clear()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| with self._last_image_lock: | |
| self.last_displayed_image = None | |
| self.image_cache.clear() | |
| self.prefetcher.cancel_all() | |
| self.display_generation += 1 | |
| self._metadata_cache = {} | |
| self._metadata_cache_index = (-1, -1) | |
| # Clear last displayed image since it references the old directory | |
| with self._last_image_lock: | |
| self.last_displayed_image = None | |
| with self._last_image_lock: | |
| self.last_displayed_image = None | |
| self.image_cache.clear() | |
| self.prefetcher.cancel_all() | |
| self.display_generation += 1 | |
| self._metadata_cache = {} | |
| self._metadata_cache_index = (-1, -1) |
🤖 Prompt for AI Agents
In faststack/faststack/app.py around lines 1225 to 1234, the code acquires
self._last_image_lock and sets self.last_displayed_image = None twice (lines
1225-1226 and again at 1233-1234); remove the second redundant with-block (the
one at 1233-1234) so last_displayed_image is only cleared once while holding the
lock, leaving the surrounding cache clears, prefetcher cancellation,
display_generation increment, and metadata resets intact.
| log.exception("Failed to compute histogram: %s", e) | ||
| self.update_status_message(f"Histogram error: {e}") |
There was a problem hiding this comment.
Remove redundant exception object from logging.exception call.
logging.exception automatically includes the exception traceback, so explicitly including the exception object in the message is redundant.
Apply this diff:
except Exception as e:
- log.exception("Failed to compute histogram: %s", e)
+ log.exception("Failed to compute histogram")
self.update_status_message(f"Histogram error: {e}")As per static analysis hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.exception("Failed to compute histogram: %s", e) | |
| self.update_status_message(f"Histogram error: {e}") | |
| log.exception("Failed to compute histogram") | |
| self.update_status_message(f"Histogram error: {e}") |
🧰 Tools
🪛 Ruff (0.14.5)
1975-1975: Redundant exception object included in logging.exception call
(TRY401)
🤖 Prompt for AI Agents
In faststack/faststack/app.py around lines 1975-1976, the log.exception call
currently includes the exception object in the formatted message
(log.exception("Failed to compute histogram: %s", e)) which is redundant because
logging.exception already includes traceback and exception info; remove the "%s"
and the exception argument so the call becomes log.exception("Failed to compute
histogram") and leave the subsequent self.update_status_message(f"Histogram
error: {e}") unchanged.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
faststack/faststack/app.py (1)
1215-1255: Duplicate clearing oflast_displayed_imageinopen_folderIn
open_folderyou clearlast_displayed_imagetwice under the same lock:with self._last_image_lock: self.last_displayed_image = None ... with self._last_image_lock: self.last_displayed_image = NoneThe second block is redundant and can be removed without changing behavior:
- with self._last_image_lock: - self.last_displayed_image = None - self.image_cache.clear() + with self._last_image_lock: + self.last_displayed_image = None + self.image_cache.clear() self.prefetcher.cancel_all() self.display_generation += 1 self._metadata_cache = {} self._metadata_cache_index = (-1, -1) - # Clear last displayed image since it references the old directory - with self._last_image_lock: - self.last_displayed_image = NoneThis mirrors the earlier clean‑up you did in similar code paths.
🧹 Nitpick comments (11)
faststack/faststack/io/indexer.py (1)
43-44: Sorting JPGs by mtime then name looks goodOrdering by
(st_mtime, name)is reasonable for pairing and reproducible iteration. Just confirm you indeed want oldest-first; if you prefer newest-first, considerreverse=Trueor negatingst_mtime.faststack/faststack/imaging/cache.py (1)
12-15: Type annotation foron_evictcan be made explicit OptionalThe new
on_evicthook wiring looks correct. To satisfy Ruff and keep annotations clear, consider making the Optional explicit and using the modern union syntax:-from typing import Any, Callable +from typing import Any, Callable -class ByteLRUCache(LRUCache): - """An LRU Cache that respects the size of its items in bytes.""" - def __init__(self, max_bytes: int, size_of: Callable[[Any], int] = len, on_evict: Callable[[], None] = None): +class ByteLRUCache(LRUCache): + """An LRU Cache that respects the size of its items in bytes.""" + def __init__( + self, + max_bytes: int, + size_of: Callable[[Any], int] = len, + on_evict: Callable[[], None] | None = None, + ):faststack/ChangeLog.md (1)
3-10: Minor wording/formatting polish in changelog
- Line 10: fix typo
increassing→increasing.- Line 9: consider wrapping the bare URL in markdown, e.g.
[stackcopy.py --lightroomimport](https://github.com/AlanRockefeller/faststack), to satisfy markdownlint and improve readability.These don’t affect behavior but keep docs clean.
faststack/faststack/app.py (4)
104-109: Cache eviction warning flow is sound, consider reset semanticsWiring
ByteLRUCache(on_evict=self._on_cache_evict)and using_has_warned_cache_fullplusQTimer.singleShotto surface a single “cache full” warning is a nice, UI‑safe pattern.If you ever want warnings to re‑arm after a user increases cache size or restarts heavy usage, consider resetting
_has_warned_cache_fullwhen:
- cache size is changed, or
- a new directory is opened and you conceptually treat it as a new session.
Not required for correctness, but something to think about.
Also applies to: 1609-1616
433-450: Histogram updates are well‑integrated; logging tweaks possibleHooking
update_histogram()fromnext_image,prev_image,jump_to_image, andset_edit_parameterwhenisHistogramVisibleis true matches user expectations and centralizes histogram generation in one place.For the logging in
update_histogramyou could align with Ruff’s suggestions and avoid passing the exception intologging.exception:- except ImportError: - log.error("NumPy not available for histogram computation") + except ImportError: + log.exception("NumPy not available for histogram computation") self.update_status_message("Histogram requires NumPy") - except Exception as e: - log.exception("Failed to compute histogram: %s", e) + except Exception: + log.exception("Failed to compute histogram") self.update_status_message(f"Histogram error: {e}")(You’d also capture
efrom the generic except if you still want it in the status message.)Functionally this is fine as‑is; the change is just logging hygiene.
Also applies to: 2012-2081
1482-1558: Crop undo branch mirrors AWB undo correctly (but is duplicated)The new
"crop"branch inundo_deletecorrectly:
- Restores the backup.
- Refreshes the image list and repositions
current_index.- Invalidates cache and prefetch.
- Restores undo history on failure.
Given the AWB undo branch does essentially the same thing, consider extracting a small helper (e.g.,
_restore_backup(saved_path, backup_path, success_status_message)) to reduce duplication.
2234-2362: Crop execution pipeline is robust; minor logging tweak possibleThe crop flow:
- Normalizes
currentCropBoxfrom various QML types with defensivetry/except.- Rejects identity crops and invalid boxes.
- Converts 0–1000 normalized coords to pixel coords with clamping.
- Creates a backup via
create_backup_filebefore save.- Preserves original format/EXIF when possible, with a safe fallback.
- Tracks the operation in
undo_historyas"crop".- Refreshes the list, re-finds the image, clears cache/prefetch, resets zoom, and updates histogram if visible.
That’s a solid end‑to‑end implementation.
If you’d like to follow Ruff’s guidance, you could swap thelog.errorin the crop‑box parsing failure forlog.exceptionto capture tracebacks when debugging:- except (TypeError, IndexError, AttributeError) as e: - self.update_status_message("Invalid crop box") - log.error("Failed to parse crop box (type: %s): %s", type(crop_box_raw), e) + except (TypeError, IndexError, AttributeError): + self.update_status_message("Invalid crop box") + log.exception("Failed to parse crop box (type: %s)", type(crop_box_raw))Optional, but helps with diagnostics.
faststack/faststack/ui/provider.py (2)
95-109: Histogram visibility and data plumbing looks correct
- Backing fields
_is_histogram_visibleand_histogram_datawith propertiesisHistogramVisibleandhistogramDataplus notify signals match the QML usage (onIsHistogramVisibleChanged,onHistogramDataChanged).- The
isHistogramVisiblesetter callingself.app_controller.update_histogram()on open ensures the window gets data immediately.histogramDatauses a QVariant property, which is appropriate for dicts passed into QML.Docstring still mentions
'r', 'g', 'b'keys, but the actual structure isr_hist,g_hist,b_hist, etc.; consider updating the comment to avoid confusion.Also applies to: 121-134, 523-546
639-668:currentCropBoxnormalization is robust; fix NBSP hyphen in commentThe new setter defensively:
- Converts
QJSValuevia.toVariant().- Handles list → tuple conversion.
- Logs and ignores invalid shapes or types.
- Enforces a strict 4‑element tuple, which matches the Python crop code’s expectations.
That’s a good defensive layer between QML and Python. Very minor nit: the comment “4‑element tuples” uses a non‑breaking hyphen; you may want to change it to a regular
-to satisfy Ruff and avoid invisible characters:- # only accept 4‑element tuples + # only accept 4-element tuplesfaststack/faststack/qml/HistogramWindow.qml (2)
1-7: Window visibility wiring is mostly good; consider syncing back touiStateon closeThe bindings and Connections ensure:
visiblefollowsuiState.isHistogramVisible, and- opening the window triggers
controller.update_histogram().One subtle UX detail: if the user closes the histogram window via the window’s close button,
uiState.isHistogramVisibleremainstrue, so:
- Histogram updates will still be computed on navigation even though the window is hidden.
- The next toggle via hotkey/menu will flip the flag from
true→false(no visible effect), thenfalse→trueto reopen.You can keep things in sync by reflecting visibility changes back into
uiState:onVisibleChanged: { - if (visible && controller) { - controller.update_histogram() - } + if (uiState && uiState.isHistogramVisible !== visible) { + uiState.isHistogramVisible = visible + } + // update_histogram() is already called when isHistogramVisible becomes true on the Python side }This avoids redundant histogram work when the window is closed.
Also applies to: 15-37
162-228: Loader repaint logic is correct but uses brittlechildrenindexingThe per‑channel loaders correctly update:
histogramDatatouiState.histogramData.[rgb]_hist.clipCount/preClipCount.- Trigger a repaint via
requestPaint().However, the way you reach the
Canvas:redLoader.item.children[0].children[1].requestPaint()relies on the exact child ordering inside
singleChannelHistogramand is fragile if the layout changes.You can make this more robust by exposing the canvas via a property alias inside the component, e.g.:
Component { id: singleChannelHistogram Item { id: root property alias canvasItem: canvas ... Canvas { id: canvas ... } } }Then use:
redLoader.item.canvasItem.requestPaint()(and similarly for green/blue). This decouples callers from the internal child structure.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
faststack/ChangeLog.md(2 hunks)faststack/faststack/app.py(24 hunks)faststack/faststack/config.py(1 hunks)faststack/faststack/imaging/cache.py(3 hunks)faststack/faststack/io/indexer.py(1 hunks)faststack/faststack/qml/HistogramWindow.qml(1 hunks)faststack/faststack/qml/Main.qml(6 hunks)faststack/faststack/ui/provider.py(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/faststack/config.py
🧰 Additional context used
🧬 Code graph analysis (1)
faststack/faststack/ui/provider.py (1)
faststack/faststack/app.py (4)
clear_all_batches(997-1006)stack_source_raws(2101-2221)apply_filter(160-176)update_histogram(2013-2080)
🪛 LanguageTool
faststack/ChangeLog.md
[grammar] ~10-~10: Ensure spelling is correct
Context: ...n the cache is full so you can consider increassing the cache size in settings. ## [1.2.0...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
faststack/ChangeLog.md
9-9: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.6)
faststack/faststack/imaging/cache.py
12-12: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
faststack/faststack/app.py
2076-2076: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
2079-2079: Redundant exception object included in logging.exception call
(TRY401)
2159-2163: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
2262-2262: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
2320-2320: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/ui/provider.py
665-665: Comment contains ambiguous ‑ (NON-BREAKING HYPHEN). Did you mean - (HYPHEN-MINUS)?
(RUF003)
🔇 Additional comments (19)
faststack/faststack/imaging/cache.py (2)
23-33: Eviction callback placement is appropriateOverriding
popitem()to log and then invokeon_evictis the right place to hook eviction without changing cachetools’ behavior. No functional issues spotted here.
41-50: Improved size calculation for decoded images is saferThe stricter
bytes/bytearraycheck andwidth*height*bytes_per_pixelfallback (defaultingchannelsto 4) is a sensible improvement overlen()and should give better memory accounting for typical RGB/RGBA buffers.faststack/faststack/app.py (9)
33-50: Pillow MAX_IMAGE_PIXELS cap is reasonableSetting
Image.MAX_IMAGE_PIXELS = 200_000_000gives good protection against decompression bombs while still covering very high‑resolution photos. No issues here; just be aware extremely large stitched panoramas above ~200 MP will still be rejected.
203-214: Display resize debounce and validation look correctThe new debug log plus
width <= 0 or height <= 0guard will avoid spurious resizes from transient 0‑dimension events and still feed the debounced_handle_resize. This should make prefetch sizing more robust.
255-274: Enter/Esc handling in crop mode is correctly short‑circuitingIntercepting Enter/Esc in
eventFilterwhenui_state.isCroppingand returningTruebefore delegating tokeybinderwill prevent accidental triggering of other actions while cropping. That flow looks consistent with the crop UX described in the changelog.
982-1007: Stacks/batches clearing API is clearer now
clear_all_stacks()and the newclear_all_batches()are nicely separated, with stacks no longer implicitly clearing batches. The UI messages (“All stacks cleared” / “All batches cleared”) give good feedback.Also applies to: 390-393
1139-1155: AWB strength change correctly forces re-decode in color-managed modesRefreshing cache/prefetch only when
get_color_mode()is'saturation'or'icc'makes sense and avoids unnecessary work in'none'mode. The sequence (clear cache → cancel prefetch → bump generation → update_prefetch → sync_ui_state) is consistent with other color-related changes.
1263-1351: Preload-all algorithm is thoughtful; concurrency model is acceptableThe new
preload_all_imageslogic:
- Skips already‑cached, non‑nearby images.
- Forces re-caching for a “nearby radius” to keep current/adjacent images hot in the LRU.
- Loads furthest‑from‑current first to reduce eviction pressure on what the user is looking at.
- Counts already‑cached images toward progress, so the bar starts non‑zero.
This is a solid design. The
completed += 1updates from future callbacks are fine under CPython’s GIL in this context.
1892-1919: Editor save flow and histogram refresh look consistent
load_image_for_editingnow also initializes aspect ratios and a default crop box inui_state, which is good prep for crop mode.save_edited_imagenow checks the return fromimage_editor.save_image(), shows an error dialog on failure, refreshes the image list, repositionscurrent_index, invalidates cache, and clears editor state.set_edit_parameterupdating the histogram when visible keeps the histogram window in sync with edits.All of this is coherent with the new editor/crop UX.
Also applies to: 1940-1983
2002-2011: Histogram and crop-mode toggles are wired correctly
toggle_histogramsimply flipsui_state.isHistogramVisibleand callsupdate_histogram()on open, which is straightforward.toggle_crop_modeandcancel_crop_modeconsistently manageui_state.isCroppingand resetcurrentCropBoxand aspect ratios, with clear status messages.The keyboard integration in
eventFilterand the help text in QML match this behavior.Also applies to: 2082-2232
2392-2415: Quick AWB now clears editor and keeps histogram in syncCalling
self.image_editor.clear()after a successful quick AWB save and refreshing the histogram when visible are good touches—they avoid stale editor state and keep the histogram window accurate.faststack/faststack/qml/Main.qml (5)
15-16: Title binding to current directory is helpfulBinding the window title to
uiState.currentDirectorygives immediate feedback about the active folder and should help users avoid confusion when switching directories.
435-458: Editor toggle behavior is consistent between menu and keyboardBoth the “Edit Image” menu item and the
Ekey now:
- Toggle
uiState.isEditorOpen.- Call
controller.load_image_for_editing()only when transitioning from closed → open.That keeps the behavior intuitive and avoids accidentally reloading when closing via
E.Also applies to: 648-662
459-478: Crop Image action is correctly delegated to controllerThe new “Crop Image” action calling
controller.toggle_crop_mode()and closing the menu is a clean hook into the Python crop pipeline. No issues seen here.
648-662: Keyboard and help text updates match new behavior
- The
Ekey handler now explicitly documents the “close without saving if open, otherwise open” semantics.- The about dialog text documents:
Ctrl+Zundoing delete/AWB/crop.Ctrl+Shift+Bquick AWB.Ofor crop mode (Enter/ESC behavior).Hfor histogram window.- Updated
Eeditor toggle semantics.This keeps the help in sync with the new functionality.
Also applies to: 832-866
915-920: Embedding HistogramWindow with theme bindings is correctInstantiating
HistogramWindowand binding itswindowBackgroundColor,primaryTextColor, andgridLineColorto the root theme makes the histogram window visually consistent. As long asuiStateandcontrollerremain context properties (they are inmain()), this integration looks good.faststack/faststack/ui/provider.py (2)
390-393: New slots cleanly expose controller features to QML
clear_all_batchesdelegates toapp_controller.clear_all_batches().stack_source_rawsandapplyFiltersimply forward to the controller.This keeps QML thin and centralizes logic in Python as intended.
Also applies to: 469-477
523-546: Histogram properties are QML-friendlyUsing:
@Property(bool, notify=is_histogram_visible_changed) def isHistogramVisible(...) ... @Property('QVariant', notify=histogram_data_changed) def histogramData(...)is exactly what the QML side expects. Emitting
is_histogram_visible_changedonly on actual value change avoids redundant updates; callingupdate_histogram()only when opening the window is also appropriate.faststack/faststack/qml/HistogramWindow.qml (1)
49-135: Single-channel histogram rendering is clear and efficientThe
singleChannelHistogramcomponent:
- Draws a grid, a “danger zone” for high values, and a normalized filled curve with outline.
- Uses per‑channel colors and separate pre‑clip/clip counts.
That’s a nice, readable Canvas implementation and should perform well for 256‑bin data.
| ItemDelegate { | ||
| width: 220 | ||
| height: 36 | ||
| text: "Stack Source RAWs" | ||
| enabled: uiState ? uiState.isStackedJpg : false | ||
| onClicked: { | ||
| if (uiState) uiState.stack_source_raws(); | ||
| actionsMenu.close() | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*.py" | grep -i "state\|controller" | head -20Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
fd -e py "ui" -o "state" -o "controller" | head -30Repository: AlanRockefeller/faststack
Length of output: 198
🏁 Script executed:
rg "isStackedJpg" --type py -A 3 -B 3Repository: AlanRockefeller/faststack
Length of output: 1107
🏁 Script executed:
rg "class AppController" --type py -A 5Repository: AlanRockefeller/faststack
Length of output: 430
🏁 Script executed:
rg "def stack_source_raws" --type py -A 15Repository: AlanRockefeller/faststack
Length of output: 2223
🏁 Script executed:
rg "stacked\\.jpg\|stacked\\.JPG" --type py -iRepository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
rg "endswith.*stacked" --type py -A 2 -B 2Repository: AlanRockefeller/faststack
Length of output: 765
Stack Source RAWs menu item wiring has case-sensitivity mismatch
The menu entry checks enabled: uiState ? uiState.isStackedJpg : false, but isStackedJpg in UIState (provider.py) uses case-sensitive endswith(" stacked.JPG"), while AppController.stack_source_raws (app.py) uses case-insensitive filename.lower().endswith(" stacked.jpg"). This disables the menu item for files like " stacked.jpg" (lowercase) even though the action would work. Align both checks to use the same case-sensitivity—recommend using case-insensitive checks in both places for robustness.
🤖 Prompt for AI Agents
In faststack/faststack/qml/Main.qml around lines 575-583, the enabled test uses
uiState.isStackedJpg but UIState currently checks case-sensitive " stacked.JPG"
while AppController.stack_source_raws uses a case-insensitive check; update the
UIState implementation in provider.py so its isStackedJpg check is
case-insensitive (e.g., lowercase the filename and compare against "
stacked.jpg" or otherwise normalize case) so both the menu enabled logic and the
action use the same case-insensitive rule.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Chores
✏️ Tip: You can customize this high-level summary in your review settings.