ui(vat-panel): collapse by default + match inspector control styling#651
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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. 📝 WalkthroughWalkthroughThe VAT baker UI section in PropertiesPanel.qml is systematically redesigned with themed components. The VAT section now collapses by default. Animation picker and output directory controls switch to themed ComboBox and TextInput variants. The FPS control undergoes substantial reimplementation with numeric validation, arrow increment/decrement zones, and keyboard handling. Shader toggles are rebuilt as custom Rectangle-based checkboxes with Repeater-driven per-engine generation. ChangesVAT Baker UI Theming and Control Replacement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 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. 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: c315ac028b
ℹ️ 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".
| MouseArea { | ||
| anchors.fill: parent | ||
| enabled: !VATBakerController.isBaking | ||
| cursorShape: enabled ? Qt.PointingHandCursor : Qt.ForbiddenCursor | ||
| onClicked: animToolsCol.includeShaders = !animToolsCol.includeShaders | ||
| } |
There was a problem hiding this comment.
Replacing CheckBox with Rectangle + MouseArea removes built-in focus, keyboard activation, and checkbox accessibility state, so users navigating by keyboard/screen reader can no longer toggle "Include shader" (and the per-engine toggles follow the same pattern). This is a behavioral regression from the previous CheckBox implementation and blocks non-mouse interaction for these export options.
Useful? React with 👍 / 👎.
| onClicked: { | ||
| animToolsCol.fps = Math.min(120, animToolsCol.fps + 1) | ||
| animFpsInput.text = String(animToolsCol.fps) |
There was a problem hiding this comment.
The up/down arrow handlers increment animToolsCol.fps, but that model value is only synchronized in onEditingFinished, so clicking an arrow while the user has an uncommitted edit in the text box uses a stale value (e.g. type 60, click ▲, and the value can jump from prior state instead of 61). This makes the custom spinner behave inconsistently compared to the previous SpinBox and can silently apply the wrong FPS.
Useful? React with 👍 / 👎.
VAT panel housekeeping after the include-shaders checkboxes landed: - Collapsed by default. VAT is a power-user export step, not a routine animation-review touchpoint — defaulting the CollapsibleSection to collapsed keeps the animation tools panel short out of the box. - Themed 14×14 checkboxes for "Include shader" + Godot/Unity/Unreal. The previous stock `Controls.CheckBox` renders ~22 px tall and carries Qt's full theming, which felt outsized inside the tight inspector rows. The new look mirrors the Enable/Loop toggles on the per-clip rows of the Animations panel (same square-with- checkmark style, same border colours). - `ThemedComboBox` for the Anim picker so the dropdown matches the one used by the Animations panel's simplify-preset selector instead of Qt's default ComboBox. - Themed text-input row for FPS — coloured-label-box pattern dropped in favour of the plain "FPS:" left label (matching the Anim / Out rows) with a custom themed Rectangle + TextInput + step-arrow Column. Same as the inspector's other numeric controls. - Themed text-input row for the output-folder path (label + box + Browse button at 22 px to match). QML-only change. No C++ behaviour change.
fernandotonon
force-pushed
the
feat/vat-panel-polish
branch
from
c315ac0 to
972bbff
Compare
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@qml/PropertiesPanel.qml`:
- Around line 720-721: The arrow key/arrow-button handlers (Keys.onUpPressed /
Keys.onDownPressed and the arrow button handlers that modify animToolsCol.fps)
must parse and clamp the current animFpsInput.text first and use that as the
base before applying +1 / -1; replace direct increments of animToolsCol.fps with
logic that reads parseInt(animFpsInput.text) (fallback to animToolsCol.fps if
parse fails), clamp to [1,120], add/subtract 1, then set animToolsCol.fps and
update animFpsInput.text to the new value; apply the same change to the other
handler sites mentioned (around lines 743-746 and 763-766).
- Around line 878-911: The custom checkbox Rectangle (id: includeShaderChk) and
its sibling Text lost keyboard and accessibility support when you replaced
Controls.CheckBox; restore focusability and assistive properties by either
reverting to or wrapping this UI in a reusable themed checkbox component
(preferred) or by adding focus: true/focusPolicy, Keys.onPressed/Keys.onReleased
handlers to toggle animToolsCol.includeShaders on Space/Enter, and set
Accessible.role, Accessible.name and Accessible.checked (bound to
animToolsCol.includeShaders) on the interactive element (includeShaderChk or its
MouseArea); ensure VATBakerController.isBaking still disables focus/interaction
and that the Text label provides the same clickable/focus behavior (or use
proper Accessible.name linking) so keyboard and screen-reader users can toggle
the shader option.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Addresses Codex P2s + CodeRabbit notes on PR #651. 1) FPS arrows + ↑/↓ key handlers now read the LIVE input text instead of the committed `animToolsCol.fps`. Previously, typing "60" and pressing ↑ before Enter would jump back to the old committed value + 1. New `stepFps(delta)` helper parses animFpsInput.text first; if it doesn't parse, falls back to the committed value. Same helper drives both arrow buttons and both keyboard handlers. 2) Themed checkboxes (Include shader + Godot/Unity/Unreal) regained keyboard reachability and assistive-tech state: - `activeFocusOnTab` so Tab navigation lands on them. - `Keys.onSpacePressed` / `onReturnPressed` / `onEnterPressed` toggle the bound property. - `Accessible.role = CheckBox`, `Accessible.name` (e.g. "Godot shader"), `Accessible.checkable = true`, `Accessible.checked` bound to the live state. Screen readers now announce the checkbox kind, label, and on/off state. - Focus-visible border (highlight colour + 2 px) so the keyboard-focused checkbox is unambiguous. - Clicking the label or the box now also calls `forceActiveFocus()` on the checkbox, matching native labelled-CheckBox behaviour. Stock `Controls.CheckBox` carries all of the above for free, but its ~22 px height looks oversized in the tight inspector rows — so we built the affordances back into the themed 14×14 Rectangle.
|
1 participant
Summary
Quick UI polish on the VAT panel that landed via #649. Stacked on top of `feat/vat-include-shaders` (PR #649) — please review/merge #649 first.
Collapsed by default
VAT is a power-user export step, not part of the routine animation-review flow. The CollapsibleSection now starts collapsed so the animation tools panel doesn't auto-occupy real estate; the user expands when they want to bake.
Themed 14×14 checkboxes
"Include shader" master + Godot/Unity/Unreal sub-checkboxes now use the same square-with-checkmark Rectangle-driven control the Animations panel uses for per-clip Enable / Loop toggles. The stock `Controls.CheckBox` renders ~22 px tall and carries Qt's full theming, which felt oversized in the tight inspector rows.
`ThemedComboBox` for Anim picker
Matches the dropdown the Animations panel's simplify-preset selector uses, instead of Qt's default ComboBox.
Themed text-input rows for FPS and Output
Test plan
Related
Follow-up to #649. No C++ behaviour change.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style