Modernize editor layout and add histogram overlay mode#89
Modernize editor layout and add histogram overlay mode#89AlanRockefeller wants to merge 5 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughAdds a switchable histogram UI: new OverlaidHistogram and HistogramModeToggle components, persistent Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageEditor as ImageEditorDialog.qml
participant Window as HistogramWindow.qml
participant Toggle as HistogramModeToggle.qml
participant Renderer as OverlaidHistogram.qml / SingleChannelHistogram.qml
participant Canvas
User->>ImageEditor: click histogram mode toggle
ImageEditor->>Toggle: forward click
Toggle-->>ImageEditor: modeRequested(overlaid)
ImageEditor->>Window: update histSettings.overlaidMode
Window->>Renderer: provide r/g/b data + clip counts (reactive)
Renderer->>Canvas: request repaint / onPaint -> draw histograms
Canvas-->>Renderer: render complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 0/1 reviews remaining, refill in 42 minutes and 22 seconds.Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e5094de74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Settings { | ||
| id: histSettings | ||
| category: "histogram" | ||
| property bool overlaidMode: true | ||
| } |
There was a problem hiding this comment.
This file now relies on Settings from import QtCore, but that QML type is only available in newer Qt6 releases while the package metadata still allows PySide6>=6.0; on 6.0–6.4 this component will fail to load (Settings is not a type), which can break app startup because Main.qml instantiates ImageEditorDialog. Either gate this by version/alternate import or bump the declared minimum PySide6 version.
Useful? React with 👍 / 👎.
| color: imageEditorDialog.textColor | ||
| opacity: 0.7 | ||
| font.italic: true | ||
| visible: (imageEditorDialog.uiStateRef ? imageEditorDialog.uiStateRef.hasRaw : false) && imageEditorDialog.sourceExpanded |
There was a problem hiding this comment.
The save-behavior label is now inside a container gated by hasRaw, so when editing images without RAW companions the UI no longer shows the overwrite warning text (saveBehaviorMessage, e.g. JPEG overwrite behavior). This removes an important destructive-action hint that was previously always visible and increases the chance of unintended overwrites.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
faststack/qml/ImageEditorDialog.qml (1)
193-236: Extract histogram mode toggle into a shared QML component.This toggle UI is duplicated with
faststack/qml/HistogramWindow.qml(Lines 70-111). A sharedHistogramModeToggle.qmlwould reduce drift and keep behavior/styles in sync.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/qml/ImageEditorDialog.qml` around lines 193 - 236, Extract the duplicated toggle UI into a new reusable QML component named HistogramModeToggle.qml that exposes a bindable property (e.g., property bool overlaidMode) and an onOverlaidModeChanged signal or use two-way binding so callers can bind histSettings.overlaidMode directly; move the two Rectangle+Text+MouseArea blocks and their styling/logic into that component (preserve widths, radii, colors, font.pixelSize, border settings and MouseArea onClicked handlers) and replace the duplicated blocks in ImageEditorDialog.qml and HistogramWindow.qml with <HistogramModeToggle overlaidMode: histSettings.overlaidMode/> (or equivalent binding) so behavior and styles remain identical and centralized.
🤖 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/HistogramWindow.qml`:
- Around line 70-111: The two custom toggle "buttons" built from
Rectangle+MouseArea (the blocks rendering "Overlaid" and "Channels" and mutating
histSettings.overlaidMode) are not keyboard-focusable; replace them with an
accessible control such as a Switch (preferred for an immediate binary-mode
toggle) or a single CheckBox bound to histSettings.overlaidMode so it handles
focus, keyboard activation and accessibility automatically; locate the two
Rectangle elements and their MouseArea handlers and remove them, then add a
Switch (or CheckBox) whose checked property is bound to
histSettings.overlaidMode and whose text/labels reflect "Overlaid"/"Channels",
ensuring the control sits inside the same RowLayout so existing layout and
styling can be adapted.
In `@faststack/qml/ImageEditorDialog.qml`:
- Around line 329-380: The expand/collapse control currently uses TapHandler (in
the RowLayout block toggling imageEditorDialog.sourceExpanded) which is not
keyboard-accessible; replace the TapHandler with a ToolButton (or a
Button/Accessible control) that wraps or replaces the interactive area, make it
focusable, provide onClicked: imageEditorDialog.sourceExpanded =
!imageEditorDialog.sourceExpanded, set accessible.role/name (or accessibleName)
and appropriate icon/text (e.g., use the existing Text arrow and
"SOURCE"/sourceBadgeText visuals inside the ToolButton), and remove the
TapHandler so keyboard users can toggle sourceExpanded with Enter/Space and
receive proper accessibility semantics.
In `@faststack/qml/OverlaidHistogram.qml`:
- Around line 13-15: Overlaid mode never exposes the rPreClip/gPreClip/bPreClip
properties to the UI; update OverlaidHistogram.qml to render the pre-clip values
alongside the existing clip visualization (same style as SingleChannelHistogram)
and bind its visual elements to property int rPreClip/gPreClip/bPreClip so
pre-clip diagnostics are visible. Then propagate those properties from the
parents by adding bindings in HistogramWindow.qml and ImageEditorDialog.qml
where OverlaidHistogram is instantiated (pass rPreClip/gPreClip/bPreClip through
the component properties at the call sites), ensuring both overlaid and
single-channel modes show pre-clip values consistently.
---
Nitpick comments:
In `@faststack/qml/ImageEditorDialog.qml`:
- Around line 193-236: Extract the duplicated toggle UI into a new reusable QML
component named HistogramModeToggle.qml that exposes a bindable property (e.g.,
property bool overlaidMode) and an onOverlaidModeChanged signal or use two-way
binding so callers can bind histSettings.overlaidMode directly; move the two
Rectangle+Text+MouseArea blocks and their styling/logic into that component
(preserve widths, radii, colors, font.pixelSize, border settings and MouseArea
onClicked handlers) and replace the duplicated blocks in ImageEditorDialog.qml
and HistogramWindow.qml with <HistogramModeToggle overlaidMode:
histSettings.overlaidMode/> (or equivalent binding) so behavior and styles
remain identical and centralized.
🪄 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: d94ea107-0d0b-48d6-8d25-47525c643567
📒 Files selected for processing (3)
faststack/qml/HistogramWindow.qmlfaststack/qml/ImageEditorDialog.qmlfaststack/qml/OverlaidHistogram.qml
| RowLayout { | ||
| Layout.fillWidth: true | ||
| Layout.fillHeight: true | ||
|
|
||
| channelName: "Green" | ||
| channelColor: "#50e150" | ||
| gridLineColor: histogramWindow.gridLineColor | ||
| dangerColor: histogramWindow.dangerColor | ||
| textColor: histogramWindow.primaryTextColor | ||
|
|
||
| histogramData: histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ? (histogramWindow.uiStateRef.histogramData["g"] || []) : [] | ||
| clipCount: histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ? (histogramWindow.uiStateRef.histogramData["g_clip"] || 0) : 0 | ||
| preClipCount: histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ? (histogramWindow.uiStateRef.histogramData["g_preclip"] || 0) : 0 | ||
| Item { Layout.fillWidth: true } | ||
| Row { | ||
| spacing: 0 | ||
| Rectangle { | ||
| width: 70; height: 20 | ||
| radius: 3 | ||
| color: histSettings.overlaidMode ? "#2c2c2c" : "transparent" | ||
| border.color: "#3a3a3a"; border.width: 1 | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: "Overlaid" | ||
| font.pixelSize: 10 | ||
| color: histSettings.overlaidMode ? "#e8e6e3" : "#6b6764" | ||
| } | ||
| MouseArea { | ||
| anchors.fill: parent | ||
| onClicked: { | ||
| histSettings.overlaidMode = true | ||
| } | ||
| } | ||
| } | ||
| Rectangle { | ||
| width: 70; height: 20 | ||
| radius: 3 | ||
| color: !histSettings.overlaidMode ? "#2c2c2c" : "transparent" | ||
| border.color: "#3a3a3a"; border.width: 1 | ||
| Text { | ||
| anchors.centerIn: parent | ||
| text: "Channels" | ||
| font.pixelSize: 10 | ||
| color: !histSettings.overlaidMode ? "#e8e6e3" : "#6b6764" | ||
| } | ||
| MouseArea { | ||
| anchors.fill: parent | ||
| onClicked: { | ||
| histSettings.overlaidMode = false | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Qt Quick Controls, are Rectangle+MouseArea widgets keyboard-focusable by default, and is checkable Button/ButtonGroup the recommended accessible pattern for a two-option toggle?
💡 Result:
No, Rectangle+MouseArea widgets are not keyboard-focusable by default in Qt Quick Controls. Rectangle and MouseArea are basic Qt Quick items without built-in focus support; keyboard focus requires explicit configuration like setting focus: true and activeFocusOnTab: true on the Rectangle, or using focusPolicy on Controls. Qt Quick Controls follow the standard Qt Quick focus system where focusPolicy controls focus reception, but primitive items like Rectangle+MouseArea do not participate by default and need manual setup for accessibility. No, checkable Button+ButtonGroup is not the recommended accessible pattern for a two-option toggle. For binary toggles (on/off), the official Qt Quick Controls documentation recommends Switch (for immediate-effect actions like Wi-Fi) or a single CheckBox/Checkable Button. ButtonGroup with checkable Buttons (or RadioButtons) is for mutually exclusive multi-selection (more than two options). For exactly two options, Switch or single CheckBox provides better semantics and accessibility via built-in Accessible support (focusable, toggleAction, etc.). Use Accessible attached properties explicitly only for custom items; standard Controls like Switch are accessible out-of-the-box.
Citations:
- 1: https://doc.qt.io/qt-5/qtquickcontrols2-focus.html
- 2: https://doc.qt.io/qt-6/qml-qtquick-mousearea.html
- 3: https://stackoverflow.com/questions/56834971/focusable-qml-components-focus-my-control-on-tab
- 4: https://doc.qt.io/qt-6/qml-qtquick-controls-buttongroup.html
- 5: https://doc.qt.io/qt-6.4/qtquickcontrols2-buttons.html
- 6: https://doc.qt.io/qt-6.2/qtquickcontrols2-buttons.html
- 7: https://doc.qt.io/qt-6.2/qml-qtquick-controls2-switch.html
- 8: https://doc.qt.io/qt-6.11/qml-qtquick-controls-control.html
- 9: https://doc.qt.io/qt-6.4/accessible-qtquick.html
- 10: https://doc.qt.io/qt-6.4/qml-qtquick-accessible.html
🏁 Script executed:
# Check if HistogramWindow.qml exists and read the relevant section
fd -t f "HistogramWindow.qml" --exec cat -n {} \; 2>/dev/null | head -150Repository: AlanRockefeller/faststack
Length of output: 7361
🏁 Script executed:
# Also check for any existing keyboard handling or accessible configurations in the file
rg -A5 -B5 "focus|keyboard|Accessible|Keys" faststack/qml/HistogramWindow.qml 2>/dev/null || echo "No keyboard/focus handling found"Repository: AlanRockefeller/faststack
Length of output: 632
Histogram mode toggle lacks keyboard accessibility.
The Rectangle + MouseArea pattern at lines 75–111 is not keyboard-focusable or keyboard-operable. Although the window has a FocusScope (line 23) with key handling infrastructure, the toggle buttons don't integrate with it, blocking keyboard-only users.
However, the suggested ButtonGroup + checkable Button fix is incorrect for a binary toggle. Per Qt Quick Controls documentation, ButtonGroup is designed for mutually exclusive multi-selection (3+ options). For exactly two options, use Switch (for immediate-effect toggles) or a single CheckBox, both of which provide built-in keyboard focus, accessibility attributes, and proper semantics.
♿ Suggested fix (use Switch for binary toggle)
+import QtQuick.Controls
@@
- Row {
- spacing: 0
- Rectangle {
- width: 70; height: 20
- radius: 3
- color: histSettings.overlaidMode ? "#2c2c2c" : "transparent"
- border.color: "#3a3a3a"; border.width: 1
- Text {
- anchors.centerIn: parent
- text: "Overlaid"
- font.pixelSize: 10
- color: histSettings.overlaidMode ? "#e8e6e3" : "#6b6764"
- }
- MouseArea {
- anchors.fill: parent
- onClicked: {
- histSettings.overlaidMode = true
- }
- }
- }
- Rectangle {
- width: 70; height: 20
- radius: 3
- color: !histSettings.overlaidMode ? "#2c2c2c" : "transparent"
- border.color: "#3a3a3a"; border.width: 1
- Text {
- anchors.centerIn: parent
- text: "Channels"
- font.pixelSize: 10
- color: !histSettings.overlaidMode ? "#e8e6e3" : "#6b6764"
- }
- MouseArea {
- anchors.fill: parent
- onClicked: {
- histSettings.overlaidMode = false
- }
- }
- }
- }
+ Row {
+ spacing: 8
+ Text {
+ anchors.verticalCenter: parent.verticalCenter
+ text: histSettings.overlaidMode ? "Overlaid" : "Channels"
+ font.pixelSize: 10
+ color: histogramWindow.primaryTextColor
+ }
+ Switch {
+ checked: histSettings.overlaidMode
+ onToggled: histSettings.overlaidMode = checked
+ }
+ }📝 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.
| RowLayout { | |
| Layout.fillWidth: true | |
| Layout.fillHeight: true | |
| channelName: "Green" | |
| channelColor: "#50e150" | |
| gridLineColor: histogramWindow.gridLineColor | |
| dangerColor: histogramWindow.dangerColor | |
| textColor: histogramWindow.primaryTextColor | |
| histogramData: histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ? (histogramWindow.uiStateRef.histogramData["g"] || []) : [] | |
| clipCount: histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ? (histogramWindow.uiStateRef.histogramData["g_clip"] || 0) : 0 | |
| preClipCount: histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ? (histogramWindow.uiStateRef.histogramData["g_preclip"] || 0) : 0 | |
| Item { Layout.fillWidth: true } | |
| Row { | |
| spacing: 0 | |
| Rectangle { | |
| width: 70; height: 20 | |
| radius: 3 | |
| color: histSettings.overlaidMode ? "#2c2c2c" : "transparent" | |
| border.color: "#3a3a3a"; border.width: 1 | |
| Text { | |
| anchors.centerIn: parent | |
| text: "Overlaid" | |
| font.pixelSize: 10 | |
| color: histSettings.overlaidMode ? "#e8e6e3" : "#6b6764" | |
| } | |
| MouseArea { | |
| anchors.fill: parent | |
| onClicked: { | |
| histSettings.overlaidMode = true | |
| } | |
| } | |
| } | |
| Rectangle { | |
| width: 70; height: 20 | |
| radius: 3 | |
| color: !histSettings.overlaidMode ? "#2c2c2c" : "transparent" | |
| border.color: "#3a3a3a"; border.width: 1 | |
| Text { | |
| anchors.centerIn: parent | |
| text: "Channels" | |
| font.pixelSize: 10 | |
| color: !histSettings.overlaidMode ? "#e8e6e3" : "#6b6764" | |
| } | |
| MouseArea { | |
| anchors.fill: parent | |
| onClicked: { | |
| histSettings.overlaidMode = false | |
| } | |
| } | |
| } | |
| } | |
| RowLayout { | |
| Layout.fillWidth: true | |
| Item { Layout.fillWidth: true } | |
| Row { | |
| spacing: 8 | |
| Text { | |
| anchors.verticalCenter: parent.verticalCenter | |
| text: histSettings.overlaidMode ? "Overlaid" : "Channels" | |
| font.pixelSize: 10 | |
| color: histogramWindow.primaryTextColor | |
| } | |
| Switch { | |
| checked: histSettings.overlaidMode | |
| onToggled: histSettings.overlaidMode = checked | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/qml/HistogramWindow.qml` around lines 70 - 111, The two custom
toggle "buttons" built from Rectangle+MouseArea (the blocks rendering "Overlaid"
and "Channels" and mutating histSettings.overlaidMode) are not
keyboard-focusable; replace them with an accessible control such as a Switch
(preferred for an immediate binary-mode toggle) or a single CheckBox bound to
histSettings.overlaidMode so it handles focus, keyboard activation and
accessibility automatically; locate the two Rectangle elements and their
MouseArea handlers and remove them, then add a Switch (or CheckBox) whose
checked property is bound to histSettings.overlaidMode and whose text/labels
reflect "Overlaid"/"Channels", ensuring the control sits inside the same
RowLayout so existing layout and styling can be adapted.
| RowLayout { | ||
| Layout.fillWidth: true | ||
| spacing: 6 | ||
|
|
||
| Text { | ||
| text: imageEditorDialog.sourceExpanded ? "▾" : "▸" | ||
| font.pixelSize: 10 | ||
| color: "#9a9795" | ||
| Layout.alignment: Qt.AlignVCenter | ||
| } | ||
|
|
||
| Text { | ||
| text: "SOURCE" | ||
| font.pixelSize: 10 | ||
| font.weight: Font.DemiBold | ||
| font.letterSpacing: 1.2 | ||
| color: "#9a9795" | ||
| Layout.alignment: Qt.AlignVCenter | ||
| } | ||
|
|
||
| Rectangle { | ||
| Layout.alignment: Qt.AlignVCenter | ||
| implicitWidth: sourceBadgeText.implicitWidth + 10 | ||
| implicitHeight: sourceBadgeText.implicitHeight + 4 | ||
| color: "#2a2010" | ||
| border.color: "#3a3010" | ||
| border.width: 1 | ||
| radius: 3 | ||
|
|
||
| Text { | ||
| id: sourceBadgeText | ||
| anchors.centerIn: parent | ||
| text: "experimental" | ||
| font.pixelSize: 9 | ||
| font.family: "IBM Plex Mono" | ||
| color: "#7a6a3a" | ||
| } | ||
| } | ||
|
|
||
| Item { Layout.fillWidth: true } | ||
| } | ||
|
|
||
| Rectangle { | ||
| Layout.fillWidth: true | ||
| height: 1 | ||
| color: "#2e2e2e" | ||
| } | ||
|
|
||
| TapHandler { | ||
| acceptedButtons: Qt.LeftButton | ||
| onTapped: imageEditorDialog.sourceExpanded = !imageEditorDialog.sourceExpanded | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Qt Quick Controls 2 accessibility, what is the recommended keyboard-accessible pattern for collapsible section headers (ToolButton/CheckBox vs TapHandler-only)?
💡 Result:
Recommended pattern: use a focusable “button-like” control (ToolButton or CheckBox) for the collapsible section header, not a TapHandler-only header. Reason (Qt accessibility specifics): Qt Quick accessibility is designed around exposing an interactive role (e.g., Button / CheckBox) and wiring the corresponding accessibility press/toggle action. The Accessible QML type documentation states that for Button-like roles you must provide an Accessible.onPressAction handler that has the same effect as clicking/tapping; for CheckBox/RadioButton roles you also provide checkable state and Accessible.toggleAction. A TapHandler is primarily a pointer handler and does not provide these accessible semantics/keyboard activation behavior by itself. In practice for a collapsible header: - If the header is a simple show/hide toggle: model it as a Button (e.g., ToolButton) with Accessible.role: Accessible.Button and Accessible.onPressAction toggling the expanded state. - If the header is conceptually a selection/checkbox (checked means expanded): use CheckBox semantics instead (Accessible.role: Accessible.CheckBox with checked + Accessible.toggleAction). TapHandler-only headers are therefore not the recommended keyboard-accessible pattern for Qt Quick Controls 2 accessibility; they need extra work to become a proper focusable, accessible control with Accessible.pressAction / onPressAction semantics.
Citations:
- 1: https://doc.qt.io/qt-6/qml-qtquick-accessible.html
- 2: https://doc.qt.io/qt-6.4/qml-qtquick-accessible.html
- 3: https://doc.qt.io/qt-6/qml-qtquick-taphandler.html
- 4: https://doc.qt.io/qt-6.4/qml-qtquick-taphandler.html
- 5: https://doc.qt.io/qt-6/qml-qtquick-controls-toolbutton.html
🏁 Script executed:
# First, verify the file exists and check the exact code at lines 329-380
wc -l faststack/qml/ImageEditorDialog.qmlRepository: AlanRockefeller/faststack
Length of output: 109
🏁 Script executed:
# Read the specific section mentioned in the review
sed -n '329,380p' faststack/qml/ImageEditorDialog.qmlRepository: AlanRockefeller/faststack
Length of output: 2155
🏁 Script executed:
# Check if there are other similar expandable sections in this file
rg "TapHandler|expandable|sourceExpanded" faststack/qml/ImageEditorDialog.qml -nRepository: AlanRockefeller/faststack
Length of output: 814
🏁 Script executed:
# Check what imports are available in this file (to verify ToolButton availability)
head -30 faststack/qml/ImageEditorDialog.qmlRepository: AlanRockefeller/faststack
Length of output: 1167
🏁 Script executed:
# Search for other expandable sections in the codebase to see if this is a pattern
rg "TapHandler.*-1.*sourceExpanded|expandable.*TapHandler" faststack/qml/ -A 5 -B 5Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Check if ToolButton is used elsewhere in this QML file
rg "ToolButton" faststack/qml/ImageEditorDialog.qmlRepository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Search for ToolButton usage patterns in other QML files
rg "ToolButton" faststack/qml/ --include="*.qml" -lRepository: AlanRockefeller/faststack
Length of output: 511
🏁 Script executed:
# Look for other TapHandler patterns in the codebase (not just this file)
rg "TapHandler" faststack/qml/ --include="*.qml" -A 2 | head -50Repository: AlanRockefeller/faststack
Length of output: 511
🏁 Script executed:
# Check the HistogramWindow.qml to see if it has similar expandable section patterns
grep -n "expand\|TapHandler" faststack/qml/HistogramWindow.qml 2>/dev/null | head -20Repository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Search for ToolButton usage in QML files with correct syntax
rg "ToolButton" faststack/qml -t qml -lRepository: AlanRockefeller/faststack
Length of output: 51
🏁 Script executed:
# Search for all TapHandler patterns in the codebase
rg "TapHandler" faststack/qml -t qml -A 1 | head -60Repository: AlanRockefeller/faststack
Length of output: 996
🏁 Script executed:
# Look for expandable/collapsible patterns - check HistogramWindow
cat faststack/qml/HistogramWindow.qml 2>/dev/null | grep -n -i "expand\|collapsed\|toggle" | head -20Repository: AlanRockefeller/faststack
Length of output: 175
🏁 Script executed:
# Check if there are other expandable sections with similar TapHandler pattern in ImageEditorDialog
rg -n "Text.*▾|Text.*▸|expandable|collapsed" faststack/qml/ImageEditorDialog.qmlRepository: AlanRockefeller/faststack
Length of output: 51
Source section expander is not keyboard-operable.
The collapse/expand interaction uses TapHandler only, preventing keyboard users from accessing this control. Use a ToolButton instead to provide keyboard navigation and proper accessibility semantics.
Suggested fix
- Text {
- text: imageEditorDialog.sourceExpanded ? "▾" : "▸"
- font.pixelSize: 10
- color: "#9a9795"
- Layout.alignment: Qt.AlignVCenter
- }
+ ToolButton {
+ text: imageEditorDialog.sourceExpanded ? "▾" : "▸"
+ checkable: true
+ checked: imageEditorDialog.sourceExpanded
+ onToggled: imageEditorDialog.sourceExpanded = checked
+ Layout.alignment: Qt.AlignVCenter
+ }
@@
- TapHandler {
- acceptedButtons: Qt.LeftButton
- onTapped: imageEditorDialog.sourceExpanded = !imageEditorDialog.sourceExpanded
- }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@faststack/qml/ImageEditorDialog.qml` around lines 329 - 380, The
expand/collapse control currently uses TapHandler (in the RowLayout block
toggling imageEditorDialog.sourceExpanded) which is not keyboard-accessible;
replace the TapHandler with a ToolButton (or a Button/Accessible control) that
wraps or replaces the interactive area, make it focusable, provide onClicked:
imageEditorDialog.sourceExpanded = !imageEditorDialog.sourceExpanded, set
accessible.role/name (or accessibleName) and appropriate icon/text (e.g., use
the existing Text arrow and "SOURCE"/sourceBadgeText visuals inside the
ToolButton), and remove the TapHandler so keyboard users can toggle
sourceExpanded with Enter/Space and receive proper accessibility semantics.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
faststack/qml/HistogramWindow.qml (1)
80-93: Consider extracting histogram data access to computed properties.The null-check pattern
histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ? (...) : []is repeated 9 times for OverlaidHistogram and again for SingleChannelHistogram. Extracting these to computed properties at the Window level would reduce duplication and improve readability.♻️ Suggested refactor
Add computed properties at the Window level (e.g., near line 60):
// Helper properties for histogram data access readonly property var histData: uiStateRef && uiStateRef.histogramData ? uiStateRef.histogramData : {} readonly property var rData: histData["r"] || [] readonly property var gData: histData["g"] || [] readonly property var bData: histData["b"] || [] readonly property int rClip: histData["r_clip"] || 0 // ... etc.Then simplify bindings:
-rData: histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ? (histogramWindow.uiStateRef.histogramData["r"] || []) : [] +rData: histogramWindow.rData🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/qml/HistogramWindow.qml` around lines 80 - 93, The repeated null-check and property access for histogramWindow.uiStateRef.histogramData across OverlaidHistogram and SingleChannelHistogram is duplicated and verbose; create Window-level computed/read-only properties (e.g., histData, rData, gData, bData, rClip, gClip, bClip, rPreClip, gPreClip, bPreClip) that evaluate histData = histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ? histogramWindow.uiStateRef.histogramData : {} and derive each channel/clip value from that, then replace the long conditional bindings in OverlaidHistogram and SingleChannelHistogram with the new properties (referencing histData, rData, gData, bData, rClip, etc.) to eliminate duplication and improve readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@faststack/qml/HistogramWindow.qml`:
- Around line 80-93: The repeated null-check and property access for
histogramWindow.uiStateRef.histogramData across OverlaidHistogram and
SingleChannelHistogram is duplicated and verbose; create Window-level
computed/read-only properties (e.g., histData, rData, gData, bData, rClip,
gClip, bClip, rPreClip, gPreClip, bPreClip) that evaluate histData =
histogramWindow.uiStateRef && histogramWindow.uiStateRef.histogramData ?
histogramWindow.uiStateRef.histogramData : {} and derive each channel/clip value
from that, then replace the long conditional bindings in OverlaidHistogram and
SingleChannelHistogram with the new properties (referencing histData, rData,
gData, bData, rClip, etc.) to eliminate duplication and improve readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6e603b31-1df4-4987-a5bf-5738cbbc0fa0
📒 Files selected for processing (5)
faststack/qml/HistogramModeToggle.qmlfaststack/qml/HistogramWindow.qmlfaststack/qml/ImageEditorDialog.qmlfaststack/qml/OverlaidHistogram.qmlpyproject.toml
✅ Files skipped from review due to trivial changes (2)
- pyproject.toml
- faststack/qml/HistogramModeToggle.qml
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/qml/OverlaidHistogram.qml
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
faststack/qml/SingleChannelHistogram.qml (1)
145-145: Footer text spacing is quite tight withspacing: 0Not a blocker, but
spacing: 1or2improves readability on dense UI sizes with minimal height impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@faststack/qml/SingleChannelHistogram.qml` at line 145, The footer's layout uses spacing: 0 which makes items too tight; update the spacing property in SingleChannelHistogram.qml (the footer Row/Column that currently sets spacing: 0 at the bottom of the component) to a small positive value (e.g., spacing: 1 or spacing: 2) to improve readability on dense UI sizes while keeping minimal height impact.
🤖 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/SingleChannelHistogram.qml`:
- Around line 149-156: The "Preclip: " and "Clipped: " Text labels in
SingleChannelHistogram.qml can be truncated in narrow 3-column layouts; update
the Text elements that use root.preClipCount and root.clipCount to avoid
truncation by either enabling eliding (set elide: Text.ElideRight and an
appropriate horizontalAlignment) or switching to compact labels when width is
small (e.g., use a conditional expression based on root.width to show "Pre:" /
"Clip:" vs "Preclip: " / "Clipped: "). Modify the Text items referencing
root.preClipCount and root.clipCount to implement one of these approaches so the
numeric values remain visible at reduced pane widths.
---
Nitpick comments:
In `@faststack/qml/SingleChannelHistogram.qml`:
- Line 145: The footer's layout uses spacing: 0 which makes items too tight;
update the spacing property in SingleChannelHistogram.qml (the footer Row/Column
that currently sets spacing: 0 at the bottom of the component) to a small
positive value (e.g., spacing: 1 or spacing: 2) to improve readability on dense
UI sizes while keeping minimal height impact.
🪄 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: cef86d4a-1602-43b4-a98d-f459de2dbefb
📒 Files selected for processing (3)
faststack/qml/HistogramWindow.qmlfaststack/qml/OverlaidHistogram.qmlfaststack/qml/SingleChannelHistogram.qml
✅ Files skipped from review due to trivial changes (1)
- faststack/qml/HistogramWindow.qml
🚧 Files skipped from review as they are similar to previous changes (1)
- faststack/qml/OverlaidHistogram.qml
1 participant
Summary by CodeRabbit
New Features
UI Improvements
Style