Add viewport title-bar display controls#438
Conversation
📝 WalkthroughWalkthroughThis PR adds a ViewportTitleBar widget for dock title bars, integrates it into EditorViewport, exposes MainWindow QAction getters for viewport display toggles, moves those actions into a grouped View menu section, updates ViewCube focus wiring, and adds tests and CMake entries to build and validate the changes. ChangesViewport Title Bar Custom Widget
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 |
![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: 011457cc08
ℹ️ 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".
| if (widget->getOgreWidget() == m_viewportOverlayTarget) | ||
| setViewportOverlayTarget(nullptr); |
There was a problem hiding this comment.
When the active viewport is closed, this path always calls setViewportOverlayTarget(nullptr), which removes the event filter from the old widget and leaves no new target installed. repositionViewportOverlayMenu() then falls back to mDockWidgetList.first() for one-time placement, but subsequent move/resize/show/hide events from that fallback viewport are no longer observed because eventFilter() only tracks m_viewportOverlayTarget. In multi-viewport sessions this can leave the overlay stale or misplaced after layout changes until the user manually focuses a viewport again.
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: 1
🤖 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 1716-1735: MainWindow::createViewportOverlayMenu creates
QQuickWidget actions but doesn't emit telemetry breadcrumbs for the user-facing
toggles; update this function after setting each root property (for
ui->actionShow_Grid, ui->actionShow_Normals, ui->actionShow_Mesh_Info,
ui->actionShow_View_Cube or when wiring their triggers) to call
SentryReporter::addBreadcrumb("ui.action", "<ActionName> toggled") with clear
messages (e.g., "Show Grid toggled", "Show Normals toggled", etc.), ensuring
each tooltip/button toggle from m_viewportOverlayMenu/root object generates the
corresponding breadcrumb so these UI actions 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: 403c5010-2ef3-40df-ab16-98166f89742e
📒 Files selected for processing (7)
qml/ViewportOverlayMenu.qmlsrc/PropertiesPanelController.cppsrc/mainwindow.cppsrc/mainwindow.hsrc/mainwindow_test.cppsrc/qml_resources.qrcui_files/mainwindow.ui
| void MainWindow::createViewportOverlayMenu() | ||
| { | ||
| if (m_viewportOverlayMenu) | ||
| return; | ||
|
|
||
| m_viewportOverlayMenu = new QQuickWidget(this); | ||
| m_viewportOverlayMenu->setObjectName(QStringLiteral("viewportOverlayMenu")); | ||
| m_viewportOverlayMenu->setResizeMode(QQuickWidget::SizeRootObjectToView); | ||
| m_viewportOverlayMenu->setClearColor(Qt::transparent); | ||
| m_viewportOverlayMenu->setAttribute(Qt::WA_TranslucentBackground); | ||
| m_viewportOverlayMenu->setAttribute(Qt::WA_AlwaysStackOnTop); | ||
| m_viewportOverlayMenu->setFocusPolicy(Qt::NoFocus); | ||
| m_viewportOverlayMenu->setFixedSize(168, 38); | ||
| m_viewportOverlayMenu->setSource(QUrl("qrc:/ViewportOverlayMenu/ViewportOverlayMenu.qml")); | ||
| if (QObject* root = m_viewportOverlayMenu->rootObject()) { | ||
| root->setProperty("gridAction", QVariant::fromValue(static_cast<QObject*>(ui->actionShow_Grid))); | ||
| root->setProperty("normalsAction", QVariant::fromValue(static_cast<QObject*>(ui->actionShow_Normals))); | ||
| root->setProperty("meshInfoAction", QVariant::fromValue(static_cast<QObject*>(ui->actionShow_Mesh_Info))); | ||
| root->setProperty("viewCubeAction", QVariant::fromValue(static_cast<QObject*>(ui->actionShow_View_Cube))); | ||
| } |
There was a problem hiding this comment.
Add breadcrumb telemetry for viewport display toggles.
The new overlay path exposes user-facing toggles (Show Grid/Normals/Mesh Info/View Cube), but this flow still lacks explicit ui.action breadcrumbs for those actions.
💡 Suggested patch
void MainWindow::createViewportOverlayMenu()
{
if (m_viewportOverlayMenu)
return;
@@
if (QObject* root = m_viewportOverlayMenu->rootObject()) {
root->setProperty("gridAction", QVariant::fromValue(static_cast<QObject*>(ui->actionShow_Grid)));
root->setProperty("normalsAction", QVariant::fromValue(static_cast<QObject*>(ui->actionShow_Normals)));
root->setProperty("meshInfoAction", QVariant::fromValue(static_cast<QObject*>(ui->actionShow_Mesh_Info)));
root->setProperty("viewCubeAction", QVariant::fromValue(static_cast<QObject*>(ui->actionShow_View_Cube)));
}
+
+ auto logViewportToggle = [](const QString& label, bool enabled) {
+ SentryReporter::addBreadcrumb(
+ "ui.action",
+ QString("Viewport overlay toggle: %1 %2")
+ .arg(label, enabled ? QStringLiteral("on") : QStringLiteral("off")));
+ };
+ connect(ui->actionShow_Grid, &QAction::toggled, this,
+ [logViewportToggle](bool v) { logViewportToggle(QStringLiteral("Grid"), v); });
+ connect(ui->actionShow_Normals, &QAction::toggled, this,
+ [logViewportToggle](bool v) { logViewportToggle(QStringLiteral("Normals"), v); });
+ connect(ui->actionShow_Mesh_Info, &QAction::toggled, this,
+ [logViewportToggle](bool v) { logViewportToggle(QStringLiteral("Mesh Info"), v); });
+ connect(ui->actionShow_View_Cube, &QAction::toggled, this,
+ [logViewportToggle](bool v) { logViewportToggle(QStringLiteral("View Cube"), v); });
+
m_viewportOverlayMenu->hide();
@@
}As per coding guidelines "Track all user-facing actions and significant operations with SentryReporter::addBreadcrumb(category, message). Use categories: \"ui.action\" for toolbar/menu clicks".
🤖 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 1716 - 1735,
MainWindow::createViewportOverlayMenu creates QQuickWidget actions but doesn't
emit telemetry breadcrumbs for the user-facing toggles; update this function
after setting each root property (for ui->actionShow_Grid,
ui->actionShow_Normals, ui->actionShow_Mesh_Info, ui->actionShow_View_Cube or
when wiring their triggers) to call SentryReporter::addBreadcrumb("ui.action",
"<ActionName> toggled") with clear messages (e.g., "Show Grid toggled", "Show
Normals toggled", etc.), ensuring each tooltip/button toggle from
m_viewportOverlayMenu/root object generates the corresponding breadcrumb so
these UI actions are tracked.
fernandotonon
force-pushed
the
codex/viewport-overlay-menu
branch
from
011457c to
0cf82c3
Compare
|
Updated this PR to fix the overlay rendering issue reported during manual testing. Root cause: the overlay was using Fix: rewrote Validation after the fix:
|
fernandotonon
force-pushed
the
codex/viewport-overlay-menu
branch
from
0cf82c3 to
fa07b6d
Compare
fernandotonon
changed the title
|
Updated per manual feedback:
Local validation:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ViewportTitleBar.cpp`:
- Around line 188-222: Replace Qt6-only calls to QMouseEvent::globalPosition()
with Qt5/Qt6-compatible guarded calls: in ViewportTitleBar::mousePressEvent and
ViewportTitleBar::mouseMoveEvent use QT_VERSION_CHECK to call event->globalPos()
(or event->globalPos() returning QPoint) when building against Qt < 6.0.0 and
event->globalPosition().toPoint() for Qt >= 6.0.0; apply the same guarded
replacement for both places that set m_dragStartGlobal and m_dragOffset and
where you compute the move position for m_dock->move(...), ensuring the types
remain QPoint and existing members m_dragStartGlobal and m_dragOffset keep their
semantics.
🪄 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: b8ec7b8c-23fd-48fe-8825-072c7b907117
📒 Files selected for processing (10)
src/CMakeLists.txtsrc/EditorViewport.cppsrc/EditorViewport.hsrc/PropertiesPanelController.cppsrc/ViewportTitleBar.cppsrc/ViewportTitleBar.hsrc/mainwindow.cppsrc/mainwindow.hsrc/mainwindow_test.cppui_files/mainwindow.ui
✅ Files skipped from review due to trivial changes (2)
- src/PropertiesPanelController.cpp
- ui_files/mainwindow.ui
| void ViewportTitleBar::mousePressEvent(QMouseEvent* event) | ||
| { | ||
| // Treat presses on the title label (or the empty strip) as the drag | ||
| // handle — anywhere a child toolbutton answers `childAt` is excluded so | ||
| // clicks on G/N/I/C/float/close fall through to the buttons. | ||
| QWidget* hit = childAt(event->pos()); | ||
| const bool onDragArea = (hit == nullptr || hit == m_titleLabel); | ||
| m_pressedOnDragArea = (event->button() == Qt::LeftButton) && onDragArea && m_dock; | ||
|
|
||
| if (m_pressedOnDragArea) { | ||
| m_dragStartGlobal = event->globalPosition().toPoint(); | ||
| // Track offset between cursor and dock top-left so detaching feels | ||
| // continuous rather than snapping the floating window to the cursor. | ||
| m_dragOffset = event->globalPosition().toPoint() - m_dock->mapToGlobal(QPoint(0, 0)); | ||
| } | ||
|
|
||
| QWidget::mousePressEvent(event); | ||
| } | ||
|
|
||
| void ViewportTitleBar::mouseMoveEvent(QMouseEvent* event) | ||
| { | ||
| if (m_pressedOnDragArea && m_dock && (event->buttons() & Qt::LeftButton)) { | ||
| const QPoint delta = event->globalPosition().toPoint() - m_dragStartGlobal; | ||
| const int distance = qMax(qAbs(delta.x()), qAbs(delta.y())); | ||
|
|
||
| if (!m_dock->isFloating() && distance > QApplication::startDragDistance()) { | ||
| // Detach into a floating window. We resize-then-float (instead | ||
| // of relying on `setFloating(true)`'s default placement) so the | ||
| // window appears under the cursor where the drag started. | ||
| m_dock->setFloating(true); | ||
| } | ||
|
|
||
| if (m_dock->isFloating()) { | ||
| m_dock->move(event->globalPosition().toPoint() - m_dragOffset); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the repo expects Qt5 compatibility and whether this file uses Qt6-only mouse APIs.
set -euo pipefail
echo "== Qt version compatibility hints in repository =="
rg -n "Qt6_FOUND|Qt5::|QT_VERSION_CHECK" src/CMakeLists.txt src/EditorViewport.cpp src/ViewportTitleBar.cpp
echo
echo "== Mouse global-position API usage in ViewportTitleBar.cpp =="
rg -n "globalPosition\\(|globalPos\\(" src/ViewportTitleBar.cppRepository: fernandotonon/QtMeshEditor
Length of output: 726
Replace QMouseEvent::globalPosition() with Qt5/Qt6-compatible alternative.
globalPosition() is a Qt6-only API. The repository maintains Qt5 compatibility (as evidenced by CMakeLists.txt fallback and version checks in EditorViewport.cpp), so this code will break Qt5 builds. Use QT_VERSION_CHECK guards to call globalPos() on Qt5 and globalPosition().toPoint() on Qt6.
Proposed fix
void ViewportTitleBar::mousePressEvent(QMouseEvent* event)
{
QWidget* hit = childAt(event->pos());
const bool onDragArea = (hit == nullptr || hit == m_titleLabel);
m_pressedOnDragArea = (event->button() == Qt::LeftButton) && onDragArea && m_dock;
if (m_pressedOnDragArea) {
- m_dragStartGlobal = event->globalPosition().toPoint();
+ `#if` QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
+ m_dragStartGlobal = event->globalPosition().toPoint();
+ `#else`
+ m_dragStartGlobal = event->globalPos();
+ `#endif`
- m_dragOffset = event->globalPosition().toPoint() - m_dock->mapToGlobal(QPoint(0, 0));
+ m_dragOffset = m_dragStartGlobal - m_dock->mapToGlobal(QPoint(0, 0));
}
QWidget::mousePressEvent(event);
}
void ViewportTitleBar::mouseMoveEvent(QMouseEvent* event)
{
if (m_pressedOnDragArea && m_dock && (event->buttons() & Qt::LeftButton)) {
- const QPoint delta = event->globalPosition().toPoint() - m_dragStartGlobal;
+ `#if` QT_VERSION >= QT_VERSION_CHECK(6, 0, 0)
+ const QPoint globalPos = event->globalPosition().toPoint();
+ `#else`
+ const QPoint globalPos = event->globalPos();
+ `#endif`
+ const QPoint delta = globalPos - m_dragStartGlobal;
const int distance = qMax(qAbs(delta.x()), qAbs(delta.y()));
if (!m_dock->isFloating() && distance > QApplication::startDragDistance()) {
m_dock->setFloating(true);
}
if (m_dock->isFloating()) {
- m_dock->move(event->globalPosition().toPoint() - m_dragOffset);
+ m_dock->move(globalPos - m_dragOffset);
}
}
}🤖 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 188 - 222, Replace Qt6-only calls to
QMouseEvent::globalPosition() with Qt5/Qt6-compatible guarded calls: in
ViewportTitleBar::mousePressEvent and ViewportTitleBar::mouseMoveEvent use
QT_VERSION_CHECK to call event->globalPos() (or event->globalPos() returning
QPoint) when building against Qt < 6.0.0 and event->globalPosition().toPoint()
for Qt >= 6.0.0; apply the same guarded replacement for both places that set
m_dragStartGlobal and m_dragOffset and where you compute the move position for
m_dock->move(...), ensuring the types remain QPoint and existing members
m_dragStartGlobal and m_dragOffset keep their semantics.
fa07b6d to
0ab59d3
Compare
|
Updated again for the first-viewport color issue. Root cause: Fix: removed the focus-driven dock palette mutation from Validation:
Note: running the EditorViewport and MainWindow filters in one combined process still hits an existing Ogre global-resource teardown conflict between fixtures, so I ran those filters separately. |
fernandotonon
force-pushed
the
codex/viewport-overlay-menu
branch
from
0ab59d3 to
fb4b4d7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/EditorViewport_test.cpp`:
- Around line 125-136: The test claims to verify behavior for a focused viewport
but never gives the widget focus; ensure the focused regression path runs by
making the PaintableEditorViewport focusable and actually focusing it before
calling paintEvent: set the viewport focus policy (e.g., Qt::StrongFocus) if
necessary and call viewport.setFocus(...) so that the subsequent call to
PaintableEditorViewport::paintEvent(&paintEvent) exercises the focused-path
logic in that class.
🪄 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: 82bc7eb0-b8c9-4cc5-a772-a6b104dd7834
📒 Files selected for processing (12)
src/CMakeLists.txtsrc/EditorViewport.cppsrc/EditorViewport.hsrc/EditorViewport_test.cppsrc/PropertiesPanelController.cppsrc/ViewportTitleBar.cppsrc/ViewportTitleBar.hsrc/mainwindow.cppsrc/mainwindow.hsrc/mainwindow_test.cpptests/CMakeLists.txtui_files/mainwindow.ui
✅ Files skipped from review due to trivial changes (3)
- src/PropertiesPanelController.cpp
- src/mainwindow_test.cpp
- ui_files/mainwindow.ui
🚧 Files skipped from review as they are similar to previous changes (6)
- src/mainwindow.h
- src/ViewportTitleBar.h
- src/CMakeLists.txt
- src/mainwindow.cpp
- src/EditorViewport.h
- src/ViewportTitleBar.cpp
| TEST_F(EditorViewportTest, PaintEventDoesNotOverridePaletteForFocusedViewport) { | ||
| PaintableEditorViewport viewport(mainWindow, 0); | ||
| QPalette markerPalette = viewport.palette(); | ||
| markerPalette.setColor(QPalette::Window, QColor(12, 34, 56)); | ||
| viewport.setPalette(markerPalette); | ||
|
|
||
| QPaintEvent paintEvent(viewport.rect()); | ||
| viewport.paintEvent(&paintEvent); | ||
|
|
||
| EXPECT_EQ(viewport.palette().color(QPalette::Window), QColor(12, 34, 56)); | ||
| EXPECT_NE(viewport.palette().color(QPalette::Window), QColor(0, 255, 127)); | ||
| } |
There was a problem hiding this comment.
Set a focus precondition for the focused-viewport regression path.
On Line [125], the test name asserts focused behavior, but no widget is focused before invoking paintEvent. A future focus-gated regression could slip through.
💡 Suggested patch
TEST_F(EditorViewportTest, PaintEventDoesNotOverridePaletteForFocusedViewport) {
PaintableEditorViewport viewport(mainWindow, 0);
+ viewport.show();
+ OgreWidget* ogre = viewport.getOgreWidget();
+ ASSERT_NE(ogre, nullptr);
+ ogre->setFocus(Qt::OtherFocusReason);
+ if (app) app->processEvents();
+ if (!ogre->hasFocus()) {
+ GTEST_SKIP() << "Focus not granted by this Qt platform plugin; focused path not testable.";
+ }
+
QPalette markerPalette = viewport.palette();
markerPalette.setColor(QPalette::Window, QColor(12, 34, 56));
viewport.setPalette(markerPalette);
QPaintEvent paintEvent(viewport.rect());
viewport.paintEvent(&paintEvent);🤖 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 125 - 136, The test claims to
verify behavior for a focused viewport but never gives the widget focus; ensure
the focused regression path runs by making the PaintableEditorViewport focusable
and actually focusing it before calling paintEvent: set the viewport focus
policy (e.g., Qt::StrongFocus) if necessary and call viewport.setFocus(...) so
that the subsequent call to PaintableEditorViewport::paintEvent(&paintEvent)
exercises the focused-path logic in that class.
|
1 participant
Summary
EditorViewporttitle bar that hosts the G/N/I/C display controls next to the dock float and close buttons.QActions so menu checkmarks, button state, and existing controllers stay synchronized.Validation
cmake --build build_local --target UnitTests -j"$(nproc)"env QT_QPA_PLATFORM=offscreen build_local/bin/UnitTests --gtest_filter=MainWindowTest.Viewport*cmake --build build_local --target QtMeshEditor -j"$(nproc)"Summary by CodeRabbit
New Features
Bug Fixes
Tests