Check if we successfully restored input focus

In rare cases, Workspace::restoreFocus() may fail, for example when the
most recently activated client is about to be destroyed or unmapped.

If it happens that we cannot restore the focus, then mark the window in
FocusIn event as active.

CCBUG: 424223

abstract_client.h

@@ -550,7 +550,7 @@ public:
     void setupWindowRules(bool ignore_temporary);
     void evaluateWindowRules();
     virtual void applyWindowRules();
-    virtual void takeFocus() = 0;
+    virtual bool takeFocus() = 0;
     virtual bool wantsInput() const = 0;
     /**
      * Whether a dock window wants input.

activation.cpp

@@ -340,12 +340,12 @@ void Workspace::activateClient(AbstractClient* c, bool force)
  *
  * @see activateClient
  */
-void Workspace::requestFocus(AbstractClient* c, bool force)
+bool Workspace::requestFocus(AbstractClient* c, bool force)
 {
-    takeActivity(c, force ? ActivityFocusForce : ActivityFocus);
+    return takeActivity(c, force ? ActivityFocusForce : ActivityFocus);
 }
 
-void Workspace::takeActivity(AbstractClient* c, ActivityFlags flags)
+bool Workspace::takeActivity(AbstractClient* c, ActivityFlags flags)
 {
     // the 'if ( c == active_client ) return;' optimization mustn't be done here
     if (!focusChangeEnabled() && (c != active_client))
@@ -353,7 +353,7 @@ void Workspace::takeActivity(AbstractClient* c, ActivityFlags flags)
 
     if (!c) {
         focusToNull();
-        return;
+        return true;
     }
 
     if (flags & ActivityFocus) {
@@ -390,16 +390,20 @@ void Workspace::takeActivity(AbstractClient* c, ActivityFlags flags)
     }
     if (!c->isShown(true)) {  // shouldn't happen, call activateClient() if needed
         qCWarning(KWIN_CORE) << "takeActivity: not shown" ;
-        return;
+        return false;
     }
 
+    bool ret = true;
+
     if (flags & ActivityFocus)
-        c->takeFocus();
+        ret &= c->takeFocus();
     if (flags & ActivityRaise)
         workspace()->raiseClient(c);
 
     if (!c->isOnActiveScreen())
         screens()->setCurrent(c->screen());
+
+    return ret;
 }
 
 /**
@@ -660,9 +664,13 @@ bool Workspace::allowFullClientRaising(const KWin::AbstractClient *c, xcb_timest
     return NET::timestampCompare(time, user_time) >= 0;   // time >= user_time
 }
 
-// called from Client after FocusIn that wasn't initiated by KWin and the client
-// wasn't allowed to activate
-void Workspace::restoreFocus()
+/**
+ * Called from X11Client after FocusIn that wasn't initiated by KWin and the client wasn't
+ * allowed to activate.
+ *
+ * Returns @c true if the focus has been restored successfully; otherwise returns @c false.
+ */
+bool Workspace::restoreFocus()
 {
     // this updateXTime() is necessary - as FocusIn events don't have
     // a timestamp *sigh*, kwin's timestamp would be older than the timestamp
@@ -670,9 +678,10 @@ void Workspace::restoreFocus()
     // the attempt to restore the focus would fail due to old timestamp
     updateXTime();
     if (should_get_focus.count() > 0)
-        requestFocus(should_get_focus.last());
+        return requestFocus(should_get_focus.last());
     else if (last_active_client)
-        requestFocus(last_active_client);
+        return requestFocus(last_active_client);
+    return true;
 }
 
 void Workspace::clientAttentionChanged(AbstractClient* c, bool set)

autotests/integration/x11_client_test.cpp

@@ -1008,8 +1008,6 @@ void X11ClientTest::testActivateFocusedWindow()
     // case no FocusIn event will be generated and the window won't be marked as active. This test
     // verifies that we handle that subtle case properly.
 
-    QSKIP("Focus is not restored properly when the active client is about to be unmapped");
-
     QScopedPointer<xcb_connection_t, XcbConnectionDeleter> connection(xcb_connect(nullptr, nullptr));
     QVERIFY(!xcb_connection_has_error(connection.data()));
 

events.cpp

@@ -1146,11 +1146,15 @@ void X11Client::focusInEvent(xcb_focus_in_event_t *e)
     // check if this client is in should_get_focus list or if activation is allowed
     bool activate =  workspace()->allowClientActivation(this, -1U, true);
     workspace()->gotFocusIn(this);   // remove from should_get_focus list
-    if (activate)
+    if (activate) {
         setActive(true);
-    else {
-        workspace()->restoreFocus();
-        demandAttention();
+    } else {
+        if (workspace()->restoreFocus()) {
+            demandAttention();
+        } else {
+            qCWarning(KWIN_CORE, "Failed to restore focus. Activating 0x%x", windowId());
+            setActive(true);
+        }
     }
 }
 

internal_client.cpp

@@ -353,8 +353,9 @@ void InternalClient::setOnAllActivities(bool set)
     // Internal clients do not support activities.
 }
 
-void InternalClient::takeFocus()
+bool InternalClient::takeFocus()
 {
+    return false;
 }
 
 void InternalClient::setNoBorder(bool set)

internal_client.h

@@ -73,7 +73,7 @@ public:
     bool supportsWindowRules() const override;
     AbstractClient *findModal(bool allow_itself = false) override;
     void setOnAllActivities(bool set) override;
-    void takeFocus() override;
+    bool takeFocus() override;
     void setNoBorder(bool set) override;
     void updateDecoration(bool check_workspace_pos, bool force = false) override;
     void updateColorScheme() override;

workspace.h

@@ -167,17 +167,17 @@ public:
     AbstractClient* clientUnderMouse(int screen) const;
 
     void activateClient(AbstractClient*, bool force = false);
-    void requestFocus(AbstractClient* c, bool force = false);
+    bool requestFocus(AbstractClient* c, bool force = false);
     enum ActivityFlag {
         ActivityFocus = 1 << 0, // focus the window
         ActivityFocusForce = 1 << 1 | ActivityFocus, // focus even if Dock etc.
         ActivityRaise = 1 << 2 // raise the window
     };
     Q_DECLARE_FLAGS(ActivityFlags, ActivityFlag)
-    void takeActivity(AbstractClient* c, ActivityFlags flags);
+    bool takeActivity(AbstractClient* c, ActivityFlags flags);
     bool allowClientActivation(const AbstractClient* c, xcb_timestamp_t time = -1U, bool focus_in = false,
                                bool ignore_desktop = false);
-    void restoreFocus();
+    bool restoreFocus();
     void gotFocusIn(const AbstractClient*);
     void setShouldGetFocus(AbstractClient*);
     bool activateNextClient(AbstractClient* c);

x11client.cpp

@@ -2021,12 +2021,20 @@ void X11Client::setOnAllActivities(bool on)
 /**
  * Performs the actual focusing of the window using XSetInputFocus and WM_TAKE_FOCUS
  */
-void X11Client::takeFocus()
+bool X11Client::takeFocus()
 {
-    if (rules()->checkAcceptFocus(info->input()))
-        m_client.focus();
-    else
+    if (rules()->checkAcceptFocus(info->input())) {
+        xcb_void_cookie_t cookie = xcb_set_input_focus_checked(connection(),
+                                                               XCB_INPUT_FOCUS_POINTER_ROOT,
+                                                               windowId(), XCB_TIME_CURRENT_TIME);
+        ScopedCPointer<xcb_generic_error_t> error(xcb_request_check(connection(), cookie));
+        if (error) {
+            qCWarning(KWIN_CORE, "Failed to focus 0x%x (error %d)", windowId(), error->error_code);
+            return false;
+        }
+    } else {
         demandAttention(false); // window cannot take input, at least withdraw urgency
+    }
     if (info->supportsProtocol(NET::TakeFocusProtocol)) {
         updateXTime();
         sendClientMessage(window(), atoms->wm_protocols, atoms->wm_take_focus);
@@ -2044,6 +2052,8 @@ void X11Client::takeFocus()
     }
     if (breakShowingDesktop)
         workspace()->setShowingDesktop(false);
+
+    return true;
 }
 
 /**

x11client.h

@@ -177,7 +177,7 @@ public:
     bool isMovableAcrossScreens() const override;
     bool isCloseable() const override; ///< May be closed by the user (May have a close button)
 
-    void takeFocus() override;
+    bool takeFocus() override;
 
     void updateDecoration(bool check_workspace_pos, bool force = false) override;
 

xdgshellclient.cpp

@@ -1033,7 +1033,7 @@ void XdgToplevelClient::doFinishMoveResize()
     scheduleConfigure();
 }
 
-void XdgToplevelClient::takeFocus()
+bool XdgToplevelClient::takeFocus()
 {
     if (wantsInput()) {
         sendPing(PingReason::FocusWindow);
@@ -1042,6 +1042,7 @@ void XdgToplevelClient::takeFocus()
     if (!keepAbove() && !isOnScreenDisplay() && !belongsToDesktop()) {
         workspace()->setShowingDesktop(false);
     }
+    return true;
 }
 
 bool XdgToplevelClient::wantsInput() const
@@ -2130,8 +2131,9 @@ bool XdgPopupClient::wantsInput() const
     return false;
 }
 
-void XdgPopupClient::takeFocus()
+bool XdgPopupClient::takeFocus()
 {
+    return false;
 }
 
 bool XdgPopupClient::acceptsFocus() const

xdgshellclient.h

@@ -164,7 +164,7 @@ public:
     void updateDecoration(bool check_workspace_pos, bool force = false) override;
     void updateColorScheme() override;
     bool supportsWindowRules() const override;
-    void takeFocus() override;
+    bool takeFocus() override;
     bool wantsInput() const override;
     bool dockWantsInput() const override;
     bool hasStrut() const override;
@@ -270,7 +270,7 @@ public:
     void updateDecoration(bool check_workspace_pos, bool force = false) override;
     void showOnScreenEdge() override;
     bool wantsInput() const override;
-    void takeFocus() override;
+    bool takeFocus() override;
     bool supportsWindowRules() const override;
 
 protected: