Test#23
Conversation
WalkthroughThis pull request adds configurable auto-level strength adaptation, AWB tint bias control, and crop state reset on image navigation. It enhances image editing with EXIF metadata handling during save, improves rotation/cropping in the image editor with aspect-ratio constraints, and expands the UI with new settings, keyboard shortcuts, and right-click crop interactions. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant QML as QML UI<br/>(SettingsDialog)
participant UIState as UIState<br/>(provider.py)
participant AppCtrl as AppController<br/>(app.py)
participant Config as Config<br/>(config.py)
participant ImgEditor as ImageEditor<br/>(editor.py)
User->>QML: Toggle "Auto Level Strength Auto"
QML->>UIState: set autoLevelStrengthAuto(true)
UIState->>AppCtrl: autoLevelStrengthAutoChanged signal
AppCtrl->>Config: set_auto_level_strength_auto(true)
Config-->>AppCtrl: persisted to INI
User->>QML: Trigger auto-levels
QML->>AppCtrl: auto_levels()
alt auto_level_strength_auto enabled
AppCtrl->>ImgEditor: compute adaptive strength<br/>(avoid clipping)
ImgEditor-->>AppCtrl: strength value
AppCtrl->>ImgEditor: apply with adaptive strength
else fixed mode
AppCtrl->>ImgEditor: apply with fixed strength
end
ImgEditor-->>AppCtrl: result (bool/success)
AppCtrl->>UIState: emit status update
UIState->>QML: refresh display
QML-->>User: show result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
faststack/ChangeLog.md (1)
21-22: Wrap bare URL in angle brackets or use Markdown link syntax.Static analysis flagged a bare URL on line 21. Converting to a proper Markdown link improves readability and compatibility.
-- Some fixes to the image cache - it doesn't expire when it shouldn't, does expire when it should, and warns you when the cache is full so you can consider increasing the cache size in settings. +- Some fixes to the image cache - it doesn't expire when it shouldn't, does expire when it should, and warns you when the cache is full so you can consider increasing the cache size in settings.Note: The static analysis tool flagged the URL on line 21 (
https://github.com/AlanRockefeller/faststack), which should be wrapped in angle brackets<url>or converted to a Markdown link[text](url).faststack/faststack/imaging/editor.py (1)
132-132: Unusedis_exportparameter - implement or remove.The
is_exportparameter is added but never used within_apply_edits. If this is a placeholder for future export-specific behavior (e.g., different quality settings), consider adding a TODO comment. Otherwise, remove it to avoid confusion.Static analysis (Ruff ARG002) correctly flagged this as unused.
- def _apply_edits(self, img: Image.Image, is_export: bool = False) -> Image.Image: + def _apply_edits(self, img: Image.Image) -> Image.Image:Or if intentional for future use:
- def _apply_edits(self, img: Image.Image, is_export: bool = False) -> Image.Image: + def _apply_edits(self, img: Image.Image, is_export: bool = False) -> Image.Image: # noqa: ARG002 - reserved for export-specific processingfaststack/faststack/config.py (2)
19-21: Document the new INI key.Per coding guidelines, new INI keys in
config.pyshould be documented. Add a comment explaining whatauto_level_strength_autocontrols.Based on learnings, document any new INI key in
config.py."auto_level_strength": "1.0", - "auto_level_strength_auto": "False", + "auto_level_strength_auto": "False", # When True, automatically adjust strength based on image histogram
38-40: Document the newtint_biasINI key.Per coding guidelines, add documentation for the new AWB tint bias setting.
"warm_bias": "6", - "tint_bias": "0", + "tint_bias": "0", # Magenta/Green bias for AWB (-50 to 50)faststack/faststack/qml/Components.qml (1)
619-801: Complex but comprehensive aspect ratio constraint handling.The logic handles all drag modes (edges, corners, new) with proper boundary clamping. A few observations:
- Multiple
var newW/var newHdeclarations in different branches are valid JavaScript but could benefit from block scoping (let) for clarity.- The corner-specific logic (lines 728-798) is thorough but verbose.
This is acceptable given the inherent complexity of aspect-ratio-constrained cropping.
Consider extracting corner constraint logic into a helper function to reduce duplication:
function constrainCorner(anchorX, anchorY, width, height, targetAspect, maxX, maxY) { // Common logic for corner constraints }faststack/faststack/app.py (2)
974-993: Preferset_edit_paramover direct dictionary manipulation.Lines 992-993 directly modify
self.image_editor.current_edits['straighten_angle'], which bypasses theset_edit_parammethod used elsewhere in this function (lines 983-984). This inconsistency could lead to missed side effects or future maintenance issues.Apply this diff to use the consistent method:
- # Also reset the straighten angle in current_edits since it affects rotation logic - if 'straighten_angle' in self.image_editor.current_edits: - self.image_editor.current_edits['straighten_angle'] = 0.0 + # Ensure straighten_angle is reset via the proper method + self.image_editor.set_edit_param('straighten_angle', 0.0)
2708-2743: Strengthen error handling in auto-strength calculation.Line 2735 catches a broad
Exception, which can mask unexpected errors. Consider catching more specific exceptions related to image processing and mathematical operations.Apply this diff to improve exception handling:
- except Exception as e: - log.warning(f"Failed to calculate auto strength: {e}") + except (AttributeError, ValueError, ZeroDivisionError) as e: + log.warning("Failed to calculate auto strength: %s", e) strength = self.auto_level_strengthAdditionally, verify the division edge case:
At line 2725, the check
abs(denom) > 0.001might not be robust enough. Consider whether stricter validation or a different threshold is needed based on your image processing requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
faststack/ChangeLog.md(1 hunks)faststack/faststack.egg-info/PKG-INFO(1 hunks)faststack/faststack/app.py(15 hunks)faststack/faststack/config.py(2 hunks)faststack/faststack/imaging/editor.py(4 hunks)faststack/faststack/qml/Components.qml(9 hunks)faststack/faststack/qml/HistogramWindow.qml(1 hunks)faststack/faststack/qml/SettingsDialog.qml(9 hunks)faststack/faststack/ui/provider.py(5 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
faststack/faststack/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
faststack/faststack/**/*.py: Use four-space indentation in Python files
Use trailing commas for multi-line structures in Python
Use descriptive snake_case identifiers for variables and functions in Python
Use CamelCase exclusively for class names and Qt signal names in Python
Include type hints in all Python code as seen inmodels.pyandAppController
Every new Python module should expose a top-levellog = logging.getLogger(__name__)logger
Format Python code with Black before opening a pull request
Lint Python code with Ruff before opening a pull request
Prefer relative paths in configuration and file handling
Files:
faststack/faststack/config.pyfaststack/faststack/ui/provider.pyfaststack/faststack/imaging/editor.pyfaststack/faststack/app.py
faststack/faststack/**/config.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
Document any new INI key in
config.py
Files:
faststack/faststack/config.py
faststack/faststack/ui/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
Treat drag-and-drop helpers like
make_hdropas security-sensitive and validate all user-supplied paths before invoking external tools
Files:
faststack/faststack/ui/provider.py
faststack/faststack/imaging/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
Cover every new branch in
imaging/orio/modules with focusedpytesttest cases
Files:
faststack/faststack/imaging/editor.py
faststack/faststack/**/app.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
app.pyis the PySide6 entrypoint that coordinates image indexing, caching, and QML screens
Files:
faststack/faststack/app.py
🧠 Learnings (2)
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/config.py : Document any new INI key in `config.py`
Applied to files:
faststack/faststack/config.py
📚 Learning: 2025-12-08T04:46:41.632Z
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: Applies to faststack/faststack/**/app.py : `app.py` is the PySide6 entrypoint that coordinates image indexing, caching, and QML screens
Applied to files:
faststack/faststack/app.py
🧬 Code graph analysis (2)
faststack/faststack/ui/provider.py (2)
faststack/faststack/app.py (5)
get_awb_tint_bias(1271-1272)set_awb_tint_bias(1275-1277)get_auto_level_strength_auto(2842-2843)set_auto_level_strength_auto(2846-2850)set_crop_box(2242-2247)faststack/faststack/imaging/editor.py (1)
set_crop_box(382-384)
faststack/faststack/imaging/editor.py (2)
faststack/faststack/ui/provider.py (2)
rotation(761-762)rotation(765-768)faststack/faststack/config.py (1)
get(86-87)
🪛 markdownlint-cli2 (0.18.1)
faststack/ChangeLog.md
21-21: Bare URL used
(MD034, no-bare-urls)
🪛 Ruff (0.14.8)
faststack/faststack/imaging/editor.py
132-132: Unused method argument: is_export
(ARG002)
436-436: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/app.py
2612-2612: Do not use bare except
(E722)
2612-2613: try-except-pass detected, consider logging the exception
(S110)
2735-2735: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (17)
faststack/faststack/imaging/editor.py (1)
420-439: LGTM - Defensive EXIF handling.The EXIF orientation sanitization and fallback logic is well-structured. The broad
Exceptioncatch on line 436 is acceptable here since EXIF parsing can fail in unpredictable ways, and the fallback (skipping EXIF to prevent double-rotation) is the safest option.faststack/faststack/qml/HistogramWindow.qml (1)
20-26: LGTM - Correct keyboard event handling.Moving
event.accepted = trueto only execute for the H key ensures other keypresses propagate correctly. The clarifying comment improves maintainability.faststack/faststack/ui/provider.py (2)
342-349: LGTM - AWB tint bias property.The property follows the established pattern with proper signal emission and delegation to the controller.
511-518: LGTM - Auto level strength auto property.The property correctly follows the existing pattern with boolean type and proper signal notification.
faststack/faststack/qml/SettingsDialog.qml (2)
249-268: LGTM - Auto Levels Strength UI.The RowLayout with slider and Auto checkbox is well-structured. The enabled/opacity binding provides clear visual feedback when auto mode is active.
340-359: LGTM - AWB Tint Bias control.The new SpinBox for tint bias follows the same pattern as warm bias, with appropriate range (-50 to 50), editability, and tooltip.
faststack/faststack/qml/Components.qml (4)
133-133: LGTM - Right-click support for crop mode.Expanding
acceptedButtonsto includeQt.RightButtonenables the right-click crop initiation feature, which is a sensible UX addition.
171-198: LGTM - Right-click crop toggle behavior.The implementation correctly:
- Cancels crop mode if already active
- Enters crop mode and initializes a new crop if inactive
- Returns early to prevent left-click handling
498-538: LGTM - Unified zoom out behavior.Changing zoom out to use cursor position (same as zoom in) provides a consistent and intuitive zoom experience. The comments explain the rationale well.
340-340: Verify rotation direction is correct.The rotation calculation sign was changed. Combined with the handle placement fix (lines 216-218), ensure the rotation behaves as expected (CW-positive per the comment on line 216).
faststack/faststack/app.py (7)
169-169: LGTM!The initialization follows the established pattern for config-backed properties and properly uses
getbooleanwith a sensible default.
1270-1277: LGTM!The AWB tint bias getters and setters follow the established pattern for config-backed properties and are correctly decorated as Qt Slots for QML integration.
481-489: Verify navigation crop reset doesn't disrupt workflows.The addition of
_reset_crop_settings()on image navigation ensures a clean state for each image. However, this means any in-progress crop adjustments will be lost when navigating away.Please confirm this behavior is intentional and won't frustrate users who might navigate while adjusting crops. Consider whether a warning or "unsaved changes" prompt would improve UX.
1786-1787: LGTM!The histogram update ensures UI consistency after undoing auto white balance, matching the pattern used for auto levels undo.
2768-2780: LGTM!The logic correctly respects the auto-strength mode by checking the return value from
auto_levels()and skipping the save operation when no adjustment is needed. This prevents unnecessary file writes and backup creation.
2841-2850: LGTM!The auto-level strength auto property implementation follows the established pattern for boolean config properties, with proper type conversions and Qt Slot decorators.
3005-3006: LGTM!The tint bias integration correctly adjusts the target A* channel in Lab color space, following the same pattern as the existing warm bias for the B* channel. The implementation aligns with standard color science practices.
| +- `H`: Show RGB Histogram | ||
| +- `I`: Show EXIF Metadata |
There was a problem hiding this comment.
Remove extraneous + prefix from keyboard shortcut entries.
The lines appear to have +- as a prefix, which looks like diff notation accidentally included in the file content. This will display incorrectly in the rendered documentation.
-+- `H`: Show RGB Histogram
-+- `I`: Show EXIF Metadata
+- `H`: Show RGB Histogram
+- `I`: Show EXIF Metadata📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| +- `H`: Show RGB Histogram | |
| +- `I`: Show EXIF Metadata | |
| - `H`: Show RGB Histogram | |
| - `I`: Show EXIF Metadata |
🤖 Prompt for AI Agents
In faststack/faststack.egg-info/PKG-INFO around lines 72 to 73, the keyboard
shortcut entries incorrectly include diff-style '+-' prefixes; remove the
leading '+ ' or '+-' characters so the lines read "H: Show RGB Histogram" and
"I: Show EXIF Metadata" (no leading plus or minus), ensuring the PKG-INFO
content contains only the intended text.
| if not isinstance(crop_box_raw, tuple) or len(crop_box_raw) != 4: | ||
| # Try to convert if it came as list | ||
| try: | ||
| crop_box_raw = tuple(crop_box_raw) if isinstance(crop_box_raw, list) else tuple(crop_box_raw.toVariant()) | ||
| except: | ||
| pass | ||
|
|
||
| if not isinstance(crop_box_raw, tuple) or len(crop_box_raw) != 4: | ||
| self.update_status_message("Invalid crop box") | ||
| return | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Fix bare except clause and add logging.
The bare except at line 2612 suppresses all exceptions without logging, making debugging difficult. This violates error handling best practices.
Apply this diff to improve error handling:
- try:
- crop_box_raw = tuple(crop_box_raw) if isinstance(crop_box_raw, list) else tuple(crop_box_raw.toVariant())
- except:
- pass
+ try:
+ crop_box_raw = tuple(crop_box_raw) if isinstance(crop_box_raw, list) else tuple(crop_box_raw.toVariant())
+ except (TypeError, AttributeError) as e:
+ log.warning("Failed to convert crop_box to tuple: %s", e)Based on coding guidelines, ensure proper error handling with specific exception types.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if not isinstance(crop_box_raw, tuple) or len(crop_box_raw) != 4: | |
| # Try to convert if it came as list | |
| try: | |
| crop_box_raw = tuple(crop_box_raw) if isinstance(crop_box_raw, list) else tuple(crop_box_raw.toVariant()) | |
| except: | |
| pass | |
| if not isinstance(crop_box_raw, tuple) or len(crop_box_raw) != 4: | |
| self.update_status_message("Invalid crop box") | |
| return | |
| if not isinstance(crop_box_raw, tuple) or len(crop_box_raw) != 4: | |
| # Try to convert if it came as list | |
| try: | |
| crop_box_raw = tuple(crop_box_raw) if isinstance(crop_box_raw, list) else tuple(crop_box_raw.toVariant()) | |
| except (TypeError, AttributeError) as e: | |
| log.warning("Failed to convert crop_box to tuple: %s", e) | |
| if not isinstance(crop_box_raw, tuple) or len(crop_box_raw) != 4: | |
| self.update_status_message("Invalid crop box") | |
| return |
🧰 Tools
🪛 Ruff (0.14.8)
2612-2612: Do not use bare except
(E722)
2612-2613: try-except-pass detected, consider logging the exception
(S110)
🤖 Prompt for AI Agents
In faststack/faststack/app.py around lines 2608 to 2618, the bare except at line
~2612 swallows all exceptions and provides no logging; change it to catch the
likely exceptions (e.g., TypeError, AttributeError, ValueError) when attempting
to convert crop_box_raw and log the error before continuing so the subsequent
validity check still runs. Specifically, replace the bare except with an except
(TypeError, AttributeError, ValueError) as e: and call the instance logger
(e.g., self.logger.exception or self.logger.error with the exception) to record
the failure to convert crop_box_raw, then allow the code to fall through so the
"Invalid crop box" status is set when appropriate.
| # 3. Cropping | ||
| crop_box = self.current_edits.get('crop_box') | ||
| if crop_box: | ||
| if crop_box and len(crop_box) == 4: | ||
| width, height = img.size | ||
| # Normalized 0-1000 to pixels | ||
| left = int(crop_box[0] * width / 1000) | ||
| top = int(crop_box[1] * height / 1000) | ||
| right = int(crop_box[2] * width / 1000) | ||
| bottom = int(crop_box[3] * height / 1000) | ||
|
|
||
| # Bounds check | ||
| left = max(0, min(width - 1, left)) | ||
| top = max(0, min(height - 1, top)) | ||
| right = max(left + 1, min(width, right)) | ||
| bottom = max(top + 1, min(height, bottom)) | ||
|
|
||
| img = img.crop((left, top, right, bottom)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "*test*.py" | grep -i "editor\|imaging" | head -20Repository: AlanRockefeller/faststack
Length of output: 111
🏁 Script executed:
rg -l "crop" --type=py | grep -i testRepository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
rg -n "def test.*crop" --type=pyRepository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Also check the structure of imaging tests
fd -t f "test" faststack/faststack/imaging/ 2>/dev/null || echo "No imaging tests found in that exact location"Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Look for pytest test directory
fd -t d "test" faststack/ 2>/dev/null | head -10Repository: AlanRockefeller/faststack
Length of output: 95
🏁 Script executed:
cat -n faststack/faststack/tests/test_editor.pyRepository: AlanRockefeller/faststack
Length of output: 1003
🏁 Script executed:
# Also check the editor.py file to understand the full cropping implementation
wc -l faststack/faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 110
🏁 Script executed:
# Get the cropping method context
sed -n '140,180p' faststack/faststack/imaging/editor.pyRepository: AlanRockefeller/faststack
Length of output: 1850
🏁 Script executed:
# Double-check if there are any other test files that might test crop functionality
rg -l "ImageEditor\|crop" faststack/faststack/tests/ --type=pyRepository: AlanRockefeller/faststack
Length of output: 51
Add pytest test cases for the cropping logic in ImageEditor.
Per coding guidelines, every new branch in imaging/ modules must have focused pytest test cases. The crop bounds checking and normalization logic (lines 150-166) introduces multiple branches—including crop_box validation, normalization from 0-1000 range to pixels, and bounds clamping with max/min operations—that are not currently tested. Add tests covering edge cases such as invalid crop_box values, extreme normalized values, and boundary conditions where left/top/right/bottom need adjustment.
🤖 Prompt for AI Agents
In faststack/faststack/imaging/editor.py around lines 150 to 166, the cropping
logic (validation of crop_box length, normalization from 0-1000 to pixel coords,
and bounds clamping) lacks pytest coverage; add focused tests that call the
ImageEditor crop path and assert expected output images or box coordinates for:
invalid crop_box values (None, wrong length, non-numeric) to ensure no crop is
applied or proper error handling, extreme normalized values (0, 1000, values
outside range like -100, 1100) to verify normalization maps to image edges, and
boundary conditions where left/top/right/bottom would be adjusted (e.g.,
normalized coords that would produce zero-width/height) to confirm clamping
enforces minimum size and stays within image dimensions.
| # Sync with ImageEditor | ||
| self.app_controller.image_editor.set_crop_box(new_value) |
There was a problem hiding this comment.
Add null check before accessing image_editor.
Direct access to self.app_controller.image_editor may raise AttributeError if image_editor hasn't been initialized when the crop box is set early in the lifecycle.
self.current_crop_box_changed.emit(new_value)
# Sync with ImageEditor
- self.app_controller.image_editor.set_crop_box(new_value)
+ if hasattr(self.app_controller, 'image_editor') and self.app_controller.image_editor:
+ self.app_controller.image_editor.set_crop_box(new_value)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Sync with ImageEditor | |
| self.app_controller.image_editor.set_crop_box(new_value) | |
| # Sync with ImageEditor | |
| if hasattr(self.app_controller, 'image_editor') and self.app_controller.image_editor: | |
| self.app_controller.image_editor.set_crop_box(new_value) |
🤖 Prompt for AI Agents
In faststack/faststack/ui/provider.py around lines 736-737, direct access to
self.app_controller.image_editor can raise AttributeError if image_editor is not
yet initialized; add a null/attribute check before calling set_crop_box: verify
image_editor exists and is not None (e.g., hasattr/getattr) and only call
set_crop_box when available, otherwise skip the call (or enqueue the crop update
for later) to avoid runtime exceptions.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.