From 2f2bfc4e59cecfdd210663fbf81c000ecddfb822 Mon Sep 17 00:00:00 2001 From: Gatis Paeglis Date: Mon, 25 Jun 2018 10:50:12 +0200 Subject: [PATCH] widgets: fix QWidget::save/restoreGeometry() MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit QWidget::restoreGeometry() is calling QWidget::move() with restoredFrameGeometry, which internally calls setGeometry() and sets positionPolicy = QWindowPrivate::WindowFrameExclusive, which is invalid: restoredFrameGeometry is WindowFrameInclusive geometry. QPA plugins rely on correctly set policies when interpreting x,y. QWidget::move() was not designed for this AFAICT, so making it to accept frame geometry is no-op. It is widely used legacy code, changing it could cause regressions. Save/restore API was introduced in Qt 4.2, at that time we did not have APIs like QWindow::setFramePosition(), so its unclear why geometry() was not stored instead. The documentation also is somewhat unclear: "[..] save the geometry when the window closes [..]" Frame or client geometry? It does not specify. And from the code we see that frame geometry was passed as client geometry, not making the original intention clearer. Besides that, restoreGeometry() is full of other undocumented assumptions where to place windows and when to fail (fortunately its easy to write your own save/restore logic). Added a Qt 6 note in the source code. What this patch changes: Now we store geometry() in saveGeometry() and use that value in restoreGeometry() by setGeometry(). This does not cause any behavior difference in window positioning (tst_QWidget::saveRestoreGeometry still works). Geometry restored from data saved with earlier versions of saveGeometry() might be positioned at: x + leftMargin, y + topMargin. This patch makes tst_QWidget::saveRestoreGeometry to always fail instead of being flaky. Blacklisting for XCB instead of selected distros. Also enabled excluded code paths for XCB on tst_QDockWidget::restoreDockWidget(). It does not seem to be flaky, maybe it was in 2015, but lot of things have changed since then. Task-number: QTBUG-66708 Change-Id: Ic86a6fd091e2c71b7550b2f476386da704253cd4 Reviewed-by: Tor Arne Vestbø --- src/widgets/kernel/qwidget.cpp | 26 +++++++++++-------- tests/auto/widgets/kernel/qwidget/BLACKLIST | 3 +-- .../widgets/kernel/qwidget/tst_qwidget.cpp | 16 +++++------- .../widgets/qdockwidget/tst_qdockwidget.cpp | 7 +---- 4 files changed, 24 insertions(+), 28 deletions(-) diff --git a/src/widgets/kernel/qwidget.cpp b/src/widgets/kernel/qwidget.cpp index 61b255e362..e59bd4098d 100644 --- a/src/widgets/kernel/qwidget.cpp +++ b/src/widgets/kernel/qwidget.cpp @@ -7356,7 +7356,8 @@ QByteArray QWidget::saveGeometry() const // Version history: // - Qt 4.2 - 4.8.6, 5.0 - 5.3 : Version 1.0 // - Qt 4.8.6 - today, 5.4 - today: Version 2.0, save screen width in addition to check for high DPI scaling. - quint16 majorVersion = 2; + // - Qt 5.12 - today : Version 3.0, save QWidget::geometry() + quint16 majorVersion = 3; quint16 minorVersion = 0; const int screenNumber = QDesktopWidgetPrivate::screenNumber(this); stream << magicNumber @@ -7372,7 +7373,8 @@ QByteArray QWidget::saveGeometry() const << qint32(screenNumber) << quint8(windowState() & Qt::WindowMaximized) << quint8(windowState() & Qt::WindowFullScreen) - << qint32(QDesktopWidgetPrivate::screenGeometry(screenNumber).width()); // 1.1 onwards + << qint32(QDesktopWidgetPrivate::screenGeometry(screenNumber).width()) // added in 2.0 + << geometry(); // added in 3.0 return array; } @@ -7412,7 +7414,7 @@ bool QWidget::restoreGeometry(const QByteArray &geometry) if (storedMagicNumber != magicNumber) return false; - const quint16 currentMajorVersion = 2; + const quint16 currentMajorVersion = 3; quint16 majorVersion = 0; quint16 minorVersion = 0; @@ -7423,7 +7425,8 @@ bool QWidget::restoreGeometry(const QByteArray &geometry) // (Allow all minor versions.) QRect restoredFrameGeometry; - QRect restoredNormalGeometry; + QRect restoredGeometry; + QRect restoredNormalGeometry; qint32 restoredScreenNumber; quint8 maximized; quint8 fullScreen; @@ -7437,6 +7440,10 @@ bool QWidget::restoreGeometry(const QByteArray &geometry) if (majorVersion > 1) stream >> restoredScreenWidth; + if (majorVersion > 2) + stream >> restoredGeometry; + + // ### Qt 6 - Perhaps it makes sense to dumb down the restoreGeometry() logic, see QTBUG-69104 if (restoredScreenNumber >= QDesktopWidgetPrivate::numScreens()) restoredScreenNumber = QDesktopWidgetPrivate::primaryScreen(); @@ -7523,14 +7530,11 @@ bool QWidget::restoreGeometry(const QByteArray &geometry) setWindowState(ws); d_func()->topData()->normalGeometry = restoredNormalGeometry; } else { - QPoint offset; -#if 0 // Used to be included in Qt4 for Q_WS_X11 - if (isFullScreen()) - offset = d_func()->topData()->fullScreenOffset; -#endif setWindowState(windowState() & ~(Qt::WindowMaximized | Qt::WindowFullScreen)); - move(restoredFrameGeometry.topLeft() + offset); - resize(restoredNormalGeometry.size()); + if (majorVersion > 2) + setGeometry(restoredGeometry); + else + setGeometry(restoredNormalGeometry); } return true; } diff --git a/tests/auto/widgets/kernel/qwidget/BLACKLIST b/tests/auto/widgets/kernel/qwidget/BLACKLIST index c86dcfb782..86c196f773 100644 --- a/tests/auto/widgets/kernel/qwidget/BLACKLIST +++ b/tests/auto/widgets/kernel/qwidget/BLACKLIST @@ -2,10 +2,9 @@ [normalGeometry] ubuntu-16.04 [saveRestoreGeometry] -ubuntu-16.04 b2qt # QTBUG-66708 -opensuse +xcb [restoreVersion1Geometry] xcb osx diff --git a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp index 5b0b4edb3f..cb0a225195 100644 --- a/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp +++ b/tests/auto/widgets/kernel/qwidget/tst_qwidget.cpp @@ -3356,6 +3356,7 @@ void tst_QWidget::restoreVersion1Geometry() QFETCH(QString, fileName); QFETCH(uint, expectedWindowState); QFETCH(QPoint, expectedPosition); + Q_UNUSED(expectedPosition); QFETCH(QSize, expectedSize); QFETCH(QRect, expectedNormalGeometry); @@ -3375,9 +3376,10 @@ void tst_QWidget::restoreVersion1Geometry() QCOMPARE(uint(widget.windowState() & WindowStateMask), expectedWindowState); if (expectedWindowState == Qt::WindowNoState) { - QCOMPARE(widget.pos(), expectedPosition); + QTRY_COMPARE(widget.geometry(), expectedNormalGeometry); QCOMPARE(widget.size(), expectedSize); } + widget.showNormal(); QVERIFY(QTest::qWaitForWindowExposed(&widget)); QTest::qWait(100); @@ -3386,20 +3388,16 @@ void tst_QWidget::restoreVersion1Geometry() QEXPECT_FAIL("", "WinRT does not support restoreGeometry", Abort); if (expectedWindowState == Qt::WindowNoState) { - QTRY_COMPARE(widget.pos(), expectedPosition); QTRY_COMPARE(widget.size(), expectedSize); + QCOMPARE(widget.geometry(), expectedNormalGeometry); } widget.showNormal(); QTest::qWait(10); - if (expectedWindowState != Qt::WindowNoState) { - // restoring from maximized or fullscreen, we can only restore to the normal geometry - QTRY_COMPARE(widget.geometry(), expectedNormalGeometry); - } else { - QTRY_COMPARE(widget.pos(), expectedPosition); - QTRY_COMPARE(widget.size(), expectedSize); - } + QTRY_COMPARE(widget.geometry(), expectedNormalGeometry); + if (expectedWindowState == Qt::WindowNoState) + QCOMPARE(widget.size(), expectedSize); #if 0 // Code for saving a new geometry*.dat files diff --git a/tests/auto/widgets/widgets/qdockwidget/tst_qdockwidget.cpp b/tests/auto/widgets/widgets/qdockwidget/tst_qdockwidget.cpp index e2b68f792c..078a3215fd 100644 --- a/tests/auto/widgets/widgets/qdockwidget/tst_qdockwidget.cpp +++ b/tests/auto/widgets/widgets/qdockwidget/tst_qdockwidget.cpp @@ -781,8 +781,6 @@ void tst_QDockWidget::restoreDockWidget() QByteArray geometry; QByteArray state; - const bool isXcb = !QGuiApplication::platformName().compare("xcb", Qt::CaseInsensitive); - const QString name = QStringLiteral("main"); const QRect availableGeometry = QGuiApplication::primaryScreen()->availableGeometry(); const QSize size = availableGeometry.size() / 5; @@ -815,8 +813,7 @@ void tst_QDockWidget::restoreDockWidget() dock->show(); QVERIFY(QTest::qWaitForWindowExposed(dock)); QTRY_VERIFY(dock->isFloating()); - if (!isXcb) // Avoid Window manager positioning issues - QTRY_COMPARE(dock->pos(), dockPos); + QTRY_COMPARE(dock->pos(), dockPos); } QVERIFY(!geometry.isEmpty()); @@ -837,8 +834,6 @@ void tst_QDockWidget::restoreDockWidget() restoreWindow.show(); QVERIFY(QTest::qWaitForWindowExposed(&restoreWindow)); QTRY_VERIFY(dock->isFloating()); - if (isXcb) - QSKIP("Skip due to Window manager positioning issues", Abort); QTRY_COMPARE(dock->pos(), dockPos); } }