Complete bottom context panel slice - #455
Merged
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds QML/C++ wiring to reveal bottom-docked tools from the UI, refactors BottomContextPanel summaries (new helper functions and Loader), updates PropertiesPanel to host bottom-tool shortcuts, ensures dock tabification is allowed, tweaks viewport/title-bar sizing and layout margins, and adds tests for mode-tracking and dock behavior. ChangesBottom Tool Reveal & Enhanced Summary UI
Sequence Diagram(s)sequenceDiagram
participant User as User / UI
participant QML as BottomContextPanel / PropertiesPanel
participant CPP as MainWindow
participant Dock as Bottom Docks
User->>QML: Click workspace shortcut (e.g., "Asset Browser")
QML->>CPP: revealBottomTool("assetBrowser") via bottomToolHost
CPP->>CPP: map toolId -> dock widget
CPP->>CPP: enable AllowTabbedDocks flag
CPP->>Dock: showBottomToolDock(selectedDock)
Dock->>Dock: show & tabify with other bottom docks
Dock->>QML: Dock becomes visible in bottom area
QML->>User: UI reflects active bottom tool
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 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 `@src/mainwindow_test.cpp`:
- Around line 390-407: The test
BottomToolRevealReturnsDetachedContextDockToBottomArea can be flaky because the
dock may not enter floating state when the main window isn't shown; before
calling window->m_bottomContextDock->setFloating(true) in MainWindowTest, call
window->show() and then app->processEvents() to ensure the window is visible to
Qt's state machine, then proceed with setFloating(true), app->processEvents(),
hide(), revealBottomTool(...), and the subsequent assertions.
In `@src/mainwindow.cpp`:
- Around line 1732-1746: revealBottomTool currently shows the requested dock
(m_bottomContextDock, m_assetBrowserDock, m_dopeSheetDock, m_curveEditorDock)
but does not emit telemetry; add a Sentry breadcrumb at the start of
MainWindow::revealBottomTool using SentryReporter::addBreadcrumb("ui.action",
"<Rail: Name>") where the message matches the existing rail labels (e.g., "Rail:
Dope Sheet", "Rail: Curve Editor", "Rail: Asset Browser", "Rail: Context")
depending on which toolId branch is taken, then call showBottomToolDock(dock) as
before so all QML-initiated reveals are tracked.
🪄 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: 2a2e9987-0710-43da-bca7-6994bd09f4cf
📒 Files selected for processing (4)
qml/BottomContextPanel.qmlsrc/mainwindow.cppsrc/mainwindow.hsrc/mainwindow_test.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/EditorViewport_test.cpp (1)
144-146: ⚡ Quick winHarden geometry assertion timing to reduce CI flakiness.
At Line 144-Line 146, a single
processEvents()aftershow()can still race with final geometry updates. Consider waiting for exposure/visibility before readinggeometry().Proposed patch
viewport.resize(640, 360); viewport.show(); - app->processEvents(); + app->processEvents(); + QTRY_VERIFY(viewport.isVisible()); + QTRY_VERIFY(viewport.getOgreWidget()->isVisible());🤖 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/EditorViewport_test.cpp` around lines 144 - 146, The current test calls viewport.show() then a single app->processEvents() before reading viewport.geometry(), which can race with final geometry updates; replace the single processEvents() with a deterministic wait for the widget/window to be exposed/visible (for example use QTest::qWaitForWindowExposed(&viewport) or wait on QWindow::exposed/QWindow::visibilityChanged or poll QWidget::isVisible() with processEvents) before calling geometry() so the geometry reflects the final displayed size.src/ViewportTitleBar.cpp (1)
177-179: ⚡ Quick winAvoid hard-locking viewport button text to 8px.
At Line 178, forcing a fixed pixel size can make labels too small under DPI scaling/accessibility font settings. Prefer scaling relative to the current font and clamping a minimum.
Proposed patch
- QFont buttonFont = button->font(); - buttonFont.setPixelSize(kViewportButtonFontPixelSize); - button->setFont(buttonFont); + QFont buttonFont = button->font(); + if (buttonFont.pointSizeF() > 0.0) { + buttonFont.setPointSizeF(qMax(7.0, buttonFont.pointSizeF() - 1.0)); + } else if (buttonFont.pixelSize() > 0) { + buttonFont.setPixelSize(qMax(8, buttonFont.pixelSize() - 1)); + } else { + buttonFont.setPixelSize(kViewportButtonFontPixelSize); + } + button->setFont(buttonFont);🤖 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/ViewportTitleBar.cpp` around lines 177 - 179, The code currently forces a fixed pixel size via kViewportButtonFontPixelSize on the button's QFont (QFont buttonFont; buttonFont.setPixelSize(kViewportButtonFontPixelSize); button->setFont(buttonFont)); change this to scale relative to the button's existing font size and clamp a sensible minimum: read the current font size (use pixelSize() or pointSizeF()), compute a scaled size (respecting devicePixelRatio or QFontMetrics scaling) and ensure it is at least a minimum value, then apply that size with setPixelSize or setPointSizeF before calling button->setFont; reference QFont, buttonFont, kViewportButtonFontPixelSize, setPixelSize, and setFont when locating and updating the code.
🤖 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 `@src/mainwindow.cpp`:
- Around line 1734-1763: revealBottomTool currently silently ignores unknown
toolId values; update it to explicitly handle the default/unknown case by adding
an else branch that logs a clear breadcrumb via SentryReporter::addBreadcrumb
(e.g., "ui.action" and a message like "Rail: Unknown bottom tool: <toolId>") and
optionally report or assert as appropriate, then avoid calling
showBottomToolDock; place this logic in MainWindow::revealBottomTool alongside
the existing branches (referencing revealBottomTool,
SentryReporter::addBreadcrumb, and showBottomToolDock) so missed QML wiring or
invalid IDs are recorded rather than being a silent no-op.
---
Nitpick comments:
In `@src/EditorViewport_test.cpp`:
- Around line 144-146: The current test calls viewport.show() then a single
app->processEvents() before reading viewport.geometry(), which can race with
final geometry updates; replace the single processEvents() with a deterministic
wait for the widget/window to be exposed/visible (for example use
QTest::qWaitForWindowExposed(&viewport) or wait on
QWindow::exposed/QWindow::visibilityChanged or poll QWidget::isVisible() with
processEvents) before calling geometry() so the geometry reflects the final
displayed size.
In `@src/ViewportTitleBar.cpp`:
- Around line 177-179: The code currently forces a fixed pixel size via
kViewportButtonFontPixelSize on the button's QFont (QFont buttonFont;
buttonFont.setPixelSize(kViewportButtonFontPixelSize);
button->setFont(buttonFont)); change this to scale relative to the button's
existing font size and clamp a sensible minimum: read the current font size (use
pixelSize() or pointSizeF()), compute a scaled size (respecting devicePixelRatio
or QFontMetrics scaling) and ensure it is at least a minimum value, then apply
that size with setPixelSize or setPointSizeF before calling button->setFont;
reference QFont, buttonFont, kViewportButtonFontPixelSize, setPixelSize, and
setFont when locating and updating the code.
🪄 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: 73c97547-7de3-4e25-a5b2-9024278800e5
📒 Files selected for processing (9)
qml/BottomContextPanel.qmlqml/ModeBar.qmlqml/PropertiesPanel.qmlsrc/EditorViewport.cppsrc/EditorViewport_test.cppsrc/ViewportTitleBar.cppsrc/mainwindow.cppsrc/mainwindow_test.cppui_files/mainwindow.ui
💤 Files with no reviewable changes (1)
- qml/ModeBar.qml
✅ Files skipped from review due to trivial changes (1)
- ui_files/mainwindow.ui
Comment on lines
+1734
to
+1763
| void MainWindow::revealBottomTool(const QString& toolId) | ||
| { | ||
| QDockWidget* dock = nullptr; | ||
| QString breadcrumb; | ||
| if (toolId == QStringLiteral("context")) | ||
| { | ||
| dock = m_bottomContextDock; | ||
| breadcrumb = QStringLiteral("Rail: Context"); | ||
| } | ||
| else if (toolId == QStringLiteral("assetBrowser")) | ||
| { | ||
| dock = m_assetBrowserDock; | ||
| breadcrumb = QStringLiteral("Rail: Asset Browser"); | ||
| } | ||
| else if (toolId == QStringLiteral("dopeSheet")) | ||
| { | ||
| dock = m_dopeSheetDock; | ||
| breadcrumb = QStringLiteral("Rail: Dope Sheet"); | ||
| } | ||
| else if (toolId == QStringLiteral("curveEditor")) | ||
| { | ||
| dock = m_curveEditorDock; | ||
| breadcrumb = QStringLiteral("Rail: Curve Editor"); | ||
| } | ||
|
|
||
| if (dock) { | ||
| SentryReporter::addBreadcrumb("ui.action", breadcrumb); | ||
| showBottomToolDock(dock); | ||
| } | ||
| } |
There was a problem hiding this comment.
Handle unknown bottom-tool IDs explicitly in revealBottomTool.
Right now unknown toolId values are a silent no-op, which makes QML wiring mistakes hard to diagnose and leaves attempted user actions untracked.
🔧 Suggested hardening
void MainWindow::revealBottomTool(const QString& toolId)
{
QDockWidget* dock = nullptr;
QString breadcrumb;
@@
else if (toolId == QStringLiteral("curveEditor"))
{
dock = m_curveEditorDock;
breadcrumb = QStringLiteral("Rail: Curve Editor");
}
- if (dock) {
- SentryReporter::addBreadcrumb("ui.action", breadcrumb);
- showBottomToolDock(dock);
- }
+ if (!dock) {
+ SentryReporter::addBreadcrumb(
+ "ui.action",
+ QStringLiteral("Rail: Unknown bottom tool (%1)").arg(toolId));
+ return;
+ }
+
+ SentryReporter::addBreadcrumb("ui.action", breadcrumb);
+ showBottomToolDock(dock);
}As per coding guidelines, "All user-facing actions and significant operations must be tracked with SentryReporter::addBreadcrumb(category, message)."
📝 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.
Suggested change
| void MainWindow::revealBottomTool(const QString& toolId) | |
| { | |
| QDockWidget* dock = nullptr; | |
| QString breadcrumb; | |
| if (toolId == QStringLiteral("context")) | |
| { | |
| dock = m_bottomContextDock; | |
| breadcrumb = QStringLiteral("Rail: Context"); | |
| } | |
| else if (toolId == QStringLiteral("assetBrowser")) | |
| { | |
| dock = m_assetBrowserDock; | |
| breadcrumb = QStringLiteral("Rail: Asset Browser"); | |
| } | |
| else if (toolId == QStringLiteral("dopeSheet")) | |
| { | |
| dock = m_dopeSheetDock; | |
| breadcrumb = QStringLiteral("Rail: Dope Sheet"); | |
| } | |
| else if (toolId == QStringLiteral("curveEditor")) | |
| { | |
| dock = m_curveEditorDock; | |
| breadcrumb = QStringLiteral("Rail: Curve Editor"); | |
| } | |
| if (dock) { | |
| SentryReporter::addBreadcrumb("ui.action", breadcrumb); | |
| showBottomToolDock(dock); | |
| } | |
| } | |
| void MainWindow::revealBottomTool(const QString& toolId) | |
| { | |
| QDockWidget* dock = nullptr; | |
| QString breadcrumb; | |
| if (toolId == QStringLiteral("context")) | |
| { | |
| dock = m_bottomContextDock; | |
| breadcrumb = QStringLiteral("Rail: Context"); | |
| } | |
| else if (toolId == QStringLiteral("assetBrowser")) | |
| { | |
| dock = m_assetBrowserDock; | |
| breadcrumb = QStringLiteral("Rail: Asset Browser"); | |
| } | |
| else if (toolId == QStringLiteral("dopeSheet")) | |
| { | |
| dock = m_dopeSheetDock; | |
| breadcrumb = QStringLiteral("Rail: Dope Sheet"); | |
| } | |
| else if (toolId == QStringLiteral("curveEditor")) | |
| { | |
| dock = m_curveEditorDock; | |
| breadcrumb = QStringLiteral("Rail: Curve Editor"); | |
| } | |
| if (!dock) { | |
| SentryReporter::addBreadcrumb( | |
| "ui.action", | |
| QStringLiteral("Rail: Unknown bottom tool (%1)").arg(toolId)); | |
| return; | |
| } | |
| SentryReporter::addBreadcrumb("ui.action", breadcrumb); | |
| showBottomToolDock(dock); | |
| } |
🤖 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 1734 - 1763, revealBottomTool currently
silently ignores unknown toolId values; update it to explicitly handle the
default/unknown case by adding an else branch that logs a clear breadcrumb via
SentryReporter::addBreadcrumb (e.g., "ui.action" and a message like "Rail:
Unknown bottom tool: <toolId>") and optionally report or assert as appropriate,
then avoid calling showBottomToolDock; place this logic in
MainWindow::revealBottomTool alongside the existing branches (referencing
revealBottomTool, SentryReporter::addBreadcrumb, and showBottomToolDock) so
missed QML wiring or invalid IDs are recorded rather than being a silent no-op.
|
3 tasks
This was referenced Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Testing
qmllint qml/ModeBar.qml qml/PropertiesPanel.qml qml/BottomContextPanel.qmlcmake --build build_local --target UnitTests -j"$(nproc)"env QT_QPA_PLATFORM=offscreen build_local/bin/UnitTests --gtest_filter=MainWindowTest.BottomContextPanelLoadsAndTracksCurrentMode:MainWindowTest.BottomToolRevealTabsContextWithOtherBottomTools:MainWindowTest.BottomToolRevealReturnsDetachedContextDockToBottomArea:MainWindowTest.ModeBarLoadsAndModeChangeUpdatesStatusIndicatorenv QT_QPA_PLATFORM=offscreen build_local/bin/UnitTests --gtest_filter=EditorViewportTest.OgreWidgetStartsFlushBelowTitleBar:EditorViewportTest.PaintEventDoesNotOverridePaletteForFocusedViewportcmake --build build_local --target QtMeshEditor -j"$(nproc)"Part of #391.
Summary by CodeRabbit
New Features
Changes