From 7274aed544aad94be7d511656d7f9de08a216002 Mon Sep 17 00:00:00 2001 From: Vlad Zagorodniy Date: Thu, 17 Jan 2019 23:12:11 +0200 Subject: [PATCH 1/5] Fix VirtualDesktopManager::createVirtualDesktop Summary: Currently, there are several issues with VirtualDesktopManager::createVirtualDesktop: (a) The method expects the number parameter to be in range [1, count + 1], but we pass [0, count]; (b) It doesn't correctly update X11 desktop numbers. This change tries to address all previously mentioned issues. BUG: 403312 FIXED-IN: 5.15.0 Test Plan: No longer able to reproduce bug 403312. Reviewers: #kwin, davidedmundson Reviewed By: #kwin, davidedmundson Subscribers: davidedmundson, hein, kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D18328 --- dbusinterface.cpp | 2 +- virtualdesktops.cpp | 12 +++++++----- virtualdesktops.h | 6 ++---- 3 files changed, 10 insertions(+), 10 deletions(-) diff --git a/dbusinterface.cpp b/dbusinterface.cpp index 2a1da8b912..c5688d62da 100644 --- a/dbusinterface.cpp +++ b/dbusinterface.cpp @@ -488,7 +488,7 @@ DBusDesktopDataVector VirtualDesktopManagerDBusInterface::desktops() const void VirtualDesktopManagerDBusInterface::createDesktop(uint position, const QString &name) { - m_manager->createVirtualDesktop(position + 1, name); + m_manager->createVirtualDesktop(position, name); } void VirtualDesktopManagerDBusInterface::setDesktopName(const QString &id, const QString &name) diff --git a/virtualdesktops.cpp b/virtualdesktops.cpp index 8826239412..124052d549 100644 --- a/virtualdesktops.cpp +++ b/virtualdesktops.cpp @@ -445,16 +445,17 @@ VirtualDesktop *VirtualDesktopManager::desktopForId(const QByteArray &id) const return nullptr; } -VirtualDesktop *VirtualDesktopManager::createVirtualDesktop(uint number, const QString &name) +VirtualDesktop *VirtualDesktopManager::createVirtualDesktop(uint position, const QString &name) { //too many, can't insert new ones if ((uint)m_desktops.count() == VirtualDesktopManager::maximum()) { return nullptr; } - const uint actualNumber = qBound(0, number, VirtualDesktopManager::maximum()); + position = qBound(0u, position, static_cast(m_desktops.count())); + auto *vd = new VirtualDesktop(this); - vd->setX11DesktopNumber(actualNumber); + vd->setX11DesktopNumber(position + 1); //TODO: depend on Qt 5.11, use toString(QUuid::WithoutBraces) vd->setId(QUuid::createUuid().toString().toUtf8()); vd->setName(name); @@ -471,15 +472,16 @@ VirtualDesktop *VirtualDesktopManager::createVirtualDesktop(uint number, const Q m_rootInfo->setDesktopName(vd->x11DesktopNumber(), vd->name().toUtf8().data()); } + m_desktops.insert(position, vd); + //update the id of displaced desktops - for (uint i = actualNumber; i < (uint)m_desktops.count(); ++i) { + for (uint i = position + 1; i < (uint)m_desktops.count(); ++i) { m_desktops[i]->setX11DesktopNumber(i + 1); if (m_rootInfo) { m_rootInfo->setDesktopName(i + 1, m_desktops[i]->name().toUtf8().data()); } } - m_desktops.insert(actualNumber - 1, vd); save(); updateRootInfo(); diff --git a/virtualdesktops.h b/virtualdesktops.h index 58914cb944..7af77cd000 100644 --- a/virtualdesktops.h +++ b/virtualdesktops.h @@ -291,13 +291,11 @@ public: /** * Create a new virtual desktop at the requested position. * The difference with setCount is that setCount always adds new desktops at the end of the chain. The Id is automatically generated. - * @param x11DesktopNumber number for the desktop. The desktop created will have an - * x11DesktopNumber guaranteed to be between 1 and numberOfDesktops(). - * Existing desktops will eventually have their x11DesktopNumber increased. + * @param position The position of the desktop. It should be in range [0, count]. * @param name The name for the new desktop, if empty the default name will be used. * @returns the new VirtualDesktop, nullptr if we reached the maximum number of desktops */ - VirtualDesktop *createVirtualDesktop(uint x11DesktopNumber, const QString &name = QString()); + VirtualDesktop *createVirtualDesktop(uint position, const QString &name = QString()); /** * Remove the virtual desktop identified by id, if it exists From 737bb2ec8601daecdf01ef45617792e20f4ba6ae Mon Sep 17 00:00:00 2001 From: Vlad Zagorodniy Date: Fri, 18 Jan 2019 00:08:54 +0200 Subject: [PATCH 2/5] Make sure that new virtual desktops can be activated by using plasma virtual desktop protocol Summary: We need to connect to PlasmaVirtualDesktopInterface::activateRequested when a virtual desktop is created, otherwise one won't be able to activate the desktop by using the pager. Reviewers: #kwin, davidedmundson Reviewed By: #kwin, davidedmundson Subscribers: kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D18342 --- virtualdesktops.cpp | 44 +++++++++++++++++++------------------------- 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/virtualdesktops.cpp b/virtualdesktops.cpp index 124052d549..7a11595933 100644 --- a/virtualdesktops.cpp +++ b/virtualdesktops.cpp @@ -54,19 +54,24 @@ void VirtualDesktopManager::setVirtualDesktopManagement(KWayland::Server::Plasma Q_ASSERT(!m_virtualDesktopManagement); m_virtualDesktopManagement = management; - connect(this, &VirtualDesktopManager::desktopCreated, this, - [this](VirtualDesktop *desktop) { - using namespace KWayland::Server; - PlasmaVirtualDesktopInterface *pvd = m_virtualDesktopManagement->createDesktop(desktop->id(), desktop->x11DesktopNumber() - 1); - pvd->setName(desktop->name()); - pvd->sendDone(); - connect(desktop, &VirtualDesktop::nameChanged, this, - [this, desktop, pvd]() { - pvd->setName(desktop->name()); - } - ); - } - ); + auto createPlasmaVirtualDesktop = [this](VirtualDesktop *desktop) { + PlasmaVirtualDesktopInterface *pvd = m_virtualDesktopManagement->createDesktop(desktop->id(), desktop->x11DesktopNumber() - 1); + pvd->setName(desktop->name()); + pvd->sendDone(); + + connect(desktop, &VirtualDesktop::nameChanged, pvd, + [desktop, pvd] { + pvd->setName(desktop->name()); + } + ); + connect(pvd, &PlasmaVirtualDesktopInterface::activateRequested, this, + [this, desktop] { + setCurrent(desktop); + } + ); + }; + + connect(this, &VirtualDesktopManager::desktopCreated, this, createPlasmaVirtualDesktop); //handle removed: from VirtualDesktopManager to the wayland interface connect(this, &VirtualDesktopManager::desktopRemoved, this, @@ -94,19 +99,8 @@ void VirtualDesktopManager::setVirtualDesktopManagement(KWayland::Server::Plasma } ); - for (quint32 i = 1; i <= count(); ++i) { - VirtualDesktop *internalDesktop = desktopForX11Id(i); - PlasmaVirtualDesktopInterface *desktop = m_virtualDesktopManagement->createDesktop(internalDesktop->id()); + std::for_each(m_desktops.constBegin(), m_desktops.constEnd(), createPlasmaVirtualDesktop); - desktop->setName(internalDesktop->name()); - desktop->sendDone(); - - connect(desktop, &PlasmaVirtualDesktopInterface::activateRequested, this, - [this, desktop] () { - setCurrent(desktopForId(desktop->id().toUtf8())); - } - ); - } //Now we are sure all ids are there save(); From bae21154ea71d1150c137f033eb492c5e77069eb Mon Sep 17 00:00:00 2001 From: Vlad Zagorodniy Date: Fri, 18 Jan 2019 12:19:50 +0200 Subject: [PATCH 3/5] Generate desktop ids without curly braces Summary: We depend on Qt 5.11, thus we can address the TODO item. Reviewers: #kwin, davidedmundson Reviewed By: #kwin, davidedmundson Subscribers: kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D18355 --- virtualdesktops.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/virtualdesktops.cpp b/virtualdesktops.cpp index 7a11595933..d9d42f3cdf 100644 --- a/virtualdesktops.cpp +++ b/virtualdesktops.cpp @@ -38,6 +38,11 @@ namespace KWin { extern int screen_number; static bool s_loadingDesktopSettings = false; +static QByteArray generateDesktopId() +{ + return QUuid::createUuid().toString(QUuid::WithoutBraces).toUtf8(); +} + VirtualDesktop::VirtualDesktop(QObject *parent) : QObject(parent) { @@ -450,8 +455,7 @@ VirtualDesktop *VirtualDesktopManager::createVirtualDesktop(uint position, const auto *vd = new VirtualDesktop(this); vd->setX11DesktopNumber(position + 1); - //TODO: depend on Qt 5.11, use toString(QUuid::WithoutBraces) - vd->setId(QUuid::createUuid().toString().toUtf8()); + vd->setId(generateDesktopId()); vd->setName(name); connect(vd, &VirtualDesktop::nameChanged, this, @@ -585,7 +589,7 @@ void VirtualDesktopManager::setCount(uint count) vd->setX11DesktopNumber(x11Number); vd->setName(defaultName(x11Number)); if (!s_loadingDesktopSettings) { - vd->setId(QUuid::createUuid().toString().toUtf8()); + vd->setId(generateDesktopId()); } m_desktops << vd; newDesktops << vd; @@ -702,14 +706,11 @@ void VirtualDesktopManager::load() } m_desktops[i-1]->setName(s.toUtf8().data()); - QString sId = group.readEntry(QStringLiteral("Id_%1").arg(i), QString()); + const QString sId = group.readEntry(QStringLiteral("Id_%1").arg(i), QString()); //load gets called 2 times, see workspace.cpp line 416 and BUG 385260 if (m_desktops[i-1]->id().isEmpty()) { - if (sId.isEmpty()) { - sId = QUuid::createUuid().toString(); - } - m_desktops[i-1]->setId(sId.toUtf8().data()); + m_desktops[i-1]->setId(sId.isEmpty() ? generateDesktopId() : sId.toUtf8()); } else { Q_ASSERT(sId.isEmpty() || m_desktops[i-1]->id() == sId.toUtf8().data()); } From 47e9e52df3874b7757253698ff1467acb40323b0 Mon Sep 17 00:00:00 2001 From: Vlad Zagorodniy Date: Sat, 19 Jan 2019 00:12:49 +0200 Subject: [PATCH 4/5] Set desktop name "atomically" when using the plasma virtual desktop protocol Reviewers: #kwin, davidedmundson Reviewed By: #kwin, davidedmundson Subscribers: kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D18373 --- virtualdesktops.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/virtualdesktops.cpp b/virtualdesktops.cpp index d9d42f3cdf..ffaa7474fa 100644 --- a/virtualdesktops.cpp +++ b/virtualdesktops.cpp @@ -88,10 +88,7 @@ void VirtualDesktopManager::setVirtualDesktopManagement(KWayland::Server::Plasma //create a new desktop when the client asks to connect (m_virtualDesktopManagement, &PlasmaVirtualDesktopManagementInterface::desktopCreateRequested, this, [this](const QString &name, quint32 position) { - VirtualDesktop *vd = createVirtualDesktop(position); - if (vd) { - vd->setName(name); - } + createVirtualDesktop(position, name); } ); From 7477a411ae6619a4f4672f0e48444f2ceac352a4 Mon Sep 17 00:00:00 2001 From: Vlad Zagorodniy Date: Sat, 19 Jan 2019 00:31:03 +0200 Subject: [PATCH 5/5] Fix "context objects" in VirtualDesktopManager::setVirtualDesktopManagement Summary: Just to make "lifetimes" more cleaner. Reviewers: #kwin, davidedmundson Reviewed By: #kwin, davidedmundson Subscribers: kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D18374 --- virtualdesktops.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/virtualdesktops.cpp b/virtualdesktops.cpp index ffaa7474fa..aba80c2a77 100644 --- a/virtualdesktops.cpp +++ b/virtualdesktops.cpp @@ -76,10 +76,10 @@ void VirtualDesktopManager::setVirtualDesktopManagement(KWayland::Server::Plasma ); }; - connect(this, &VirtualDesktopManager::desktopCreated, this, createPlasmaVirtualDesktop); + connect(this, &VirtualDesktopManager::desktopCreated, m_virtualDesktopManagement, createPlasmaVirtualDesktop); //handle removed: from VirtualDesktopManager to the wayland interface - connect(this, &VirtualDesktopManager::desktopRemoved, this, + connect(this, &VirtualDesktopManager::desktopRemoved, m_virtualDesktopManagement, [this](VirtualDesktop *desktop) { m_virtualDesktopManagement->removeDesktop(desktop->id()); } @@ -106,7 +106,7 @@ void VirtualDesktopManager::setVirtualDesktopManagement(KWayland::Server::Plasma //Now we are sure all ids are there save(); - connect(this, &VirtualDesktopManager::currentChanged, this, + connect(this, &VirtualDesktopManager::currentChanged, m_virtualDesktopManagement, [this]() { for (auto *deskInt : m_virtualDesktopManagement->desktops()) { if (deskInt->id() == currentDesktop()->id()) {