From 52dcd47850138ac257e3ad7e6eeae205bcff4aa6 Mon Sep 17 00:00:00 2001 From: Axel Spoerl Date: Fri, 28 Oct 2022 15:23:41 +0200 Subject: [PATCH] emit QWindow::windowStateChanged only when state has changed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Upon programmatic window state changes, windowStateChange was fired once in QWindow::setWindowStates and once when the visual state had been changed by the window interface. This patch adds if guards to ensure that the singal is fired only once. It adds a corresponding autotest to tst_QWindow. tst_QWidget::resizePropagation() is adapted to no longer expect double signal emission. Fixes: QTBUG-102478 Pick-to: 6.4 6.2 Change-Id: If093c0a883d76d8a676e4fab90db6b0676452267 Reviewed-by: Tor Arne Vestbø --- src/gui/kernel/qguiapplication.cpp | 11 ++- src/gui/kernel/qwindow.cpp | 7 +- tests/auto/gui/kernel/qwindow/tst_qwindow.cpp | 98 +++++++++++++++++++ .../widgets/kernel/qwidget/tst_qwidget.cpp | 65 +++++++----- 4 files changed, 153 insertions(+), 28 deletions(-) diff --git a/src/gui/kernel/qguiapplication.cpp b/src/gui/kernel/qguiapplication.cpp index 36030cac42a..c721cfd8022 100644 --- a/src/gui/kernel/qguiapplication.cpp +++ b/src/gui/kernel/qguiapplication.cpp @@ -2509,11 +2509,16 @@ void QGuiApplicationPrivate::processWindowStateChangedEvent(QWindowSystemInterfa { if (QWindow *window = wse->window.data()) { QWindowPrivate *windowPrivate = qt_window_private(window); - const auto newEffectiveState = QWindowPrivate::effectiveState(windowPrivate->windowState); - QWindowStateChangeEvent e(wse->oldState); + const auto originalEffectiveState = QWindowPrivate::effectiveState(windowPrivate->windowState); + windowPrivate->windowState = wse->newState; - emit window->windowStateChanged(newEffectiveState); + const auto newEffectiveState = QWindowPrivate::effectiveState(windowPrivate->windowState); + if (newEffectiveState != originalEffectiveState) + emit window->windowStateChanged(newEffectiveState); + windowPrivate->updateVisibility(); + + QWindowStateChangeEvent e(wse->oldState); QGuiApplication::sendSpontaneousEvent(window, &e); } } diff --git a/src/gui/kernel/qwindow.cpp b/src/gui/kernel/qwindow.cpp index d01ff11aa76..13e2051e14c 100644 --- a/src/gui/kernel/qwindow.cpp +++ b/src/gui/kernel/qwindow.cpp @@ -1384,8 +1384,13 @@ void QWindow::setWindowStates(Qt::WindowStates state) if (d->platformWindow) d->platformWindow->setWindowState(state); + + auto originalEffectiveState = QWindowPrivate::effectiveState(d->windowState); d->windowState = state; - emit windowStateChanged(QWindowPrivate::effectiveState(d->windowState)); + auto newEffectiveState = QWindowPrivate::effectiveState(d->windowState); + if (newEffectiveState != originalEffectiveState) + emit windowStateChanged(newEffectiveState); + d->updateVisibility(); } diff --git a/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp b/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp index d2d89c0ebf4..1cac1820d96 100644 --- a/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp +++ b/tests/auto/gui/kernel/qwindow/tst_qwindow.cpp @@ -88,6 +88,7 @@ private slots: void activateDeactivateEvent(); void qobject_castOnDestruction(); void touchToMouseTranslationByPopup(); + void stateChangeSignal(); private: QPoint m_availableTopLeft; @@ -2778,6 +2779,103 @@ void tst_QWindow::touchToMouseTranslationByPopup() QTRY_COMPARE(window.mouseReleaseButton, int(Qt::LeftButton)); } +// Test that windowStateChanged is not emitted on noop change (QTBUG-102478) +void tst_QWindow::stateChangeSignal() +{ + // Test only for Windows, Linux and macOS +#if !defined(Q_OS_LINUX) && !defined(Q_OS_WINDOWS) && !defined(Q_OS_DARWIN) + QSKIP("Singular windowStateChanged signal emission is guaranteed for Linux, Windows and macOS only.\n" + "On other operating systems, the signal may be emitted twice."); +#endif + QWindow w; + Q_ASSUME(connect (&w, &QWindow::windowStateChanged, [](Qt::WindowState s){qCDebug(lcTests) << "State change to" << s;})); + QSignalSpy spy(&w, SIGNAL(windowStateChanged(Qt::WindowState))); + unsigned short signalCount = 0; + QList effectiveStates; + Q_ASSUME(connect(&w, &QWindow::windowStateChanged, [&effectiveStates](Qt::WindowState state) + { effectiveStates.append(state); })); + // Part 1: + // => test signal emission on programmatic state changes + QCOMPARE(w.windowState(), Qt::WindowNoState); + // - wait for target state to be set + // - wait for signal spy to have reached target count + // - extract state from signal and compare to target +#define CHECK_STATE(State)\ + QTRY_VERIFY(QTest::qWaitFor([&w](){return (w.windowState() == State); }));\ + CHECK_SIGNAL(State) +#define CHECK_SIGNAL(State)\ + QTRY_COMPARE(spy.count(), signalCount);\ + if (signalCount > 0) {\ + QVariantList list = spy.at(signalCount - 1).toList();\ + QCOMPARE(list.count(), 1);\ + bool ok;\ + const int stateInt = list.at(0).toInt(&ok);\ + QVERIFY(ok);\ + const Qt::WindowState newState = static_cast(stateInt);\ + QCOMPARE(newState, State);\ + } + // Check initialization + CHECK_STATE(Qt::WindowNoState); + // showMaximized after init + // expected behavior: signal emitted once with state == WindowMaximized + ++signalCount; + w.showMaximized(); + CHECK_STATE(Qt::WindowMaximized); + // setWindowState to normal + // expected behavior: signal emitted once with state == WindowNoState + ++signalCount; + w.setWindowState(Qt::WindowNoState); + CHECK_STATE(Qt::WindowNoState); + // redundant setWindowState to normal - except windows, where the no-op is counted + // expected behavior: No emits. + // On Windows, a no-op state change causes a no-op resize and repaint, leading to a + // no-op state change and singal emission. +#ifdef Q_OS_WINDOWS + ++signalCount; + ++signalCount; +#endif + w.setWindowState(Qt::WindowNoState); + CHECK_STATE(Qt::WindowNoState); + // setWindowState to minimized + // expected behavior: signal emitted once with state == WindowMinimized + ++signalCount; + w.showMinimized(); + CHECK_STATE(Qt::WindowMinimized); + // setWindowState to Normal + // expected behavior: signal emitted once with state == WindowNoState + ++signalCount; + w.showNormal(); + CHECK_STATE(Qt::WindowNoState); + /* + - Testcase showFullScreen is omitted: Depending on window manager, + WindowFullScreen can be mapped to WindowMaximized + - Transition from WindowMinimized to WindowMaximized is omitted: + WindowNoState to WindowMaximized + */ + // Part 2: + // => test signal emission on simulated user interaction + // To test the code path, inject state change events into the QPA event queue. + // Test the signal emission only, not the window's actual visible state. + + // Flush pending events and clear + QCoreApplication::processEvents(); + spy.clear(); + effectiveStates.clear(); + signalCount = 0; + // Maximize window + QWindowSystemInterface::handleWindowStateChanged(&w, Qt::WindowMaximized, w.windowState()); + ++signalCount; + CHECK_SIGNAL(Qt::WindowMaximized); + // Normalize window + QWindowSystemInterface::handleWindowStateChanged(&w, Qt::WindowNoState, w.windowState()); + ++signalCount; + CHECK_SIGNAL(Qt::WindowNoState); + // Minimize window + QWindowSystemInterface::handleWindowStateChanged(&w, Qt::WindowMinimized, w.windowState()); + ++signalCount; + CHECK_SIGNAL(Qt::WindowMinimized); +} + #include QTEST_MAIN(tst_QWindow) diff --git a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp index abc939ce93c..1e58b35e88e 100644 --- a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp +++ b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp @@ -2752,18 +2752,15 @@ void tst_QWidget::resizePropagation() QSKIP("resizePropagation test is designed for XCB only"); #endif - // Platform specific notes: - // Linux/XCB: - // - Unless maximized, a widget and its corresponding window can have different sizes - // - windowStateChanged can be fired multiple times (QTBUG-102478) when widget is maximized - // // Windows: // When a widget is maximized after it has been resized, the widget retains its original size, - // while the window shows maximum size + // while the window shows maximum size. + // windowStateChanged signal gets fired on a no-op change from/to WindowNoState // Initialize widget and signal spy for window handle QWidget widget; widget.showMaximized(); + QVERIFY(QTest::qWaitForWindowExposed(&widget)); QWindow *window = widget.windowHandle(); QTRY_VERIFY(window); QSignalSpy spy(window, &QWindow::windowStateChanged); @@ -2773,11 +2770,15 @@ void tst_QWidget::resizePropagation() const QSize size1 = QSize(screenSize.width() * 0.5, screenSize.height() * 0.5); const QSize size2 = QSize(screenSize.width() * 0.625, screenSize.height() * 0.833); - auto verifyResize = [&](const QSize &size, Qt::WindowState windowState, bool checkCountIncrement, bool checkTargetSize) + enum CountIncrementCheck {Equal, Greater}; + enum TargetSizeCheck {Fail, Warn}; + auto verifyResize = [&](const QSize &size, Qt::WindowState windowState, + CountIncrementCheck checkCountIncrement, + TargetSizeCheck checkTargetSize) { // Capture count of latest async signals - if (!checkCountIncrement) - count = spy.size(); + if (checkCountIncrement == Equal) + count = spy.count(); // Resize if required if (size.isValid()) @@ -2787,13 +2788,24 @@ void tst_QWidget::resizePropagation() QVERIFY(QTest::qWaitForWindowExposed(&widget)); // Check signal count and qDebug output for fail analysis - if (checkCountIncrement) { - QTRY_VERIFY(spy.size() > count); - qDebug() << "spy count:" << spy.size() << "previous count:" << count; - count = spy.size(); - } else { - qDebug() << spy << widget.windowState() << window->windowState(); - QCOMPARE(spy.size(), count); + switch (checkCountIncrement) { + case Greater: { + auto logger = qScopeGuard([&](){ + qDebug() << "spy count:" << spy.count() << "previous count:" << count; + }); + QTRY_VERIFY(spy.count() > count); + logger.dismiss(); + count = spy.count(); + } + break; + case Equal: { + auto logger = qScopeGuard([&](){ + qDebug() << spy << widget.windowState() << window->windowState(); + }); + QCOMPARE(spy.count(), count); + logger.dismiss(); + } + break; } // QTRY necessary because state changes are propagated async @@ -2801,32 +2813,37 @@ void tst_QWidget::resizePropagation() QTRY_COMPARE(window->windowState(), windowState); // Check target size with fail or warning - if (checkTargetSize) { + switch (checkTargetSize) { + case Fail: QCOMPARE(widget.size(), window->size()); - } else if (widget.size() != window->size()) { - qWarning() << m_platform << "size mismtach tolerated. Widget:" - << widget.size() << "Window:" << window->size(); + break; + case Warn: + if (widget.size() != window->size()) { + qWarning() << m_platform << "size mismtach tolerated. Widget:" + << widget.size() << "Window:" << window->size(); + } + break; } }; // test state and size consistency of maximized window - verifyResize(QSize(), Qt::WindowMaximized, true, true); + verifyResize(QSize(), Qt::WindowMaximized, Equal, Fail); if (QTest::currentTestFailed()) return; // test state transition, state and size consistency after resize - verifyResize(size1, Qt::WindowNoState, true, !xcb ); + verifyResize(size1, Qt::WindowNoState, Greater, xcb ? Warn : Fail ); if (QTest::currentTestFailed()) return; // test unchanged state, state and size consistency after resize - verifyResize(size2, Qt::WindowNoState, false, !xcb); + verifyResize(size2, Qt::WindowNoState, Equal, xcb ? Warn : Fail); if (QTest::currentTestFailed()) return; // test state transition, state and size consistency after maximize widget.showMaximized(); - verifyResize(QSize(), Qt::WindowMaximized, true, xcb); + verifyResize(QSize(), Qt::WindowMaximized, Greater, xcb ? Fail : Warn); if (QTest::currentTestFailed()) return;