Filter inspector sections by editor mode - #435
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. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces an inspector tab routing system by adding mode-aware enum definitions and helper methods to EditorModeController, exposing them to QML, and refactoring PropertiesPanel to use these identifiers for tab selection and section visibility instead of hardcoded indices. The "Mode Tools" UI gains a dedicated header with current/all filtering, and section visibility conditions are unified via a new helper function. ChangesInspector Tab Routing & Mode Tools System
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 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 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 |
e8111ae to
0f79d63
Compare
|
Updated the PR to fix Mode Tools live refresh while staying on the Mode Tools tab. Root cause: the QML visibility binding called an invokable that read Fix: Validation rerun:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
qml/PropertiesPanel.qml (1)
56-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winConsider resetting
showAllModeToolson mode change.
onModeChangedresetscurrentTabto the mode-appropriate default but leavesshowAllModeToolssticky. A user who toggled "All" and then switches modes will land on the new mode'sModeToolsTabwith all sections still expanded — contradicting the PR's stated default of "current-mode tools." If this is intentional (preserve explicit user preference), a brief comment would clarify the decision.💡 If reset is desired
function onModeChanged() { if (root.shouldKeepExplicitTab(root.currentTab)) return root.currentTab = root.defaultTabForMode(EditorModeController.currentMode) + root.showAllModeTools = false }🤖 Prompt for 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. In `@qml/PropertiesPanel.qml` around lines 56 - 63, onModeChanged updates root.currentTab but leaves the "All sections" toggle sticky (showAllModeTools), so when switching modes users can end up on the new ModeToolsTab with everything expanded; update onModeChanged (inside the branch where you retarget the Inspector/Mode-Tools pair — i.e. after the shouldKeepExplicitTab check and before/after setting root.currentTab) to reset showAllModeTools to the default for the new mode (e.g., set showAllModeTools = false or call the existing initializer for mode-tool state) so ModeToolsTab starts in the current-mode view; reference showAllModeTools, onModeChanged, root.currentTab, root.defaultTabForMode and EditorModeController.currentMode when making the change.
🧹 Nitpick comments (2)
src/EditorModeController_test.cpp (1)
80-88: ⚡ Quick win
defaultInspectorTabForModeis not tested forEditModeorValidationMode.
modeHasModeToolsasserts all four non-object modes (lines 73–77), butdefaultInspectorTabForModeis only exercised forObjectMode,AnimationMode,MaterialMode, and an invalid ID. Adding the two missing modes would fully close the contract and guard against futuremodeHasModeToolschanges not being reflected indefaultInspectorTabForMode.✅ Proposed additions
EXPECT_EQ(ctrl->defaultInspectorTabForMode(EditorModeController::AnimationMode), EditorModeController::ModeToolsTab); EXPECT_EQ(ctrl->defaultInspectorTabForMode(EditorModeController::MaterialMode), EditorModeController::ModeToolsTab); + EXPECT_EQ(ctrl->defaultInspectorTabForMode(EditorModeController::EditMode), + EditorModeController::ModeToolsTab); + EXPECT_EQ(ctrl->defaultInspectorTabForMode(EditorModeController::ValidationMode), + EditorModeController::ModeToolsTab); EXPECT_EQ(ctrl->defaultInspectorTabForMode(99), EditorModeController::InspectorTab);🤖 Prompt for 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. In `@src/EditorModeController_test.cpp` around lines 80 - 88, Add assertions for the two missing modes so defaultInspectorTabForMode is validated for all non-object modes; specifically call ctrl->defaultInspectorTabForMode(EditorModeController::EditMode) and ctrl->defaultInspectorTabForMode(EditorModeController::ValidationMode) and expect EditorModeController::ModeToolsTab for both, mirroring the existing checks for AnimationMode and MaterialMode (keep the existing invalid-ID/InspectorTab check unchanged).qml/PropertiesPanel.qml (1)
85-117: ⚡ Quick winDecouple Repeater tab labels from
InspectorTabIdenum values.The model array
["Inspector", "Scene", "Mode Tools", "History"]is compared/assigned via its implicitindex(0–3), which silently couples the label order to theInspectorTabIdenum order. Reordering either without updating the other would silently mis-route everysectionVisiblebinding downstream. All other visibility checks already useroot.inspectorTab/root.sceneTab/ etc. — the Repeater should too.♻️ Proposed fix
- model: [ "Inspector", "Scene", "Mode Tools", "History" ] + model: [ + { label: "Inspector", id: root.inspectorTab }, + { label: "Scene", id: root.sceneTab }, + { label: "Mode Tools", id: root.modeToolsTab }, + { label: "History", id: root.historyTab } + ] Rectangle { ... - color: root.currentTab === index + color: root.currentTab === modelData.id ... - font.bold: root.currentTab === index + font.bold: root.currentTab === modelData.id ... - text: modelData + text: modelData.label ... - onClicked: root.currentTab = index + onClicked: root.currentTab = modelData.id🤖 Prompt for 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. In `@qml/PropertiesPanel.qml` around lines 85 - 117, The Repeater currently relies on the implicit numeric index (0–3) to match InspectorTabId values, which couples label order to enum order; change the Repeater.model from a plain string array to an array of objects that include an explicit id referencing the root tab IDs (e.g. { label: "Inspector", id: root.inspectorTab }, { label: "Scene", id: root.sceneTab }, { label: "Mode Tools", id: root.modeToolsTab }, { label: "History", id: root.historyTab }), then update usages: use modelData.label for the Text, compare root.currentTab === modelData.id for the Rectangle color/font.bold logic, and set onClicked: root.currentTab = modelData.id so tab selection no longer depends on the Repeater index.
🤖 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.
Outside diff comments:
In `@qml/PropertiesPanel.qml`:
- Around line 56-63: onModeChanged updates root.currentTab but leaves the "All
sections" toggle sticky (showAllModeTools), so when switching modes users can
end up on the new ModeToolsTab with everything expanded; update onModeChanged
(inside the branch where you retarget the Inspector/Mode-Tools pair — i.e. after
the shouldKeepExplicitTab check and before/after setting root.currentTab) to
reset showAllModeTools to the default for the new mode (e.g., set
showAllModeTools = false or call the existing initializer for mode-tool state)
so ModeToolsTab starts in the current-mode view; reference showAllModeTools,
onModeChanged, root.currentTab, root.defaultTabForMode and
EditorModeController.currentMode when making the change.
---
Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 85-117: The Repeater currently relies on the implicit numeric
index (0–3) to match InspectorTabId values, which couples label order to enum
order; change the Repeater.model from a plain string array to an array of
objects that include an explicit id referencing the root tab IDs (e.g. { label:
"Inspector", id: root.inspectorTab }, { label: "Scene", id: root.sceneTab }, {
label: "Mode Tools", id: root.modeToolsTab }, { label: "History", id:
root.historyTab }), then update usages: use modelData.label for the Text,
compare root.currentTab === modelData.id for the Rectangle color/font.bold
logic, and set onClicked: root.currentTab = modelData.id so tab selection no
longer depends on the Repeater index.
In `@src/EditorModeController_test.cpp`:
- Around line 80-88: Add assertions for the two missing modes so
defaultInspectorTabForMode is validated for all non-object modes; specifically
call ctrl->defaultInspectorTabForMode(EditorModeController::EditMode) and
ctrl->defaultInspectorTabForMode(EditorModeController::ValidationMode) and
expect EditorModeController::ModeToolsTab for both, mirroring the existing
checks for AnimationMode and MaterialMode (keep the existing
invalid-ID/InspectorTab check unchanged).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f808167e-6629-4094-a7b3-0ee0b86d2f55
📒 Files selected for processing (4)
qml/PropertiesPanel.qmlsrc/EditorModeController.cppsrc/EditorModeController.hsrc/EditorModeController_test.cpp
- PropertiesPanel.qml: reset `showAllModeTools` back to false when the editor mode changes. Otherwise toggling "All" in Animation and switching to Material would silently land on Material's ModeToolsTab with every section expanded — contradicting the "current-mode tools" default the filter advertises. - PropertiesPanel.qml: change the top-level tab Repeater model from a plain `["Inspector", "Scene", "Mode Tools", "History"]` array (indexed via the Repeater's implicit `index`) to objects that carry an explicit `id` referencing `root.inspectorTab` / `sceneTab` / `modeToolsTab` / `historyTab`. Reordering either the array or the `InspectorTabId` enum no longer silently mis-routes every `sectionVisible` binding downstream. - EditorModeController_test.cpp: extend `InspectorTabPolicyDefaultsByMode` to also assert `defaultInspectorTabForMode(EditMode)` and `defaultInspectorTabForMode(ValidationMode)` — closes the contract gap with `modeHasModeTools` already covering all four non-object modes. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed all three CodeRabbit findings in 0f4fbf3:
Validation:
|
|
1 participant
Summary
EditorModeControllerAPIs.Current/Allfilter inside the Mode Tools tab so current-mode tools are the default while non-current sections remain reachable.PropertiesPanel.qmlto use tab constants and reusable section visibility helpers instead of hard-coded tab/mode checks.Validation
qmllint qml/PropertiesPanel.qmlcmake --build build_local --target UnitTests -j"$(nproc)"env QT_QPA_PLATFORM=offscreen build_local/bin/UnitTests --gtest_filter=EditorModeControllerTest.*:MainWindowTest.ModeBarLoadsAndModeChangeUpdatesStatusIndicatorcmake --build build_local --target QtMeshEditor -j"$(nproc)"Tracker
Summary by CodeRabbit
New Features
Improvements