Clean up QML#75
Merged
AlanRockefeller merged 2 commits intomainfrom Apr 13, 2026
Merged
Conversation
WalkthroughRefactors many QML components to use bound, component-scoped references ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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. 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
faststack/qml/Components.qml (1)
514-521:⚠️ Potential issue | 🟠 MajorGuard
onDisplaySizeChanged()untiluiStateRefis initialized.This function is invoked from
mainImage.Component.onCompleted, butuiStateRefis assigned later inloupeView.Component.onCompleted. In QML, child component completion handlers can fire before parent handlers, creating a startup race condition whereloupeView.uiStateRefis stillnullwhenreportDisplaySize()executes.Minimal fix
function reportDisplaySize() { - if (imageViewport.width > 0 && imageViewport.height > 0) { + if (loupeView.uiStateRef && imageViewport.width > 0 && imageViewport.height > 0) { var dpr = Screen.devicePixelRatio loupeView.uiStateRef.onDisplaySizeChanged( Math.round(imageViewport.width * dpr), Math.round(imageViewport.height * dpr) ) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/qml/Components.qml` around lines 514 - 521, reportDisplaySize() can run before loupeView.uiStateRef is initialized (called from mainImage.Component.onCompleted while loupeView.Component.onCompleted sets uiStateRef later), causing a null dereference; guard the call by checking that loupeView.uiStateRef is non-null before invoking onDisplaySizeChanged (i.e., in reportDisplaySize() verify loupeView.uiStateRef !== null/undefined and return or defer if not initialized) and keep the Math.round(...) DPI math unchanged so the update only runs when the uiStateRef is ready.
🧹 Nitpick comments (3)
faststack/qml/ExifDialog.qml (1)
21-21: This pattern is systemic—consider addressing across all affected QML files, not just ExifDialog.Line 21 uses global symbol probing with
typeof controller !== "undefined" ? controller : null. While this makes the fallback explicit, the same pattern appears in 10+ other QML files (ThumbnailTile, JumpToImageDialog, FilterDialog, HistogramWindow, DeleteBatchDialog, DarkenToolPanel, BatchProgressDialog, ThumbnailGridView). If refactoring to explicit parent injection, apply consistently across the codebase for maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/qml/ExifDialog.qml` at line 21, The code uses global symbol probing (property var controllerRef: typeof controller !== "undefined" ? controller : null) which is fragile; replace this pattern with explicit parent injection: add a required property on the root QML component (e.g., property var controllerRef) and have parents pass controller when instantiating the component (or use property alias to an id on the parent), update ExifDialog.qml (replace the typeof check with a plain property var controllerRef) and apply the same change to the other affected components (ThumbnailTile, JumpToImageDialog, FilterDialog, HistogramWindow, DeleteBatchDialog, DarkenToolPanel, BatchProgressDialog, ThumbnailGridView) to ensure consistent, maintainable wiring of controller references across the codebase.faststack/tests/thumbnail_view/test_qml_delegate_contract.py (1)
25-30: Make the QML contract scan less brittle.This regex treats every
required propertyanywhere inThumbnailTile.qmlas part of the model/delegate contract and only matches lines indented with exactly four spaces. A formatting-only cleanup or a nested helper delegate with its ownrequired propertywill fail this test even when the actualThumbnailTilecontract is unchanged.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/thumbnail_view/test_qml_delegate_contract.py` around lines 25 - 30, The test _thumbnail_tile_required_properties currently uses a brittle regex that only matches lines with exactly four spaces; update it to first extract the ThumbnailTile component block from ThumbnailTile.qml (locate the component start like "ThumbnailTile" or the root QML component and its matching braces) and then run a looser regex such as r"^\s*required property \w+ (\w+)" with MULTILINE on that extracted block so you only capture top-level required properties for the tile delegate while accepting any indentation changes.faststack/qml/ThumbnailTile.qml (1)
31-50: Optional follow-up: collapse the mirror layer.This works for the lint cleanup, but the
tile*proxy properties now duplicate the entire delegate contract. A later pass could bind the UI directly to the required roles to remove one maintenance layer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/qml/ThumbnailTile.qml` around lines 31 - 50, ThumbnailTile.qml currently duplicates the entire delegate contract via the tile* proxy properties (e.g., tileIndex, tileFilePath, tileIsFolder, tileThumbnailSource, etc.); collapse this mirror layer by removing the redundant tile* properties and rewire the UI bindings to the original delegate properties (index, filePath, fileName, isFolder, isStacked, isUploaded, isEdited, isRestacked, isFavorite, isTodo, isInBatch, isCurrent, thumbnailSource, folderStats, isSelected, isParentFolder, hasBackups, hasDeveloped), or keep only the small subset of tile* proxies actually used and update consumers to bind directly to the original names so the component no longer maintains a duplicated contract.
🤖 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/qml/SettingsDialog.qml`:
- Around line 671-679: The SpinBox value is only initialized inside
prefetchRadiusLoader.onLoaded, so reopening the dialog can show a stale control
value; update the loader's "value" whenever the dialog is opened or when
settingsDialog.prefetchRadius changes. Modify code that opens the dialog (or add
an onVisible/onOpened handler on settingsDialog) to call
settingsDialog.setLoaderProperty(prefetchRadiusLoader, "value",
settingsDialog.prefetchRadius) unconditionally, and/or add a binding or observer
from settingsDialog.prefetchRadius to call setLoaderProperty so the
styledSpinBox (via prefetchRadiusLoader) is always synced; keep the existing
connectLoaderSignal("valueChanged") handler to write back into
settingsDialog.prefetchRadius.
---
Outside diff comments:
In `@faststack/qml/Components.qml`:
- Around line 514-521: reportDisplaySize() can run before loupeView.uiStateRef
is initialized (called from mainImage.Component.onCompleted while
loupeView.Component.onCompleted sets uiStateRef later), causing a null
dereference; guard the call by checking that loupeView.uiStateRef is non-null
before invoking onDisplaySizeChanged (i.e., in reportDisplaySize() verify
loupeView.uiStateRef !== null/undefined and return or defer if not initialized)
and keep the Math.round(...) DPI math unchanged so the update only runs when the
uiStateRef is ready.
---
Nitpick comments:
In `@faststack/qml/ExifDialog.qml`:
- Line 21: The code uses global symbol probing (property var controllerRef:
typeof controller !== "undefined" ? controller : null) which is fragile; replace
this pattern with explicit parent injection: add a required property on the root
QML component (e.g., property var controllerRef) and have parents pass
controller when instantiating the component (or use property alias to an id on
the parent), update ExifDialog.qml (replace the typeof check with a plain
property var controllerRef) and apply the same change to the other affected
components (ThumbnailTile, JumpToImageDialog, FilterDialog, HistogramWindow,
DeleteBatchDialog, DarkenToolPanel, BatchProgressDialog, ThumbnailGridView) to
ensure consistent, maintainable wiring of controller references across the
codebase.
In `@faststack/qml/ThumbnailTile.qml`:
- Around line 31-50: ThumbnailTile.qml currently duplicates the entire delegate
contract via the tile* proxy properties (e.g., tileIndex, tileFilePath,
tileIsFolder, tileThumbnailSource, etc.); collapse this mirror layer by removing
the redundant tile* properties and rewire the UI bindings to the original
delegate properties (index, filePath, fileName, isFolder, isStacked, isUploaded,
isEdited, isRestacked, isFavorite, isTodo, isInBatch, isCurrent,
thumbnailSource, folderStats, isSelected, isParentFolder, hasBackups,
hasDeveloped), or keep only the small subset of tile* proxies actually used and
update consumers to bind directly to the original names so the component no
longer maintains a duplicated contract.
In `@faststack/tests/thumbnail_view/test_qml_delegate_contract.py`:
- Around line 25-30: The test _thumbnail_tile_required_properties currently uses
a brittle regex that only matches lines with exactly four spaces; update it to
first extract the ThumbnailTile component block from ThumbnailTile.qml (locate
the component start like "ThumbnailTile" or the root QML component and its
matching braces) and then run a looser regex such as r"^\s*required property \w+
(\w+)" with MULTILINE on that extracted block so you only capture top-level
required properties for the tile delegate while accepting any indentation
changes.
🪄 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: b2f0bdec-08e7-4412-b4b6-a9911270a138
📒 Files selected for processing (16)
faststack/qml/BatchProgressDialog.qmlfaststack/qml/Components.qmlfaststack/qml/DarkenToolPanel.qmlfaststack/qml/DeleteBatchDialog.qmlfaststack/qml/ExifDialog.qmlfaststack/qml/FilterDialog.qmlfaststack/qml/HistogramWindow.qmlfaststack/qml/ImageEditorDialog.qmlfaststack/qml/JumpToImageDialog.qmlfaststack/qml/Main.qmlfaststack/qml/MenuActionItem.qmlfaststack/qml/SettingsDialog.qmlfaststack/qml/ThumbnailGridView.qmlfaststack/qml/ThumbnailTile.qmlfaststack/tests/thumbnail_view/test_qml_delegate_contract.pyfaststack/thumbnail_view/model.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
faststack/tests/thumbnail_view/test_qml_delegate_contract.py (1)
34-36: Addstrict=Truetozip()call to catch parser divergence.The two sequences (
root_body.splitlines()andsanitized_root_body.splitlines()) should always have equal length and line count, since_sanitize_qml()preserves character count and newlines. Addingstrict=Truemakes this assumption explicit and will catch bugs if the sanitization or extraction logic ever breaks.Proposed patch
- for raw_line, sanitized_line in zip( - root_body.splitlines(), sanitized_root_body.splitlines() - ): + for raw_line, sanitized_line in zip( + root_body.splitlines(), sanitized_root_body.splitlines(), strict=True + ):The project requires Python ≥3.11, which fully supports
strict=True(available since 3.10).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/tests/thumbnail_view/test_qml_delegate_contract.py` around lines 34 - 36, The zip over root_body.splitlines() and sanitized_root_body.splitlines() should use strict=True to fail fast on any length mismatch; update the zip(...) call in the loop comparing root_body.splitlines() and sanitized_root_body.splitlines() (in the test that validates _sanitize_qml behavior) to zip(..., strict=True) so any parser/sanitizer divergence raises immediately.faststack/qml/SettingsDialog.qml (1)
986-998: Consider adding the same focus guard used by other SpinBox bindings.
awbTintBiasLoaderkeeps itsvaluebinding active while focused, unlike nearby controls. Aligning behavior can avoid edit-time churn and keep interaction consistent.♻️ Suggested small consistency patch
Binding { target: awbTintBiasLoader.item property: "value" value: settingsDialog.awbTintBias - when: awbTintBiasLoader.item + when: awbTintBiasLoader.item + && !settingsDialog.loaderProperty(awbTintBiasLoader, "activeFocus", false) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/qml/SettingsDialog.qml` around lines 986 - 998, The Binding on awbTintBiasLoader.item.value keeps updating while the SpinBox is focused; mirror other SpinBox bindings by adding a focus guard so the Binding is only active when the control exists and is not focused. Update the Binding's when expression to require awbTintBiasLoader.item && !awbTintBiasLoader.item.focus (or activeFocus if your codebase uses that), and ensure any connectLoaderSignal handler (the "valueChanged" callback that sets settingsDialog.awbTintBias) continues to sync user edits while focused but does not re-enable the programmatic binding until focus is lost.
🤖 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/qml/SettingsDialog.qml`:
- Around line 65-68: Remove the Component.onCompleted block that assigns uiState
and controller into settingsDialog (the lines where settingsDialog.uiStateRef =
uiState and settingsDialog.controllerRef = controller) and instead declare
defensive properties on settingsDialog using the typeof pattern so uiStateRef
and controllerRef default to null when context properties are absent (i.e., set
settingsDialog.uiStateRef and settingsDialog.controllerRef via typeof uiState
!== "undefined" ? uiState : null and typeof controller !== "undefined" ?
controller : null); this matches the pattern used by
JumpToImageDialog/HistogramWindow/FilterDialog/ExifDialog/DeleteBatchDialog and
ensures safe fallback without runtime assignment in Component.onCompleted.
---
Nitpick comments:
In `@faststack/qml/SettingsDialog.qml`:
- Around line 986-998: The Binding on awbTintBiasLoader.item.value keeps
updating while the SpinBox is focused; mirror other SpinBox bindings by adding a
focus guard so the Binding is only active when the control exists and is not
focused. Update the Binding's when expression to require awbTintBiasLoader.item
&& !awbTintBiasLoader.item.focus (or activeFocus if your codebase uses that),
and ensure any connectLoaderSignal handler (the "valueChanged" callback that
sets settingsDialog.awbTintBias) continues to sync user edits while focused but
does not re-enable the programmatic binding until focus is lost.
In `@faststack/tests/thumbnail_view/test_qml_delegate_contract.py`:
- Around line 34-36: The zip over root_body.splitlines() and
sanitized_root_body.splitlines() should use strict=True to fail fast on any
length mismatch; update the zip(...) call in the loop comparing
root_body.splitlines() and sanitized_root_body.splitlines() (in the test that
validates _sanitize_qml behavior) to zip(..., strict=True) so any
parser/sanitizer divergence raises immediately.
🪄 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: 47484926-651e-49a3-8953-3fbd5b604ba2
📒 Files selected for processing (3)
faststack/qml/Components.qmlfaststack/qml/SettingsDialog.qmlfaststack/tests/thumbnail_view/test_qml_delegate_contract.py
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/qml/Components.qml
Comment on lines
+65
to
+68
| Component.onCompleted: { | ||
| settingsDialog.uiStateRef = uiState | ||
| settingsDialog.controllerRef = controller | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where SettingsDialog is instantiated and opened
rg -n -C4 'SettingsDialog|id:\s*settingsDialog|settingsDialog\.open\(' faststack/qml
# 2) Find where uiState/controller are introduced and propagated
rg -n -C4 '\buiState\b|\bcontroller\b|uiStateRef|controllerRef' faststack/qmlRepository: AlanRockefeller/faststack
Length of output: 50382
🏁 Script executed:
head -80 faststack/qml/SettingsDialog.qml | tail -40Repository: AlanRockefeller/faststack
Length of output: 1493
🏁 Script executed:
sed -n '120,180p' faststack/qml/SettingsDialog.qmlRepository: AlanRockefeller/faststack
Length of output: 3017
Use the safer typeof pattern for context property assignment like other dialogs.
Component.onCompleted assigns unqualified uiState and controller into settingsDialog.uiStateRef / settingsDialog.controllerRef (lines 65-68). While the code guards these refs at usage sites (lines 128, 161, 175), the assignment itself lacks the defensive typeof check used consistently by JumpToImageDialog, HistogramWindow, FilterDialog, ExifDialog, and DeleteBatchDialog. Replace with:
property var uiStateRef: typeof uiState !== "undefined" ? uiState : null
property var controllerRef: typeof controller !== "undefined" ? controller : nullRemove the Component.onCompleted block (lines 65-68) since context properties are available at declaration time. This matches the codebase pattern and ensures graceful fallback if context is unavailable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/qml/SettingsDialog.qml` around lines 65 - 68, Remove the
Component.onCompleted block that assigns uiState and controller into
settingsDialog (the lines where settingsDialog.uiStateRef = uiState and
settingsDialog.controllerRef = controller) and instead declare defensive
properties on settingsDialog using the typeof pattern so uiStateRef and
controllerRef default to null when context properties are absent (i.e., set
settingsDialog.uiStateRef and settingsDialog.controllerRef via typeof uiState
!== "undefined" ? uiState : null and typeof controller !== "undefined" ?
controller : null); this matches the pattern used by
JumpToImageDialog/HistogramWindow/FilterDialog/ExifDialog/DeleteBatchDialog and
ensures safe fallback without runtime assignment in Component.onCompleted.
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.
Ran qmllint on all of the QML and cleaned up some of the warnings.
Summary by CodeRabbit
Bug Fixes
New Features
Tests