Fix bug in cropping, update Turbojpeg search#90
Merged
AlanRockefeller merged 2 commits intomainfrom May 4, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThree independent changes: (1) TurboJPEG discovery is cached per candidate set with consolidated one-time fallback warnings and platform-specific install hints; (2) QML crop overlay was refactored from imperative rect updates to binding-driven geometry with revision tracking; (3) UIState QML properties now expose concrete JS-friendly types ( ChangesTurboJPEG Initialization Caching
QML Crop Overlay — Declarative Binding Refactor
UIState QML Type Refinements
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 2
🧹 Nitpick comments (2)
faststack/qml/Components.qml (1)
361-361: 💤 Low valueUnused property can be removed.
The
cropBoxproperty appears to be dead code now that_liveCropBox()is used for all crop box access. The new revision-based pattern supersedes this direct property binding.🧹 Remove unused property
- property var cropBox: loupeView.uiStateRef ? loupeView.uiStateRef.currentCropBox : [0, 0, 1000, 1000]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/qml/Components.qml` at line 361, The property declaration "property var cropBox" is dead now that the code uses the revision-based getter _liveCropBox(); remove the unused property declaration (property var cropBox ...) from Components.qml and update any remaining usages to call _liveCropBox() (or use the existing uiStateRef.currentCropBox via _liveCropBox()) so nothing references the removed symbol; ensure no other bindings depend on cropBox before committing the deletion.faststack/ui/provider.py (1)
1228-1235: 💤 Low valueMinor internal type inconsistency.
The getter converts to
list, but internally_current_crop_boxis initialized as a list (line 322) while the setter converts incoming values to tuple (line 1251). Sincelist != tuplein Python even when elements match, the comparison at line 1273 will beTrueon the first property set, causing a spurious signal emission.Consider initializing with a tuple for consistency:
♻️ Optional: Initialize as tuple for consistency
- self._current_crop_box = [0, 0, 1000, 1000] + self._current_crop_box = (0, 0, 1000, 1000)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/ui/provider.py` around lines 1228 - 1235, The getter currentCropBox returns a list while the internal attribute _current_crop_box is set/compared as a tuple by the setter, causing the first set to look different and emit a spurious change signal; fix by making the internal representation consistent—initialize _current_crop_box as a tuple (e.g., () instead of []) and ensure the setter that currently converts incoming values to tuple keeps that behavior so comparisons against _current_crop_box match, or alternatively change the setter to store a list; update only _current_crop_box initialization to tuple to match the setter and keep the currentCropBox getter converting to list for QML.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@faststack/imaging/turbo.py`:
- Around line 116-120: The warning text passed to _warn_fallback_once was
changed from "PyTurboJPEG not found" to "PyTurboJPEG is not installed", which
breaks the existing unit test that asserts the exact substring; either revert
the message back to the original "PyTurboJPEG not found" in the call to
_warn_fallback_once (keeping _install_hint() as-is), or update the test named
test_turbo.py that asserts the old substring to expect the new "PyTurboJPEG is
not installed" wording instead — modify whichever you choose so the string in
the _warn_fallback_once(...) call and the test assertion match exactly.
- Around line 100-106: The current _warn_fallback_once uses a single global
boolean _fallback_warning_emitted which suppresses all subsequent warnings
regardless of message; change it to track deduplication per message by replacing
the boolean with a set (e.g., _fallback_warnings_emitted: set[str]) and update
_warn_fallback_once to check if the given message is already in that set, add it
when first emitted, and then call log.warning(message, *args) only for new
messages; keep the function name _warn_fallback_once and the log.warning call so
callers need no change.
---
Nitpick comments:
In `@faststack/qml/Components.qml`:
- Line 361: The property declaration "property var cropBox" is dead now that the
code uses the revision-based getter _liveCropBox(); remove the unused property
declaration (property var cropBox ...) from Components.qml and update any
remaining usages to call _liveCropBox() (or use the existing
uiStateRef.currentCropBox via _liveCropBox()) so nothing references the removed
symbol; ensure no other bindings depend on cropBox before committing the
deletion.
In `@faststack/ui/provider.py`:
- Around line 1228-1235: The getter currentCropBox returns a list while the
internal attribute _current_crop_box is set/compared as a tuple by the setter,
causing the first set to look different and emit a spurious change signal; fix
by making the internal representation consistent—initialize _current_crop_box as
a tuple (e.g., () instead of []) and ensure the setter that currently converts
incoming values to tuple keeps that behavior so comparisons against
_current_crop_box match, or alternatively change the setter to store a list;
update only _current_crop_box initialization to tuple to match the setter and
keep the currentCropBox getter converting to list for QML.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e855e6e6-d904-4392-b91b-3bbd32dd3e2f
📒 Files selected for processing (3)
faststack/imaging/turbo.pyfaststack/qml/Components.qmlfaststack/ui/provider.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Improvements
Tests