v1.1#17
Conversation
WalkthroughThis PR introduces FastStack, a comprehensive ultra-fast JPG/RAW image viewer with caching and focus stacking support. It adds a complete application comprising Python backend modules for image processing (JPEG decoding, caching, prefetching with ICC color management), file I/O (scanning, RAW pairing, metadata sidecars, filesystem watching), integration with Helicon Focus and Photoshop, and a QML-based UI. A second update adds an image editor with exposure, contrast, saturation, white balance, cropping, and auto white balance functionality. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant QML as QML UI
participant Controller as AppController
participant Indexer as Image IO
participant Cache as ByteLRUCache
participant Prefetcher
participant Sidecar as SidecarManager
User->>QML: Open directory
QML->>Controller: load(directory)
Controller->>Indexer: find_images(dir)
Indexer-->>Controller: [ImageFile, ...]
Controller->>Sidecar: load()
Sidecar-->>Controller: Sidecar
Controller->>Controller: _load_image_at(0)
Controller->>Cache: Decode & cache image
Controller->>Prefetcher: update_prefetch(0)
Prefetcher-->>Cache: Async decode adjacent
Controller->>QML: currentIndexChanged, imageCountChanged
QML-->>User: Display image
User->>QML: Press next image
QML->>Controller: next_image()
Controller->>Controller: current_index++
Controller->>Prefetcher: update_prefetch(idx, is_navigation=true)
Controller->>QML: currentIndexChanged, metadataChanged
QML-->>User: Display next image
sequenceDiagram
actor User
participant QML as ImageEditorDialog
participant Controller as AppController
participant Editor as ImageEditor
participant Cache as ByteLRUCache
User->>QML: Open Editor
QML->>Controller: load_image_for_editing()
Controller->>Editor: load_image(current_path)
Editor-->>Controller: preview loaded
QML->>User: Show editor sliders
User->>QML: Adjust brightness
QML->>Controller: set_edit_parameter("brightness", value)
Controller->>Editor: set_edit_param("brightness", value)
Controller->>Editor: get_preview_data()
Editor-->>Controller: preview image
Controller->>Cache: Cache preview
Controller->>QML: currentEditedImageDataChanged
QML-->>User: Show edited preview
User->>QML: Save Edited Image
QML->>Controller: save_edited_image()
Controller->>Editor: save_image()
Editor-->>Controller: saved path
Controller->>Controller: reload image list & cache
Controller->>QML: imageListChanged
QML-->>User: Display saved image
Estimated code review effort🎯 5 (Critical) | ⏱️ ~150+ minutes Key areas requiring detailed review:
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: 32
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)
12-12: Fix typo in main README feature list.Same as the working-menus README, this should read “high-performance decoding”.
-- **Instant Navigation:** Sub-10ms next/previous image switching, high peformance decoding via `PyTurboJPEG`. +- **Instant Navigation:** Sub-10ms next/previous image switching, high-performance decoding via `PyTurboJPEG`.faststack/faststack/qml/Main.qml (1)
424-424: Inconsistent help text for remove from batch/stack.Line 424 states "X or S: Remove current image from batch/stack", but based on the keystrokes mapping, S is bound to
toggle_stack_membership, not removal. Only X should be listed here.Apply this diff:
- " X or S: Remove current image from batch/stack<br><br>" + + " X: Remove current image from batch/stack<br><br>" +faststack/faststack/app.py (1)
318-335: Fix type annotation and implementation ofAppController.get_preview_data()Verification confirms a real type mismatch:
AppController.get_preview_data()at line 1643 declares return typeOptional[bytes]with@Slot(result=bytes), but its implementation returns aDecodedImageobject fromself.image_editor.get_preview_data().
- ImageEditor.get_preview_data() (editor.py:215) returns
Optional[DecodedImage]and constructsDecodedImage(buffer=..., width=..., height=..., bytes_per_line=...).- AppController.get_preview_data() (app.py:1643) receives this
DecodedImageand returns it directly as if it werebytes, which will fail if QML calls this slot expecting byte data.The
get_decoded_image()method (lines 318-335) itself is correct—it callsself.image_editor.get_preview_data()directly and properly returnsDecodedImage.Required fix at app.py:1643: Either change the return type and
@Slotresult toDecodedImage, or serialize theDecodedImageto bytes and update the implementation accordingly to match the declared return type.
🧹 Nitpick comments (45)
faststack.working-menus/ChangeLog.md (2)
100-105: Consider more formal verb choice (LanguageTool suggestion).The phrasing "Fixed an issue where panning..." could use slightly more formal wording, though this is optional. LanguageTool suggests reconsidering the verb usage for style consistency.
116-122: Use hyphen for compound adjective (grammar fix).The phrase "full res image" should be hyphenated as "full-res image" when used as a compound adjective modifying "image". Additionally, "Make it use" would be more formal as a task statement.
Apply this diff for improved grammar:
### Todo -Make it use the full res image when zooming in +Make it use the full-res image when zooming in When multiple stacks are selected, call Helicon multiple timesfaststack.working-menus/faststack.egg-info/PKG-INFO (1)
3-25: Avoid committing stalePKG-INFOor clarify its role vs the 1.1 release.This metadata advertises
Version: 1.0and a “Version 1.0 - November 21, 2025” description while the main project is moving to 1.1. If this egg-info isn’t actually used for distribution, consider removing it from VCS; if it is used, it should be aligned with the main package version to avoid confusing installers/tools.faststack.working-menus/faststack/logging_setup.py (1)
21-37: Guard against adding duplicate file handlers on repeatedsetup_logging()calls.If
setup_logging()is called more than once in a process (e.g., tests, re-init), you’ll get duplicate log lines because a newRotatingFileHandleris always added to the root logger. It’s cheap to guard against this by checking for an existing handler targeting the same log file.- root_logger = logging.getLogger() - # Set log level based on debug flag - root_logger.setLevel(logging.DEBUG if debug else logging.INFO) - root_logger.addHandler(handler) + root_logger = logging.getLogger() + # Set log level based on debug flag + root_logger.setLevel(logging.DEBUG if debug else logging.INFO) + + # Avoid adding duplicate rotating file handlers for the same log file + if not any( + isinstance(h, logging.handlers.RotatingFileHandler) + and getattr(h, "baseFilename", None) == str(log_file) + for h in root_logger.handlers + ): + root_logger.addHandler(handler) </blockquote></details> <details> <summary>faststack.working-menus/faststack/qml/Main.qml.bak (2)</summary><blockquote> `120-130`: **Fix window drag logic to avoid exaggerated movement (likely copied into real Main.qml).** `onPositionChanged` always computes `delta` from the original press position but accumulates it on every move, so the window moves too far as the mouse moves. Either update `lastMousePos` each time or compute absolute positioning instead of incremental updates. ```diff - onPositionChanged: function(mouse) { - var delta = Qt.point(mouse.x - lastMousePos.x, mouse.y - lastMousePos.y) - root.x += delta.x - root.y += delta.y - } + onPositionChanged: function(mouse) { + var delta = Qt.point(mouse.x - lastMousePos.x, mouse.y - lastMousePos.y) + root.x += delta.x + root.y += delta.y + lastMousePos = Qt.point(mouse.x, mouse.y) + }Even though this is a
.bakfile, it’s worth fixing if you ever copy/paste from it.
241-259: Update about dialog key-bindings text to match current shortcuts.The help text still says
G: Toggle Grid View (not implemented), while the READMEs describeGas “Go to image #” and you now have a JumpToImageDialog. To avoid confusing users, either update this dialog to the current bindings or clearly mark it as legacy if it’s only kept as a backup.faststack/faststack/imaging/editor.py (2)
82-87: Replaceexcept Exceptionwith proper logging and narrower error handling.
load_image()andsave_image()swallow all exceptions and only+import logging @@ -from faststack.models import DecodedImage -from PySide6.QtGui import QImage +from faststack.models import DecodedImage +from PySide6.QtGui import QImage + +logger = logging.getLogger(__name__) @@ - except Exception as e: - print(f"Error loading image for editing: {e}") + except Exception: + # Log full stack trace; return False so callers can handle gracefully + logger.exception("Error loading image for editing: %s", filepath) self.original_image = None self._preview_image = None return False @@ - try: + try: @@ - except Exception as e: - print(f"Warning: Could not preserve EXIF data: {e}") + except Exception: + logger.exception("Warning: Could not preserve EXIF data for %s", original_path) # Fall back to saving without EXIF if there's an issue final_img.save(original_path, format='JPEG', quality=95) @@ - except Exception as e: - print(f"Failed to save edited image or backup: {e}") + except Exception: + logger.exception("Failed to save edited image or backup for %s", original_path) return NoneYou can further narrow the caught exceptions later if needed, but this at least gets failures into your log instead of stdout.
Also applies to: 270-293
245-285: Confirmsave_image()is only used for JPEGs, or guard against non-JPEG input.
save_image()always writesformat='JPEG'back tooriginal_path, regardless of the original extension. That’s fine if the editor is strictly for JPGs, but if it ever receives PNG/other formats, you’ll silently convert them to JPEG while keeping the old extension.Consider either:
- Asserting/validating that
original_path.suffixis a JPEG extension before saving, or- Deriving the save format from the original file type instead of hard-coding JPEG.
This will make the behavior explicit and avoid surprising conversions if usage expands beyond JPG.
faststack.working-menus/faststack/imaging/prefetch.py (3)
129-241: Prefetch window and scheduling look sound, but consider current‑index prioritization.The asymmetric window computation and scheduled set per generation look correct and avoid re‑submitting tasks. However,
submit_task’spriorityflag is never used here, so the currently viewed image isn’t explicitly prioritized over neighbors from this code path. If you rely on the prefetcher itself (rather than an external caller) to prioritize the current index, consider callingsubmit_task(current_index, self.generation, priority=True)or similar when appropriate.
87-128: Saturation compensation logic is correct; optional micro‑optimizations.The shape assertions and RGB reshaping are consistent with how
arr,w,h, andbytes_per_lineare constructed in this module, and the math for desaturation is correct. If you ever profile this path and find it hot, you could eliminate one reshape and the extra temporary fromastype(np.float32)by reusing buffers and in‑place operations, but it’s not essential right now.
272-460: Running verification scripts to inspect the cache implementation:Add thread-safety synchronization to cache operations and consider futures cleanup for stale generations.
The cachetools.LRUCache classes are not thread-safe, and access from multiple threads must be properly synchronized with a suitable lock object. Since
ByteLRUCacheextends this unsafe base class andcache_put(bound to__setitem__) is called directly from worker threads in_decode_and_cache, concurrent writes risk cache corruption.Recommendation:
- Wrap cache operations with a threading lock: add
self.cache_lock = threading.Lock()toPrefetcher.__init__, then usewith self.cache_lock: self.cache_put(...)at line 453.- For futures cleanup: consider clearing
self.futures[index]in_decode_and_cacheafter detecting a stale generation (lines 273–276, 441–443) to allow prompt re-submission of the current image on zoom/mode changes, rather than waiting for the old future to complete.faststack.working-menus/patch (2)
40-117: Edit application flow is clear; consider optional bounds‑checking and logging.The edit pipeline (crop → brightness → contrast → saturation → white balance) is straightforward and deterministic, and using PNG for previews avoids cumulative JPEG artifacts. You rely on the UI to keep normalized parameters in the intended ranges; if you expect any external callers, you might want to clamp
brightness/contrast/saturationto[-1, 1]andwhite_balance_*to the expected range, and consider logging whenload_imagefails instead of printing to stdout.
336-407: Controller↔editor wiring looks consistent; minor UX enhancements possible.
- Loading the current image into
ImageEditorand callingreset_edit_parameters()when the current image changes is a good reset point.- Slots
set_edit_parameterandset_crop_selection_normalizedcorrectly keep the Python editor state, previews, and QML properties in sync.Optional tweaks:
- In
set_crop_selection_normalized, you might also sethasActiveCropSelectionvia the model to ensure consistency if other code paths manage it.- In
save_or_discard_edits, consider logging whensave_changesis true butsave_image()fails, so failures surface beyond the UI.faststack/ChangeLog.md (1)
3-31: Verify 1.1.0 release notes match actual shortcuts and behaviorThe 1.1.0 notes (E to open editor, Ctrl+S to save, A for auto WB, B for batch toggle) look clear, but they’re tightly coupled to UI logic and key handling elsewhere. Please double-check that:
- The documented keys match what
Main.qml/ controller actually bind.- Auto-WB and editor behavior (auto-save, undo, backup naming) match what’s implemented.
This keeps the ChangeLog trustworthy for users.
faststack.working-menus/faststack/benchmark_decode.py (1)
1-20: Make benchmark script safe to import and path-configurableRight now the benchmark runs on import and hard‑codes a Windows‑specific path, which makes accidental imports brittle and limits reuse.
Consider refactoring to something like:
import argparse def main(): parser = argparse.ArgumentParser() parser.add_argument("image", type=Path) parser.add_argument("--iterations", type=int, default=20) args = parser.parse_args() print(f"TurboJPEG available: {TURBO_AVAILABLE}") start = time.perf_counter() for _ in range(args.iterations): with open(args.image, "rb") as f: with mmap.mmap(f.fileno(), 0, access=mmap.ACCESS_READ) as mmapped: jpeg_bytes = mmapped[:] decode_jpeg_resized(jpeg_bytes, 1920, 1080) elapsed = time.perf_counter() - start print(f"Average time (with mmap): {elapsed/args.iterations*1000:.1f}ms") if __name__ == "__main__": main()This keeps the benchmark useful without impacting the package if imported.
faststack.working-menus/faststack/tests/test_pairing.py (1)
36-74: Pairing tests cover key scenarios; consider minor style cleanupsThe tests exercise:
- Directory scan ordering and pairing (
test_find_images)._find_raw_pairbehavior for perfect match, out‑of‑range delta, “closest of many”, and empty candidates.This lines up well with the indexer implementation and should catch regressions in RAW–JPG pairing.
For Ruff’s E702 warnings and readability, you might optionally split the semicolon‑separated statements, e.g.:
jpg_stat = MagicMock() jpg_stat.st_mtime = 1000.0instead of keeping them on one line.
faststack.working-menus/faststack/models.py (1)
7-44: Unify Optional/union style and confirm sidecar metadata fieldsTwo small points here:
- Type hints currently use
Optional[...]andDict/List, while other modules (e.g._find_raw_pair) are already on thePath | Noneand builtindict[...]/list[...]style. To keep Ruff (RUF013) and style consistent, consider:from __future__ import annotations # if not already at module top elsewhere class ImageFile: path: Path raw_pair: Path | None = None ... class EntryMetadata: stack_id: int | None = None stacked_date: str | None = None ...
- Tests and example sidecar JSON refer to
flagandrejectfields onEntryMetadata. If those are still part of the sidecar schema, they should probably be added here; if they’ve been removed in v2, the tests/loader should be updated accordingly.faststack.working-menus/faststack/tests/test_executable_validator.py (1)
40-41: Clean up unused lambda args/variables and consider asserting on log warnings.
- Lines 40, 62, and 123: the
lambda self: ...assignments for__str__trigger Ruff’s ARG005 (“unused lambda argument”). If you want to keep this approach, preferlambda _self: ...or, better, configure the MagicMock directly:mock_path_instance.__str__.return_value = photoshop_pathThis avoids the unused-parameter warning and aligns with how
MagicMockis typically customized.
Lines 66 and 127:
erroris unpacked but never used. If you just need to keep the tuple shape, rename to_erroror_to satisfy Ruff (RUF059) and document intent.For
test_suspicious_path_with_traversal, the docstring mentions that suspicious paths are “flagged”, but the test only checksis_validand not that a warning was actually logged. If you want stronger coverage, you could usecaplogto assert that a warning about a suspicious path was emitted.These changes are small but will keep the test suite cleaner and closer to what static analysis and the docstrings imply.
Also applies to: 62-63, 66-66, 123-124, 127-127
faststack.working-menus/faststack/imaging/cache.py (1)
10-45: Alignget_decoded_image_sizedocstring with actual DecodedImage usage and consider logging on the fallback path.
get_decoded_image_size()currently documents “decoded image tuple (buffer, qimage)” but in practice it operates onDecodedImageinstances (imported fromfaststack.models). That mismatch can be confusing when someone reads the docstring or reuses this helper.Consider tightening this up:
def get_decoded_image_size(item) -> int: """Return the size in bytes of a DecodedImage buffer.""" from faststack.models import DecodedImage if isinstance(item, DecodedImage): ... else: # Log unexpected type so we notice misuses log.warning("ByteLRUCache received non-DecodedImage item of type %s", type(item)) return 1This makes the expected type explicit and turns the “should not happen” path into something that’s observable when it does happen.
faststack.working-menus/faststack/qml/JumpToImageDialog.qml (1)
32-37: Guard the Ok button against invalid/empty input so the dialog doesn’t silently close without jumping.Right now
onAcceptedonly callscontroller.jump_to_image(...)when the parsed value is within[1, maxImageCount], but the dialog still closes even if the field is empty or out of range. From a user’s perspective that looks like “Ok” did nothing.A simple way to tighten this up is to tie the Ok button’s enabled state to the validator:
- If you’re using the default
standardButtons, you can add a footer with an explicit OkButton:footer: DialogButtonBox { standardButtons: Dialog.Cancel Button { text: "OK" enabled: imageNumberField.acceptableInput onClicked: jumpDialog.accept() } }or, if you stick with
standardButtons, you can bindenabledon theDialogButtonBox’s Ok button viacomponent.onCompletedlookup.That way, “Ok” is only clickable when the input passes
IntValidator, and you avoid the “dialog closes but nothing happens” UX trap.Also applies to: 54-72
faststack.working-menus/faststack/qml/Components.qml (1)
86-123: Drag-outside detection is tied toloupeView’s size; consider anchoring to the Window for robustness.The drag-and-drop trigger checks:
var globalPos = mapToItem(null, mouse.x, mouse.y) if (globalPos.x < 0 || globalPos.y < 0 || globalPos.x > loupeView.width || globalPos.y > loupeView.height) { isDraggingOutside = true controller.start_drag_current_image() }This works as long as
loupeViewfills the top-level window starting at (0,0). If layout changes (padding, nested containers, multi-window),loupeView.width/heightmay no longer match the window’s bounds, and the “outside” check will misfire.If you want this behavior to be resilient, you might:
- Map to the actual
Windowitem instead ofnulland compare with that window’s width/height, or- Expose the window size via
uiStateor a rootid(e.g.,rootWindow) and compare against that.That keeps the drag-outside heuristic correct even if the viewer layout evolves.
faststack.working-menus/faststack/config.py (1)
58-67: Uselogging.exceptionon save failures to capture stack traces.In
save(), IO failures are logged withlog.error, which drops the traceback:except IOError as e: log.error(f"Failed to save config to {self.config_path}: {e}")Switching to
log.exceptionhere will retain the same message but also record the stack trace, which is usually what you want when config persistence fails:except IOError: log.exception("Failed to save config to %s", self.config_path)That should satisfy the Ruff hint and make diagnosing config write problems easier.
faststack.working-menus/faststack/io/helicon.py (2)
70-70: Remove redundant exception object from log.exception call.The
log.exception()method automatically includes exception information, so explicitly passing{e}is redundant.Apply this diff:
- log.exception(f"Invalid helicon args format: {e}") + log.exception("Invalid helicon args format")
86-91: Remove redundant exception objects from log.exception calls.The
log.exception()method automatically captures and logs exception information, so explicitly including{e}in the message is redundant.Apply this diff:
- log.exception(f"Failed to launch Helicon Focus: {e}") + log.exception("Failed to launch Helicon Focus") return False, None - except (IOError, PermissionError) as e: - log.exception(f"Failed to create temporary file for Helicon Focus: {e}") + except (IOError, PermissionError): + log.exception("Failed to create temporary file for Helicon Focus") return False, Nonefaststack.working-menus/faststack/io/indexer.py (1)
64-84: Remove unused jpg_path parameter.The
jpg_pathparameter in_find_raw_pairis never used in the function body. It can be removed to simplify the signature.Apply this diff:
def _find_raw_pair( - jpg_path: Path, jpg_stat: os.stat_result, potential_raws: List[Tuple[Path, os.stat_result]] ) -> Path | None: """Finds the best RAW pair for a JPG from a list of candidates."""And update the call site:
image_files: List[ImageFile] = [] for jpg_path, jpg_stat in jpgs: - raw_pair = _find_raw_pair(jpg_path, jpg_stat, raws.get(jpg_path.stem, [])) + raw_pair = _find_raw_pair(jpg_stat, raws.get(jpg_path.stem, [])) image_files.append(ImageFile(faststack.working-menus/faststack/io/executable_validator.py (1)
106-112: Consider restructuring exception handling for clarity.The
return Truestatement could be moved to anelseblock for clearer control flow, though the current implementation is functionally correct.Apply this diff:
def _is_subpath(path: Path, parent: Path) -> bool: """Check if path is a subpath of parent.""" try: path.resolve().relative_to(parent.resolve()) - return True except (ValueError, RuntimeError): return False + else: + return Truefaststack.working-menus/faststack/io/sidecar.py (2)
57-60: Use log.exception instead of log.error in exception handler.When logging within an exception handler,
log.exception()is preferred overlog.error()as it automatically includes the stack trace, which aids debugging.Apply this diff:
except (json.JSONDecodeError, TypeError) as e: - log.error(f"Failed to load or parse sidecar file {self.path}: {e}") + log.exception(f"Failed to load or parse sidecar file {self.path}") # Consider backing up the corrupted file here return Sidecar()
87-89: Use log.exception instead of log.error in exception handler.When logging within an exception handler,
log.exception()is preferred as it automatically includes the stack trace.Apply this diff:
except (IOError, TypeError) as e: - log.error(f"Failed to save sidecar file {self.path}: {e}") + log.exception(f"Failed to save sidecar file {self.path}")faststack.working-menus/faststack/qml/SettingsDialog.qml (2)
18-25: Cache usage Timer behaves correctly; consider eager refresh on openThe Timer wiring looks good and stops when the dialog closes. If you want to avoid showing
0.00 GBfor the first second, you could also refreshcacheUsageimmediately when the dialog becomes visible:onVisibleChanged: { cacheUsageTimer.running = visible if (visible) { + cacheUsage = uiState.get_cache_usage_gb() controller.dialog_opened() } else { controller.dialog_closed() } }Also applies to: 164-171
59-73: Path existence checks may be a bit aggressive
visible: uiState.check_path_exists(heliconPathField.text)/photoshopPathField.textre-run the check on every text change. Ifcheck_path_exists()is more than a cheap local stat call, consider debouncing (e.g., checking ononEditingFinishedor via a short Timer) to avoid blocking the UI while typing.Also applies to: 83-97
faststack.working-menus/faststack/imaging/jpeg.py (1)
29-30: Simplifylogging.exceptioncalls to match Ruff’s TRY401 guidance
logging.exception()already logs the active exception and stack trace; interpolatingeinto the message is redundant and triggers TRY401. You can keep the context while simplifying the calls:- except Exception as e: - log.exception(f"PyTurboJPEG failed to decode image: {e}. Trying Pillow.") + except Exception: + log.exception("PyTurboJPEG failed to decode image. Trying Pillow.")Apply the same pattern to the other
logging.exceptionsites in this module (Pillow also failed..., thumbnail, resized path) to quiet TRY401 and keep logs cleaner.Also applies to: 38-39, 66-67, 75-76, 129-130, 147-148
faststack/faststack/qml/ImageEditorDialog.qml.old (1)
1-196: Consider dropping this.qml.oldsnapshot from the repoThis appears to be an older copy of the Image Editor dialog, with the real implementation now in
ImageEditorDialog.qml.new. Since.qml.oldwon’t be loaded by the engine and largely duplicates logic, it’s probably better to delete it (or move it to docs/notes) to avoid confusion about which editor QML is authoritative.faststack/faststack/qml/ImageEditorDialog.qml.new (1)
136-218: Slider wiring looks solid; optionally guard against negativeslidersPressedCountThe
editSlidercomponent’s backend binding and reversal logic look consistent, and the updated visuals are nice. To make the globalslidersPressedCounta bit more defensive, you could avoid ever letting it go negative in case of unexpected press/release sequences:- onPressedChanged: { - if (pressed) editDialog.slidersPressedCount++; else editDialog.slidersPressedCount--; - } + onPressedChanged: { + if (pressed) { + editDialog.slidersPressedCount++; + } else if (editDialog.slidersPressedCount > 0) { + editDialog.slidersPressedCount--; + } + }faststack.working-menus/faststack/ui/provider.py (1)
28-36: Silence Ruff ARG002 by marking unused requestImage parameters
sizeandrequestedSizeare unused but required byQQuickImageProvider.requestImage’s signature. To keep the override correct and appease Ruff, consider renaming them to underscore-prefixed names.- def requestImage(self, id: str, size: object, requestedSize: object) -> QImage: + def requestImage(self, id: str, _size: object, _requestedSize: object) -> QImage:faststack/faststack/ui/provider.py (2)
87-140: Editor-related state wiring is coherent; consider minor cleanupThe added editor signals/fields (_is_editor_open, crop, tone/contrast/color params, etc.) line up with the QML keys and give a clear, one-to-one bridge between backend edits and UI.
Two small follow‑ups to consider:
default_directory_changedis declared but never used by any property; either wire it to a@Propertyor drop it.reset_editor_state()resets all edit parameters and cropping, but notanySliderPressedorisEditorOpen. If you ever reuse it outside the “editor just closed” path, it might be worth explicitly clearing those too.def reset_editor_state(self): """Resets all UI state variables for the editor.""" + self.anySliderPressed = False self.brightness = 0.0 ... self.clarity = 0.0
28-61: Mirror Ruff fixes from working-menus provider here as wellSame pattern as the working-menus ImageProvider:
size/requestedSizeare unused but required by the Qt override, and the invalid ID handler would benefit fromlog.exceptionfor full tracebacks.- def requestImage(self, id: str, size: object, requestedSize: object) -> QImage: + def requestImage(self, id: str, _size: object, _requestedSize: object) -> QImage: ... - except (ValueError, IndexError) as e: - log.error(f"Invalid image ID requested from QML: {id}. Error: {e}") + except (ValueError, IndexError): + log.exception("Invalid image ID requested from QML: %s", id)faststack/faststack/app.py (3)
624-642:reset_editor_stateintegration: consider clearing editor-open state as well
reset_edit_parameters()usesui_state.reset_editor_state()to reset sliders/crop, andsave_edited_image()callsreset_edit_parameters()after saving. That’s good for visual consistency.If the intended UX after a full save is “editor remains open but parameters snap back to neutral”, current behavior is fine. If instead you want saving to conceptually “finish editing”, consider also toggling
ui_state.isEditorOpenoff here or at the caller, soget_decoded_imagestops returning the editor preview and normal cache behavior resumes immediately.Not strictly required, but worth confirming UX.
1188-1192: Avoid redundantimport timeinsidedelete_current_image
timeis already imported at module level. The innerimport timeinsidedelete_current_imageis redundant and can be dropped:- if deleted_files: - import time - timestamp = time.time() + if deleted_files: + timestamp = time.time() self.delete_history.append((jpg_path, raw_path)) self.undo_history.append(("delete", (jpg_path, raw_path), timestamp))Minor, but it keeps imports centralized.
1919-1955: QML import-path setup is reasonable; watch for environment interactionsThe new environment and import-path setup:
- Sets
QML2_IMPORT_PATHrelative to__file__.- Adds PySide6’s
qmldirectory,qrc:/qt-project.org/imports, your ownqmldirectory, andQt5Compatintoengine.addImportPath.This should help resolve
Qt5Compat.GraphicalEffectsand custom imports across platforms. Just be mindful if callers also setQML2_IMPORT_PATHbefore launching FastStack—your override will win. If that’s a concern, you might want to append to any existing value instead of blindly replacing it.faststack.working-menus/faststack/app.py (4)
16-45: DuplicateImageProviderimport
ImageProvideris imported twice (once withUIStateat Line 16 and again alone at Line 43). The second import is redundant and triggers Ruff F811.You can safely drop the second import:
-from faststack.ui.provider import ImageProvider
643-678: Clean up f-strings without interpolation and unused locals in batch/stack helpersThere are a couple of small Ruff issues in this block:
self.update_status_message(f"Removed from batch")self.update_status_message(f"Removed from stack")These are plain strings; the
fprefix is unnecessary (F541). Also, in_get_batch_info, the loop indexiand thecount_in_batch/pos_in_batchlocals are never used (B007/F841).Suggested minimal clean-up:
- self.update_status_message(f"Removed from batch") + self.update_status_message("Removed from batch") ... - self.update_status_message(f"Removed from stack") + self.update_status_message("Removed from stack")And in
_get_batch_info:- for i, (start, end) in enumerate(self.batches): + for start, end in self.batches: if start <= index <= end: - count_in_batch = end - start + 1 - pos_in_batch = index - start + 1 info = "In Batch" breakThese changes are behavior‑preserving and quiet the lints.
907-914: Preferlog.exceptionin exception handlers for better diagnosticsThe OSError/ValueError handlers currently use
log.error, which drops the traceback:except OSError as e: log.error("Error deleting temporary file %s: %s", tmp_path, e)Since these paths are rare and debugging them benefits from full context, consider
log.exception, which includes the stack trace while keeping the message:- except OSError as e: - log.error("Error deleting temporary file %s: %s", tmp_path, e) + except OSError: + log.exception("Error deleting temporary file %s", tmp_path)Likewise for delete/restore failures and invalid Photoshop args. Not required for correctness, but improves supportability.
Also applies to: 1120-1127, 1292-1299, 1351-1358
1506-1531: Tighten_get_batch_infoimplementation
_get_batch_infocurrently computescount_in_batchandpos_in_batchbut only returns"In Batch", and Ruff flags the unused locals and loop index. Since you’re not showing position/count in the footer anymore, you can simplify:- def _get_batch_info(self, index: int) -> str: - """Get batch info for the given index.""" - info = "" - 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 + def _get_batch_info(self, index: int) -> str: + """Get batch info for the given index.""" + info = "" + for start, end in self.batches: + if start <= index <= end: + info = "In Batch" + breakBehavior stays identical, and the lints go away.
faststack/patch (2)
21-155: ImageEditor core logic is reasonable, but consider stronger clamping and error reportingThe editing pipeline (crop → brightness/contrast/saturation → white balance BY/MG → PNG preview / JPEG save) looks coherent and matches the normalized
[-1.0, 1.0]UI model.A couple of non‑blocking improvements:
- Clamp the normalized parameters defensively in
set_edit_param(e.g., to[-1.0, 1.0]) so misuse from non‑QML callers can’t drive brightness/contrast factors far outside[0, 2].- Replace bare
load_image/save_imageerror paths withloggingso failures are visible in the same log pipeline as the rest of the app.Example for clamping:
def set_edit_param(self, key: str, value: float) -> bool: """Update a single edit parameter (normalized -1.0 to 1.0).""" - if key in self.current_edits and key != 'crop_box': - self.current_edits[key] = value + if key in self.current_edits and key != 'crop_box': + clamped = max(-1.0, min(1.0, value)) + self.current_edits[key] = clamped return True
607-745: QML edit/crop UX is generally solid; verify crop normalization vs image letterboxingThe main QML changes wire together:
Ekey toggling edit mode with a save/discard prompt whenedits_pendingis true.- ENTER key to either execute crop (via
app_controller.execute_crop()) or confirm the save/discard dialog.- Conditional image source based on
appModel.isEditing, switching to the edited provider.- Mouse-based crop selection normalized against
mainImage.width/heightand sent to Python viaset_crop_selection_normalized.- A red rectangle overlay that tracks the selection, plus a clear
MessageDialogfor save/discard.Two edge cases to double‑check in practice:
Letterboxing: If
mainImageusesfillMode: Image.PreserveAspectFit(or similar), the actual drawn image may be letterboxed withinmainImage.width/height. Normalizingmouse.x/mouse.yby the fullmainImagesize will include the black bars, so crop coordinates won’t map 1:1 to the underlying image pixels. You may need to account for the visible image rect (subtracting margins) before normalizing.Edited source refresh: This ties back to the provider comment – if caching prevents
requestPixmapfrom being called on everycurrentEditedImageDataChanged, the user will see stale edited previews despite the QML wiring being correct.Functionally the flow is well thought out; just verify these two behaviors at runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (53)
faststack.working-menus/ChangeLog.md(1 hunks)faststack.working-menus/LICENSE(1 hunks)faststack.working-menus/README.md(1 hunks)faststack.working-menus/faststack.egg-info/PKG-INFO(1 hunks)faststack.working-menus/faststack.egg-info/SOURCES.txt(1 hunks)faststack.working-menus/faststack.egg-info/dependency_links.txt(1 hunks)faststack.working-menus/faststack.egg-info/entry_points.txt(1 hunks)faststack.working-menus/faststack.egg-info/requires.txt(1 hunks)faststack.working-menus/faststack.egg-info/top_level.txt(1 hunks)faststack.working-menus/faststack/app.py(1 hunks)faststack.working-menus/faststack/benchmark_decode.py(1 hunks)faststack.working-menus/faststack/config.py(1 hunks)faststack.working-menus/faststack/imaging/cache.py(1 hunks)faststack.working-menus/faststack/imaging/jpeg.py(1 hunks)faststack.working-menus/faststack/imaging/prefetch.py(1 hunks)faststack.working-menus/faststack/io/executable_validator.py(1 hunks)faststack.working-menus/faststack/io/helicon.py(1 hunks)faststack.working-menus/faststack/io/indexer.py(1 hunks)faststack.working-menus/faststack/io/sidecar.py(1 hunks)faststack.working-menus/faststack/io/watcher.py(1 hunks)faststack.working-menus/faststack/logging_setup.py(1 hunks)faststack.working-menus/faststack/models.py(1 hunks)faststack.working-menus/faststack/qml/Components.qml(1 hunks)faststack.working-menus/faststack/qml/FilterDialog.qml(1 hunks)faststack.working-menus/faststack/qml/JumpToImageDialog.qml(1 hunks)faststack.working-menus/faststack/qml/Main.qml(1 hunks)faststack.working-menus/faststack/qml/Main.qml.bak(1 hunks)faststack.working-menus/faststack/qml/SettingsDialog.qml(1 hunks)faststack.working-menus/faststack/tests/test_cache.py(1 hunks)faststack.working-menus/faststack/tests/test_executable_validator.py(1 hunks)faststack.working-menus/faststack/tests/test_pairing.py(1 hunks)faststack.working-menus/faststack/tests/test_sidecar.py(1 hunks)faststack.working-menus/faststack/ui/keystrokes.py(1 hunks)faststack.working-menus/faststack/ui/keystrokes.py.bak(1 hunks)faststack.working-menus/faststack/ui/provider.py(1 hunks)faststack.working-menus/faststack/ui/provider.py.bak(1 hunks)faststack.working-menus/patch(1 hunks)faststack.working-menus/pyproject.toml(1 hunks)faststack.working-menus/requirements.txt(1 hunks)faststack/ChangeLog.md(1 hunks)faststack/README.md(4 hunks)faststack/faststack.egg-info/PKG-INFO(2 hunks)faststack/faststack/app.py(18 hunks)faststack/faststack/imaging/editor.py(1 hunks)faststack/faststack/qml/Components.qml(1 hunks)faststack/faststack/qml/ImageEditorDialog.qml(1 hunks)faststack/faststack/qml/ImageEditorDialog.qml.new(1 hunks)faststack/faststack/qml/ImageEditorDialog.qml.old(1 hunks)faststack/faststack/qml/Main.qml(7 hunks)faststack/faststack/ui/keystrokes.py(1 hunks)faststack/faststack/ui/provider.py(3 hunks)faststack/patch(1 hunks)faststack/pyproject.toml(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (11)
faststack.working-menus/faststack/imaging/cache.py (1)
faststack.working-menus/faststack/models.py (1)
DecodedImage(35-44)
faststack.working-menus/faststack/ui/keystrokes.py (1)
faststack/faststack/ui/keystrokes.py (3)
Keybinder(7-110)_call(61-74)handle_key_press(76-110)
faststack.working-menus/faststack/tests/test_cache.py (1)
faststack.working-menus/faststack/imaging/cache.py (1)
ByteLRUCache(10-28)
faststack.working-menus/faststack/io/helicon.py (2)
faststack.working-menus/faststack/io/executable_validator.py (1)
validate_executable_path(23-95)faststack.working-menus/faststack/config.py (1)
get(68-69)
faststack.working-menus/faststack/config.py (1)
faststack.working-menus/faststack/logging_setup.py (1)
get_app_data_dir(8-13)
faststack/faststack/imaging/editor.py (2)
faststack.working-menus/faststack/models.py (1)
DecodedImage(35-44)faststack/faststack/app.py (2)
get_preview_data(1643-1648)set_crop_box(1663-1668)
faststack.working-menus/faststack/imaging/prefetch.py (4)
faststack.working-menus/faststack/models.py (2)
ImageFile(8-12)DecodedImage(35-44)faststack.working-menus/faststack/imaging/jpeg.py (2)
decode_jpeg_rgb(22-40)decode_jpeg_resized(99-149)faststack.working-menus/faststack/config.py (1)
get(68-69)faststack.working-menus/faststack/app.py (2)
get_display_info(183-186)shutdown(1224-1259)
faststack.working-menus/faststack/benchmark_decode.py (1)
faststack.working-menus/faststack/imaging/jpeg.py (1)
decode_jpeg_resized(99-149)
faststack.working-menus/faststack/tests/test_pairing.py (1)
faststack.working-menus/faststack/io/indexer.py (2)
find_images(20-62)_find_raw_pair(64-84)
faststack.working-menus/faststack/io/sidecar.py (3)
faststack.working-menus/faststack/models.py (2)
Sidecar(27-32)EntryMetadata(15-23)faststack.working-menus/faststack/app.py (1)
load(269-282)faststack.working-menus/faststack/io/watcher.py (3)
stop(64-70)start(49-62)is_alive(72-74)
faststack.working-menus/faststack/ui/provider.py (3)
faststack.working-menus/faststack/models.py (1)
DecodedImage(35-44)faststack.working-menus/faststack/app.py (4)
get_decoded_image(313-370)get_current_metadata(519-550)get_color_mode(987-989)get_saturation_factor(1025-1027)faststack.working-menus/faststack/config.py (1)
get(68-69)
🪛 LanguageTool
faststack.working-menus/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.0 - November 21, 2025 # By Alan Rockefeller Ultra-fast, cach...
(MISSING_COMMA_AFTER_YEAR)
[grammar] ~12-~12: Ensure spelling is correct
Context: ...0ms next/previous image switching, high peformance decoding via PyTurboJPEG. - **Zoom & ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
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.1 - November 22, 2025 # By Alan Rockefeller Ultra-fast, cach...
(MISSING_COMMA_AFTER_YEAR)
faststack.working-menus/ChangeLog.md
[style] ~103-~103: Consider using a different verb for a more formal wording.
Context: ...he JPEG scaling factor calculation. - Fixed an issue where panning the image was no...
(FIX_RESOLVE)
[grammar] ~118-~118: Use a hyphen to join words.
Context: ...sion 0.4 ### Todo Make it use the full res image when zooming in When multiple ...
(QB_NEW_EN_HYPHEN)
🪛 Ruff (0.14.5)
faststack.working-menus/faststack/io/helicon.py
70-70: Redundant exception object included in logging.exception call
(TRY401)
77-77: subprocess call: check for execution of untrusted input
(S603)
85-85: Consider moving this statement to an else block
(TRY300)
87-87: Redundant exception object included in logging.exception call
(TRY401)
90-90: Redundant exception object included in logging.exception call
(TRY401)
faststack.working-menus/faststack/config.py
66-66: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack.working-menus/faststack/tests/test_executable_validator.py
40-40: Unused lambda argument: self
(ARG005)
62-62: Unused lambda argument: self
(ARG005)
66-66: Unpacked variable error is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
123-123: Unused lambda argument: self
(ARG005)
127-127: Unpacked variable error is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
faststack/faststack/imaging/editor.py
81-81: Consider moving this statement to an else block
(TRY300)
82-82: Do not catch blind exception: Exception
(BLE001)
285-285: Do not catch blind exception: Exception
(BLE001)
290-290: Consider moving this statement to an else block
(TRY300)
291-291: Do not catch blind exception: Exception
(BLE001)
faststack.working-menus/faststack/io/executable_validator.py
110-110: Consider moving this statement to an else block
(TRY300)
faststack.working-menus/faststack/imaging/jpeg.py
30-30: Redundant exception object included in logging.exception call
(TRY401)
39-39: Redundant exception object included in logging.exception call
(TRY401)
65-65: Consider moving this statement to an else block
(TRY300)
67-67: Redundant exception object included in logging.exception call
(TRY401)
76-76: Redundant exception object included in logging.exception call
(TRY401)
130-130: Redundant exception object included in logging.exception call
(TRY401)
148-148: Redundant exception object included in logging.exception call
(TRY401)
faststack.working-menus/faststack/imaging/prefetch.py
455-455: Consider moving this statement to an else block
(TRY300)
faststack.working-menus/faststack/tests/test_sidecar.py
14-14: PEP 484 prohibits implicit Optional
Convert to T | None
(RUF013)
faststack.working-menus/faststack/io/indexer.py
39-39: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
65-65: Unused function argument: jpg_path
(ARG001)
faststack.working-menus/faststack/app.py
43-43: Redefinition of unused ImageProvider from line 16
Remove definition: ImageProvider
(F811)
364-364: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
644-644: f-string without any placeholders
Remove extraneous f prefix
(F541)
678-678: f-string without any placeholders
Remove extraneous f prefix
(F541)
913-913: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1124-1124: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1297-1297: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1357-1357: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1366-1366: subprocess call: check for execution of untrusted input
(S603)
1522-1522: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
1524-1524: Local variable count_in_batch is assigned to but never used
Remove assignment to unused variable count_in_batch
(F841)
1525-1525: Local variable pos_in_batch is assigned to but never used
Remove assignment to unused variable pos_in_batch
(F841)
faststack.working-menus/faststack/tests/test_pairing.py
55-55: Multiple statements on one line (semicolon)
(E702)
58-58: Multiple statements on one line (semicolon)
(E702)
58-58: Multiple statements on one line (semicolon)
(E702)
63-63: Multiple statements on one line (semicolon)
(E702)
63-63: Multiple statements on one line (semicolon)
(E702)
68-68: Multiple statements on one line (semicolon)
(E702)
68-68: Multiple statements on one line (semicolon)
(E702)
69-69: Multiple statements on one line (semicolon)
(E702)
69-69: Multiple statements on one line (semicolon)
(E702)
faststack/faststack/app.py
137-137: Undefined name Tuple
(F821)
655-655: f-string without any placeholders
Remove extraneous f prefix
(F541)
689-689: f-string without any placeholders
Remove extraneous f prefix
(F541)
1818-1818: Local variable b_diff is assigned to but never used
Remove assignment to unused variable b_diff
(F841)
1881-1881: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
faststack.working-menus/faststack/io/sidecar.py
58-58: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
88-88: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack.working-menus/faststack/ui/provider.py
28-28: Unused method argument: size
(ARG002)
28-28: Unused method argument: requestedSize
(ARG002)
64-64: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
🧩 Analysis chain
Verify that generated build artifacts are excluded from version control.
The egg-info/ directory contains auto-generated Python packaging metadata and should typically not be committed to version control. These directories are created during package installation and should be excluded via .gitignore.
Please verify that your .gitignore file includes entries to exclude build artifacts:
If egg-info directories are present in the repository, consider removing them with git rm -r --cached faststack.working-menus/faststack.egg-info/ and updating .gitignore.
🏁 Script executed:
#!/bin/bash
# Description: Verify that egg-info and other build artifacts are excluded from version control
# Check if .gitignore exists and contains egg-info exclusions
if [ -f .gitignore ]; then
echo "=== Checking .gitignore for build artifact exclusions ==="
grep -E '(egg-info|build|dist|\.eggs)' .gitignore || echo "WARNING: No egg-info or build artifact exclusions found"
else
echo "WARNING: .gitignore file not found"
fi
# List all egg-info directories currently in the repository
echo ""
echo "=== Egg-info directories in repository ==="
fd -t d 'egg-info$' || echo "No egg-info directories found"Length of output: 188
Add egg-info/ to .gitignore and remove committed build artifacts.
The egg-info/ directories are auto-generated Python packaging metadata that should not be committed to version control. Your .gitignore currently excludes dist/ and build/ but is missing egg-info.
Remove the committed directories and update .gitignore:
egg-info
*.egg-info/
.eggs/
Then run:
git rm -r --cached faststack/faststack.egg-info/ faststack.working-menus/faststack.egg-info/
🤖 Prompt for AI Agents
faststack.working-menus/faststack.egg-info/dependency_links.txt around lines 1
to 1: the repo contains committed egg-info build artifacts that should be
ignored; update .gitignore to add entries for egg-info, *.egg-info/, and .eggs/
and then remove the committed egg-info directories from the repository by
removing them from the git index (e.g., use git rm -r --cached on
faststack/faststack.egg-info/ and faststack.working-menus/faststack.egg-info/),
then commit the updated .gitignore and the removal so the generated metadata is
no longer tracked.
| [console_scripts] | ||
| faststack = faststack.app:main |
There was a problem hiding this comment.
🧩 Analysis chain
Confirm alignment between this entry point and pyproject/CLI design.
This wires the faststack console script to faststack.app:main, while your pyproject.toml (per summary) also defines a faststack entry pointing to faststack.app:cli. Having two different targets for the same script name across metadata sources can be confusing.
Decide whether faststack should launch the GUI (main) or CLI (cli) and keep that consistent in all packaging metadata (pyproject, setup.cfg, egg‑info generation, etc.).
🏁 Script executed:
#!/bin/bash
# Check for other console script definitions for 'faststack'
rg -n "console_scripts" -C3
rg -n "faststack.app:cli" -C3
rg -n "faststack.app:main" -C3Length of output: 1328
Resolve the faststack entry point mismatch between pyproject.toml and egg-info.
faststack.working-menus/pyproject.toml:31 defines faststack = "faststack.app:cli", while faststack.working-menus/faststack.egg-info/entry_points.txt:2 defines faststack = faststack.app:main. These must align. The egg-info file is auto-generated from pyproject.toml during package installation; verify which target is correct and ensure pyproject.toml reflects it, so the next build regenerates entry_points.txt consistently.
🤖 Prompt for AI Agents
In faststack.working-menus/faststack.egg-info/entry_points.txt lines 1-2, the
console script is defined as faststack = faststack.app:main but pyproject.toml
defines faststack = "faststack.app:cli"; pick the correct callable (either
app:cli or app:main), update pyproject.toml to match that target, then
rebuild/reinstall the package so setuptools regenerates
faststack.egg-info/entry_points.txt with the same entry point; verify by running
the installed console script and adjust the callable name in the code if
necessary.
| PySide6<7.0,>=6.0 | ||
| PyTurboJPEG<2.0,>=1.8 | ||
| numpy<3.0,>=2.0 | ||
| cachetools<6.0,>=5.0 | ||
| watchdog<5.0,>=4.0 | ||
| typer<1.0,>=0.12 | ||
| Pillow<11.0,>=10.0 | ||
| pytest<9.0,>=8.0 |
There was a problem hiding this comment.
🧩 Analysis chain
Pinned dependency ranges look coherent; validate against your target environments.
The version constraints for PySide6, PyTurboJPEG, numpy, Pillow, etc., are modern and consistent with the features used in this PR. Just ensure:
- These ranges match what’s declared in
pyproject.toml/other packaging files. - Your supported platforms (Python version, OS) are tested against at least one combination in these ranges, especially numpy/Pillow + PySide6.
No changes strictly required here; this is more of a deployment sanity check.
🏁 Script executed:
#!/bin/bash
# Compare requires.txt with pyproject.toml for drift
fd pyproject.toml -x cat {}
cat faststack.working-menus/faststack.egg-info/requires.txtLength of output: 1710
Dependency drift detected: typer is declared in requires.txt but missing from pyproject.toml.
The requires.txt includes typer<1.0,>=0.12, which does not appear in either pyproject.toml configuration (version 1.0 or 1.1). This mismatch must be resolved:
- If
typeris a genuine runtime dependency, add it topyproject.tomldependencies. - If
typeris not needed, regeneraterequires.txtfrompyproject.tomlto ensure consistency.
Verify whether typer is actually used in the codebase, then align the two configuration sources.
🤖 Prompt for AI Agents
In faststack.working-menus/faststack.egg-info/requires.txt around lines 1-8,
requires.txt lists "typer<1.0,>=0.12" but pyproject.toml (both v1.0 and v1.1)
lacks this dependency; inspect the codebase to confirm whether typer is actually
used and then either add typer with the matching constraint to the
pyproject.toml [project] dependencies (if it is a runtime dependency) or
remove/omit typer from requires.txt and regenerate the egg-info requires list
from pyproject.toml (if it is not needed), ensuring both files stay consistent.
| # Cache miss: need to decode synchronously to ensure correct image displays | ||
| if _debug_mode: | ||
| decode_start = time.perf_counter() | ||
| log.info("Cache miss for index %d (gen: %d). Blocking decode.", index, display_gen) | ||
|
|
||
| # Submit with priority=True to cancel pending prefetch tasks and free up workers | ||
| future = self.prefetcher.submit_task(index, self.prefetcher.generation, priority=True) | ||
| if future: | ||
| try: | ||
| # Wait for decode to complete (blocking but fast for JPEGs) | ||
| result = future.result(timeout=5.0) # 5 second timeout as safety | ||
| if result: | ||
| decoded_index, decoded_display_gen = result | ||
| cache_key = f"{decoded_index}_{decoded_display_gen}" | ||
| if cache_key in self.image_cache: | ||
| decoded = self.image_cache[cache_key] | ||
| with self._last_image_lock: | ||
| self.last_displayed_image = decoded | ||
| if _debug_mode: | ||
| elapsed = time.perf_counter() - decode_start | ||
| log.info("Decoded image %d in %.3fs", index, elapsed) | ||
| return decoded | ||
| except concurrent.futures.TimeoutError: | ||
| log.exception("Timeout decoding image at index %d", index) | ||
| with self._last_image_lock: | ||
| return self.last_displayed_image | ||
| except concurrent.futures.CancelledError: | ||
| log.warning("Decode cancelled for index %d", index) | ||
| with self._last_image_lock: | ||
| return self.last_displayed_image | ||
| except Exception as e: | ||
| log.exception("Error decoding image at index %d", index) | ||
| with self._last_image_lock: | ||
| return self.last_displayed_image |
There was a problem hiding this comment.
Drop unused exception variable in get_decoded_image
In the generic exception handler, e is never used:
except Exception as e:
log.exception("Error decoding image at index %d", index)Given log.exception already captures the traceback, you can drop the variable to satisfy Ruff F841 and slightly simplify:
- except Exception as e:
+ except Exception:
log.exception("Error decoding image at index %d", index)
with self._last_image_lock:
return self.last_displayed_image🧰 Tools
🪛 Ruff (0.14.5)
364-364: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🤖 Prompt for AI Agents
In faststack.working-menus/faststack/app.py around lines 334 to 367, the generic
exception handler captures an unused variable `e`, triggering a lint F841;
change the handler from `except Exception as e:` to `except Exception:` and
leave the body unchanged (keep the log.exception call and the
with-self._last_image_lock return) so the traceback is still logged but the
unused variable is removed.
| # -------------------------------------------------------------------------- | ||
| # Image Provider Setup | ||
| - self.image_provider = ImageProvider(self.app_controller.app_state_model) | ||
| + # Use a single provider for both original and edited images | ||
| + self.image_provider = ImageProvider(self.app_controller.app_state_model, is_edited_provider=False) | ||
| self.engine.addImageProvider("imageProvider", self.image_provider) | ||
| + | ||
| + # Separate provider for the live-edited image preview | ||
| + self.edited_image_provider = ImageProvider(self.app_controller.app_state_model, is_edited_provider=True) | ||
| + self.engine.addImageProvider("editedImageProvider", self.edited_image_provider) | ||
|
|
||
| # -------------------------------------------------------------------------- | ||
| # Engine Context Setup | ||
| self.engine.rootContext().setContextProperty("appModel", self.app_controller.app_state_model) | ||
| + | ||
| + # -------------------------------------------------------------------------- | ||
| + # Connect signals for edit mode to manage image source | ||
| + self.app_controller.app_state_model.isEditingChanged.connect(self._handle_editing_toggle) | ||
| + # Connect data change signal to the edited provider | ||
| + self.app_controller.app_state_model.currentEditedImageDataChanged.connect( | ||
| + lambda: self.edited_image_provider.image_data = self.app_controller.app_state_model.current_edited_image_data | ||
| + ) | ||
|
|
||
| # Load the QML file | ||
| qml_file = Path(__file__).parent / "qml" / "main.qml" | ||
| @@ -74,6 +86,14 @@ | ||
| if self.engine.rootObjects(): | ||
| self.engine.rootObjects()[0].show() | ||
|
|
||
| + def _handle_editing_toggle(self): | ||
| + """Update the image provider source when editing mode changes.""" | ||
| + is_editing = self.app_controller.app_state_model.is_editing | ||
| + | ||
| + # If starting edit mode, ensure the initial edited data is generated | ||
| + if is_editing: | ||
| + # Trigger the edited image data update to show the initial (unmodified) preview | ||
| + self.app_controller.app_state_model.current_edited_image_data = self.app_controller.image_editor.get_preview_data() | ||
|
|
There was a problem hiding this comment.
_handle_editing_toggle uses a non‑existent is_editing attribute
In the app wrapper:
def _handle_editing_toggle(self):
"""Update the image provider source when editing mode changes."""
is_editing = self.app_controller.app_state_model.is_editing
if is_editing:
self.app_controller.app_state_model.current_edited_image_data = \
self.app_controller.image_editor.get_preview_data()AppStateModel exposes isEditing (property) and the backing field _is_editing, but not is_editing. Accessing is_editing will raise AttributeError when the isEditingChanged signal fires.
Also, you don’t actually use is_editing for branching beyond the if check, so you can read the public property directly:
def _handle_editing_toggle(self):
"""Update the image provider source when editing mode changes."""
- is_editing = self.app_controller.app_state_model.is_editing
-
- # If starting edit mode, ensure the initial edited data is generated
- if is_editing:
+ # If starting edit mode, ensure the initial edited data is generated
+ if self.app_controller.app_state_model.isEditing:
# Trigger the edited image data update to show the initial (unmodified) preview
self.app_controller.app_state_model.current_edited_image_data = (
self.app_controller.image_editor.get_preview_data()
)🤖 Prompt for AI Agents
In faststack/patch around lines 295 to 332, _handle_editing_toggle reads a
non-existent attribute is_editing on app_controller.app_state_model which will
raise AttributeError; change the code to read the public property
(app_controller.app_state_model.isEditing) instead and use that value for the
conditional (or eliminate the temporary and test the property directly),
ensuring you set current_edited_image_data via
app_controller.app_state_model.current_edited_image_data when editing begins.
| + # Editing parameters (normalized to -1.0 to 1.0) | ||
| + self._edits_pending = False | ||
| + self._brightness = 0.0 | ||
| + self._contrast = 0.0 | ||
| + self._saturation = 0.0 | ||
| + self._white_balance_by = 0.0 | ||
| + self._white_balance_mg = 0.0 | ||
| + self._has_active_crop_selection = False | ||
| + self._current_edited_image_data = None # Holds image data bytes for QML provider | ||
| + self._current_aspect_ratio_index = 0 | ||
| + self._aspect_ratio_names = [r["name"] for r in self.app_controller.get_aspect_ratios()] | ||
| + | ||
| @Property(str, notify=currentImagePathChanged) | ||
| def current_image_path(self): | ||
| return str(self._current_image_file.path) if self._current_image_file else "" | ||
| @@ -176,3 +187,94 @@ | ||
| if self._is_cropping != value: | ||
| self._is_cropping = value | ||
| self.isCroppingChanged.emit() | ||
| + | ||
| + @Property(bool, notify=editsPendingChanged) | ||
| + def edits_pending(self): | ||
| + return self._edits_pending | ||
| + | ||
| + @edits_pending.setter | ||
| + def edits_pending(self, value: bool): | ||
| + if self._edits_pending != value: | ||
| + self._edits_pending = value | ||
| + self.editsPendingChanged.emit() | ||
| + | ||
| + @Property(float, notify=brightnessChanged) | ||
| + def brightness(self): | ||
| + return self._brightness | ||
| + | ||
| + @brightness.setter | ||
| + def brightness(self, value: float): | ||
| + if self._brightness != value: | ||
| + self._brightness = value | ||
| + self.brightnessChanged.emit() | ||
| + | ||
| + @Property(float, notify=contrastChanged) | ||
| + def contrast(self): | ||
| + return self._contrast | ||
| + | ||
| + @contrast.setter | ||
| + def contrast(self, value: float): | ||
| + if self._contrast != value: | ||
| + self._contrast = value | ||
| + self.contrastChanged.emit() | ||
| + | ||
| + @Property(float, notify=saturationChanged) | ||
| + def saturation(self): | ||
| + return self._saturation | ||
| + | ||
| + @saturation.setter | ||
| + def saturation(self, value: float): | ||
| + if self._saturation != value: | ||
| + self._saturation = value | ||
| + self.saturationChanged.emit() | ||
| + | ||
| + @Property(float, notify=whiteBalanceBYChanged) | ||
| + def whiteBalanceBY(self): | ||
| + return self._white_balance_by | ||
| + | ||
| + @whiteBalanceBY.setter | ||
| + def whiteBalanceBY(self, value: float): | ||
| + if self._white_balance_by != value: | ||
| + self._white_balance_by = value | ||
| + self.whiteBalanceBYChanged.emit() | ||
| + | ||
| + @Property(float, notify=whiteBalanceMGChanged) | ||
| + def whiteBalanceMG(self): | ||
| + return self._white_balance_mg | ||
| + | ||
| + @whiteBalanceMG.setter | ||
| + def whiteBalanceMG(self, value: float): | ||
| + if self._white_balance_mg != value: | ||
| + self._white_balance_mg = value | ||
| + self.whiteBalanceMGChanged.emit() | ||
| + | ||
| + @Property(bool, notify=hasActiveCropSelectionChanged) | ||
| + def hasActiveCropSelection(self): | ||
| + return self._has_active_crop_selection | ||
| + | ||
| + @hasActiveCropSelection.setter | ||
| + def hasActiveCropSelection(self, value: bool): | ||
| + if self._has_active_crop_selection != value: | ||
| + self._has_active_crop_selection = value | ||
| + self.hasActiveCropSelectionChanged.emit() | ||
| + | ||
| + @Property(QByteArray, notify=currentEditedImageDataChanged) | ||
| + def current_edited_image_data(self): | ||
| + return self._current_edited_image_data | ||
| + | ||
| + @current_edited_image_data.setter | ||
| + def current_edited_image_data(self, value: Optional[bytes]): | ||
| + data = QByteArray(value) if value is not None else QByteArray() | ||
| + if self._current_edited_image_data != data: | ||
| + self._current_edited_image_data = data | ||
| + self.currentEditedImageDataChanged.emit() | ||
| + | ||
| + @Property("QStringList", notify=aspectRatioNamesChanged) | ||
| + def aspectRatioNames(self): | ||
| + return self._aspect_ratio_names | ||
| + | ||
| + @Property(int, notify=currentAspectRatioIndexChanged) | ||
| + def currentAspectRatioIndex(self): | ||
| + return self._current_aspect_ratio_index | ||
| + | ||
| + @currentAspectRatioIndex.setter | ||
| + def currentAspectRatioIndex(self, value: int): | ||
| + if self._current_aspect_ratio_index != value: | ||
| + self._current_aspect_ratio_index = value | ||
| + self.currentAspectRatioIndexChanged.emit() | ||
| + | ||
| + @Slot() | ||
| + def reset_edit_parameters(self): | ||
| + self.brightness = self.contrast = self.saturation = 0.0 | ||
| + self.whiteBalanceBY = self.whiteBalanceMG = 0.0 | ||
| + self.edits_pending = False | ||
| + self.isCropping = False | ||
| + self.hasActiveCropSelection = False | ||
| --- a/faststack/ui/provider.py |
There was a problem hiding this comment.
reset_edit_parameters only resets UI state, not the underlying ImageEditor
reset_edit_parameters lives on AppStateModel:
@Slot()
def reset_edit_parameters(self):
self.brightness = self.contrast = self.saturation = 0.0
self.whiteBalanceBY = self.whiteBalanceMG = 0.0
self.edits_pending = False
self.isCropping = False
self.hasActiveCropSelection = FalseThis correctly resets the model properties and clears the “edits pending” flag, but it never resets ImageEditor.current_edits or regenerates a fresh preview. As a result:
- The “Reset All Edits” button in
ImageEditorDialog.qmlwill visually reset sliders, but the underlying preview and eventual saved image stay with the old edits. - On a pure “UI reset”, the user might think they’re back to the original, but the backend still holds modifications.
To keep UI and backend in sync, route the reset through ImageEditor and update the preview:
@Slot()
def reset_edit_parameters(self):
- self.brightness = self.contrast = self.saturation = 0.0
- self.whiteBalanceBY = self.whiteBalanceMG = 0.0
- self.edits_pending = False
- self.isCropping = False
- self.hasActiveCropSelection = False
+ # Reset UI-side properties
+ self.brightness = self.contrast = self.saturation = 0.0
+ self.whiteBalanceBY = self.whiteBalanceMG = 0.0
+ self.edits_pending = False
+ self.isCropping = False
+ self.hasActiveCropSelection = False
+
+ # Reset backend editor state and preview
+ editor = self.app_controller.image_editor
+ editor.current_edits = editor._initial_edits()
+ self.current_edited_image_data = editor.get_preview_data()(Or better, expose a public ImageEditor.reset_edits() helper instead of touching _initial_edits() directly.)
🤖 Prompt for AI Agents
In faststack/patch around lines 442-564, reset_edit_parameters only clears UI
properties but does not reset the ImageEditor backend; update this method to
call into the ImageEditor to clear its edits and regenerate the preview (either
call a new public ImageEditor.reset_edits() helper or an existing method that
clears ImageEditor.current_edits and re-renders the preview), then update
current_edited_image_data (emit currentEditedImageDataChanged) with the fresh
image bytes and ensure edits_pending is set to False so UI and backend remain in
sync.
| class ImageProvider(QQuickImageProvider): | ||
| """Custom image provider for QML to retrieve images from the cache/disk.""" | ||
|
|
||
| - def __init__(self, app_state_model: "AppStateModel"): | ||
| + def __init__(self, app_state_model: "AppStateModel", is_edited_provider: bool): | ||
| super().__init__(QQuickImageProvider.Pixmap) | ||
| self.app_state_model = app_state_model | ||
| self.app_controller = self.app_state_model.app_controller | ||
| + self.is_edited_provider = is_edited_provider | ||
| self.current_path: Optional[Path] = None | ||
| self.current_data: Optional[DecodedImage] = None | ||
| self.last_load_time: float = 0 | ||
| + | ||
| + # For edited provider, we store the raw byte data | ||
| + self.image_data: QByteArray = QByteArray() | ||
| + self.edited_pixmap: QPixmap = QPixmap() | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
Edited image provider: confirm refresh behavior and handle empty QSize
The edited ImageProvider branch:
if self.is_edited_provider and not self.image_data.isEmpty():
self.edited_pixmap.loadFromData(self.image_data, "PNG")
# Scale the pixmap to the requested size
return self.edited_pixmap.scaled(size, Qt.KeepAspectRatio, Qt.SmoothTransformation)Two things to watch:
-
Refresh semantics: QML’s
Imagecachesimage://URLs by default. Your edited source is a constant:source: appModel.isEditing ? "image://editedImageProvider/currentEdit" : "image://imageProvider/" + appModel.current_image_path
Since the URL doesn’t change when
currentEditedImageDataChangedfires, QML may keep returning the cached pixmap and never re‑callrequestPixmap, so live slider changes won’t be visible.Common fixes:
- Set
cache: falseon theImageshowing the edited content, or - Append a changing token (e.g., an incrementing generation) to the
idso the URL changes whencurrent_edited_image_datachanges.
- Set
-
Zero
sizehandling: WhensourceSizeis not set, thesizepassed torequestPixmapcan be(0, 0). In that casescaled(size, ...)produces a null pixmap. Typical provider implementations return the original pixmap and optionally setsizeinstead of scaling when the requested size is empty.
A defensive pattern:
if self.is_edited_provider and not self.image_data.isEmpty():
if not self.edited_pixmap.loadFromData(self.image_data, "PNG"):
return QPixmap()
if size.width() > 0 and size.height() > 0:
return self.edited_pixmap.scaled(size, Qt.KeepAspectRatio, Qt.SmoothTransformation)
return self.edited_pixmapI'll verify the review comment by examining the codebase for the concerns raised about QML caching and size handling.
🏁 Script executed:
#!/bin/bash
# Find the requestPixmap implementation in the ImageProvider class
rg -nA 30 "def requestPixmap" faststack/patch --type pyLength of output: 1214
🏁 Script executed:
#!/bin/bash
# Search for the edited image provider URL in QML files
rg -nC 5 "editedImageProvider" --type qmlLength of output: 0
🏁 Script executed:
#!/bin/bash
# Check if Image elements have cache:false or sourceSize set
rg -nC 5 "image://editedImageProvider\|cache:\s*false\|sourceSize" --type qmlLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any size parameter checks in requestPixmap or related methods
rg -nC 3 "size\.width\(\)|size\.height\(\)|isEmpty\(\)" faststack/patch --type pyLength of output: 443
🏁 Script executed:
#!/bin/bash
# Find where currentEditedImageDataChanged signal is emitted
rg -nC 3 "currentEditedImageDataChanged" faststack/patch --type pyLength of output: 1397
🏁 Script executed:
#!/bin/bash
# Check for cache invalidation or URL token generation logic
rg -nC 5 "cache|generation|token|refresh|invalidate" faststack/patch --type py | head -100Length of output: 542
🏁 Script executed:
#!/bin/bash
# Find the QML Image element that uses editedImageProvider
fd -e qml | xargs rg -l "Image\|editedImageProvider"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Get full context of requestPixmap implementation
rg -nA 50 "class ImageProvider" faststack/patch --type py | grep -A 50 "requestPixmap"Length of output: 1363
🏁 Script executed:
#!/bin/bash
# Check the actual behavior of scaled() with (0,0) size
rg -nB 5 -A 10 "\.scaled\(size" faststack/patch --type pyLength of output: 764
🏁 Script executed:
#!/bin/bash
# Look for the Image element in QML that shows edited content
fd -e qml -x rg -l "current_edited_image_data\|currentEditedImageData\|editedImageProvider"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find all QML Image declarations to understand the structure
fd -e qml -x grep -n "Image\s*{" Length of output: 29
🏁 Script executed:
#!/bin/bash
# Search for Image sourceSize configuration in QML
fd -e qml -x grep -n "sourceSize"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for the complete requestPixmap method signature and body
ast-grep --pattern 'def requestPixmap($_, $_) {
$$$
}'Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find all QML Image sources that use image protocol
fd -e qml -x rg -n "source:\s*\"image://"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there's any cache handling or URL parameter modification
rg -nC 10 "is_edited_provider" faststack/patch --type pyLength of output: 3441
🏁 Script executed:
#!/bin/bash
# Search for current_edited_image_data or similar edited data handling
rg -nC 5 "current_edit\|edited.*data\|editedImage" faststack/patch --type pyLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for the Image element source binding more carefully
fd -e qml -x rg -B 5 -A 5 "editedImageProvider"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if sourceSize is set on any Image element
fd -e qml -x rg -B 3 -A 3 "sourceSize"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find where the edited pixmap is set/updated
rg -nC 5 "self.edited_pixmap\|editedPixmap" faststack/patch --type pyLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for Image cache property settings
fd -e qml -x rg -B 2 -A 2 "cache:\s*"Length of output: 481
🏁 Script executed:
#!/bin/bash
# Examine the structure around line 567-583 more carefully
sed -n '550,620p' faststack/patchLength of output: 2827
🏁 Script executed:
#!/bin/bash
# Look for the complete ImageProvider implementation
sed -n '567,700p' faststack/patchLength of output: 5518
🏁 Script executed:
#!/bin/bash
# Find where QML loads the ImageProvider or Image element
fd -e qml | head -20Length of output: 519
🏁 Script executed:
#!/bin/bash
# Check what files reference ImageProvider in QML context
rg -n "ImageProvider\|imageProvider\|editedImageProvider" --type qmlLength of output: 0
🏁 Script executed:
#!/bin/bash
# Look for the Image element that displays the edited content
rg -nB 10 -A 10 "Image.*id\|Image.*source" --type qml | head -60Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find QML files that might contain the Image element
fd -e qmlLength of output: 519
🏁 Script executed:
#!/bin/bash
# Check the actual requestPixmap implementation for size handling
sed -n '600,750p' faststack/patch | rg -A 40 "def requestPixmap"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find all methods that handle size parameter
rg -nC 8 "def requestPixmap|def .*size.*:" faststack/patch --type py | head -80Length of output: 771
🏁 Script executed:
#!/bin/bash
# Get the full class definition including requestPixmap
python3 << 'EOF'
with open('faststack/patch', 'r') as f:
lines = f.readlines()
in_image_provider = False
in_request_pixmap = False
indent_level = 0
for i, line in enumerate(lines[566:750], start=567):
if 'class ImageProvider' in line:
in_image_provider = True
if in_image_provider:
if 'def requestPixmap' in line:
in_request_pixmap = True
if in_request_pixmap:
print(f"{i}: {line}", end='')
if line.strip() and not line[0].isspace() and i > 600:
break
elif 'def requestPixmap' in line:
in_request_pixmap = True
print(f"{i}: {line}", end='')
indent_level = len(line) - len(line.lstrip())
elif in_request_pixmap:
print(f"{i}: {line}", end='')
if line.strip() and len(line) - len(line.lstrip()) <= indent_level and line.strip():
if not line.strip().startswith('#'):
break
EOFLength of output: 897
🏁 Script executed:
#!/bin/bash
# Simpler approach - show the entire ImageProvider class from line 567 onwards
sed -n '567,750p' faststack/patchLength of output: 7430
🏁 Script executed:
#!/bin/bash
# Look for the requestPixmap method implementation
grep -n "def requestPixmap" faststack/patchLength of output: 55
I need to verify one more detail about Qt's sourceSize behavior and whether the zero-size concern is still valid with the current configuration.
🏁 Script executed:
#!/bin/bash
# Verify the exact sourceSize configuration in context
sed -n '100,150p' faststack/patch | grep -B 5 -A 5 "sourceSize"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any other Image elements using editedImageProvider without sourceSize
grep -n "editedImageProvider\|Image {" faststack/patch | head -40Length of output: 161
🏁 Script executed:
#!/bin/bash
# Look for the complete Image element definition with all properties
sed -n '105,130p' faststack/patchLength of output: 1036
🏁 Script executed:
#!/bin/bash
# Search for any dynamic sourceSize or potential (0,0) scenarios
rg -n "sourceSize|width|height" faststack/patch | grep -i "source\|edit\|image" | head -30Length of output: 596
🏁 Script executed:
#!/bin/bash
# Check if mainImage is guaranteed to have non-zero dimensions
sed -n '1,200p' faststack/patch | grep -B 3 -A 3 "id.*mainImage\|anchors.fill"Length of output: 232
🏁 Script executed:
#!/bin/bash
# Look for the parent Rectangle or layout that contains mainImage
sed -n '50,150p' faststack/patch | grep -B 10 "id: mainImage"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Find the Image element definition with id: mainImage
sed -n '600,750p' faststack/patch | grep -B 20 "id: mainImage\|image://editedImageProvider"Length of output: 783
🏁 Script executed:
#!/bin/bash
# Get the Image element with all its properties
rg -n "Image\s*\{" faststack/patch -A 30 | grep -A 30 "mainImage\|editedImageProvider"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search around line 650-660 for full Image element
sed -n '640,680p' faststack/patchLength of output: 1551
🏁 Script executed:
#!/bin/bash
# Check the parent RowLayout that contains mainImage
sed -n '630,660p' faststack/patchLength of output: 1195
Add defensive size checks to handle edge cases in requestPixmap
The caching concern is already addressed: the Image element has cache: false set, so the constant URL won't cause stale pixmaps.
However, the zero-size handling still needs attention. The requestPixmap implementation should validate the size parameter before scaling, since scaled(QSize(0, 0), ...) returns a null pixmap. While sourceSize is currently bound to mainImage dimensions, this provides only runtime protection if the binding exists.
Add explicit size validation in faststack/ui/provider.py at the edited image provider branch (around line 592-595):
if self.is_edited_provider and not self.image_data.isEmpty():
if not self.edited_pixmap.loadFromData(self.image_data, "PNG"):
return QPixmap()
if size.width() > 0 and size.height() > 0:
return self.edited_pixmap.scaled(size, Qt.KeepAspectRatio, Qt.SmoothTransformation)
return self.edited_pixmapThis defensive pattern ensures the provider returns a valid pixmap even if sourceSize is removed or the Image component initializes before dimensions are available.
🤖 Prompt for AI Agents
In faststack/patch around lines 567 to 583, the edited-image branch in
requestPixmap must defensively handle zero-size requests: check that
self.is_edited_provider and self.image_data is not empty, attempt to load the
edited_pixmap from self.image_data (return an empty QPixmap on load failure),
then if size.width() > 0 and size.height() > 0 return edited_pixmap.scaled(size,
Qt.KeepAspectRatio, Qt.SmoothTransformation), otherwise return the unscaled
edited_pixmap; this prevents scaled(QSize(0,0)) from producing a null pixmap
when the QML Image sourceSize is unavailable.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/faststack/imaging/jpeg.py (1)
117-141: Incorrect max_dim calculation leads to suboptimal scaling.Line 123 uses
max(width, height)to determine the TurboJPEG scaling factor, but this is incorrect for non-square target dimensions. When the target is, say, 100×200 and the image is 1000×1000, passingmax_dim=200may result in a 200×200 decode that exceeds the 100-pixel width constraint. Although the Pillow thumbnail at lines 134–138 compensates, this adds unnecessary overhead.The correct approach is to choose
max_dimbased on which dimension (width or height) is the limiting factor after accounting for aspect ratios.Apply this diff to calculate the correct
max_dim:img_width, img_height, _, _ = jpeg_decoder.decode_header(jpeg_bytes) - # Calculate best scaling factor for TurboJPEG (supports 1/8, 1/4, 1/2, etc.) - scale_factor = _get_turbojpeg_scaling_factor(img_width, img_height, max(width, height)) + # Determine which dimension is the limiting factor + if img_width * height > img_height * width: + # Image is wider relative to target box; width is the constraint + max_dim = width + else: + # Image is taller relative to target box; height is the constraint + max_dim = height + + scale_factor = _get_turbojpeg_scaling_factor(img_width, img_height, max_dim)This ensures TurboJPEG decodes as close as possible to the final size, minimizing the need for the subsequent Pillow resize.
♻️ Duplicate comments (3)
faststack/faststack/app.py (3)
10-10: ImportTupleso the undo_history type hint and Ruff F821 are satisfied
self.undo_history: List[Tuple[str, Any, float]]usesTuplebut it isn’t imported fromtyping, which Ruff flags as F821 and can also confuse tooling.Minimal fix:
-from typing import Optional, List, Dict, Any +from typing import Optional, List, Dict, Any, TupleAlso applies to: 137-138
1188-1192: Auto white balance undo still looks for_backupfilenames, which won’t match the editor’s-backuppatternThe unified
undo_deletenow handles"auto_white_balance"actions, but the backup lookup still hardcodes an underscore suffix:elif action_type == "auto_white_balance": filepath, saved_path = action_data filepath_obj = Path(filepath) backup_path = filepath_obj.parent / f"{filepath_obj.stem}_backup{filepath_obj.suffix}" ... if backup_path.exists(): filepath_obj.unlink() backup_path.rename(filepath_obj)Per the
ImageEditor.save_image()implementation (infaststack/imaging/editor.py), backups are created using a-backupsuffix with possible numbering (e.g.name-backup.jpg,name-backup2.jpg, etc.), not_backup. As a result, undo for auto white balance will almost always report “Backup not found” even when a valid backup exists.A minimal fix that matches the documented pattern and supports numbered backups:
- filepath, saved_path = action_data - filepath_obj = Path(filepath) - backup_path = filepath_obj.parent / f"{filepath_obj.stem}_backup{filepath_obj.suffix}" + filepath, saved_path = action_data + filepath_obj = Path(filepath) + # Look for backups like "<stem>-backup[<n>].ext" + candidates = sorted( + filepath_obj.parent.glob(f"{filepath_obj.stem}-backup*{filepath_obj.suffix}") + ) + backup_path = candidates[-1] if candidates else None ... - try: - if backup_path.exists(): + try: + if backup_path and backup_path.exists(): filepath_obj.unlink() backup_path.rename(filepath_obj) ... - else: + else: self.update_status_message("Backup not found") log.warning("Backup not found at %s", backup_path) self.undo_history.append(("auto_white_balance", (filepath, saved_path), timestamp))This should make Ctrl+Z reliably undo Quick Auto White Balance operations.
Also applies to: 1217-1308
1715-1858: Auto white balance logic is mathematically consistent; just drop the unusedb_diffThe new
auto_white_balanceimplementation correctly:
- Uses the original full-resolution image for statistics.
- Computes channel means and a grey-world target.
- Derives BY/MG shifts using the two-equation system consistent with the editor’s documented WB model.
- Scales/clamps values into the editor’s expected [-1, 1] range and updates both editor parameters and UIState, then triggers a refresh.
One small cleanup:
b_diffis computed but never used, which Ruff flags as F841:r_diff = r_mean - grey_target g_diff = g_mean - grey_target b_diff = b_mean - grey_target # unusedYou can safely remove
b_difffor now:- r_diff = r_mean - grey_target - g_diff = g_mean - grey_target - b_diff = b_mean - grey_target + r_diff = r_mean - grey_target + g_diff = g_mean - grey_targetIf you later extend the system to incorporate the blue equation explicitly, you can reintroduce it with a corresponding adjustment.
🧹 Nitpick comments (4)
faststack/README.md (1)
12-25: Fix minor typo in feature descriptionLine 12 has a small typo:
"high-peformance"→"high-performance". Everything else in this section reads clearly.faststack/faststack/qml/Main.qml (2)
224-241: “Open Folder…” menu item currently has no user-visible effectThe File → “Open Folder…” action only logs
"Open folder triggered"and closes the menu; it doesn’t open a directory dialog or change the viewed folder.If this is intended as a functional entry point, consider wiring it to the existing folder-selection flow (e.g., a controller slot that opens a QFileDialog and reloads images), or add a TODO so it’s clear this is placeholder behavior.
596-616: Guard Ctrl+S save against key auto‑repeatIn the key handler,
Eignores auto-repeat, but the Ctrl+S path doesn’t:else if (event.key === Qt.Key_S && (event.modifiers & Qt.ControlModifier)) { if (uiState.isEditorOpen) { controller.save_edited_image() event.accepted = true } }Holding Ctrl+S could therefore trigger multiple saves. Mirroring the
!event.isAutoRepeatguard here would make the behavior more robust:else if (event.key === Qt.Key_S && (event.modifiers & Qt.ControlModifier) && !event.isAutoRepeat) { ... }faststack/faststack/app.py (1)
666-699: Avoid double-saving sidecar stacks when removing from a stackIn
remove_from_batch_or_stack, the stack-removal path updates and saves the sidecar twice—once before updatingself.stacks, and again afterwards:self.sidecar.data.stacks = self.stacks # BEFORE self.stacks is replaced self.sidecar.save() ... if stack_modified: self.stacks = new_stacks self.sidecar.data.stacks = self.stacks self.sidecar.save()The first save writes the old stack list and is immediately overwritten by the second save. This adds unnecessary disk I/O and makes the comment misleading.
You can safely drop the earlier assignment/save and only persist after
self.stacksis updated:- self.sidecar.data.stacks = self.stacks # Update sidecar BEFORE self.stacks is replaced - self.sidecar.save() ... if stack_modified: self.stacks = new_stacks self.sidecar.data.stacks = self.stacks self.sidecar.save()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
faststack/ChangeLog.md(1 hunks)faststack/README.md(4 hunks)faststack/faststack.egg-info/PKG-INFO(2 hunks)faststack/faststack.egg-info/requires.txt(0 hunks)faststack/faststack/app.py(18 hunks)faststack/faststack/imaging/jpeg.py(4 hunks)faststack/faststack/io/helicon.py(1 hunks)faststack/faststack/qml/Components.qml(2 hunks)faststack/faststack/qml/Main.qml(8 hunks)faststack/pyproject.toml(1 hunks)faststack/requirements.txt(0 hunks)
💤 Files with no reviewable changes (2)
- faststack/requirements.txt
- faststack/faststack.egg-info/requires.txt
🚧 Files skipped from review as they are similar to previous changes (4)
- faststack/faststack/qml/Components.qml
- faststack/pyproject.toml
- faststack/faststack.egg-info/PKG-INFO
- faststack/ChangeLog.md
🧰 Additional context used
🪛 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.2 - November 22, 2025 # By Alan Rockefeller Ultra-fast, cach...
(MISSING_COMMA_AFTER_YEAR)
[grammar] ~12-~12: Ensure spelling is correct
Context: ...0ms next/previous image switching, high-peformance decoding via PyTurboJPEG. - **Zoom & ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 Ruff (0.14.5)
faststack/faststack/app.py
137-137: Undefined name Tuple
(F821)
655-655: f-string without any placeholders
Remove extraneous f prefix
(F541)
689-689: f-string without any placeholders
Remove extraneous f prefix
(F541)
1815-1815: Local variable b_diff is assigned to but never used
Remove assignment to unused variable b_diff
(F841)
🔇 Additional comments (13)
faststack/faststack/imaging/jpeg.py (5)
12-29: LGTM: Robust PyTurboJPEG initialization with proper fallback.The nested try-except-else structure correctly handles both import failures and initialization failures, setting the TURBO_AVAILABLE flag appropriately in each case. The logging provides good visibility into which decoder is being used.
31-49: LGTM: Proper color space handling and robust fallback.The use of
pixel_format=TJPF_RGBwithflags=0ensures correct YCbCr→RGB conversion. The exception handling appropriately falls back to Pillow when TurboJPEG fails.
52-87: LGTM: Well-structured thumbnail generation.The function properly leverages TurboJPEG's hardware-accelerated scaling and includes a defensive Pillow resize step (lines 71-74) if the decoded thumbnail still exceeds
max_dim. The fallback path is sound.
90-107: LGTM: Efficient scaling factor selection.The function correctly identifies the largest TurboJPEG scaling factor that keeps both dimensions within
max_dim. Sorting in descending order and returning early optimizes the common case.
143-160: LGTM: Smart resampling selection in Pillow fallback.The dynamic choice of resampling filter based on scale factor (BILINEAR for >4× downscales, LANCZOS otherwise) balances performance and quality effectively.
faststack/faststack/io/helicon.py (1)
22-24: Docstring now correctly documents the tuple return contractThe updated return-value description matches the actual
(success: bool, tmp_path: Optional[Path])behavior across all branches; no further changes needed here.faststack/faststack/app.py (6)
329-334: Editor live preview integration in get_decoded_image and get_preview_data looks coherentThe early-return in
get_decoded_imagewhen the editor is open for the current file, plus theget_preview_dataslot that delegates toImageEditor.get_preview_data(), provides a clean path for showing live edits without fighting the prefetch/cache pipeline:if self.ui_state.isEditorOpen and self.image_editor.original_image and \ str(self.image_editor.current_filepath) == str(self.image_files[index].path): preview_data = self.image_editor.get_preview_data() if preview_data: return preview_dataand
@Slot(result=DecodedImage) def get_preview_data(self) -> Optional[DecodedImage]: return self.image_editor.get_preview_data()The logic is sound; assuming QML calls this via
controller.get_preview_data()for previews, this should integrate well with the existingImageProvider.Also applies to: 1614-1659
708-885: Batch/stack toggle helpers are a nice UX improvementThe new
toggle_batch_membershipandtoggle_stack_membershiphelpers provide intuitive “one-key” add/remove behavior, correctly:
- Detecting membership in existing ranges.
- Splitting ranges or merging/normalizing overlapping/adjacent ranges.
- Updating sidecar, cache indices, summary signals, and UI sync.
The algorithms look correct for edge cases (single-element ranges, adjacency, overlaps) and should behave well as the number of batches/stacks grows.
1399-1429: Photoshop integration’s backup-stripping and RAW preference logic looks solidThe new
edit_in_photoshopflow correctly:
- Strips
-backup,-backup2, or-backup-1style suffixes from the JPG stem viare.sub(r'-backup(-?\d+)?$', '', original_stem).- Prefers an existing
image_file.raw_pairwhen available.- Falls back to searching for a RAW file using
RAW_EXTENSIONSand the original (non-backup) stem.- Only uses the JPG when no RAW is found, and logs that choice.
This should behave well for edited/backup outputs while still honoring RAW-first editing where possible.
1524-1534: Status message helper is a good centralization of transient UI messages
update_status_messagecentralizes status text updates and timed clearing, and you’ve already threaded it into many new paths (batch/stack actions, deletes, Photoshop, auto WB, etc.). This should make future UX tweaks (timeouts, styling) much simpler.
1552-1611: Drag-and-drop now correctly filters non-existent files and marks only successfully dragged items as uploadedIn
start_drag_current_image:
- You compute
existing_indicesfrom the collected batch/current indices before buildingfile_paths, ensuring you never drag missing files.- After a successful drag (
CopyorMove), you loop overexisting_indicesto mark only those items as uploaded, then clear all batches and refresh metadata/UI.This avoids errors on stale paths and keeps the “uploaded” metadata consistent with what was actually dragged.
1908-1952: QML import path setup for PySide6 looks appropriateThe additional QML import paths in
main():engine.addImportPath(os.path.join(os.path.dirname(PySide6.__file__), "qml")) engine.addImportPath("qrc:/qt-project.org/imports") engine.addImportPath(os.path.join(os.path.dirname(__file__), "qml")) engine.addImportPath(os.path.join(os.path.dirname(PySide6.__file__), "qml", "Qt5Compat"))combined with
os.environ["QML2_IMPORT_PATH"]pointing at your localqmldirectory, should make it much easier for PySide6 to resolve both your own QML and QtQuick/Qt5Compat modules without relying on environment-specific configuration.faststack/faststack/qml/Main.qml (1)
245-255: Fix only the filter dialog binding; settings dialog bindings are correctThe settings dialog methods (
get_helicon_path(),get_photoshop_path(),get_cache_size(),get_prefetch_radius(),get_default_directory()) are already correctly exposed via@Slotwrappers onUIStateinfaststack/faststack/ui/provider.py(lines 310–366). The QML code at lines 245–255 is correct as written.However, the filter dialog at line 845 has an issue:
uiState.applyFilter(filterString)does not exist onUIState. The methodapply_filter()exists only onAppController.Fix required at Main.qml line 845:
- if (uiState) uiState.applyFilter(filterString) + if (controller) controller.apply_filter(filterString)Likely an incorrect or invalid review comment.
1 participant
Summary by CodeRabbit
Release Notes - Version 1.2
New Features
UI/UX Improvements
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.