Fix some bugs#25
Conversation
WalkthroughAdds histogram update throttling, per-image "restacked" metadata with UI exposure and toggle, crop/straighten execution improvements, QML image-editor and histogram UI refactor (including a new SingleChannelHistogram), logging in the image editor, minor imaging/input validation tweaks, test adjustments, and cleanup of debug/config files. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant QML
participant AppController
participant Timer as histogram_timer
participant ImageEditor
participant UIState
User->>QML: interaction (zoom/crop/slider)
QML->>AppController: update_histogram(requestArgs)
AppController->>AppController: store _pending_histogram_args
AppController->>Timer: start/restart 50ms timer
Note over Timer: timer fires after 50ms
Timer->>AppController: _perform_update_histogram()
AppController->>AppController: validate UI visible / not in edit mode
alt zoomed / crop region
AppController->>ImageEditor: request cropped region
end
AppController->>UIState: set histogramData / clip counts
UIState->>QML: metadataChanged / histogramData notified
QML->>QML: SingleChannelHistogram re-renders channels
sequenceDiagram
actor User
participant QML
participant AppController
participant EntryMetadata
participant Sidecar
participant UIState
User->>QML: trigger restack (Stack Source Raws / Helicon)
QML->>AppController: stack_source_raws() / toggle_restacked()
AppController->>EntryMetadata: set restacked = true, restacked_date = today
AppController->>Sidecar: sidecar.save(currentEntry)
AppController->>UIState: emit metadataChanged
UIState->>QML: isRestacked / restackedDate update
QML->>QML: display "Restacked on [date]"
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
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: 5
🧹 Nitpick comments (6)
faststack/faststack/config.py (1)
18-20: Add more comprehensive documentation for the new INI keys.While inline comments are present, the coding guidelines require documenting new INI keys. Consider adding more detailed documentation that explains:
- How these auto-level parameters interact with each other
- When and how the auto-level feature is triggered
- The practical impact of adjusting these values
The current comments describe what each value is but not when or how they're used in the application.
Based on coding guidelines that require documenting new INI keys in
config.py.faststack/faststack/imaging/prefetch.py (1)
417-418: Timing variable captured but not logged.The
t_after_copytimestamp is now captured in both fallback paths (lines 418, 451) to align with the primary ICC path. However, unlike the primary ICC path (line 388-392), the fallback paths don't include this timing in their log messages (lines 422-425, 455-457).🔎 Optional: Include copy timing in fallback logs for consistency
For the ICC fallback path:
if self.debug: decoder = "TurboJPEG" if TURBO_AVAILABLE else "Pillow" - log.info("ICC fallback decode timing for index %d (%s): read=%.3fs, decode=%.3fs, total=%.3fs, size=%dx%d", + log.info("ICC fallback decode timing for index %d (%s): read=%.3fs, decode=%.3fs, copy=%.3fs, total=%.3fs, size=%dx%d", index, decoder, t_after_fallback_read - t_before_fallback_read, t_after_fallback_decode - t_after_fallback_read, + t_after_copy - t_after_fallback_decode, - t_after_fallback_decode - t_start, w, h) + t_after_copy - t_start, w, h)For the no-ICC-profile path:
if self.debug: decoder = "TurboJPEG" if TURBO_AVAILABLE else "Pillow" - log.info("Standard decode timing (no ICC profile) for index %d (%s): read=%.3fs, decode=%.3fs, total=%.3fs, size=%dx%d", + log.info("Standard decode timing (no ICC profile) for index %d (%s): read=%.3fs, decode=%.3fs, copy=%.3fs, total=%.3fs, size=%dx%d", index, decoder, t_after_read - t_before_read, t_after_decode - t_after_read, + t_after_copy - t_after_decode, - t_after_decode - t_start, w, h) + t_after_copy - t_start, w, h)Also applies to: 450-451
faststack/faststack/imaging/editor.py (1)
57-57: Consider using logging.exception for better stack traces.For exceptions caught in except blocks,
logging.exceptionautomatically includes the stack trace, making debugging easier thanlogging.error.Example refactor
except OSError as e: - log.error(f"Failed to create backup: {e}") + log.exception(f"Failed to create backup: {e}") return NoneAlso applies to: 235-235, 528-528, 636-636, 645-645
faststack/faststack/qml/Components.qml (1)
657-801: Rotate branch now correctly avoids clobbering crop box; minor naming cleanup possibleThe new
cb/startBoxusage and earlyreturnin therotatebranch fix the prior issue where rotation could overwrite the crop box while dragging. The move/resize path in theelsenow only runs for non-rotate drag modes, which is correct.Very minor: in the “Seed cropBoxStart variables” block you set
cropBoxStartLeft/TopfromstartBoxandcropBoxStartRight/Bottomfrombox. Since both areuiState.currentCropBox, using one variable consistently would reduce reader confusion.faststack/faststack/tests/test_editor_rotation.py (1)
84-87: Tests now correctly target the export path and allow for rotation roundingSwitching to
_apply_edits(..., for_export=True)and comparingres_avsres_bwith a small pixel tolerance is appropriate for rotated/cropped images where rounding can differ slightly. The broader 60–80px bounds and< 5deltas look reasonable.If you find this test flaky in practice, consider inlining a short comment near the
< 5checks explaining why that specific tolerance was chosen (rounding + PIL/NumPy differences), but it’s not strictly necessary.Also applies to: 132-136, 180-185
faststack/faststack/app.py (1)
1822-1839: Batch delete landing index logic is fine; drop unusedprevious_indexThe revised landing logic after batch deletion—using
min_deleted_indexand clamping to[0, len(image_files)-1]—is reasonable and matches the intent of staying conceptually at the start of the deleted range.
previous_index = self.current_index(Line 1838) is now unused and can be removed to satisfy Ruff (F841) and reduce noise.Small cleanup
- min_deleted_index = min(sorted_indices) - - previous_index = self.current_index # This might be inside the deleted range + min_deleted_index = min(sorted_indices)Also applies to: 1886-1895
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
faststack/faststack/tests/dummy_images/test.jpgis excluded by!**/*.jpg
📒 Files selected for processing (27)
faststack/ChangeLog.mdfaststack/README.mdfaststack/faststack/app.pyfaststack/faststack/config.pyfaststack/faststack/debug_output.txtfaststack/faststack/faststack.jsonfaststack/faststack/imaging/cache.pyfaststack/faststack/imaging/editor.pyfaststack/faststack/imaging/jpeg.pyfaststack/faststack/imaging/prefetch.pyfaststack/faststack/io/helicon.pyfaststack/faststack/models.pyfaststack/faststack/next.promptfaststack/faststack/qml/Components.qmlfaststack/faststack/qml/DeleteBatchDialog.qmlfaststack/faststack/qml/FilterDialog.qmlfaststack/faststack/qml/HistogramWindow.qmlfaststack/faststack/qml/ImageEditorDialog.qmlfaststack/faststack/qml/Main.qmlfaststack/faststack/qml/SingleChannelHistogram.qmlfaststack/faststack/tests/test_editor.pyfaststack/faststack/tests/test_editor_rotation.pyfaststack/faststack/tests/test_executable_validator.pyfaststack/faststack/tests/test_pairing.pyfaststack/faststack/ui/provider.pyfaststack/pyproject.tomlfaststack/test_output.txt
💤 Files with no reviewable changes (3)
- faststack/faststack/debug_output.txt
- faststack/faststack/faststack.json
- faststack/faststack/next.prompt
🧰 Additional context used
📓 Path-based instructions (7)
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/tests/test_executable_validator.pyfaststack/faststack/imaging/jpeg.pyfaststack/faststack/imaging/editor.pyfaststack/faststack/tests/test_editor_rotation.pyfaststack/faststack/config.pyfaststack/faststack/models.pyfaststack/faststack/tests/test_editor.pyfaststack/faststack/imaging/cache.pyfaststack/faststack/imaging/prefetch.pyfaststack/faststack/io/helicon.pyfaststack/faststack/ui/provider.pyfaststack/faststack/tests/test_pairing.pyfaststack/faststack/app.py
faststack/faststack/**/test_*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
faststack/faststack/**/test_*.py: Place new test cases intest_*.pyfiles located beside the modules being tested
Mock filesystem and Qt dependencies in tests to keep them deterministic
Mark tests that perform long disk scans withpytest.mark.slow
Files:
faststack/faststack/tests/test_executable_validator.pyfaststack/faststack/tests/test_editor_rotation.pyfaststack/faststack/tests/test_editor.pyfaststack/faststack/tests/test_pairing.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/jpeg.pyfaststack/faststack/imaging/editor.pyfaststack/faststack/imaging/cache.pyfaststack/faststack/imaging/prefetch.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/io/**/*.py
📄 CodeRabbit inference engine (faststack/faststack/AGENTS.md)
faststack/faststack/io/**/*.py: Cover every new branch inimaging/orio/modules with focusedpytesttest cases
Validate all user-supplied paths before invoking external tools such as Helicon Focus
Files:
faststack/faststack/io/helicon.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/**/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 (6)
📚 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/**/test_*.py : Mock filesystem and Qt dependencies in tests to keep them deterministic
Applied to files:
faststack/faststack/tests/test_executable_validator.pyfaststack/faststack/tests/test_editor.pyfaststack/faststack/tests/test_pairing.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: `ui/` module contains providers and keystroke maps shared with QML
Applied to files:
faststack/faststack/qml/Main.qmlfaststack/faststack/qml/HistogramWindow.qml
📚 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: `imaging/` module manages decode workflow, metadata, editing, and ICC caches
Applied to files:
faststack/faststack/imaging/editor.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/**/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/**/*.py : Prefer relative paths in configuration and file handling
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/imaging/editor.py (2)
faststack/faststack/config.py (1)
save(76-84)faststack/faststack/io/sidecar.py (1)
save(61-90)
faststack/faststack/ui/provider.py (2)
faststack/faststack/app.py (3)
get_current_metadata(723-756)update_histogram(2555-2571)set_crop_box(2476-2481)faststack/faststack/imaging/editor.py (1)
set_crop_box(536-538)
🪛 Ruff (0.14.10)
faststack/faststack/imaging/editor.py
57-57: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
235-235: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
526-526: Consider moving this statement to an else block
(TRY300)
528-528: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
590-590: Do not catch blind exception: Exception
(BLE001)
624-624: Do not catch blind exception: Exception
(BLE001)
636-636: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
645-645: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
faststack/faststack/ui/provider.py
623-623: Do not catch blind exception: Exception
(BLE001)
faststack/faststack/app.py
1838-1838: Local variable previous_index is assigned to but never used
Remove assignment to unused variable previous_index
(F841)
2714-2715: try-except-pass detected, consider logging the exception
(S110)
2714-2714: Do not catch blind exception: Exception
(BLE001)
2735-2735: Redundant exception object included in logging.exception call
(TRY401)
2954-2955: try-except-pass detected, consider logging the exception
(S110)
2954-2954: Do not catch blind exception: Exception
(BLE001)
2957-2957: Multiple statements on one line (colon)
(E701)
🔇 Additional comments (22)
faststack/pyproject.toml (1)
27-32: Good practice to separate dev dependencies.The addition of optional dev dependencies follows Python packaging best practices and keeps production installations lean. The pytest 8.0.2 version is available on PyPI and the
>=8.0,<9.0constraint is sound for maintaining stability. Note that pytest 9.0.2 was released in December 2025; consider testing compatibility with the latest major version if you wish to adopt it.faststack/faststack/io/helicon.py (1)
25-26: LGTM! Documentation clarifies caller responsibility.The updated docstring correctly documents that the caller must clean up the temporary file after Helicon Focus completes processing, which aligns with the
delete=Falseparameter on line 49.faststack/faststack/imaging/cache.py (1)
39-39: LGTM! Improved eviction logging message.The updated log message provides clearer information about cache eviction and is consistent with the logging format used in
__setitem__(line 32).faststack/faststack/tests/test_pairing.py (1)
6-6: LGTM! Removed unused import.The
patchimport has been correctly removed as it's not used anywhere in this test file. All tests only requireMagicMock.faststack/faststack/tests/test_executable_validator.py (1)
89-89: LGTM! More explicit patch syntax.Using the keyword argument
new='nt'instead of a positional argument makes the intent clearer and improves readability.faststack/faststack/qml/DeleteBatchDialog.qml (1)
83-83: LGTM! Consistent color binding.The change ensures all button text uses the same
textColorproperty, maintaining visual consistency across the dialog.faststack/README.md (1)
18-18: LGTM! Documentation formatting improved.The line formatting makes the feature descriptions clearer and easier to read.
faststack/ChangeLog.md (1)
16-16: LGTM! Changelog formatting improved.The formatting changes improve readability and organization of the changelog entries.
Also applies to: 106-107, 109-109
faststack/faststack/ui/provider.py (4)
295-305: LGTM! Restacked metadata properties correctly implemented.The new
isRestackedandrestackedDateproperties follow the established pattern for other metadata properties (stacked, uploaded, edited) and correctly delegate toget_current_metadata().
621-624: Good defensive programming for histogram updates.Wrapping the histogram update in a try-except block prevents UI disruption if histogram generation fails. While the static analysis tool flags catching blind
Exception, this is appropriate for UI code where robustness is prioritized over granular error handling.
626-646: Comprehensive editor state reset implementation.The
reset_editor_stateslot correctly resets all 15 editor-related properties to their default values, covering brightness, contrast, saturation, white balance, sharpness, rotation, exposure, highlights, shadows, vibrance, vignette, blacks, whites, clarity, crop settings, and aspect ratio.
788-789: Good guard check prevents null pointer errors.The hasattr check and existence validation before calling
set_crop_boxprevents crashes whenimage_editoris not yet initialized.faststack/faststack/imaging/editor.py (1)
1-1: LGTM! Logging integration improves observability.The migration from print statements to structured logging using
log.error,log.warning, andlog.infosignificantly improves debugging capabilities across backup creation, image loading, editing, saving, and timestamp restoration workflows.As per coding guidelines, every new Python module should expose a top-level logger.
Also applies to: 16-16, 57-57, 235-235, 523-523, 528-528, 556-556, 588-591, 609-625, 631-636, 645-645, 653-653
faststack/faststack/tests/test_editor.py (1)
24-24: LGTM! Test improvements handle filesystem realities.The changes appropriately handle:
- Path comparison via string representation (Line 24) - avoids potential Path object identity issues
- Increased mtime tolerance to 2 seconds (Line 27) - accommodates filesystem timestamp granularity (FAT32 and some network filesystems have 2-second precision)
Also applies to: 27-27
faststack/faststack/models.py (1)
24-25: LGTM! Metadata fields follow established patterns.The new
restackedandrestacked_datefields are correctly implemented following the same pattern as existing metadata pairs (stacked/stacked_date, uploaded/uploaded_date, edited/edited_date).faststack/faststack/qml/FilterDialog.qml (2)
76-76: LGTM! Defensive null checks improve robustness.The guard checks for
controllerexistence and method availability prevent crashes if the controller is not properly initialized or if methods are missing. This aligns with the broader UI stability improvements in this PR.Also applies to: 82-84, 87-89
51-54: Nice UI polish on the TextField styling.The updated background styling (lighter color, visible border, rounded corners) improves visual clarity and follows modern UI design patterns.
faststack/faststack/qml/SingleChannelHistogram.qml (1)
1-131: Single-channel histogram component is well-factored and robust to empty dataThe component cleanly encapsulates per-channel rendering, handles null/empty data and zero
maxValsafely, and exposes all the right knobs (colors, minimal mode, clip/pre-clip counts) for reuse in both the histogram window and editor.faststack/faststack/qml/HistogramWindow.qml (1)
11-12: Histogram window wiring to per-channel component looks solidThe window now sensibly supports small sizes, consumes
uiState.histogramDatavia threeSingleChannelHistograminstances, and the H key handler correctly marks events as accepted to avoid propagation. This matches the new backend histogram structure.Also applies to: 20-24, 52-56, 57-100
faststack/faststack/app.py (2)
181-187: Histogram throttling and zoom-region logic look correctThe new
histogram_timer+_pending_histogram_argssetup properly coalesces rapid calls toupdate_histogram, caps work to ~20 FPS, and safely bails out when neither the histogram window nor the editor is visible. The ROI computation whenzoom > 1.1 and ui_state.isZoomeduses decoded dimensions and pan/scale coherently and falls back to the full frame otherwise.Also applies to: 2554-2597
675-697: Restacked metadata wiring is consistent with existing flags
toggle_restacked()mirrors the existing uploaded/edited/stacked toggles (including date stamping, sidecar save, cache invalidation, andsync_ui_state()), andget_current_metadata()now exposesrestackedandrestacked_date. Marking images as restacked instack_source_raws()on Helicon success completes the flow so the new footer label in QML can reflect this state.Also applies to: 742-752, 2915-2924
faststack/faststack/qml/Main.qml (1)
634-649: Editor toggle & restack footer wiring are correct; no E-key conflicts detectedThe new
Shortcutfor"E"cleanly centralizes editor open/close in QML withQt.ApplicationShortcutcontext and correctly callscontroller.load_image_for_editing()only on open. The Ctrl+S handler is properly scoped touiState.isEditorOpenand prevents event propagation. The "Restacked on …" footer label is correctly wired to displayuiState.restackedDateand only shows whenimageCount > 0andisRestackedis true.No conflicting plain E-key bindings exist in the codebase, and the implementation aligns with the new restack metadata infrastructure.
|
|
||
| import numpy as np | ||
| from PIL import Image as PILImage, ImageCms | ||
| from PySide6.QtCore import QTimer |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check if the file exists and examine its content
cat -n faststack/faststack/imaging/prefetch.pyRepository: AlanRockefeller/faststack
Length of output: 32936
Remove unused QTimer import.
QTimer is imported on line 14 but is never used in this file. The timing implementation uses time.perf_counter() instead. Remove the unused import.
🤖 Prompt for AI Agents
In faststack/faststack/imaging/prefetch.py around line 14, the file imports
QTimer from PySide6.QtCore but never uses it; remove the unused QTimer import
(delete the "from PySide6.QtCore import QTimer" line) so the module no longer
contains an unused import and linting warnings are resolved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
faststack/faststack/qml/Components.qml (1)
660-676: Consider consolidating variable names for clarity.Within the same rotation handle hit-test block, you read
uiState.currentCropBoxtwice and store it in two different variables (cbat line 660 andstartBoxat line 670). While both serve different purposes (aspect calculation vs. seeding start positions), using the same variable name would improve readability since they reference identical data at the same point in time.🔎 Suggested consolidation
var k = mainMouseArea.mapFromItem(rotateKnob, rotateKnob.width/2, rotateKnob.height/2) var dxk = mouse.x - k.x var dyk = mouse.y - k.y var distk = Math.sqrt(dxk*dxk + dyk*dyk) if (distk < 22 * Screen.devicePixelRatio) { // a little forgiving cropDragMode = "rotate" // crop center in mainMouseArea coords -> Changed to IMAGE center to avoid feedback loop var c = mainMouseArea.mapFromItem(mainImage, mainImage.width/2, mainImage.height/2) cropStartAngle = Math.atan2(mouse.y - c.y, mouse.x - c.x) * 180 / Math.PI cropStartRotation = cropRotation // Calculate start aspect ratio (in pixels) + var cropBox = uiState.currentCropBox - var cb = uiState.currentCropBox - if (cb && cb.length === 4) { - var boxW = (cb[2] - cb[0]) / 1000 * mainImage.width - var boxH = (cb[3] - cb[1]) / 1000 * mainImage.height + if (cropBox && cropBox.length === 4) { + var boxW = (cropBox[2] - cropBox[0]) / 1000 * mainImage.width + var boxH = (cropBox[3] - cropBox[1]) / 1000 * mainImage.height cropStartAspect = boxW / boxH } // Seed cropBoxStart variables - var startBox = uiState.currentCropBox - if (startBox && startBox.length === 4) { - cropBoxStartLeft = startBox[0] - cropBoxStartTop = startBox[1] - cropBoxStartRight = startBox[2] - cropBoxStartBottom = startBox[3] + if (cropBox && cropBox.length === 4) { + cropBoxStartLeft = cropBox[0] + cropBoxStartTop = cropBox[1] + cropBoxStartRight = cropBox[2] + cropBoxStartBottom = cropBox[3] } isCropDragging = true return }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
faststack/faststack/qml/Components.qml
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: CR
Repo: AlanRockefeller/faststack PR: 0
File: faststack/faststack/AGENTS.md:0-0
Timestamp: 2025-12-08T04:46:41.632Z
Learning: `imaging/` module manages decode workflow, metadata, editing, and ICC caches
🔇 Additional comments (2)
faststack/faststack/qml/Components.qml (2)
752-752: Verify that decoupling aspect ratio from rotation is the intended behavior.The change from
pendingAspect = cropStartAspecttopendingAspect = -1means rotation no longer attempts to maintain the crop box's aspect ratio. This value is passed tocontroller.set_straighten_angle()(line 574), which likely interprets-1as "no aspect constraint."Please confirm this is the desired behavior. If users expect the crop box to maintain its aspect ratio during rotation (especially when an aspect ratio preset is selected), this change might cause unexpected results.
758-760: Excellent fix: Early return prevents state corruption during rotation.The early return after rotation handling (line 759) effectively prevents the move/resize logic from overwriting the crop box state during rotation operations. This separation of concerns aligns with the PR objective to fix crop/rotate bugs and ensures that rotation and move/resize operations don't interfere with each other.
The explicit
elsebranch (line 760) makes the code flow clearer and more maintainable.
1 participant
Fix histogram and bugs in crop/rotate
Summary by CodeRabbit
New Features
Bug Fixes
Performance
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.