From 686c4835c195fbc6c86025e7a374c6132ac9cce7 Mon Sep 17 00:00:00 2001 From: Volker Hilsheimer Date: Wed, 30 Oct 2024 11:03:47 +0100 Subject: [PATCH] QMenu: don't show an empty menu MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If a menu contains no or only invisible actions, then its sizeHint might become invalid (depending on style), resulting in an invalid geometry. In that case, return early from popup(). Otherwise we try to show popup window with an invalid size, which results in unpredictable behavior, depending on the windowing system. To minimize possible fallout from this change, don't return early from QWidget::show (or QWindow::setVisible), but fix this locally in QMenu. The QMenuBar test used empty menus to confirm that hovering over other menu bar entries would highlight the menu bar entry under the mojuse, and close the currently open menu. This can be tested better and more reliably with menus that are not empty, which is (probably) also going to fix the test on wayland. Fixes: QTBUG-129108 Change-Id: Icc52528e89baefea04b3b27e6f02674bf74162b2 Reviewed-by: Richard Moe Gustavsen Reviewed-by: Tor Arne Vestbø (cherry picked from commit 353ce5344fbde5a6cecbdd2c131e1cf0f4b7f383) Reviewed-by: Qt Cherry-pick Bot --- src/widgets/widgets/qmenu.cpp | 11 +++++- .../auto/widgets/widgets/qmenu/tst_qmenu.cpp | 34 +++++++++++++++++++ .../widgets/widgets/qmenubar/tst_qmenubar.cpp | 5 ++- 3 files changed, 46 insertions(+), 4 deletions(-) diff --git a/src/widgets/widgets/qmenu.cpp b/src/widgets/widgets/qmenu.cpp index 86865506c74..5c4b60a8c6a 100644 --- a/src/widgets/widgets/qmenu.cpp +++ b/src/widgets/widgets/qmenu.cpp @@ -2459,6 +2459,13 @@ void QMenuPrivate::popup(const QPoint &p, QAction *atAction, PositionFunction po } } + // do nothing if we don't have a valid size, e.g. when all actions are invisible + if (!size.isValid()) { + eventLoop = nullptr; + syncAction = nullptr; + return; + } + const QPoint mouse = QGuiApplicationPrivate::lastCursorPosition.toPoint(); mousePopupPos = mouse; const bool snapToMouse = !causedPopup.widget && (QRect(p.x() - 3, p.y() - 3, 6, 6).contains(mouse)); @@ -2505,6 +2512,7 @@ void QMenuPrivate::popup(const QPoint &p, QAction *atAction, PositionFunction po } } } + const int subMenuOffset = q->style()->pixelMetric(QStyle::PM_SubMenuOverlap, nullptr, q); QMenu *caused = qobject_cast(causedPopup.widget); if (caused && caused->geometry().width() + menuSizeHint.width() + subMenuOffset < screen.width()) { @@ -2667,7 +2675,8 @@ QAction *QMenuPrivate::exec(const QPoint &p, QAction *action, PositionFunction p popup(p, action, positionFunction); QPointer guard = q; - (void) evtLoop.exec(); + if (eventLoop) // popup might have reset if there was nothing to show + (void)eventLoop->exec(); if (guard.isNull()) return nullptr; diff --git a/tests/auto/widgets/widgets/qmenu/tst_qmenu.cpp b/tests/auto/widgets/widgets/qmenu/tst_qmenu.cpp index aa6823ed690..52f24505dc0 100644 --- a/tests/auto/widgets/widgets/qmenu/tst_qmenu.cpp +++ b/tests/auto/widgets/widgets/qmenu/tst_qmenu.cpp @@ -118,6 +118,8 @@ private slots: void nestedTearOffDetached(); void closeMenuOnClickIfMouseHasntMoved(); + void invisibleActions(); + protected slots: void onActivated(QAction*); void onHighlighted(QAction*); @@ -2145,5 +2147,37 @@ void tst_QMenu::closeMenuOnClickIfMouseHasntMoved() QTest::mouseClick(&contextMenu, Qt::RightButton, {}, contextMenu.mapFromGlobal(pos)); } +void tst_QMenu::invisibleActions() +{ + QWidget window; + window.resize(100, 100); + window.show(); + + const QPoint globalPos = window.mapToGlobal(window.rect().center()); + + QVERIFY(QTest::qWaitForWindowExposed(&window)); + + QMenu contextMenu; + QList actions; + for (int i = 0; i < 5; ++i) + actions << contextMenu.addAction("action"); + QVERIFY(contextMenu.sizeHint().isValid()); + + contextMenu.popup(globalPos); + QVERIFY(contextMenu.isVisible()); + + contextMenu.close(); + + for (const auto &action : actions) + action->setVisible(false); + + contextMenu.popup(globalPos); + QCOMPARE(contextMenu.isVisible(), contextMenu.sizeHint().isValid()); + + // if it wasn't shown previously, then exec() shouldn't do anything either + if (!contextMenu.isVisible()) + QVERIFY(!contextMenu.exec()); +} + QTEST_MAIN(tst_QMenu) #include "tst_qmenu.moc" diff --git a/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp b/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp index 06750b099bf..a41d23c0ac2 100644 --- a/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp +++ b/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp @@ -1090,15 +1090,14 @@ void tst_QMenuBar::task256322_highlight() if (!QGuiApplication::platformName().compare(QLatin1String("minimal"), Qt::CaseInsensitive)) QSKIP("Highlighting does not work correctly for minimal platform"); - if (QGuiApplication::platformName().startsWith(QLatin1String("wayland"), Qt::CaseInsensitive)) - QSKIP("Wayland: This fails. Figure out why."); - QMainWindow win; win.menuBar()->setNativeMenuBar(false); //we can't check the geometry of native menubars QMenu menu; + menu.addAction("New"); QAction *file = win.menuBar()->addMenu(&menu); file->setText("file"); QMenu menu2; + menu2.addAction("Open"); QAction *file2 = win.menuBar()->addMenu(&menu2); file2->setText("file2"); QAction *nothing = win.menuBar()->addAction("nothing");