diff --git a/abstract_client.cpp b/abstract_client.cpp index 3a18b467d8..a4eff0a9d9 100644 --- a/abstract_client.cpp +++ b/abstract_client.cpp @@ -903,16 +903,19 @@ void AbstractClient::move(int x, int y, ForceGeometry_t force) setPendingGeometryUpdate(PendingGeometryNormal); return; } + const QRect oldBufferGeometry = bufferGeometryBeforeUpdateBlocking(); + const QRect oldClientGeometry = clientGeometryBeforeUpdateBlocking(); + const QRect oldFrameGeometry = frameGeometryBeforeUpdateBlocking(); doMove(x, y); + updateGeometryBeforeUpdateBlocking(); updateWindowRules(Rules::Position); screens()->setCurrent(this); workspace()->updateStackingOrder(); // client itself is not damaged - emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking()); - emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); + emit bufferGeometryChanged(this, oldBufferGeometry); + emit clientGeometryChanged(this, oldClientGeometry); + emit frameGeometryChanged(this, oldFrameGeometry); addRepaintDuringGeometryUpdates(); - updateGeometryBeforeUpdateBlocking(); } bool AbstractClient::startMoveResize() diff --git a/autotests/integration/internal_window.cpp b/autotests/integration/internal_window.cpp index fb48db92b2..ba2780d6d0 100644 --- a/autotests/integration/internal_window.cpp +++ b/autotests/integration/internal_window.cpp @@ -75,6 +75,7 @@ private Q_SLOTS: void testChangeWindowType_data(); void testChangeWindowType(); void testEffectWindow(); + void testReentrantSetFrameGeometry(); }; class HelperWindow : public QRasterWindow @@ -791,6 +792,34 @@ void InternalWindowTest::testEffectWindow() QCOMPARE(effects->findWindow(&win)->internalWindow(), &win); } +void InternalWindowTest::testReentrantSetFrameGeometry() +{ + // This test verifies that calling setFrameGeometry() from a slot connected directly + // to the frameGeometryChanged() signal won't cause an infinite recursion. + + // Create an internal window. + QSignalSpy clientAddedSpy(workspace(), &Workspace::internalClientAdded); + QVERIFY(clientAddedSpy.isValid()); + HelperWindow win; + win.setGeometry(0, 0, 100, 100); + win.show(); + QTRY_COMPARE(clientAddedSpy.count(), 1); + auto client = clientAddedSpy.first().first().value(); + QVERIFY(client); + QCOMPARE(client->pos(), QPoint(0, 0)); + + // Let's pretend that there is a script that really wants the client to be at (100, 100). + connect(client, &AbstractClient::frameGeometryChanged, this, [client]() { + client->setFrameGeometry(QRect(QPoint(100, 100), client->size())); + }); + + // Trigger the lambda above. + client->move(QPoint(40, 50)); + + // Eventually, the client will end up at (100, 100). + QCOMPARE(client->pos(), QPoint(100, 100)); +} + } WAYLANDTEST_MAIN(KWin::InternalWindowTest) diff --git a/autotests/integration/x11_client_test.cpp b/autotests/integration/x11_client_test.cpp index 9a966d5e41..31e7dd9f1a 100644 --- a/autotests/integration/x11_client_test.cpp +++ b/autotests/integration/x11_client_test.cpp @@ -61,6 +61,7 @@ private Q_SLOTS: void testCaptionMultipleWindows(); void testFullscreenWindowGroups(); void testActivateFocusedWindow(); + void testReentrantSetFrameGeometry(); }; void X11ClientTest::initTestCase() @@ -1069,5 +1070,54 @@ void X11ClientTest::testActivateFocusedWindow() QVERIFY(Test::waitForWindowDestroyed(client1)); } +void X11ClientTest::testReentrantSetFrameGeometry() +{ + // This test verifies that calling setFrameGeometry() from a slot connected directly + // to the frameGeometryChanged() signal won't cause an infinite recursion. + + // Create a test window. + QScopedPointer c(xcb_connect(nullptr, nullptr)); + QVERIFY(!xcb_connection_has_error(c.data())); + const QRect windowGeometry(0, 0, 100, 200); + xcb_window_t w = xcb_generate_id(c.data()); + xcb_create_window(c.data(), XCB_COPY_FROM_PARENT, w, rootWindow(), + windowGeometry.x(), + windowGeometry.y(), + windowGeometry.width(), + windowGeometry.height(), + 0, XCB_WINDOW_CLASS_INPUT_OUTPUT, XCB_COPY_FROM_PARENT, 0, nullptr); + xcb_size_hints_t hints; + memset(&hints, 0, sizeof(hints)); + xcb_icccm_size_hints_set_position(&hints, 1, windowGeometry.x(), windowGeometry.y()); + xcb_icccm_size_hints_set_size(&hints, 1, windowGeometry.width(), windowGeometry.height()); + xcb_icccm_set_wm_normal_hints(c.data(), w, &hints); + xcb_change_property(c.data(), XCB_PROP_MODE_REPLACE, w, atoms->wm_client_leader, XCB_ATOM_WINDOW, 32, 1, &w); + xcb_map_window(c.data(), w); + xcb_flush(c.data()); + + QSignalSpy windowCreatedSpy(workspace(), &Workspace::clientAdded); + QVERIFY(windowCreatedSpy.isValid()); + QVERIFY(windowCreatedSpy.wait()); + X11Client *client = windowCreatedSpy.first().first().value(); + QVERIFY(client); + QCOMPARE(client->pos(), QPoint(0, 0)); + + // Let's pretend that there is a script that really wants the client to be at (100, 100). + connect(client, &AbstractClient::frameGeometryChanged, this, [client]() { + client->setFrameGeometry(QRect(QPoint(100, 100), client->size())); + }); + + // Trigger the lambda above. + client->move(QPoint(40, 50)); + + // Eventually, the client will end up at (100, 100). + QCOMPARE(client->pos(), QPoint(100, 100)); + + // Destroy the test window. + xcb_destroy_window(c.data(), w); + xcb_flush(c.data()); + QVERIFY(Test::waitForWindowDestroyed(client)); +} + WAYLANDTEST_MAIN(X11ClientTest) #include "x11_client_test.moc" diff --git a/autotests/integration/xdgshellclient_test.cpp b/autotests/integration/xdgshellclient_test.cpp index e3ca678fc4..ed74211572 100644 --- a/autotests/integration/xdgshellclient_test.cpp +++ b/autotests/integration/xdgshellclient_test.cpp @@ -121,6 +121,7 @@ private Q_SLOTS: void testXdgWindowGeometryFullScreen(); void testXdgWindowGeometryMaximize(); void testPointerInputTransform(); + void testReentrantSetFrameGeometry(); }; void TestXdgShellClient::initTestCase() @@ -1645,5 +1646,33 @@ void TestXdgShellClient::testPointerInputTransform() QVERIFY(Test::waitForWindowDestroyed(client)); } +void TestXdgShellClient::testReentrantSetFrameGeometry() +{ + // This test verifies that calling setFrameGeometry() from a slot connected directly + // to the frameGeometryChanged() signal won't cause an infinite recursion. + + // Create an xdg-toplevel surface and wait for the compositor to catch up. + QScopedPointer surface(Test::createSurface()); + QScopedPointer shellSurface(Test::createXdgShellStableSurface(surface.data())); + AbstractClient *client = Test::renderAndWaitForShown(surface.data(), QSize(200, 100), Qt::red); + QVERIFY(client); + QCOMPARE(client->pos(), QPoint(0, 0)); + + // Let's pretend that there is a script that really wants the client to be at (100, 100). + connect(client, &AbstractClient::frameGeometryChanged, this, [client]() { + client->setFrameGeometry(QRect(QPoint(100, 100), client->size())); + }); + + // Trigger the lambda above. + client->move(QPoint(40, 50)); + + // Eventually, the client will end up at (100, 100). + QCOMPARE(client->pos(), QPoint(100, 100)); + + // Destroy the xdg-toplevel surface. + shellSurface.reset(); + QVERIFY(Test::waitForWindowDestroyed(client)); +} + WAYLANDTEST_MAIN(TestXdgShellClient) #include "xdgshellclient_test.moc" diff --git a/internal_client.cpp b/internal_client.cpp index 91c0e0adcc..958ce3b47e 100644 --- a/internal_client.cpp +++ b/internal_client.cpp @@ -100,7 +100,7 @@ bool InternalClient::eventFilter(QObject *watched, QEvent *event) QRect InternalClient::bufferGeometry() const { - return frameGeometry() - frameMargins(); + return m_clientGeometry; } QStringList InternalClient::activities() const @@ -525,23 +525,25 @@ void InternalClient::commitGeometry(const QRect &rect) return; } + // The client geometry and the buffer geometry are the same. + const QRect oldClientGeometry = m_clientGeometry; + const QRect oldFrameGeometry = m_frameGeometry; + m_clientGeometry = frameRectToClientRect(rect); m_frameGeometry = rect; addWorkspaceRepaint(visibleRect()); + updateGeometryBeforeUpdateBlocking(); syncGeometryToInternalWindow(); - if (bufferGeometryBeforeUpdateBlocking() != bufferGeometry()) { - emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking()); + if (oldClientGeometry != m_clientGeometry) { + emit bufferGeometryChanged(this, oldClientGeometry); + emit clientGeometryChanged(this, oldClientGeometry); } - if (clientGeometryBeforeUpdateBlocking() != clientGeometry()) { - emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); + if (oldFrameGeometry != m_frameGeometry) { + emit frameGeometryChanged(this, oldFrameGeometry); } - if (frameGeometryBeforeUpdateBlocking() != frameGeometry()) { - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); - } - emit geometryShapeChanged(this, frameGeometryBeforeUpdateBlocking()); - updateGeometryBeforeUpdateBlocking(); + emit geometryShapeChanged(this, oldFrameGeometry); if (isResize()) { performMoveResize(); diff --git a/x11client.cpp b/x11client.cpp index bbc76f1398..55ec57d3ed 100644 --- a/x11client.cpp +++ b/x11client.cpp @@ -2892,22 +2892,25 @@ void X11Client::move(int x, int y, ForceGeometry_t force) } return; } + const QRect oldBufferGeometry = bufferGeometryBeforeUpdateBlocking(); + const QRect oldFrameGeometry = frameGeometryBeforeUpdateBlocking(); + const QRect oldClientGeometry = clientGeometryBeforeUpdateBlocking(); updateServerGeometry(); updateWindowRules(Rules::Position); + updateGeometryBeforeUpdateBlocking(); screens()->setCurrent(this); workspace()->updateStackingOrder(); // client itself is not damaged - if (bufferGeometryBeforeUpdateBlocking() != bufferGeometry()) { - emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking()); + if (oldBufferGeometry != bufferGeometry()) { + emit bufferGeometryChanged(this, oldBufferGeometry); } - if (clientGeometryBeforeUpdateBlocking() != clientGeometry()) { - emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); + if (oldClientGeometry != clientGeometry()) { + emit clientGeometryChanged(this, oldClientGeometry); } - if (frameGeometryBeforeUpdateBlocking() != frameGeometry()) { - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); + if (oldFrameGeometry != frameGeometry()) { + emit frameGeometryChanged(this, oldFrameGeometry); } addRepaintDuringGeometryUpdates(); - updateGeometryBeforeUpdateBlocking(); } bool X11Client::belongToSameApplication(const X11Client *c1, const X11Client *c2, SameApplicationChecks checks) @@ -4183,8 +4186,14 @@ void X11Client::setFrameGeometry(const QRect &rect, ForceGeometry_t force) setPendingGeometryUpdate(PendingGeometryNormal); return; } + + const QRect oldBufferGeometry = bufferGeometryBeforeUpdateBlocking(); + const QRect oldFrameGeometry = frameGeometryBeforeUpdateBlocking(); + const QRect oldClientGeometry = clientGeometryBeforeUpdateBlocking(); + updateServerGeometry(); updateWindowRules(Rules::Position|Rules::Size); + updateGeometryBeforeUpdateBlocking(); // keep track of old maximize mode // to detect changes @@ -4192,21 +4201,20 @@ void X11Client::setFrameGeometry(const QRect &rect, ForceGeometry_t force) workspace()->updateStackingOrder(); // Need to regenerate decoration pixmaps when the buffer size is changed. - if (bufferGeometryBeforeUpdateBlocking().size() != m_bufferGeometry.size()) { + if (oldBufferGeometry.size() != m_bufferGeometry.size()) { discardWindowPixmap(); } - if (bufferGeometryBeforeUpdateBlocking() != m_bufferGeometry) { - emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking()); + if (oldBufferGeometry != m_bufferGeometry) { + emit bufferGeometryChanged(this, oldBufferGeometry); } - if (clientGeometryBeforeUpdateBlocking() != m_clientGeometry) { - emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); + if (oldClientGeometry != m_clientGeometry) { + emit clientGeometryChanged(this, oldClientGeometry); } - if (frameGeometryBeforeUpdateBlocking() != m_frameGeometry) { - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); + if (oldFrameGeometry != m_frameGeometry) { + emit frameGeometryChanged(this, oldFrameGeometry); } - emit geometryShapeChanged(this, frameGeometryBeforeUpdateBlocking()); + emit geometryShapeChanged(this, oldFrameGeometry); addRepaintDuringGeometryUpdates(); - updateGeometryBeforeUpdateBlocking(); } void X11Client::plainResize(int w, int h, ForceGeometry_t force) @@ -4251,25 +4259,28 @@ void X11Client::plainResize(int w, int h, ForceGeometry_t force) setPendingGeometryUpdate(PendingGeometryNormal); return; } + const QRect oldBufferGeometry = bufferGeometryBeforeUpdateBlocking(); + const QRect oldFrameGeometry = frameGeometryBeforeUpdateBlocking(); + const QRect oldClientGeometry = clientGeometryBeforeUpdateBlocking(); updateServerGeometry(); updateWindowRules(Rules::Position|Rules::Size); + updateGeometryBeforeUpdateBlocking(); screens()->setCurrent(this); workspace()->updateStackingOrder(); - if (bufferGeometryBeforeUpdateBlocking().size() != m_bufferGeometry.size()) { + if (oldBufferGeometry.size() != m_bufferGeometry.size()) { discardWindowPixmap(); } - if (bufferGeometryBeforeUpdateBlocking() != bufferGeometry()) { - emit bufferGeometryChanged(this, bufferGeometryBeforeUpdateBlocking()); + if (oldBufferGeometry != m_bufferGeometry) { + emit bufferGeometryChanged(this, oldBufferGeometry); } - if (clientGeometryBeforeUpdateBlocking() != clientGeometry()) { - emit clientGeometryChanged(this, clientGeometryBeforeUpdateBlocking()); + if (oldClientGeometry != m_clientGeometry) { + emit clientGeometryChanged(this, oldClientGeometry); } - if (frameGeometryBeforeUpdateBlocking() != frameGeometry()) { - emit frameGeometryChanged(this, frameGeometryBeforeUpdateBlocking()); + if (oldFrameGeometry != m_frameGeometry) { + emit frameGeometryChanged(this, oldFrameGeometry); } - emit geometryShapeChanged(this, frameGeometryBeforeUpdateBlocking()); + emit geometryShapeChanged(this, oldFrameGeometry); addRepaintDuringGeometryUpdates(); - updateGeometryBeforeUpdateBlocking(); } void X11Client::updateServerGeometry()