Release v1.0 — new features#16
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds a full image-editing feature with live preview, crop and edit parameters (brightness/contrast/saturation/white-balance/rotate/save with EXIF preservation), an auto white-balance action, redesigned batch/stack membership handling, a byte-aware image cache and adaptive prefetcher with ICC handling, filesystem sidecar and watcher, executable validators/Helicon launcher, QML UI rework (menus, editor dialog, dialogs), packaging/metadata updates, and new unit tests and tooling. Changes
Sequence Diagram(s)sequenceDiagram
participant QML as QML UI
participant UI as UIState/ImageProvider
participant C as AppController
participant E as ImageEditor
participant FS as Sidecar/Indexer/Prefetch
Note over QML, C: Open editor (E key / Edit action)
QML->>UI: request isEditorOpen true
UI->>C: controller.load_image_for_editing()
C->>E: load_image(path)
E-->>C: preview ready (bytes)
C->>UI: set currentEditedImageData (notify)
UI->>QML: image source updated (edited provider)
Note over QML, C: User adjusts slider / crop
QML->>C: set_edit_parameter / set_crop_selection_normalized
C->>E: set_edit_param / set_crop_box
E-->>C: updated preview
C->>UI: update currentEditedImageData
UI->>QML: live preview refresh
Note over QML, C: Save
QML->>C: save_edited_image
C->>E: save_image() (backup + write with EXIF)
E-->>C: saved path
C->>FS: update sidecar/metadata and UI state
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (53)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
faststack/README.md (1)
16-16: Photoshop shortcut docs still mention E while code and shortcuts use PThe feature bullet still says “Edit current image in Photoshop (E key)”, but the keyboard shortcuts section (line 55),
Keybinder(Qt.Key_P →edit_in_photoshop), andMain.qmlnow exposePas the Photoshop key. Please update the feature line to referencePinstead ofEto avoid confusing users.Also applies to: 55-55
faststack/faststack/qml/Main.qml (1)
355-386: Update key-bindings dialog text for S/B/X to match new behaviorSwitching the Photoshop entry to
P: Edit in Photoshophere is correct and matchesKeybinderand the README. However, the earlier line still saysX or S: Remove current image from batch/stack, which no longer matches the code:
Snow callstoggle_stack_membershipBnow callstoggle_batch_membershipXis the only key that callsremove_from_batch_or_stackSuggest updating this block to something like:
- " \\: Clear all batches<br>" + - " X or S: Remove current image from batch/stack<br><br>" + + " \\: Clear all batches<br>" + + " S: Toggle selection for stacking<br>" + + " B: Toggle selection for batch drag-and-drop<br>" + + " X: Remove current image from batch/stack<br><br>" +so the on-screen help matches
keystrokes.pyand the new toggle semantics.faststack/faststack/app.py (1)
615-696: Tidy up remove_from_batch_or_stack: redundant sidecar save and minor lint issuesThe range-splitting logic for both batches and stacks looks sound, but there are a few cleanups worth making:
- Inside the stack-removal branch you call
self.sidecar.data.stacks = self.stacksandself.sidecar.save()before overwritingself.stackswithnew_stacks, then do the same again after the loop. The first assignment/save persists stale data and is redundant; keeping only the final block afterself.stacks = new_stacksis clearer and avoids unnecessary disk I/O.update_status_message(f"Removed from batch")andupdate_status_message(f"Removed from stack")are f-strings without placeholders; tools like Ruff flag these as F541. They should be plain string literals.- Behavior note: as written, an image that happens to be in both a batch and a stack will be removed from the first matching batch only (
removedbecomes True so the stack-removal branch is skipped). If you ever allow overlapping membership and wantXto remove from both, you’d need to adjust theremoved/batch_modified/stack_modifiedflow.A minimal diff addressing the first two points:
- log.info("Removed index %d from batch [%d, %d]", self.current_index, start, end) - self.update_status_message(f"Removed from batch") + log.info("Removed index %d from batch [%d, %d]", self.current_index, start, end) + self.update_status_message("Removed from batch") @@ - self.sidecar.data.stacks = self.stacks # Update sidecar BEFORE self.stacks is replaced - self.sidecar.save() @@ - log.info("Removed index %d from stack [%d, %d]", self.current_index, start, end) - self.update_status_message(f"Removed from stack") + log.info("Removed index %d from stack [%d, %d]", self.current_index, start, end) + self.update_status_message("Removed from stack") @@ - if stack_modified: - self.stacks = new_stacks - self.sidecar.data.stacks = self.stacks - self.sidecar.save() + if stack_modified: + self.stacks = new_stacks + self.sidecar.data.stacks = self.stacks + self.sidecar.save()
🧹 Nitpick comments (3)
faststack/faststack/app.py (3)
738-841: toggle_stack_membership logic is robust; doc/spec alignment may be usefulThe stack toggle algorithm looks correct:
- When the image is in an existing stack, it splits/shrinks the appropriate
[start, end]range.- When not in any stack:
- Creates a new single-image stack if none exist.
- Otherwise finds the nearest stack (backward/forward scan over
image_files), extends that stack to include the current index, then sorts and merges overlapping/adjacent ranges.- Sidecar
stacksis updated once at the end and saved; metadata cache and stack summary signals are refreshed.Given this is now the primary way to manage stacks, it may be worth double‑checking that user-facing docs (README, ChangeLog, key-bindings text) explicitly describe this “join nearest stack / create new stack” behavior so users aren’t surprised by how
Sgrows stacks.
845-875: launch_helicon now relies solely on stacks; update docstring/log textThe implementation now only launches Helicon Focus when
self.stacksis non-empty; there is no longer any concept of an ad-hoc “selection” list. However:
- The docstring still says “with selected files (RAW preferred, JPG fallback) or stacks”.
- The warning message is “No selection or stacks defined to launch Helicon Focus.”
To avoid confusion now that stacks are the only driver, consider tightening this to something like:
- """Launches Helicon Focus with selected files (RAW preferred, JPG fallback) or stacks.""" + """Launches Helicon Focus once per defined stack (RAW preferred, JPG fallback).""" @@ - else: - log.warning("No selection or stacks defined to launch Helicon Focus.") + else: + log.warning("No stacks defined to launch Helicon Focus.")and aligning README wording with that “stacks-only” behavior.
1519-1531: Simplify _get_batch_info and avoid unused localsSwitching
_get_batch_infoto return just"In Batch"is fine given the simplified UI, butcount_in_batchandpos_in_batchare now computed and unused. You can simplify this block and avoid potential “assigned but never used” lint warnings:- for i, (start, end) in enumerate(self.batches): - if start <= index <= end: - count_in_batch = end - start + 1 - pos_in_batch = index - start + 1 - info = "In Batch" - break + for start, end in self.batches: + if start <= index <= end: + info = "In Batch" + breakEverything else in the method (including “Batch Start Marked” handling and logging) looks good.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
faststack/ChangeLog.md(1 hunks)faststack/README.md(3 hunks)faststack/faststack/app.py(8 hunks)faststack/faststack/qml/Main.qml(1 hunks)faststack/faststack/ui/keystrokes.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
faststack/faststack/app.py (3)
faststack/faststack/io/watcher.py (1)
start(49-62)faststack/faststack/io/sidecar.py (1)
save(62-91)faststack/faststack/ui/provider.py (1)
launch_helicon(258-259)
🪛 Ruff (0.14.5)
faststack/faststack/app.py
644-644: f-string without any placeholders
Remove extraneous f prefix
(F541)
678-678: f-string without any placeholders
Remove extraneous f prefix
(F541)
🔇 Additional comments (6)
faststack/README.md (2)
22-22: “Has Memory” description matches new metadata behaviorThe updated “Has Memory” bullet now explicitly mentions edited/stacked/uploaded, which aligns with the sidecar flags and footer labels. No issues here.
41-46: Updated G/S/B shortcuts are consistent with controller/keybinderThe shortcut descriptions for
G,S, andBmatch the new methods (show_jump_to_image_dialog,toggle_stack_membership,toggle_batch_membership) wired inKeybinderandAppController. This keeps the README in sync with the implementation.faststack/faststack/ui/keystrokes.py (1)
27-36: New S/B/P key mappings are consistent with AppController and docsMapping
Qt.Key_Stotoggle_stack_membership,Qt.Key_Btotoggle_batch_membership, andQt.Key_Ptoedit_in_photoshoplines up with the new methods inAppControllerand with the README/Main.qml key-binding text. No functional issues spotted.Also applies to: 44-47
faststack/faststack/app.py (3)
577-604: end_current_batch UX and state updates look correctThe batch range creation, sorting, metadata cache invalidation, and
dataChanged/UI sync are all wired correctly. The new warning/status message path whenbatch_start_indexisNone(“Press '{' first.”) is a nice touch and doesn’t introduce any state inconsistencies.
697-737: toggle_batch_membership correctly toggles range membershipThe new
toggle_batch_membershipimplementation correctly:
- Detects whether the current index is already inside any batch.
- Splits/shrinks the first matching range when removing.
- Creates a single-index
[i, i]batch and keepsself.batchessorted when adding.- Invalidates metadata cache, emits
dataChanged, and resyncs UI.This aligns with the README’s “Toggle selection of current image for batch drag & drop”.
1431-1505: Filtering drag payload to existing files is a good robustness improvementThe changes to
start_drag_current_imageto:
- Build
file_indicesfrom the batch indices,- Filter them to
existing_indiceswherepath.exists(),- Drag only
file_pathsfor existing files, and- Mark only those existing indices as uploaded after a successful drag,
prevent nonexistent paths from entering the drag payload or being marked as uploaded. This is a solid defensive change and keeps sidecar metadata in sync with what was actually dragged.
| @@ -1,5 +1,7 @@ | |||
| # ChangeLog | |||
|
|
|||
| Todo: Add image brightness control / cropping. Make batches a bit more intuitive. Make it work on Linux / Mac. Create Windows .exe. Write better documentation / help. Add splash screen / icon. | |||
There was a problem hiding this comment.
Align changelog description of S/X behavior with new toggle semantics
The new Todo line is fine, but in the 1.0.0 entry Line 10 still says “X or S keys remove individual images from batches/stacks (shrinks or splits ranges)”. In this PR, S becomes a toggle for stack membership while X remains the removal key (see toggle_stack_membership, toggle_batch_membership, and remove_from_batch_or_stack in app.py and mappings in keystrokes.py). Consider rephrasing that bullet so the shipped 1.0.0 changelog matches actual behavior.
Also applies to: 8-11
🤖 Prompt for AI Agents
faststack/ChangeLog.md lines 3 and 8-11: the 1.0.0 changelog text still states
“X or S keys remove individual images from batches/stacks (shrinks or splits
ranges)”, but the code now makes S a toggle for stack membership and X the
removal key; update the bullet to reflect actual behavior by replacing that
sentence with one that says X removes individual images from batches/stacks
(shrinks or splits ranges) and S toggles stack membership for the selected
image(s), and verify any other mentions in lines 8-11 use the new semantics.
1 participant
Summary by CodeRabbit
New Features
UI/UX Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.