fix some bugs#31
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughAdds recycle-bin aware delete/undo, RawTherapee path detection and UI, fixes rotation direction, hardens prefetch for empty files, exposes dialog open state to QML, updates Settings UI, and adds tests and reproduction/verification scripts. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant QML as QML (Settings / Actions)
participant UI as UIState
participant App as AppController
participant FS as Filesystem
participant History as Delete/Undo History
QML->>UI: request delete (single/batch)
UI->>App: call delete APIs
App->>App: build DeleteRecord(s)
App->>FS: _move_to_recycle(src)
alt same-device move ok
FS-->>App: dest path
else collision exists
App->>FS: compute unique name, attempt move (rename/replace)
FS-->>App: dest path or error
end
App->>History: push DeleteRecord into delete_history / undo_history
App-->>UI: emit dialogStateChanged? (if dialog counters changed)
UI-->>QML: isDialogOpen updated (property notify)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @faststack/imaging/editor.py:
- Around line 540-542: Replace the typographical RIGHT SINGLE QUOTATION MARK in
the inline comment that reads “That’s usually acceptable…” with a standard ASCII
apostrophe so the comment becomes "That's usually acceptable…"; locate the
comment near the note about applying the stretch uniformly to RGB in editor.py
and update the punctuation only.
🧹 Nitpick comments (7)
reproduce_issue.py (2)
1-4: Unused import:sysThe
sysmodule is imported but never used in this script.🔎 Suggested fix
import os import pathlib -import sys
17-18: Consider splitting one-liner conditionals for readability.Per Ruff E701, these one-liner
ifstatements could be split onto separate lines for better readability, though this is a minor style concern for a debugging script.🔎 Suggested fix
# Clean up previous run - if source_file.exists(): source_file.unlink() - if dest_file.exists(): dest_file.unlink() + if source_file.exists(): + source_file.unlink() + if dest_file.exists(): + dest_file.unlink()And similarly for lines 37-38:
# Reset for fix test - if not source_file.exists(): source_file.touch() - if not dest_file.exists(): dest_file.touch() + if not source_file.exists(): + source_file.touch() + if not dest_file.exists(): + dest_file.touch()Also applies to: 37-38
verify_fix.py (2)
1-1: Remove leading blank line.The file starts with a blank line, which is unconventional for Python files.
27-29: ClarifyNamedTemporaryFileusage pattern.Calling
f.close()inside thewithblock is unusual. The file is closed, then its name is used later. While this works, consider usingdelete=Falsewith manual cleanup (which you already do), but without thewithblock for clarity:f = tempfile.NamedTemporaryFile(delete=False) path = f.name f.close()The current approach works but may confuse readers about the file's state.
faststack/config.py (1)
11-14: Move imports to the top of the file with other standard library imports.These imports should be grouped with the existing stdlib imports at the top (lines 3-5) for PEP 8 compliance.
🔎 Suggested fix
import configparser import logging +import glob +import os +import re +import sys from pathlib import Path from faststack.logging_setup import get_app_data_dir log = logging.getLogger(__name__) -import sys -import glob -import os -import refaststack/app.py (2)
1843-1874: Address static analysis hints and remove redundant import.
Redundant import:
timeis already imported at module level (line 8), so theimport timeat line 1859 is unnecessary.Use
log.exceptionfor error cases: Lines 1852 and 1873 uselog.errorbut should uselog.exceptionto capture the stack trace when handling exceptions.🔎 Proposed fix
def _move_to_recycle(self, src: Path) -> Optional[Path]: """Moves a file to the recycle bin safely, handling collisions and cross-device moves.""" if not src.exists() or not src.is_file(): return None # Ensure recycle bin exists try: self.recycle_bin_dir.mkdir(parents=True, exist_ok=True) except OSError as e: - log.error("Failed to create recycle bin: %s", e) + log.exception("Failed to create recycle bin: %s", e) return None dest = self.recycle_bin_dir / src.name # Handle collisions with timestamp loop if dest.exists(): - import time timestamp = int(time.time()) base_name = f"{src.stem}.{timestamp}" dest = self.recycle_bin_dir / f"{base_name}{src.suffix}" counter = 1 while dest.exists(): dest = self.recycle_bin_dir / f"{base_name}_{counter}{src.suffix}" counter += 1 try: shutil.move(str(src), str(dest)) log.info("Moved %s to recycle bin: %s", src.name, dest.name) return dest except OSError as e: - log.error("Failed to recycle %s: %s", src.name, e) + log.exception("Failed to recycle %s: %s", src.name, e) return None
1888-1904: Minor: Remove redundantimport time.The
timemodule is already imported at the module level (line 8), so the import at line 1893 is unnecessary.🔎 Proposed fix
# Add to delete history if anything was moved if recycled_jpg or recycled_raw: - import time timestamp = time.time() # Store tuple of (src, bin_path) for each file # Format: ( (jpg_src, jpg_bin), (raw_src, raw_bin) ) record = ( (jpg_path, recycled_jpg), (raw_path, recycled_raw) )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test_deletion_repro/recycle_bin/test_image.jpgis excluded by!**/*.jpg
📒 Files selected for processing (13)
faststack/app.pyfaststack/config.pyfaststack/imaging/editor.pyfaststack/imaging/prefetch.pyfaststack/qml/FilterDialog.qmlfaststack/qml/Main.qmlfaststack/qml/SettingsDialog.qmlfaststack/tests/test_editor_rotation.pyfaststack/ui/provider.pyreproduce_issue.pyreproduce_mmap_error.pyverify_fix.pyverify_fix_simple.py
🧰 Additional context used
🧬 Code graph analysis (5)
verify_fix_simple.py (1)
verify_fix.py (1)
verify(25-66)
faststack/imaging/editor.py (1)
faststack/ui/provider.py (2)
rotation(869-870)rotation(873-876)
faststack/ui/provider.py (1)
faststack/app.py (2)
get_rawtherapee_path(1316-1317)set_rawtherapee_path(1319-1321)
faststack/app.py (2)
faststack/ui/provider.py (2)
get_rawtherapee_path(501-502)set_rawtherapee_path(505-506)faststack/config.py (2)
get(171-172)save(161-169)
faststack/tests/test_editor_rotation.py (1)
faststack/imaging/editor.py (3)
rotate_image_cw(790-795)_apply_edits(278-486)rotate_image_ccw(797-802)
🪛 Ruff (0.14.10)
verify_fix_simple.py
26-26: Local variable mmapped is assigned to but never used
Remove assignment to unused variable mmapped
(F841)
30-30: Do not catch blind exception: Exception
(BLE001)
faststack/imaging/editor.py
541-541: Comment contains ambiguous ’ (RIGHT SINGLE QUOTATION MARK). Did you mean ``` (GRAVE ACCENT)?
(RUF003)
reproduce_issue.py
17-17: Multiple statements on one line (colon)
(E701)
18-18: Multiple statements on one line (colon)
(E701)
37-37: Multiple statements on one line (colon)
(E701)
38-38: Multiple statements on one line (colon)
(E701)
49-49: Do not catch blind exception: Exception
(BLE001)
reproduce_mmap_error.py
17-17: Local variable mmapped is assigned to but never used
Remove assignment to unused variable mmapped
(F841)
26-26: Do not catch blind exception: Exception
(BLE001)
verify_fix.py
60-60: Do not catch blind exception: Exception
(BLE001)
faststack/config.py
45-45: Do not catch blind exception: Exception
(BLE001)
faststack/app.py
1852-1852: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
1871-1871: Consider moving this statement to an else block
(TRY300)
1873-1873: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (27)
faststack/imaging/editor.py (1)
287-292: LGTM! Critical rotation bug fix.The corrected mapping now properly applies:
ROTATE_90for 90-degree rotation (counter-clockwise)ROTATE_270for 270-degree rotation (counter-clockwise, equivalent to 90° clockwise)This aligns with the regression tests in
faststack/tests/test_editor_rotation.pythat verify the CW/CCW pixel mappings.faststack/tests/test_editor_rotation.py (1)
189-297: LGTM! Comprehensive rotation regression tests.The tests effectively validate:
- Rotation state updates (270° for CW, 90° for CCW)
- Pixel-level transformations using a 4-quadrant test image
- Correct mapping of quadrants after rotation
The
create_quadrant_imagehelper is well-designed for visual verification, and the pixel assertions precisely verify the expected CW/CCW behavior. These tests provide strong regression coverage for the rotation fix infaststack/imaging/editor.py.faststack/imaging/prefetch.py (2)
323-326: LGTM! Prevents mmap error on empty files.This guard correctly prevents the
ValueError: cannot mmap an empty filethat would occur when attempting to memory-map an empty image file. The early return with a warning is the appropriate handling strategy.This fix is validated by the reproduction and verification scripts (
reproduce_mmap_error.pyandverify_fix_simple.py) included in this PR.
523-533: LGTM! Improved saturation compensation with explicit validation.The refactor clarifies the buffer layout expectations:
- Creates an explicit 1D view (
arr) usingravel()for in-place modification- Adds debug assertions to validate contiguity, size, and dtype
- Passes the 1D view to
apply_saturation_compensationinstead of the buffer directlyThese changes make the code more maintainable and catch potential buffer layout issues early during development.
verify_fix_simple.py (1)
1-37: LGTM! Simple verification script for empty file handling.This standalone verification script correctly validates the empty file size check added to
faststack/imaging/prefetch.py(lines 323-326). The script confirms that:
- Empty files are detected via
os.path.getsize()- Processing is skipped before attempting mmap
The static analysis warnings (unused variable, broad exception) are acceptable for standalone verification scripts that prioritize simplicity and robustness over strict linting rules.
reproduce_mmap_error.py (1)
1-32: LGTM! Effective reproduction script for mmap error.This script successfully reproduces the
ValueError: cannot mmap an empty filecondition that the fix infaststack/imaging/prefetch.py(lines 323-326) addresses. The script:
- Creates an empty temporary file
- Attempts to mmap it in the same pattern as the application code
- Verifies the expected ValueError is raised
The static analysis warnings are acceptable for reproduction scripts that prioritize clarity and isolation of the issue over strict linting compliance.
reproduce_issue.py (1)
40-50: Test script logic is sound for demonstratingPath.replacevsPath.rename.The script correctly demonstrates that
Path.replace()atomically overwrites the destination (even if it exists), whilePath.rename()may raiseFileExistsErroron Windows. This is a useful reproduction case for the recycle-bin collision fix.verify_fix.py (1)
33-66: Test logic correctly validates Prefetcher behavior with empty files.The script properly instantiates the
Prefetcher, calls_decode_and_cachewith appropriate parameters, and verifies thatNoneis returned for an empty file (graceful failure). The cleanup infinallyblocks ensures proper resource management.faststack/config.py (2)
149-158: Path re-detection logic is well-implemented.The validation correctly:
- Only runs on Windows (
sys.platform == "win32")- Checks if the configured path exists
- Attempts re-detection only when the path is missing
- Updates and saves only if a new, different path is found
This provides a good fallback for users who upgrade RawTherapee.
36-44: Natural sort key correctly compares multi-digit version numbers.The
re.split(r'(\d+)', path)pattern with a capturing group properly groups consecutive digits, and the list comprehension correctly converts digit sequences to integers for numeric comparison. Testing confirms that5.10correctly sorts greater than5.9in descending order:['10.2', '5.10', '5.9', '5.8', '2.15'].faststack/qml/Main.qml (1)
618-634: Shortcut correctly disabled when dialog is open.The binding
enabled: uiState ? !uiState.isDialogOpen : trueproperly prevents the "E" keyboard shortcut from toggling the editor while a dialog (Settings, Filter, etc.) is open. This improves UX by avoiding unintended state changes during dialog interactions.faststack/qml/FilterDialog.qml (1)
74-92: Dialog lifecycle handlers correctly notify Python of open/close state.The
onOpenedandonClosedhandlers properly callcontroller.dialog_opened()andcontroller.dialog_closed()to synchronize dialog state with the Python backend. This enables features like disabling keyboard shortcuts while dialogs are open (as seen in Main.qml'sisDialogOpenbinding).faststack/ui/provider.py (4)
162-162: Signal declaration follows established patterns.The
isDialogOpenChangedsignal is consistent with other boolean state signals in this class (e.g.,is_editor_open_changed,isDecodingChanged).
500-506: RawTherapee path slots correctly delegate to controller.The
get_rawtherapee_pathandset_rawtherapee_pathslots follow the same pattern as existing path accessors (get_helicon_path,get_photoshop_path), maintaining API consistency.
632-640:isDialogOpenproperty implementation is correct.The property follows the standard pattern with:
- Notify signal specified
- Return type annotation
- Change detection to avoid spurious emissions
This enables QML to reactively bind to dialog state (as used in Main.qml's shortcut enabling).
210-216: Dialog state tracking properly wired to controller signal.The initialization correctly:
- Initializes
_is_dialog_open = False- Connects to
app_controller.dialogStateChangedsignal (defined atAppControllerin faststack/app.py:95)- Provides handler
_on_dialog_state_changedto update statefaststack/qml/SettingsDialog.qml (3)
32-32: RawTherapee path property correctly integrated into settings lifecycle.The
rawtherapeePathproperty is properly:
- Declared as a string property (line 32)
- Initialized from
uiState.get_rawtherapee_path()inopen()(line 68)- Synchronized to the text field in
onVisibleChanged(line 104)- Persisted via
uiState.set_rawtherapee_path()insaveSettings()(line 116)This follows the same pattern as
heliconPathandphotoshopPath.Also applies to: 68-68, 104-104, 116-116
468-500: RawTherapee Path UI section is well-structured.The new UI section correctly implements:
- Label with consistent styling
- Text field via Loader with
textEditedsignal connection- Browse button that updates both property and field
- Status indicator using
uiState.check_path_exists()This matches the existing Helicon and Photoshop path sections exactly.
705-710: Binding target refactors improve correctness.Changing from
parent.itemto explicit loader IDs (e.g.,clipThresholdLoader.item,autoLevelStrengthLoader.item) ensures bindings reference the correct loader's item. Theparent.itempattern can be fragile if the component hierarchy changes.Also applies to: 739-744, 829-834, 859-864, 889-894, 928-933, 953-958, 978-983, 1003-1008
faststack/app.py (8)
14-21: LGTM!Good use of type aliases to improve readability of the delete record structure. The
shutilimport is appropriately added to support cross-device file moves.
95-95: LGTM!Signal appropriately defined for dialog state tracking.
148-148: LGTM!Counter-based approach correctly handles nested dialog tracking.
199-204: LGTM!Type annotations and comments clearly document the new delete record structure.
721-737: LGTM!The refactored dialog tracking correctly handles nested dialogs with proper threshold-based signal emission. The
max(0, ...)guard prevents negative counter values.
1316-1322: LGTM!Methods follow the established pattern for external application path configuration (consistent with
get_helicon_path/set_helicon_pathandget_photoshop_path/set_photoshop_path).
1999-2006: LGTM!The batch deletion logic correctly uses the new
_move_to_recyclehelper and maintains consistency with theDeleteRecordformat.
2050-2109: LGTM!The undo logic correctly handles the new
DeleteRecordformat with proper safety checks:
- Validates both source and bin paths before attempting restore
- Prevents accidental overwrites if a file already exists at the destination
- Uses
shutil.movefor cross-device compatibility- Properly re-adds to history on failure for retry capability
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.