From 1a0020b18ffb0ef22234a096d4ade6095e1bded8 Mon Sep 17 00:00:00 2001 From: David Edmundson Date: Thu, 4 Feb 2021 13:10:18 +0000 Subject: [PATCH] Move PlasmaWindowInterface lifespan management to the caller PlasmaWindowInterface is a "Server-managed multicasting resources". We no longer need our wrapper to outlive objects so we can tidy that up. It's weird to have a method call to an object delete the object, so memory management is moved to the caller to be consistent. --- .../client/test_plasma_window_model.cpp | 9 +-- .../client/test_wayland_windowmanagement.cpp | 43 +++++--------- .../plasmawindowmanagement_interface.cpp | 57 ++++--------------- .../plasmawindowmanagement_interface.h | 22 +------ 4 files changed, 30 insertions(+), 101 deletions(-) diff --git a/src/wayland/autotests/client/test_plasma_window_model.cpp b/src/wayland/autotests/client/test_plasma_window_model.cpp index c7a9383e25..8e063e8b23 100644 --- a/src/wayland/autotests/client/test_plasma_window_model.cpp +++ b/src/wayland/autotests/client/test_plasma_window_model.cpp @@ -286,7 +286,7 @@ void PlasmaWindowModelTest::testAddRemoveRows() // now let's remove that again QSignalSpy rowRemovedSpy(model, &PlasmaWindowModel::rowsRemoved); QVERIFY(rowRemovedSpy.isValid()); - w->unmap(); + w->deleteLater(); QVERIFY(rowRemovedSpy.wait()); QCOMPARE(rowRemovedSpy.count(), 1); QVERIFY(!rowRemovedSpy.first().at(0).toModelIndex().isValid()); @@ -296,10 +296,6 @@ void PlasmaWindowModelTest::testAddRemoveRows() // now the model is empty again QCOMPARE(model->rowCount(), 0); QVERIFY(!model->index(0).isValid()); - - QSignalSpy wDestroyedSpy(w, &QObject::destroyed); - QVERIFY(wDestroyedSpy.isValid()); - QVERIFY(wDestroyedSpy.wait()); } void PlasmaWindowModelTest::testDefaultData_data() @@ -826,8 +822,7 @@ void PlasmaWindowModelTest::testCreateWithUnmappedWindow() QVERIFY(unmappedSpy.isValid()); QSignalSpy destroyedSpy(window, &PlasmaWindow::destroyed); QVERIFY(destroyedSpy.isValid()); - // unmap should be triggered, but not yet the destroyed - w->unmap(); + w->deleteLater(); QVERIFY(unmappedSpy.wait()); QVERIFY(destroyedSpy.isEmpty()); diff --git a/src/wayland/autotests/client/test_wayland_windowmanagement.cpp b/src/wayland/autotests/client/test_wayland_windowmanagement.cpp index 14d870e820..faf96e1b29 100644 --- a/src/wayland/autotests/client/test_wayland_windowmanagement.cpp +++ b/src/wayland/autotests/client/test_wayland_windowmanagement.cpp @@ -241,15 +241,12 @@ void TestWindowManagement::testUseAfterUnmap() QVERIFY(unmappedSpy.isValid()); QSignalSpy destroyedSpy(m_window, &QObject::destroyed); QVERIFY(destroyedSpy.isValid()); - m_windowInterface->unmap(); + m_windowInterface->deleteLater(); + m_windowInterface = nullptr; m_window->requestClose(); QVERIFY(unmappedSpy.wait()); QVERIFY(destroyedSpy.wait()); m_window = nullptr; - QSignalSpy serverDestroyedSpy(m_windowInterface, &QObject::destroyed); - QVERIFY(serverDestroyedSpy.isValid()); - QVERIFY(serverDestroyedSpy.wait()); - m_windowInterface = nullptr; } void TestWindowManagement::testServerDelete() @@ -283,14 +280,11 @@ void TestWindowManagement::testActiveWindowOnUnmapped() QVERIFY(destroyedSpy.isValid()); QSignalSpy serverDestroyedSpy(m_windowInterface, &QObject::destroyed); QVERIFY(serverDestroyedSpy.isValid()); - m_windowManagementInterface->unmapWindow(m_windowInterface); + delete m_windowInterface; + m_windowInterface = nullptr; QVERIFY(activeWindowChangedSpy.wait()); QVERIFY(!m_windowManagement->activeWindow()); - QVERIFY(destroyedSpy.wait()); - QCOMPARE(destroyedSpy.count(), 1); - m_window = nullptr; - QVERIFY(serverDestroyedSpy.wait()); - m_windowInterface = nullptr; + } void TestWindowManagement::testDeleteActiveWindow() @@ -318,14 +312,16 @@ void TestWindowManagement::testCreateAfterUnmap() { // this test verifies that we don't get a protocol error on client side when creating an already unmapped window. QCOMPARE(m_windowManagement->children().count(), 1); + + QSignalSpy windowAddedSpy(m_windowManagement, &KWayland::Client::PlasmaWindowManagement::windowCreated); + // create and unmap in one go // client will first handle the create, the unmap will be sent once the server side is bound - auto serverWindow = m_windowManagementInterface->createWindow(this, QUuid::createUuid()); - serverWindow->unmap(); + auto serverWindow = m_windowManagementInterface->createWindow(m_windowManagementInterface, QUuid::createUuid()); + delete serverWindow; QCOMPARE(m_windowManagementInterface->children().count(), 0); - QCoreApplication::instance()->processEvents(); - QCoreApplication::instance()->processEvents(QEventLoop::WaitForMoreEvents); - QTRY_COMPARE(m_windowManagement->children().count(), 2); + + windowAddedSpy.wait(); auto window = dynamic_cast(m_windowManagement->children().last()); QVERIFY(window); // now this is not yet on the server, on the server it will be after next roundtrip @@ -334,15 +330,8 @@ void TestWindowManagement::testCreateAfterUnmap() QSignalSpy clientDestroyedSpy(window, &QObject::destroyed); QVERIFY(clientDestroyedSpy.isValid()); QVERIFY(clientDestroyedSpy.wait()); + // Verify that any wrappers created for our temporary window are now gone QCOMPARE(m_windowManagement->children().count(), 1); - // the server side created a helper PlasmaWindowInterface with PlasmaWindowManagementInterface as parent - // it emitted unmapped so we can be sure it will be destroyed directly - QCOMPARE(m_windowManagementInterface->children().count(), 1); - auto helperWindow = qobject_cast(m_windowManagementInterface->children().first()); - QVERIFY(helperWindow); - QSignalSpy helperDestroyedSpy(helperWindow, &QObject::destroyed); - QVERIFY(helperDestroyedSpy.isValid()); - QVERIFY(helperDestroyedSpy.wait()); } void TestWindowManagement::testRequests_data() @@ -515,7 +504,7 @@ void TestWindowManagement::testParentWindow() // now let's create a second window QSignalSpy windowAddedSpy(m_windowManagement, &PlasmaWindowManagement::windowCreated); QVERIFY(windowAddedSpy.isValid()); - auto serverTransient = m_windowManagementInterface->createWindow(this, QUuid::createUuid()); + QScopedPointer serverTransient(m_windowManagementInterface->createWindow(this, QUuid::createUuid())); serverTransient->setParentWindow(m_windowInterface); QVERIFY(windowAddedSpy.wait()); auto transient = windowAddedSpy.first().first().value(); @@ -534,7 +523,7 @@ void TestWindowManagement::testParentWindow() QCOMPARE(transient->parentWindow().data(), parentWindow); // now let's try to unmap the parent - m_windowInterface->unmap(); + m_windowInterface->deleteLater(); m_window = nullptr; m_windowInterface = nullptr; QVERIFY(parentWindowChangedSpy.wait()); @@ -628,8 +617,6 @@ void TestWindowManagement::testPid() QScopedPointer newWindow( windowSpy.first().first().value()); QVERIFY(newWindow); QVERIFY(newWindow->pid() == 0); - - } void TestWindowManagement::testApplicationMenu() diff --git a/src/wayland/plasmawindowmanagement_interface.cpp b/src/wayland/plasmawindowmanagement_interface.cpp index 0e5e6ab514..744b9c6271 100644 --- a/src/wayland/plasmawindowmanagement_interface.cpp +++ b/src/wayland/plasmawindowmanagement_interface.cpp @@ -73,7 +73,6 @@ public: PlasmaWindowManagementInterface *wm; bool unmapped = false; - bool destroyed = false; PlasmaWindowInterface *parentWindow = nullptr; QMetaObject::Connection parentWindowDestroyConnection; QStringList plasmaVirtualDesktops; @@ -92,7 +91,6 @@ public: protected: void org_kde_plasma_window_bind_resource(Resource *resource) override; - void org_kde_plasma_window_destroy_resource(Resource *resource) override; void org_kde_plasma_window_set_state(Resource *resource, uint32_t flags, uint32_t state) override; void org_kde_plasma_window_set_virtual_desktop(Resource *resource, uint32_t number) override; void org_kde_plasma_window_set_minimized_geometry(Resource *resource, wl_resource *panel, uint32_t x, uint32_t y, uint32_t width, uint32_t height) override; @@ -199,10 +197,9 @@ void PlasmaWindowManagementInterfacePrivate::org_kde_plasma_window_management_ge return; } } - // create a temp window just for the resource and directly send an unmapped - PlasmaWindowInterface *window = new PlasmaWindowInterface(q, q); - window->d->unmapped = true; - window->d->add(resource->client(), id, resource->version()); + // create a temp window just for the resource, bind then immediately delete it, sending an unmap event + PlasmaWindowInterface window(q, q); + window.d->add(resource->client(), id, resource->version()); } void PlasmaWindowManagementInterfacePrivate::org_kde_plasma_window_management_get_window_by_uuid(Resource *resource, uint32_t id, const QString &internal_window_uuid) @@ -214,10 +211,9 @@ void PlasmaWindowManagementInterfacePrivate::org_kde_plasma_window_management_ge ); if (it == windows.constEnd()) { qCWarning(KWAYLAND_SERVER) << "Could not find window with uuid" << internal_window_uuid; - // create a temp window just for the resource and directly send an unmapped - PlasmaWindowInterface *window = new PlasmaWindowInterface(q, q); - window->d->unmapped = true; - window->d->add(resource->client(), id, resource->version()); + // create a temp window just for the resource, bind then immediately delete it, sending an unmap event + PlasmaWindowInterface window(q, q); + window.d->add(resource->client(), id, resource->version()); return; } (*it)->d->add(resource->client(), id, resource->version()); @@ -269,16 +265,6 @@ QList PlasmaWindowManagementInterface::windows() const return d->windows; } -void PlasmaWindowManagementInterface::unmapWindow(PlasmaWindowInterface *window) -{ - if (!window) { - return; - } - d->windows.removeOne(window); - Q_ASSERT(!d->windows.contains(window)); - window->d->unmap(); -} - void PlasmaWindowManagementInterface::setStackingOrder(const QVector& stackingOrder) { if (d->stackingOrder == stackingOrder) { @@ -311,14 +297,7 @@ PlasmaWindowInterfacePrivate::PlasmaWindowInterfacePrivate(PlasmaWindowManagemen PlasmaWindowInterfacePrivate::~PlasmaWindowInterfacePrivate() { - destroyed = true; - const auto clientResources = resourceMap(); - for (auto resource : clientResources) { - if(!unmapped) { - send_unmapped(resource->handle); - } - wl_resource_destroy(resource->handle); - } + unmap(); } void PlasmaWindowInterfacePrivate::org_kde_plasma_window_destroy(Resource *resource) @@ -326,15 +305,6 @@ void PlasmaWindowInterfacePrivate::org_kde_plasma_window_destroy(Resource *resou wl_resource_destroy(resource->handle); } -void PlasmaWindowInterfacePrivate::org_kde_plasma_window_destroy_resource(Resource *resource) -{ - Q_UNUSED(resource) - // Auto destruct when all resources gone - if (unmapped && resourceMap().isEmpty() && !destroyed) { - delete q; - } -} - void PlasmaWindowInterfacePrivate::org_kde_plasma_window_bind_resource(Resource *resource) { @@ -366,10 +336,6 @@ void PlasmaWindowInterfacePrivate::org_kde_plasma_window_bind_resource(Resource send_parent_window(resource->handle, resourceForParent(parentWindow, resource)); - if (unmapped) { - send_unmapped(resource->handle); - } - if (geometry.isValid() && resource->version() >= ORG_KDE_PLASMA_WINDOW_GEOMETRY_SINCE_VERSION) { send_geometry(resource->handle, geometry.x(), geometry.y(), geometry.width(), geometry.height()); } @@ -493,16 +459,15 @@ void PlasmaWindowInterfacePrivate::setVirtualDesktop(quint32 desktop) void PlasmaWindowInterfacePrivate::unmap() { + if (unmapped) { + return; + } unmapped = true; const auto clientResources = resourceMap(); for (auto resource : clientResources) { send_unmapped(resource->handle); } - - if (clientResources.isEmpty()) { - delete q; - } } void PlasmaWindowInterfacePrivate::setState(org_kde_plasma_window_management_state flag, bool set) @@ -751,7 +716,7 @@ void PlasmaWindowInterface::setVirtualDesktop(quint32 desktop) void PlasmaWindowInterface::unmap() { - d->wm->unmapWindow(this); + d->unmap(); } QHash PlasmaWindowInterface::minimizedGeometries() const diff --git a/src/wayland/plasmawindowmanagement_interface.h b/src/wayland/plasmawindowmanagement_interface.h index a27f1a7d4a..7f50aa462c 100644 --- a/src/wayland/plasmawindowmanagement_interface.h +++ b/src/wayland/plasmawindowmanagement_interface.h @@ -40,23 +40,6 @@ public: PlasmaWindowInterface *createWindow(QObject *parent, const QUuid &uuid); QList windows() const; - /** - * Unmaps the @p window previously created with {@link createWindow}. - * The window will be unmapped and removed from the list of {@link windows}. - * - * Unmapping a @p window indicates to the client that it should destroy the - * resource created for the window. Once all resources for the @p window are - * destroyed, the @p window will get deleted automatically. There is no need - * to manually delete the @p window. A manual delete will trigger the unmap - * and resource destroy at the same time and can result in protocol errors on - * client side if it still accesses the resource before receiving the unmap event. - * - * @see createWindow - * @see windows - * @since 5.23 - **/ - void unmapWindow(PlasmaWindowInterface *window); - /** * Associate a PlasmaVirtualDesktopManagementInterface to this window management. * It's necessary to associate one in orderto use the plasma virtual desktop features @@ -149,10 +132,9 @@ public: /** * This method removes the Window and the Client is supposed to release the resource - * bound for this Window. Once all resources are released the Window gets deleted. + * bound for this Window. * - * Prefer using {@link PlasmaWindowManagementInterface::unmapWindow}. - * @see PlasmaWindowManagementInterface::unmapWindow + * No more events should be sent afterwards. **/ void unmap();