From 395cc4f9454c15b188f31c8e76e65dcc6ebc2d45 Mon Sep 17 00:00:00 2001 From: David Edmundson Date: Mon, 18 May 2020 14:43:38 +0100 Subject: [PATCH] Manage active selection as active DataSource than DataDevice Summary: A DataDevice will have zero or one active DataSource as the seclection. In the existing code we track the current data device then update it to the newest data device when the source inside a data device changes. If we store the active data source inside Seat instead of the device everything becomes somewhat simpler and safer. An entire unit test vanishes as that case of an externally set DataDevice with no source can no longer happen. There's also a lot of duplication that's been merged in this patch so we have one path. There are some technical behavioural changes in particular we do cleanup when the source vanishes rather than the data device, but if anything that seems safer and more correct. It's a precursor for introducing an abstraction class round the source without needing to meddle with too much code. Test Plan: Relevant unit tests passed, ran with it for a while with no issue. Reviewers: #kwin Differential Revision: https://phabricator.kde.org/D29328 --- .../autotests/client/test_datadevice.cpp | 10 +- .../autotests/client/test_wayland_seat.cpp | 52 +---------- src/wayland/datadevice_interface.cpp | 9 +- src/wayland/datadevice_interface.h | 2 +- src/wayland/seat_interface.cpp | 91 +++++++------------ src/wayland/seat_interface.h | 9 +- src/wayland/seat_interface_p.h | 8 +- 7 files changed, 54 insertions(+), 127 deletions(-) diff --git a/src/wayland/autotests/client/test_datadevice.cpp b/src/wayland/autotests/client/test_datadevice.cpp index b90d9b6efe..1d1ecd4c08 100644 --- a/src/wayland/autotests/client/test_datadevice.cpp +++ b/src/wayland/autotests/client/test_datadevice.cpp @@ -184,9 +184,11 @@ void TestDataDevice::testCreate() QVERIFY(!deviceInterface->selection()); QVERIFY(deviceInterface->parentResource()); + + // this will probably fail, we need to make a selection client side QVERIFY(!m_seatInterface->selection()); - m_seatInterface->setSelection(deviceInterface); - QCOMPARE(m_seatInterface->selection(), deviceInterface); + m_seatInterface->setSelection(deviceInterface->selection()); + QCOMPARE(m_seatInterface->selection(), deviceInterface->selection()); // and destroy QSignalSpy destroyedSpy(deviceInterface, &QObject::destroyed); @@ -404,7 +406,7 @@ void TestDataDevice::testSetSelection() // send selection to datadevice QSignalSpy selectionOfferedSpy(dataDevice.data(), SIGNAL(selectionOffered(KWayland::Client::DataOffer*))); QVERIFY(selectionOfferedSpy.isValid()); - deviceInterface->sendSelection(deviceInterface); + deviceInterface->sendSelection(deviceInterface->selection()); QVERIFY(selectionOfferedSpy.wait()); QCOMPARE(selectionOfferedSpy.count(), 1); auto dataOffer = selectionOfferedSpy.first().first().value(); @@ -439,7 +441,7 @@ void TestDataDevice::testSetSelection() dataDevice.reset(); QVERIFY(unboundSpy.wait()); // send a selection to the unbound data device - deviceInterface->sendSelection(deviceInterface); + deviceInterface->sendSelection(deviceInterface->selection()); } void TestDataDevice::testSendSelectionOnSeat() diff --git a/src/wayland/autotests/client/test_wayland_seat.cpp b/src/wayland/autotests/client/test_wayland_seat.cpp index 3db6ee069d..a9d41be04b 100644 --- a/src/wayland/autotests/client/test_wayland_seat.cpp +++ b/src/wayland/autotests/client/test_wayland_seat.cpp @@ -26,6 +26,7 @@ #include "../../src/server/buffer_interface.h" #include "../../src/server/compositor_interface.h" #include "../../src/server/datadevicemanager_interface.h" +#include "../../src/server/datasource_interface.h" #include "../../src/server/display.h" #include "../../src/server/keyboard_interface.h" #include "../../src/server/pointer_interface.h" @@ -72,7 +73,6 @@ private Q_SLOTS: void testCast(); void testDestroy(); void testSelection(); - void testSelectionNoDataSource(); void testDataDeviceForKeyboardSurface(); void testTouch(); void testDisconnect(); @@ -1921,54 +1921,6 @@ void TestWaylandSeat::testSelection() QVERIFY(cancelledSpy.isValid()); m_seatInterface->setSelection(ddi); QVERIFY(cancelledSpy.wait()); - - // Copy already cleared selection, BUG 383054 - ddi->sendSelection(ddi); -} - -void TestWaylandSeat::testSelectionNoDataSource() -{ - // this test verifies that the server doesn't crash when using setSelection with - // a DataDevice which doesn't have a DataSource yet - using namespace KWayland::Client; - using namespace KWaylandServer; - // first create the DataDevice - QScopedPointer ddmi(m_display->createDataDeviceManager()); - ddmi->create(); - QSignalSpy ddiCreatedSpy(ddmi.data(), &DataDeviceManagerInterface::dataDeviceCreated); - QVERIFY(ddiCreatedSpy.isValid()); - Registry registry; - QSignalSpy dataDeviceManagerSpy(®istry, &Registry::dataDeviceManagerAnnounced); - QVERIFY(dataDeviceManagerSpy.isValid()); - registry.setEventQueue(m_queue); - registry.create(m_connection->display()); - QVERIFY(registry.isValid()); - registry.setup(); - - QVERIFY(dataDeviceManagerSpy.wait()); - QScopedPointer ddm(registry.createDataDeviceManager(dataDeviceManagerSpy.first().first().value(), - dataDeviceManagerSpy.first().last().value())); - QVERIFY(ddm->isValid()); - - QScopedPointer dd(ddm->getDataDevice(m_seat)); - QVERIFY(dd->isValid()); - QVERIFY(ddiCreatedSpy.wait()); - auto ddi = ddiCreatedSpy.first().first().value(); - QVERIFY(ddi); - - // now create a surface and pass it keyboard focus - QSignalSpy surfaceCreatedSpy(m_compositorInterface, &CompositorInterface::surfaceCreated); - QVERIFY(surfaceCreatedSpy.isValid()); - QScopedPointer surface(m_compositor->createSurface()); - QVERIFY(surface->isValid()); - QVERIFY(surfaceCreatedSpy.wait()); - auto serverSurface = surfaceCreatedSpy.first().first().value(); - QVERIFY(!m_seatInterface->selection()); - m_seatInterface->setFocusedKeyboardSurface(serverSurface); - QCOMPARE(m_seatInterface->focusedKeyboardSurface(), serverSurface); - - // now let's set the selection - m_seatInterface->setSelection(ddi); } void TestWaylandSeat::testDataDeviceForKeyboardSurface() @@ -2021,7 +1973,7 @@ void TestWaylandSeat::testDataDeviceForKeyboardSurface() QVERIFY(ddiCreatedSpy.wait()); auto ddi = ddiCreatedSpy.first().first().value(); QVERIFY(ddi); - m_seatInterface->setSelection(ddi); + m_seatInterface->setSelection(ddi->selection()); // switch to other client // create a surface and pass it keyboard focus diff --git a/src/wayland/datadevice_interface.cpp b/src/wayland/datadevice_interface.cpp index 97da3d8a9e..afbabd5d54 100644 --- a/src/wayland/datadevice_interface.cpp +++ b/src/wayland/datadevice_interface.cpp @@ -209,15 +209,10 @@ DataSourceInterface *DataDeviceInterface::selection() const return d->selection; } -void DataDeviceInterface::sendSelection(DataDeviceInterface *other) +void DataDeviceInterface::sendSelection(DataSourceInterface *other) { Q_D(); - auto otherSelection = other->selection(); - if (!otherSelection) { - sendClearSelection(); - return; - } - auto r = d->createDataOffer(otherSelection); + auto r = d->createDataOffer(other); if (!r) { return; } diff --git a/src/wayland/datadevice_interface.h b/src/wayland/datadevice_interface.h index 5a84ea2a45..d7e8f30943 100644 --- a/src/wayland/datadevice_interface.h +++ b/src/wayland/datadevice_interface.h @@ -51,7 +51,7 @@ public: DataSourceInterface *selection() const; - void sendSelection(DataDeviceInterface *other); + void sendSelection(DataSourceInterface *other); void sendClearSelection(); /** * The event is sent when a drag-and-drop operation is ended because the implicit grab is removed. diff --git a/src/wayland/seat_interface.cpp b/src/wayland/seat_interface.cpp index ea4a02e447..b438e8b004 100644 --- a/src/wayland/seat_interface.cpp +++ b/src/wayland/seat_interface.cpp @@ -253,26 +253,17 @@ void SeatInterface::Private::registerDataDevice(DataDeviceInterface *dataDevice) auto dataDeviceCleanup = [this, dataDevice] { dataDevices.removeOne(dataDevice); keys.focus.selections.removeOne(dataDevice); - - if (currentSelection == dataDevice) { - // current selection is cleared - currentSelection = nullptr; - emit q->selectionChanged(nullptr); - for (auto selection: keys.focus.selections) { - selection->sendClearSelection(); - } - } }; QObject::connect(dataDevice, &QObject::destroyed, q, dataDeviceCleanup); QObject::connect(dataDevice, &Resource::unbound, q, dataDeviceCleanup); QObject::connect(dataDevice, &DataDeviceInterface::selectionChanged, q, [this, dataDevice] { - updateSelection(dataDevice, true); + updateSelection(dataDevice); } ); QObject::connect(dataDevice, &DataDeviceInterface::selectionCleared, q, [this, dataDevice] { - updateSelection(dataDevice, false); + updateSelection(dataDevice); } ); QObject::connect(dataDevice, &DataDeviceInterface::dragStarted, q, @@ -331,14 +322,13 @@ void SeatInterface::Private::registerDataDevice(DataDeviceInterface *dataDevice) // same client? if (keys.focus.surface->client() == dataDevice->client()) { keys.focus.selections.append(dataDevice); - if (currentSelection && currentSelection->selection()) { + if (currentSelection) { dataDevice->sendSelection(currentSelection); } } } } - void SeatInterface::Private::registerTextInput(TextInputInterface *ti) { // text input version 0 might call this multiple times @@ -382,44 +372,13 @@ void SeatInterface::Private::endDrag(quint32 serial) emit q->dragEnded(); } -void SeatInterface::Private::cancelPreviousSelection(DataDeviceInterface *dataDevice) +void SeatInterface::Private::updateSelection(DataDeviceInterface *dataDevice) { - if (!currentSelection) { + // if the update is from the focussed window we should inform the active client + if (!(keys.focus.surface && (keys.focus.surface->client() == dataDevice->client()))) { return; } - if (auto s = currentSelection->selection()) { - if (currentSelection != dataDevice) { - // only if current selection is not on the same device - // that would cancel the newly set source - s->cancel(); - } - } -} - -void SeatInterface::Private::updateSelection(DataDeviceInterface *dataDevice, bool set) -{ - bool selChanged = currentSelection != dataDevice; - if (keys.focus.surface && (keys.focus.surface->client() == dataDevice->client())) { - // cancel the previous selection - cancelPreviousSelection(dataDevice); - // new selection on a data device belonging to current keyboard focus - currentSelection = dataDevice; - } - if (dataDevice == currentSelection) { - // need to send out the selection - for (auto focussedDevice: qAsConst(keys.focus.selections)) { - if (set) { - focussedDevice->sendSelection(dataDevice); - } else { - focussedDevice->sendClearSelection(); - currentSelection = nullptr; - selChanged = true; - } - } - } - if (selChanged) { - emit q->selectionChanged(currentSelection); - } + q->setSelection(dataDevice->selection()); } void SeatInterface::setHasKeyboard(bool has) @@ -1595,29 +1554,43 @@ TextInputInterface *SeatInterface::focusedTextInput() const return d->textInput.focus.textInput; } -DataDeviceInterface *SeatInterface::selection() const +DataSourceInterface *SeatInterface::selection() const { Q_D(); return d->currentSelection; } -void SeatInterface::setSelection(DataDeviceInterface *dataDevice) +void SeatInterface::setSelection(DataSourceInterface *selection) { Q_D(); - if (d->currentSelection == dataDevice) { + if (d->currentSelection == selection) { return; } - // cancel the previous selection - d->cancelPreviousSelection(dataDevice); - d->currentSelection = dataDevice; - for (auto focussedDevice: qAsConst(d->keys.focus.selections)) { - if (dataDevice && dataDevice->selection()) { - focussedDevice->sendSelection(dataDevice); + + if (d->currentSelection) { + d->currentSelection->cancel(); + disconnect(d->currentSelection, nullptr, this, nullptr); + } + + if (selection) { + auto cleanup = [this]() { + setSelection(nullptr); + }; + connect(selection, &DataSourceInterface::unbound, this, cleanup); + connect(selection, &QObject::destroyed, this, cleanup); + } + + d->currentSelection = selection; + + for (auto focussedSelection: qAsConst(d->keys.focus.selections)) { + if (selection) { + focussedSelection->sendSelection(selection); } else { - focussedDevice->sendClearSelection(); + focussedSelection->sendClearSelection(); } } - emit selectionChanged(dataDevice); + + emit selectionChanged(selection); } } diff --git a/src/wayland/seat_interface.h b/src/wayland/seat_interface.h index 3da92d56e1..0f7609b311 100644 --- a/src/wayland/seat_interface.h +++ b/src/wayland/seat_interface.h @@ -23,6 +23,7 @@ namespace KWaylandServer { class DataDeviceInterface; +class DataSourceInterface; class Display; class SurfaceInterface; class TextInputInterface; @@ -692,8 +693,10 @@ public: * @since 5.24 * @see selectionChanged * @see setSelection + * This may be null **/ - DataDeviceInterface *selection() const; + DataSourceInterface *selection() const; + /** * This method allows to manually set the @p dataDevice for the current clipboard selection. * The clipboard selection is handled automatically in SeatInterface. @@ -707,7 +710,7 @@ public: * @see selectionChanged * @since 5.24 **/ - void setSelection(DataDeviceInterface *dataDevice); + void setSelection(DataSourceInterface *selection); static SeatInterface *get(wl_resource *native); @@ -736,7 +739,7 @@ Q_SIGNALS: * @see selection * @see setSelection **/ - void selectionChanged(DataDeviceInterface*); + void selectionChanged(DataSourceInterface*); /** * Emitted when a drag'n'drop operation is started diff --git a/src/wayland/seat_interface_p.h b/src/wayland/seat_interface_p.h index 387f334231..fd303e2a97 100644 --- a/src/wayland/seat_interface_p.h +++ b/src/wayland/seat_interface_p.h @@ -37,7 +37,6 @@ public: void registerDataDevice(DataDeviceInterface *dataDevice); void registerTextInput(TextInputInterface *textInput); void endDrag(quint32 serial); - void cancelPreviousSelection(DataDeviceInterface *newlySelectedDataDevice); QString name; bool pointer = false; @@ -49,8 +48,11 @@ public: QVector keyboards; QVector touchs; QVector dataDevices; + QVector textInputs; - DataDeviceInterface *currentSelection = nullptr; + + // the last thing copied into the clipboard content + DataSourceInterface *currentSelection = nullptr; // Pointer related members struct Pointer { @@ -165,7 +167,7 @@ private: void getPointer(wl_client *client, wl_resource *resource, uint32_t id); void getKeyboard(wl_client *client, wl_resource *resource, uint32_t id); void getTouch(wl_client *client, wl_resource *resource, uint32_t id); - void updateSelection(DataDeviceInterface *dataDevice, bool set); + void updateSelection(DataDeviceInterface *dataDevice); static Private *cast(wl_resource *r); static void unbind(wl_resource *r);