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

@@ -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

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<KWin::AbstractClient*>)
+Q_DECLARE_OPERATORS_FOR_FLAGS(KWin::AbstractClient::SameApplicationChecks)
 
 #endif

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<Client*>(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 {

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<const Client*>(other);
     if (!c2) {
         return false;
     }
-    return Client::belongToSameApplication(this, c2, active_hack);
+    return Client::belongToSameApplication(this, c2, checks);
 }
 
 void Client::updateTabGroupStates(TabGroup::States states)

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;

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

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()) {

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;

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;
             }
         }