From 1ae7990a959ce2c3fad0a6aef684cf058b07cf1e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Martin=20Fl=C3=B6ser?= Date: Sun, 5 Nov 2017 10:10:17 +0100 Subject: [PATCH] Allow a cross-process check for same applications Summary: Commit 5d9027b110 introduced a regression in TabBox by using the generic framework inside KWin to test for same application. What I did not consider was that the code in TabBox was "broken by design". It didn't use the generic check as that is too strict and considers windows from different processes as not belonging to the same application. But this is not wanted in the case of TabBox. On the other hand the change itself is an improvement to also support Wayland in a better way and not have special handling situations. Thus just reverting would not help. Instead this change addresses the problem by extending the internal API and to allow more adjustements. So far there was already an "active_hack" boolean flag. This is extended to proper flags with an additional flag to allow cross application checks. The checks in Client which would filter out different applications check for this flag and are skipped if set. In addition ShellClient also adds support for this flag and compares for the desktop file name. Thus we get in TabBox the same behavior as before with the advantage of having a better shared code base working on both X11 and Wayland. BUG: 386043 FIXED-IN: 5.11.4 Test Plan: Started two kwrite processes on X11, clicked new in one of them, used Alt+` and verified that there are three windows shown. Reviewers: #kwin, #plasma Subscribers: plasma-devel, kwin Tags: #kwin Differential Revision: https://phabricator.kde.org/D8661 --- abstract_client.cpp | 4 ++-- abstract_client.h | 12 ++++++++---- activation.cpp | 8 ++++---- client.cpp | 4 ++-- client.h | 4 ++-- group.cpp | 10 ++++++---- shell_client.cpp | 10 +++++++--- shell_client.h | 2 +- tabbox/tabbox.cpp | 4 ++-- 9 files changed, 34 insertions(+), 24 deletions(-) diff --git a/abstract_client.cpp b/abstract_client.cpp index c26e2dfdc7..264d685397 100644 --- a/abstract_client.cpp +++ b/abstract_client.cpp @@ -104,9 +104,9 @@ void AbstractClient::updateMouseGrab() { } -bool AbstractClient::belongToSameApplication(const AbstractClient *c1, const AbstractClient *c2, bool active_hack) +bool AbstractClient::belongToSameApplication(const AbstractClient *c1, const AbstractClient *c2, SameApplicationChecks checks) { - return c1->belongsToSameApplication(c2, active_hack); + return c1->belongsToSameApplication(c2, checks); } bool AbstractClient::isTransient() const diff --git a/abstract_client.h b/abstract_client.h index 0f486c6f33..e153db82db 100644 --- a/abstract_client.h +++ b/abstract_client.h @@ -647,8 +647,12 @@ public: **/ virtual void killWindow() = 0; - // TODO: remove boolean trap - static bool belongToSameApplication(const AbstractClient* c1, const AbstractClient* c2, bool active_hack = false); + enum class SameApplicationCheck { + RelaxedForActive = 1 << 0, + AllowCrossProcesses = 1 << 1 + }; + Q_DECLARE_FLAGS(SameApplicationChecks, SameApplicationCheck) + static bool belongToSameApplication(const AbstractClient* c1, const AbstractClient* c2, SameApplicationChecks checks = SameApplicationChecks()); bool hasApplicationMenu() const; bool applicationMenuActive() const { @@ -765,8 +769,7 @@ protected: * Default implementation does nothig. **/ virtual void doMinimize(); - // TODO: remove boolean trap - virtual bool belongsToSameApplication(const AbstractClient *other, bool active_hack) const = 0; + virtual bool belongsToSameApplication(const AbstractClient *other, SameApplicationChecks checks) const = 0; virtual void doSetSkipTaskbar(); virtual void doSetSkipPager(); @@ -1155,5 +1158,6 @@ inline void AbstractClient::setPendingGeometryUpdate(PendingGeometry_t update) Q_DECLARE_METATYPE(KWin::AbstractClient*) Q_DECLARE_METATYPE(QList) +Q_DECLARE_OPERATORS_FOR_FLAGS(KWin::AbstractClient::SameApplicationChecks) #endif diff --git a/activation.cpp b/activation.cpp index 9ded1a0569..c048390cbe 100644 --- a/activation.cpp +++ b/activation.cpp @@ -609,7 +609,7 @@ bool Workspace::allowClientActivation(const KWin::AbstractClient *c, xcb_timesta // Unconditionally allow intra-client passing around for lower stealing protections // unless the active client has High interest - if (AbstractClient::belongToSameApplication(c, ac, true) && protection < FSP::High) { + if (AbstractClient::belongToSameApplication(c, ac, AbstractClient::SameApplicationCheck::RelaxedForActive) && protection < FSP::High) { qCDebug(KWIN_CORE) << "Activation: Belongs to active application"; return true; } @@ -659,7 +659,7 @@ bool Workspace::allowFullClientRaising(const KWin::AbstractClient *c, xcb_timest return true; // no active client -> always allow } // TODO window urgency -> return true? - if (AbstractClient::belongToSameApplication(c, ac, true)) { + if (AbstractClient::belongToSameApplication(c, ac, AbstractClient::SameApplicationCheck::RelaxedForActive)) { qCDebug(KWIN_CORE) << "Raising: Belongs to active application"; return true; } @@ -753,13 +753,13 @@ xcb_timestamp_t Client::readUserTimeMapTimestamp(const KStartupInfoId *asn_id, c // from already running application if this application // is not the active one (unless focus stealing prevention is turned off). Client* act = dynamic_cast(workspace()->mostRecentlyActivatedClient()); - if (act != NULL && !belongToSameApplication(act, this, true)) { + if (act != NULL && !belongToSameApplication(act, this, SameApplicationCheck::RelaxedForActive)) { bool first_window = true; auto sameApplicationActiveHackPredicate = [this](const Client *cl) { // ignore already existing splashes, toolbars, utilities and menus, // as the app may show those before the main window return !cl->isSplash() && !cl->isToolbar() && !cl->isUtility() && !cl->isMenu() - && cl != this && Client::belongToSameApplication(cl, this, true); + && cl != this && Client::belongToSameApplication(cl, this, SameApplicationCheck::RelaxedForActive); }; if (isTransient()) { auto clientMainClients = [this] () -> ClientList { diff --git a/client.cpp b/client.cpp index f3432667a2..52cbc1f98a 100644 --- a/client.cpp +++ b/client.cpp @@ -2110,13 +2110,13 @@ void Client::addDamage(const QRegion &damage) Toplevel::addDamage(damage); } -bool Client::belongsToSameApplication(const AbstractClient *other, bool active_hack) const +bool Client::belongsToSameApplication(const AbstractClient *other, SameApplicationChecks checks) const { const Client *c2 = dynamic_cast(other); if (!c2) { return false; } - return Client::belongToSameApplication(this, c2, active_hack); + return Client::belongToSameApplication(this, c2, checks); } void Client::updateTabGroupStates(TabGroup::States states) diff --git a/client.h b/client.h index 656536ceee..2633a788d9 100644 --- a/client.h +++ b/client.h @@ -247,7 +247,7 @@ public: /// Does 'delete c;' static void deleteClient(Client* c); - static bool belongToSameApplication(const Client* c1, const Client* c2, bool active_hack = false); + static bool belongToSameApplication(const Client* c1, const Client* c2, SameApplicationChecks checks = SameApplicationChecks()); static bool sameAppWindowRoleMatch(const Client* c1, const Client* c2, bool active_hack); void killWindow() override; @@ -377,7 +377,7 @@ private: protected: virtual void debug(QDebug& stream) const; void addDamage(const QRegion &damage) override; - bool belongsToSameApplication(const AbstractClient *other, bool active_hack) const override; + bool belongsToSameApplication(const AbstractClient *other, SameApplicationChecks checks) const override; void doSetActive() override; void doSetKeepAbove() override; void doSetKeepBelow() override; diff --git a/group.cpp b/group.cpp index 8648d4dcc5..ab79034ceb 100644 --- a/group.cpp +++ b/group.cpp @@ -419,7 +419,7 @@ bool Toplevel::resourceMatch(const Toplevel* c1, const Toplevel* c2) // Client //**************************************** -bool Client::belongToSameApplication(const Client* c1, const Client* c2, bool active_hack) +bool Client::belongToSameApplication(const Client* c1, const Client* c2, SameApplicationChecks checks) { bool same_app = false; @@ -438,16 +438,18 @@ bool Client::belongToSameApplication(const Client* c1, const Client* c2, bool ac same_app = true; // same client leader // tests that mean they most probably don't belong together - else if (c1->pid() != c2->pid() + else if ((c1->pid() != c2->pid() && !checks.testFlag(SameApplicationCheck::AllowCrossProcesses)) || c1->wmClientMachine(false) != c2->wmClientMachine(false)) ; // different processes else if (c1->wmClientLeader() != c2->wmClientLeader() && c1->wmClientLeader() != c1->window() // if WM_CLIENT_LEADER is not set, it returns window(), - && c2->wmClientLeader() != c2->window()) // don't use in this test then + && c2->wmClientLeader() != c2->window() // don't use in this test then + && !checks.testFlag(SameApplicationCheck::AllowCrossProcesses)) ; // different client leader else if (!resourceMatch(c1, c2)) ; // different apps - else if (!sameAppWindowRoleMatch(c1, c2, active_hack)) + else if (!sameAppWindowRoleMatch(c1, c2, checks.testFlag(SameApplicationCheck::RelaxedForActive)) + && !checks.testFlag(SameApplicationCheck::AllowCrossProcesses)) ; // "different" apps else if (c1->pid() == 0 || c2->pid() == 0) ; // old apps that don't have _NET_WM_PID, consider them different diff --git a/shell_client.cpp b/shell_client.cpp index c72105ceb0..cf6166cc79 100644 --- a/shell_client.cpp +++ b/shell_client.cpp @@ -553,9 +553,13 @@ QByteArray ShellClient::windowRole() const return QByteArray(); } -bool ShellClient::belongsToSameApplication(const AbstractClient *other, bool active_hack) const +bool ShellClient::belongsToSameApplication(const AbstractClient *other, SameApplicationChecks checks) const { - Q_UNUSED(active_hack) + if (checks.testFlag(SameApplicationCheck::AllowCrossProcesses)) { + if (other->desktopFileName() == desktopFileName()) { + return true; + } + } if (auto s = other->surface()) { return s->client() == surface()->client(); } @@ -854,7 +858,7 @@ void ShellClient::takeFocus() // check that it doesn't belong to the desktop const auto &clients = waylandServer()->clients(); for (auto c: clients) { - if (!belongsToSameApplication(c, false)) { + if (!belongsToSameApplication(c, SameApplicationChecks())) { continue; } if (c->isDesktop()) { diff --git a/shell_client.h b/shell_client.h index 0b15b7a4a8..967b5fca63 100644 --- a/shell_client.h +++ b/shell_client.h @@ -156,7 +156,7 @@ public: protected: void addDamage(const QRegion &damage) override; - bool belongsToSameApplication(const AbstractClient *other, bool active_hack) const override; + bool belongsToSameApplication(const AbstractClient *other, SameApplicationChecks checks) const override; void doSetActive() override; Layer layerForDock() const override; void changeMaximize(bool horizontal, bool vertical, bool adjust) override; diff --git a/tabbox/tabbox.cpp b/tabbox/tabbox.cpp index c5d53dce92..e718afade3 100644 --- a/tabbox/tabbox.cpp +++ b/tabbox/tabbox.cpp @@ -200,7 +200,7 @@ bool TabBoxHandlerImpl::checkApplications(TabBoxClient* client) const continue; } if ((c = dynamic_cast< TabBoxClientImpl* >(client.data()))) { - if (AbstractClient::belongToSameApplication(c->client(), current)) { + if (AbstractClient::belongToSameApplication(c->client(), current, AbstractClient::SameApplicationCheck::AllowCrossProcesses)) { return false; } } @@ -212,7 +212,7 @@ bool TabBoxHandlerImpl::checkApplications(TabBoxClient* client) const return false; } if ((c = dynamic_cast< TabBoxClientImpl* >(pointer.data()))) { - if (AbstractClient::belongToSameApplication(c->client(), current)) { + if (AbstractClient::belongToSameApplication(c->client(), current, AbstractClient::SameApplicationCheck::AllowCrossProcesses)) { return true; } }