From 555885072d8ff126168994d6d9c3b9692b1b1b00 Mon Sep 17 00:00:00 2001 From: Vlad Zahorodnii Date: Wed, 22 Jul 2020 14:00:11 +0300 Subject: [PATCH] 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 | 2 +- activation.cpp | 31 +++++++++++++++-------- autotests/integration/x11_client_test.cpp | 2 -- events.cpp | 12 ++++++--- internal_client.cpp | 3 ++- internal_client.h | 2 +- workspace.h | 6 ++--- x11client.cpp | 18 ++++++++++--- x11client.h | 2 +- xdgshellclient.cpp | 6 +++-- xdgshellclient.h | 4 +-- 11 files changed, 56 insertions(+), 32 deletions(-) diff --git a/abstract_client.h b/abstract_client.h index ca923e325a..c5e3dbd80d 100644 --- a/abstract_client.h +++ b/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. diff --git a/activation.cpp b/activation.cpp index c2218069c6..f0b7d19b80 100644 --- a/activation.cpp +++ b/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) diff --git a/autotests/integration/x11_client_test.cpp b/autotests/integration/x11_client_test.cpp index 31e7dd9f1a..2e078e50d3 100644 --- a/autotests/integration/x11_client_test.cpp +++ b/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 connection(xcb_connect(nullptr, nullptr)); QVERIFY(!xcb_connection_has_error(connection.data())); diff --git a/events.cpp b/events.cpp index 3ac3ffc062..57d87033b7 100644 --- a/events.cpp +++ b/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); + } } } diff --git a/internal_client.cpp b/internal_client.cpp index 958ce3b47e..120f537def 100644 --- a/internal_client.cpp +++ b/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) diff --git a/internal_client.h b/internal_client.h index b1b3b968fc..e131653c07 100644 --- a/internal_client.h +++ b/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; diff --git a/workspace.h b/workspace.h index e6884d6b92..9e3c1bb5b9 100644 --- a/workspace.h +++ b/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); diff --git a/x11client.cpp b/x11client.cpp index 55ec57d3ed..8f48a26202 100644 --- a/x11client.cpp +++ b/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 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; } /** diff --git a/x11client.h b/x11client.h index 8c863f71b6..132584b542 100644 --- a/x11client.h +++ b/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; diff --git a/xdgshellclient.cpp b/xdgshellclient.cpp index 5922a6d2c1..1399cbfca1 100644 --- a/xdgshellclient.cpp +++ b/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 diff --git a/xdgshellclient.h b/xdgshellclient.h index 2e542f4aa5..b0a8e5ed12 100644 --- a/xdgshellclient.h +++ b/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: