Skip to content

Modernize editor mode surfaces - #432

Merged
fernandotonon merged 4 commits into
masterfrom
codex/editor-ui-viewport-defaults
May 8, 2026
Merged

Modernize editor mode surfaces#432
fernandotonon merged 4 commits into
masterfrom
codex/editor-ui-viewport-defaults

Conversation

@fernandotonon

@fernandotonon fernandotonon commented May 7, 2026

Copy link
Copy Markdown
Owner

Summary

  • Adds the editor mode controller, mode bar, inspector tabs, bottom tool grouping, and bottom context panel UI surfaces.
  • Clarifies whether transforms affect node placement or mesh geometry, including edit mode always reporting Mesh Geometry.
  • Sets default viewport MSAA to 4x and near clip distance to 0.01, bumps the app version to 2.36.0, and adds focused coverage for the new UI/controller paths.

Validation

  • cmake -S . -B build_local
  • cmake --build build_local --target QtMeshEditor -j"$(nproc)"
  • cmake --build build_local --target UnitTests -j"$(nproc)"
  • qmllint qml/PropertiesPanel.qml qml/SceneTreeNode.qml qml/PreferencesDialog.qml qml/BottomContextPanel.qml qml/ModeBar.qml
  • env QT_QPA_PLATFORM=offscreen build_local/bin/UnitTests --gtest_filter=OgreWidgetTest.ViewportDefaultsApplyWhenUnset:PropertiesPanelControllerTests.TransformTargetMetadataExplainsSelectionImpact:MainWindowTest.AssetBrowserMenuActionTracksDockVisibility:MainWindowTest.BottomToolDockRedocksAndUsesDefaultDockedHeight:EditorModeControllerTest.*
  • build_local/bin/QtMeshEditor --version

Notes

  • The accidental direct push to master was restored with normal revert commits because branch protection rejected force-pushing master back.
  • A full UnitTests run aborted late in MCPServerTest.CameraTools_WithMainWindowExecuteSuccessPaths with an OGRE MatPreviewRTT cleanup exception; that same test passes when run alone, so this appears to be an existing order-dependent full-suite issue rather than a regression from this PR.

Summary by CodeRabbit

Release Notes – Version 2.36.0

  • New Features

    • Added a dedicated mode bar for seamless switching between Object, Edit, Animation, Material, and Validation editor modes.
    • Introduced a bottom context panel displaying mode-specific metrics and action buttons.
    • Redesigned Properties panel with tabbed Inspector layout for improved organization.
    • Added type-based badges in the scene tree for better visual clarity.
  • Improvements

    • Adjusted default near clip distance setting for improved viewport visibility.
    • Enhanced viewport settings management with configurable rendering parameters.

@coderabbitai

coderabbitai Bot commented May 7, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Rate limit exceeded

@fernandotonon has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 35 minutes and 27 seconds before requesting another review.

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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 39165e01-f843-4172-8b11-7d5478be6c94

📥 Commits

Reviewing files that changed from the base of the PR and between e3d07d0 and ec9b754.

📒 Files selected for processing (10)
  • qml/BottomContextPanel.qml
  • qml/PropertiesPanel.qml
  • src/EditorModeController.cpp
  • src/EditorModeController_test.cpp
  • src/OgreWidget.cpp
  • src/PropertiesPanelController.cpp
  • src/PropertiesPanelController.h
  • src/mainwindow.cpp
  • src/mainwindow.h
  • src/mainwindow_test.cpp
📝 Walkthrough

Walkthrough

This PR implements a mode-driven editor architecture by introducing EditorModeController singleton to manage five editor modes (Object, Edit, Animation, Material, Validation), adding corresponding QML UI components (ModeBar, BottomContextPanel) with a tabbed PropertiesPanel layout, integrating into MainWindow with dock management and toolbar wiring, and adding transform target metadata properties to PropertiesPanelController.

Changes

Editor Mode System & UI Refactoring

Layer / File(s) Summary
Public API & Data Contracts
src/EditorModeController.h, src/PropertiesPanelController.h, src/ViewportSettingsKeys.h
New EditorModeController class defines Mode enum (Object/Edit/Animation/Material/Validation) with singleton lifecycle and QML-invokable mode switching. PropertiesPanelController adds Q_PROPERTY declarations for transform target metadata (kind/label/detail/affectsMesh). ViewportSettingsKeys adds inline default accessors for FSAA, near/far clip, and camera speed.
Mode Controller Implementation
src/EditorModeController.cpp
Implements singleton instance management, connects to EditModeController signals, validates and routes mode changes via setModeInternal(), maps modes to display names, and emits state-change signals. Records Sentry breadcrumb on mode transitions.
Transform Target Metadata
src/PropertiesPanelController.cpp
Adds methods to compute and expose transform target kind/label/detail based on current selection and edit-mode state (nodes, edit-mesh, entities, sub-entities, mixed). Connects to EditModeController::editModeChanged to refresh metadata on edit-mode transitions.
Viewport Settings Integration
src/OgreWidget.cpp
Replaces hard-coded camera/FSAA defaults with ViewportSettingsKeys accessors in applyViewportCameraFromSettings() and initOgreWindow().
Mode Toolbar Component
qml/ModeBar.qml
New QML component rendering clickable mode buttons, status text, and Edit/Exit Edit toggle; wires EditorModeController.requestMode() and toggleObjectEditMode().
Mode-Driven Context Panel
qml/BottomContextPanel.qml
New QML component with expandable header that conditionally loads summary layouts (object/edit/animation/material/validation) based on EditorModeController.currentMode, displaying mode-specific metrics and action buttons.
Tabbed Properties Panel
qml/PropertiesPanel.qml
Adds currentTab property and tab header UI (Inspector/Scene/Mode Tools/History); conditionally shows sections (Transform, Animations, etc.) based on current tab and selection state; "Edit Mode Indicator" button now calls EditorModeController.toggleObjectEditMode().
Scene Tree Impact Badges
qml/SceneTreeNode.qml
Adds helper functions to derive node type and map to badge text/colors; adds conditional badge Rectangle with color switching based on node selection state.
Near Clip Default Update
qml/PreferencesDialog.qml
Updates "Near Clip Distance" fallback default from 0.1 to 0.01 in TextInput binding and onEditingFinished.
MainWindow Integration
src/mainwindow.h, src/mainwindow.cpp
Adds private members for bottom-context dock and mode-bar UI; introduces createModeSurfaces(), configureBottomToolDock(), showBottomToolDock(), tabifyBottomToolDocks(), and updateToolRailForMode() helpers. Registers EditorModeController as QML singleton before loading Properties panel; connects mode-change signals to edit-mode indicator and toolbar rail updates; refactors Asset Browser/Dope Sheet/Curve Editor toggling to use unified dock management.
Build Configuration
CMakeLists.txt, src/CMakeLists.txt, tests/CMakeLists.txt, src/qml_resources.qrc
Bumps project version 2.35.0 → 2.36.0. Adds EditorModeController.cpp/h to source lists. Registers new QML resources (ModeBar.qml, BottomContextPanel.qml).
Test Coverage
src/EditorModeController_test.cpp, src/PropertiesPanelController_test.cpp, src/OgreWidget_test.cpp, src/mainwindow_test.cpp
New tests verify mode name mapping, mode switching, transform target metadata across selection scenarios, viewport default application, and bottom dock re-docking behavior.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

  • fernandotonon/QtMeshEditor#305: Both PRs move edit-mode topology actions into the main toolbar and update MainWindow toolbar/PropertiesPanel wiring.
  • fernandotonon/QtMeshEditor#281: Both PRs integrate edit-mode controller support and add corresponding QML bindings (EditorModeController, transform metadata) with PropertiesPanel changes.
  • fernandotonon/QtMeshEditor#221: Both PRs modify PropertiesPanelController tests and add transform-target properties with corresponding test coverage.

Poem

🐰 Five modes now shine in a coordinated dance,
The controller orchestrates each transformation's chance,
Tabs guide the inspector, badges mark the way,
A mode-driven editor brighter every day!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 8.93% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Modernize editor mode surfaces' directly reflects the main change: adding editor mode controller, mode bar, inspector tabs, and bottom context panel UI surfaces.
Description check ✅ Passed The PR description follows the template structure with Summary and Validation sections, clearly listing features, technical changes, validation steps, and notes about testing.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/editor-ui-viewport-defaults

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e3d07d00d8

ℹ️ 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".

Comment thread src/OgreWidget.cpp
Comment on lines +251 to +252
ViewportSettingsKeys::fsaaSamples(),
ViewportSettingsKeys::defaultFsaaSamples()).toInt();

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Restore safe default when FSAA setting is unset

In OgreWidget::initOgreWindow, first-run users now get defaultFsaaSamples() (4) whenever Viewport/fsaaSamples is absent, which means we always request FSAA=4 by default. This regresses environments where MSAA is not reliably supported (the previous code explicitly avoided this by defaulting to 0 to prevent black viewports), so fresh installs on those setups can start with an unusable viewport. Please keep the unset default at 0 or add a fallback path that retries window creation without FSAA if the initial render window fails.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (8)
src/EditorModeController_test.cpp (1)

27-51: ⚡ Quick win

No test coverage for EditMode and ObjectMode request paths

requestMode(EditorModeController::EditMode) is described in the PR as having special behavior — it delegates to EditModeController to enter/exit edit mode. Similarly, ObjectMode is the default state. Neither path is exercised by the current tests. Consider adding at least a requestMode(ObjectMode) smoke test and a minimal requestMode(EditMode) test (verifying currentMode() updates or that the delegation to EditModeController is attempted).

🤖 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 27 - 51, Add tests covering
the edit/object mode paths: call
EditorModeController::requestMode(EditorModeController::ObjectMode) and assert
currentMode() becomes ObjectMode (smoke test), and call
requestMode(EditorModeController::EditMode) and assert either
EditorModeController::currentMode() updates to EditMode or that
EditModeController::instance()/enterEditMode() is invoked (use a mock or spy on
EditModeController if available) so the delegation path through
EditModeController is exercised; target the EditorModeController::requestMode,
EditorModeController::currentMode, and EditModeController entry points in your
new test cases.
src/PropertiesPanelController.cpp (1)

271-314: ⚡ Quick win

transformTargetLabel, transformTargetDetail, and transformAffectsMesh each re-invoke transformTargetKind() independently

Each of these three methods calls transformTargetKind(), which in turn accesses EditModeController::instance() and queries the SelectionSet three times separately. More importantly, they compare return strings against QStringLiteral values — any change to the string constants in transformTargetKind() silently breaks all callers. Consider either:

  1. Enum: Return a TransformTarget enum from transformTargetKind() (and keep the string version as a separate display helper).
  2. Compute-once: Add a private helper TransformTarget resolveTargetKind() const and call it once per public method.
🤖 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/PropertiesPanelController.cpp` around lines 271 - 314, The three methods
transformTargetLabel(), transformTargetDetail(), and transformAffectsMesh()
repeatedly call transformTargetKind() and rely on string literals, which is
fragile and causes multiple SelectionSet queries; change transformTargetKind()
to return a typed TransformTarget enum (e.g., enum class TransformTarget { None,
Node, Mesh, EditMesh, Submesh, MixedGeometry, Mixed }) and update callers to
switch on that enum (keeping a separate helper to produce user-facing strings if
needed), or alternatively add a private resolveTargetKind() that computes the
QString/enum once per public method and reuse that value; update
transformTargetLabel(), transformTargetDetail(), and transformAffectsMesh() to
use the enum/once-computed value instead of re-invoking transformTargetKind()
and comparing QStringLiteral values.
src/mainwindow_test.cpp (1)

832-832: ⚡ Quick win

Consider referencing a named constant instead of the bare literal 180

The assertion on Line 832 (and Line 848) hardcodes 180 as the expected maximumHeight. If the implementation constant changes in mainwindow.cpp, this test will silently diverge and fail with an opaque number mismatch rather than a clear signal. Exposing the constant (e.g., MainWindow::kDefaultDockedMaxHeight) and asserting against it would make the coupling explicit.

🤖 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/mainwindow_test.cpp` at line 832, The test hardcodes the literal 180 when
asserting maximumHeight
(EXPECT_EQ(window->m_assetBrowserDock->widget()->maximumHeight(), 180)), which
is brittle; change the assertion to reference a named public constant from the
implementation (e.g., MainWindow::kDefaultDockedMaxHeight) so the test reads
EXPECT_EQ(window->m_assetBrowserDock->widget()->maximumHeight(),
MainWindow::kDefaultDockedMaxHeight) and update the test includes if needed to
access that symbol.
qml/SceneTreeNode.qml (1)

29-59: ⚖️ Poor tradeoff

typeLabel(), impactBadgeText(), and impactBadgeColor() are non-reactive functions used in QML bindings

These three JS functions call treeModel.data(nodeIndex, 259) directly. QML property bindings that invoke plain JS functions won't automatically re-evaluate when the underlying model data changes (unlike accessing QML properties), so the badge could display stale values if the node type changes without a re-selection event triggering a re-evaluation.

This is consistent with the existing pattern in the file (e.g., the icon color binding on Lines 133–143 already does the same), so there's no regression here. Flagging as a broader awareness note.

🤖 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/SceneTreeNode.qml` around lines 29 - 59, Convert the non-reactive JS
functions typeLabel(), impactBadgeText(), and impactBadgeColor() into QML
properties so their values re-evaluate when treeModel or nodeIndex change; e.g.,
replace function typeLabel() with a property string typeLabel that computes
treeModel ? (treeModel.data(nodeIndex, 259) || "") : "", then replace
impactBadgeText() and impactBadgeColor() with property string impactBadgeText
and property color impactBadgeColor that switch on typeLabel() (using the same
cases "Node"/"Group"/"Mesh"/"Submesh") and fall back to "" or
PropertiesPanelController.borderColor respectively.
qml/PropertiesPanel.qml (2)

33-41: 💤 Low value

currentTab is force-reset on every mode change.

onModeChanged unconditionally writes root.currentTab = 0 or 2, so a user reading the Scene outline (tab 1) or Undo History (tab 3) is pulled away every time the mode changes — including via Tab (toggle Edit). Consider only resetting when entering a new mode for the first time, or only when the current tab is Inspector, e.g.:

♻️ Suggested behavior
     Connections {
         target: EditorModeController
         function onModeChanged() {
-            if (root.showModeToolsForMode(EditorModeController.currentMode))
-                root.currentTab = 2
-            else
-                root.currentTab = 0
+            // Only auto-jump when the user is on a tab that has no mode-aware content.
+            if (root.currentTab === 1 || root.currentTab === 3)
+                return
+            root.currentTab = root.showModeToolsForMode(EditorModeController.currentMode) ? 2 : 0
         }
     }
🤖 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 33 - 41, The handler onModeChanged
currently force-assigns root.currentTab to 0 or 2 on every mode change; change
it to only switch tabs when the user is currently on the Inspector (so we don't
yank them out of Outline/Undo) or when this mode is entered for the first time
by tracking the previous mode. Update the Connections::onModeChanged to: check
root.currentTab and only set root.currentTab = 2 when
root.showModeToolsForMode(EditorModeController.currentMode) is true AND
root.currentTab === /*Inspector index*/ 0 (or alternatively add a root.lastMode
property and only change tabs if EditorModeController.currentMode !==
root.lastMode, then set root.lastMode = EditorModeController.currentMode). Use
the existing symbols onModeChanged, root.currentTab,
EditorModeController.currentMode and root.showModeToolsForMode to locate and
implement the conditional/lastMode logic.

215-300: 🏗️ Heavy lift

Consider gating "Mode Tools" sections by EditorModeController.currentMode in addition to data availability.

All currentTab === 2 sections are visibility-gated only on data (editModeActive, hasAnimations, MeshLodController.hasSelection, etc.) and not on the active mode. As a result, when the user is in AnimationMode with a selected mesh that has materials and validation data, Material Presets and Mesh Validation will all render in the same Mode Tools tab as Animations/Animation Control, which dilutes the mode-driven separation introduced by ModeBar/EditorModeController.

A small AND with EditorModeController.currentMode === EditorModeController.<Mode> per section would keep the new mode model cohesive:

♻️ Suggested gating
             // ---- Animations ----
             CollapsibleSection {
                 title: "Animations"
-                sectionVisible: root.currentTab === 2 && PropertiesPanelController.hasAnimations
+                sectionVisible: root.currentTab === 2
+                    && EditorModeController.currentMode === EditorModeController.AnimationMode
+                    && PropertiesPanelController.hasAnimations
                 ...
             }
             ...
             // ---- Material Presets ----
             CollapsibleSection {
                 title: "Material Presets"
-                sectionVisible: root.currentTab === 2 && PropertiesPanelController.hasSelection
+                sectionVisible: root.currentTab === 2
+                    && EditorModeController.currentMode === EditorModeController.MaterialMode
+                    && PropertiesPanelController.hasSelection
                 ...
             }
             ...
             // ---- Mesh Validation ----
             CollapsibleSection {
                 title: "Mesh Validation"
-                sectionVisible: root.currentTab === 2 && MeshValidator.hasSelection
+                sectionVisible: root.currentTab === 2
+                    && EditorModeController.currentMode === EditorModeController.ValidationMode
+                    && MeshValidator.hasSelection
                 ...
             }
🤖 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 215 - 300, Several CollapsibleSection
blocks gated only by root.currentTab === 2 should also require the editor mode;
update each section that currently uses "root.currentTab === 2 && ..." to also
AND-check EditorModeController.currentMode === EditorModeController.<Mode>.
Specifically: gate Edit Mode Tools with EditorModeController.currentMode ===
EditorModeController.EditMode, gate Animations and Animation Control with
EditorModeController.currentMode === EditorModeController.AnimationMode, gate
LOD Generation with EditorModeController.currentMode ===
EditorModeController.MeshMode (or LODMode if that exists), and gate Material
Presets and Mesh Validation with EditorModeController.currentMode ===
EditorModeController.MaterialMode / EditorModeController.ValidationMode (use the
exact enum names available). Modify the Component.sectionVisible expressions in
PropertiesPanel.qml for editModeToolsComponent, animationComponent,
animControlComponent, lodComponent, materialPresetsComponent, and
validationComponent accordingly.
src/PropertiesPanelController.h (1)

42-45: ⚡ Quick win

Binding mechanism is working correctly; consider renaming signal for clarity.

selectionChanged is intentionally re-emitted on EditModeController::editModeChanged (confirmed in PropertiesPanelController.cpp line 59–60), so QML bindings for transformTargetKind, transformTargetLabel, transformTargetDetail, and transformAffectsMesh will update correctly when edit mode toggles without a selection delta. However, the signal name selectionChanged is semantically misleading since it also fires on edit-mode transitions. Consider renaming to transformTargetMetadataChanged or adding a dedicated notifier signal to improve code clarity for future maintainers.

🤖 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/PropertiesPanelController.h` around lines 42 - 45, The Q_PROPERTY NOTIFY
uses selectionChanged but that signal is also re-emitted on
EditModeController::editModeChanged, which is semantically confusing; add a
dedicated notifier named transformTargetMetadataChanged (or rename
selectionChanged) and update the Q_PROPERTY declarations for
transformTargetKind, transformTargetLabel, transformTargetDetail, and
transformAffectsMesh to use NOTIFY transformTargetMetadataChanged; then update
the emitter sites in PropertiesPanelController.cpp (the place currently
re-emitting selectionChanged on edit-mode changes) to emit
transformTargetMetadataChanged instead and leave selectionChanged reserved for
actual selection updates so bindings remain correct and intent is clear.
src/mainwindow.cpp (1)

1014-1040: ⚡ Quick win

Missing objectName on loopCutAction and convertToQuadsAction breaks mode-driven visibility convention.

Every other edit-mode toolbar action set up in this hunk gets a modeEditXxxAction object name (Extrude, Bevel, Knife, Merge, Delete, Subdivide, Fill, VertexPaint), and updateToolRailForMode() uses that prefix to toggle visibility per editor mode. loopCutAction (Line 1026) and convertToQuadsAction (Line 1040) are added without an objectName, so updateToolRailForMode() will not include them in its mode pass — they survive today only because refreshTopoButtons independently flips them on isEditModeActive(). That implicit second source of truth is exactly the kind of drift updateToolRailForMode() is meant to centralise.

♻️ Proposed fix
     QAction* loopCutAction = ui->objectsToolbar->addWidget(loopCutButton);
+    loopCutAction->setObjectName("modeEditLoopCutAction");
@@
     QAction* convertToQuadsAction = ui->objectsToolbar->addWidget(convertToQuadsButton);
+    convertToQuadsAction->setObjectName("modeEditConvertToQuadsAction");
🤖 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/mainwindow.cpp` around lines 1014 - 1040, loopCutAction and
convertToQuadsAction are missing objectName entries so updateToolRailForMode()
can't toggle them by the modeEdit* convention; set the widget actions'
objectName to "modeEditLoopCutAction" and "modeEditConvertToQuadsAction"
respectively after creating loopCutAction and convertToQuadsAction (same spot
where other modeEdit... names are set) so updateToolRailForMode() will include
them (this removes the accidental reliance on refreshTopoButtons).
🤖 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/BottomContextPanel.qml`:
- Around line 118-124: The Button references materialEditorAction but that
identifier is not declared in BottomContextPanel.qml; declare an explicit QML
property such as property var materialEditorAction: null (or property
alias/required property if you want to enforce injection) at the top of the
component so the Button's onClicked guard has a defined property to check and
won't silently rely on an ambient context value.

In `@src/EditorModeController.cpp`:
- Around line 91-103: syncFromEditMode() unconditionally clobbers the status
text to "Object mode" even when the controller is in other modes; change the
logic so setStatusText(QStringLiteral("Object mode")) is only called when you
actually switch to ObjectMode (i.e., when you call setModeInternal(ObjectMode,
false) or when m_currentMode != ObjectMode), and remove the redundant
setStatusText(edit->modeLabel()) in the EditMode branch since
setModeInternal(EditMode, false) already updates the status text; use
editController()/edit->modeLabel(), m_currentMode, EditMode and ObjectMode to
guide the conditional so the status bar is only updated when the mode is
actually changed.
- Around line 24-26: The status text is hard-coded to "Object mode" even when
m_currentMode is initialized to EditMode; update the constructor logic so after
setting m_currentMode = edit->isEditModeActive() ? EditMode : ObjectMode it
calls setStatusText(...) with the matching text for the current mode (use the
same strings exposed by statusText()), e.g. choose "Edit mode" when
m_currentMode == EditMode and "Object mode" when m_currentMode == ObjectMode so
callers of statusText() immediately see the correct value; adjust the code paths
that set status text (constructor and any places that mutate m_currentMode) to
reuse the same selection logic.

In `@src/OgreWidget.cpp`:
- Around line 80-92: Add Sentry breadcrumbs when applying viewport settings:
call SentryReporter::addBreadcrumb(category, message) immediately before or
after applying camera speed (cam->setCameraSpeed), near clip
(cam->getCamera()->setNearClipDistance) and far clip
(cam->getCamera()->setFarClipDistance) using the settings values retrieved from
settings.value(...) / ViewportSettingsKeys; include the setting key and applied
value in the message. Also add a breadcrumb in the other FSAA/viewport-setting
path referenced around the other application (the block that applies FSAA /
related viewport settings) so both locations emit
SentryReporter::addBreadcrumb(category, message) with clear category and message
describing the applied setting and value.

---

Nitpick comments:
In `@qml/PropertiesPanel.qml`:
- Around line 33-41: The handler onModeChanged currently force-assigns
root.currentTab to 0 or 2 on every mode change; change it to only switch tabs
when the user is currently on the Inspector (so we don't yank them out of
Outline/Undo) or when this mode is entered for the first time by tracking the
previous mode. Update the Connections::onModeChanged to: check root.currentTab
and only set root.currentTab = 2 when
root.showModeToolsForMode(EditorModeController.currentMode) is true AND
root.currentTab === /*Inspector index*/ 0 (or alternatively add a root.lastMode
property and only change tabs if EditorModeController.currentMode !==
root.lastMode, then set root.lastMode = EditorModeController.currentMode). Use
the existing symbols onModeChanged, root.currentTab,
EditorModeController.currentMode and root.showModeToolsForMode to locate and
implement the conditional/lastMode logic.
- Around line 215-300: Several CollapsibleSection blocks gated only by
root.currentTab === 2 should also require the editor mode; update each section
that currently uses "root.currentTab === 2 && ..." to also AND-check
EditorModeController.currentMode === EditorModeController.<Mode>. Specifically:
gate Edit Mode Tools with EditorModeController.currentMode ===
EditorModeController.EditMode, gate Animations and Animation Control with
EditorModeController.currentMode === EditorModeController.AnimationMode, gate
LOD Generation with EditorModeController.currentMode ===
EditorModeController.MeshMode (or LODMode if that exists), and gate Material
Presets and Mesh Validation with EditorModeController.currentMode ===
EditorModeController.MaterialMode / EditorModeController.ValidationMode (use the
exact enum names available). Modify the Component.sectionVisible expressions in
PropertiesPanel.qml for editModeToolsComponent, animationComponent,
animControlComponent, lodComponent, materialPresetsComponent, and
validationComponent accordingly.

In `@qml/SceneTreeNode.qml`:
- Around line 29-59: Convert the non-reactive JS functions typeLabel(),
impactBadgeText(), and impactBadgeColor() into QML properties so their values
re-evaluate when treeModel or nodeIndex change; e.g., replace function
typeLabel() with a property string typeLabel that computes treeModel ?
(treeModel.data(nodeIndex, 259) || "") : "", then replace impactBadgeText() and
impactBadgeColor() with property string impactBadgeText and property color
impactBadgeColor that switch on typeLabel() (using the same cases
"Node"/"Group"/"Mesh"/"Submesh") and fall back to "" or
PropertiesPanelController.borderColor respectively.

In `@src/EditorModeController_test.cpp`:
- Around line 27-51: Add tests covering the edit/object mode paths: call
EditorModeController::requestMode(EditorModeController::ObjectMode) and assert
currentMode() becomes ObjectMode (smoke test), and call
requestMode(EditorModeController::EditMode) and assert either
EditorModeController::currentMode() updates to EditMode or that
EditModeController::instance()/enterEditMode() is invoked (use a mock or spy on
EditModeController if available) so the delegation path through
EditModeController is exercised; target the EditorModeController::requestMode,
EditorModeController::currentMode, and EditModeController entry points in your
new test cases.

In `@src/mainwindow_test.cpp`:
- Line 832: The test hardcodes the literal 180 when asserting maximumHeight
(EXPECT_EQ(window->m_assetBrowserDock->widget()->maximumHeight(), 180)), which
is brittle; change the assertion to reference a named public constant from the
implementation (e.g., MainWindow::kDefaultDockedMaxHeight) so the test reads
EXPECT_EQ(window->m_assetBrowserDock->widget()->maximumHeight(),
MainWindow::kDefaultDockedMaxHeight) and update the test includes if needed to
access that symbol.

In `@src/mainwindow.cpp`:
- Around line 1014-1040: loopCutAction and convertToQuadsAction are missing
objectName entries so updateToolRailForMode() can't toggle them by the modeEdit*
convention; set the widget actions' objectName to "modeEditLoopCutAction" and
"modeEditConvertToQuadsAction" respectively after creating loopCutAction and
convertToQuadsAction (same spot where other modeEdit... names are set) so
updateToolRailForMode() will include them (this removes the accidental reliance
on refreshTopoButtons).

In `@src/PropertiesPanelController.cpp`:
- Around line 271-314: The three methods transformTargetLabel(),
transformTargetDetail(), and transformAffectsMesh() repeatedly call
transformTargetKind() and rely on string literals, which is fragile and causes
multiple SelectionSet queries; change transformTargetKind() to return a typed
TransformTarget enum (e.g., enum class TransformTarget { None, Node, Mesh,
EditMesh, Submesh, MixedGeometry, Mixed }) and update callers to switch on that
enum (keeping a separate helper to produce user-facing strings if needed), or
alternatively add a private resolveTargetKind() that computes the QString/enum
once per public method and reuse that value; update transformTargetLabel(),
transformTargetDetail(), and transformAffectsMesh() to use the
enum/once-computed value instead of re-invoking transformTargetKind() and
comparing QStringLiteral values.

In `@src/PropertiesPanelController.h`:
- Around line 42-45: The Q_PROPERTY NOTIFY uses selectionChanged but that signal
is also re-emitted on EditModeController::editModeChanged, which is semantically
confusing; add a dedicated notifier named transformTargetMetadataChanged (or
rename selectionChanged) and update the Q_PROPERTY declarations for
transformTargetKind, transformTargetLabel, transformTargetDetail, and
transformAffectsMesh to use NOTIFY transformTargetMetadataChanged; then update
the emitter sites in PropertiesPanelController.cpp (the place currently
re-emitting selectionChanged on edit-mode changes) to emit
transformTargetMetadataChanged instead and leave selectionChanged reserved for
actual selection updates so bindings remain correct and intent is clear.
🪄 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: defaults

Review profile: CHILL

Plan: Pro

Run ID: 10b4fd0d-4dd6-43f3-9f3a-7d886faee568

📥 Commits

Reviewing files that changed from the base of the PR and between d4d1299 and e3d07d0.

📒 Files selected for processing (21)
  • CMakeLists.txt
  • qml/BottomContextPanel.qml
  • qml/ModeBar.qml
  • qml/PreferencesDialog.qml
  • qml/PropertiesPanel.qml
  • qml/SceneTreeNode.qml
  • src/CMakeLists.txt
  • src/EditorModeController.cpp
  • src/EditorModeController.h
  • src/EditorModeController_test.cpp
  • src/OgreWidget.cpp
  • src/OgreWidget_test.cpp
  • src/PropertiesPanelController.cpp
  • src/PropertiesPanelController.h
  • src/PropertiesPanelController_test.cpp
  • src/ViewportSettingsKeys.h
  • src/mainwindow.cpp
  • src/mainwindow.h
  • src/mainwindow_test.cpp
  • src/qml_resources.qrc
  • tests/CMakeLists.txt

Comment thread qml/BottomContextPanel.qml
Comment thread src/EditorModeController.cpp
Comment thread src/EditorModeController.cpp
Comment thread src/OgreWidget.cpp Outdated
- EditorModeController: pick the matching status text on construction
  and stop double-setting it from syncFromEditMode (setModeInternal
  already updates it on transitions).
- OgreWidget: emit Sentry breadcrumbs when applying camera speed,
  near/far clip and FSAA viewport settings, in line with project
  telemetry conventions.
- BottomContextPanel.qml: declare materialEditorAction as an explicit
  property; mainwindow.cpp now sets it on the QML root object instead
  of relying on an ambient context property.
- PropertiesPanelController: route the four transformTarget* properties
  through a new transformTargetMetadataChanged signal and a single
  resolveTransformTarget enum so the label/detail/affects-mesh strings
  share one selection probe and can't drift away from the kind names.
- PropertiesPanel.qml: don't yank the user away from Scene/History tabs
  on mode change, and gate Animations/Material Presets/Mesh Validation
  sections by EditorModeController.currentMode so the Mode Tools tab
  stays mode-cohesive.
- mainwindow: tag loopCutAction and convertToQuadsAction with
  modeEdit*Action object names so updateToolRailForMode() can drive
  them centrally; expose kDefaultDockedHeight on MainWindow so tests
  reference the constant instead of a bare 180 literal.
- EditorModeControllerTest: cover the ObjectMode-after-AnimationMode
  status path and the EditMode hint when no mesh is selected.

Co-authored-by: Cursor <cursoragent@cursor.com>
@sonarqubecloud

sonarqubecloud Bot commented May 8, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant