Bug 335637 - Default focus stealing prevention behavior has become too strict
Summary: Default focus stealing prevention behavior has become too strict
Status: RESOLVED FIXED
Alias: None
Product: kwin
Classification: Unclassified
Component: core (show other bugs)
Version: git master
Platform: unspecified Linux
: NOR normal (vote)
Target Milestone: 5
Assignee: KWin default assignee
URL: https://git.reviewboard.kde.org/r/118456
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-01 04:17 UTC by Eike Hein
Modified: 2014-06-03 12:04 UTC (History)
0 users

See Also:
Latest Commit:
Version Fixed In:
mgraesslin: ReviewRequest+


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Eike Hein 2014-06-01 04:17:27 UTC
This report is going to be fairly vague, but I felt it was important to have a talk about.

It appears that kwin's default focus stealing behavior has become much more strict in Plasma Next vs. Plasma 1. That is, the default "Low" setting behaves differently now.

Here's a test case: Add a Konsole launcher to the panel. Activate it. Konsole will start, but its window will appear behind the currently active window - it's neither raised nor given focus. In almost all situations, however, my next step is to do just that, so I can use the Konsole I just launched. The extra step is a hassle.

I didn't think all that much about this until recently, when I quizzed a user about their kwin settings in an unrelated bug report and they mentioned they had set their focus stealing prevention level to "None" to work around just this problem. And indeed it does.

Reproducible: Always
Comment 1 Thomas Lübking 2014-06-01 06:25:13 UTC
"change" is due to lazy xTime() update by 99% chance - or the launcher getting into the focus chain (thus has the newer usertime by the mouserelease)

Also see:
https://git.reviewboard.kde.org/r/110919/
should go along
https://git.reviewboard.kde.org/r/110918/
based on this thoughts
http://forum.kde.org/viewtopic.php?f=83&t=101833

WARNING: the patches will not apply cleanly on master (but still should on 4.11)
Comment 2 Eike Hein 2014-06-01 08:38:25 UTC
Can we accelerate progress on this somehow? I think this is fairly important to fix for the release; the current behavior is a huge nuisance.
Comment 3 Thomas Lübking 2014-06-01 09:13:34 UTC
As for this bug, calling "updateXTime();" before calling xTime() in updateUserTime() in activation.cpp should be sufficient.

Only question is whether we should still tackle this more generally. See bug #335187

Proposal there is to flag the timestamp dirty at the end of each eventcycle and have

inline
KWIN_EXPORT xcb_timestamp_t xTime()
{
    if (m_timeStampDirty) {
        xcb_timestamp_t time = QX11Info::getTimestamp();
        QX11Info::setAppTime(time);
        return time;
    }
    return QX11Info::appTime();
}

then scratch updateXTime() and seek to replace xTime() by XCB_CURRENT_TIME wherever suitable .

I can however not say whether this can/should be done before the P/N release.
We've atm 33 lines of xTime() (quite some are already comments) and 13 calls to updateXTime()
Comment 4 Martin Flöser 2014-06-01 09:28:34 UTC
(In reply to comment #2)
> Can we accelerate progress on this somehow? I think this is fairly important
> to fix for the release; the current behavior is a huge nuisance.

hey, it's Sunday and you opened that bug just a few hours ago. A little bit realistic time handling wouldn't hurt :-)

I sent a reply which somewhere got lost: are you running https://git.reviewboard.kde.org/r/118234/ ? If not, could you please try whether that improves the situation.
Comment 5 Eike Hein 2014-06-01 09:47:51 UTC
>  hey, it's Sunday and you opened that bug just a few hours ago. A little bit realistic time handling wouldn't hurt :-)

For the record, my "can we accelerate" was because Thomas' review requests he pointed to as fixes are 11 months old, and their description says "needs a lot of testing" -- I didn't mean in the context of today.


> are you running https://git.reviewboard.kde.org/r/118234/ 

Nope - testing in a moment.
Comment 6 Eike Hein 2014-06-01 09:53:16 UTC
> If not, could you please try whether that improves the situation.

Yep, seems to fix it! In the same situation, with focus stealing prevention set back to Low, Konsole is now raised and focused.
Comment 7 Martin Flöser 2014-06-01 09:59:32 UTC
(In reply to comment #3)

> I can however not say whether this can/should be done before the P/N release.
> We've atm 33 lines of xTime() (quite some are already comments) and 13 calls
> to updateXTime()

sounds like post-release to me. I think there are quite some which can be removed. E.g. I added one updateXTime() in TabBox when activated through globalaccel, but that was in fact just a workaround for kglobalaccel not setting the proper timestamp as it used to be.

Also I think that it should be possible to remove quite some updateXTime if we properly update the timestamp from the events we get (and check where it's done in Qt).
Comment 8 Eike Hein 2014-06-01 13:40:42 UTC
I may have been too early with the "yes, it's fixed" above - right now, still in the same session with the same settings, launching Konsole from the panel no longer raises and focuses it. I don't know at what point or why the behavior changed.
Comment 9 Martin Flöser 2014-06-01 13:47:09 UTC
On Sunday 01 June 2014 13:40:42 you wrote:
> I may have been too early with the "yes, it's fixed" above - right now,
> still in the same session with the same settings, launching Konsole from
> the panel no longer raises and focuses it. I don't know at what point or
> why the behavior changed.

are we talking about Konsole/4 or Konsole/5 here? It's quite possible that 
this is a regression in Konsole (after all it's still a unique application)
Comment 10 Eike Hein 2014-06-01 14:00:13 UTC
Konsole/5, but I'm aware of its single-process mechanic and it's a fresh process in all the test cases. The same also happens with Kate, though.
Comment 11 Thomas Lübking 2014-06-01 15:37:58 UTC
diff --git a/activation.cpp b/activation.cpp
index d98c7d9..ca194da 100644
--- a/activation.cpp
+++ b/activation.cpp
@@ -661,8 +661,10 @@ void Workspace::clientAttentionChanged(Client* c, bool set)
 void Client::updateUserTime(xcb_timestamp_t time)
 {
     // copied in Group::updateUserTime
-    if (time == XCB_TIME_CURRENT_TIME)
+    if (time == XCB_TIME_CURRENT_TIME) {
+        updateXTime();
         time = xTime();
+    }
     if (time != -1U
             && (m_userTime == XCB_TIME_CURRENT_TIME
                 || NET::timestampCompare(time, m_userTime) > 0)) {    // time > user_time
Comment 12 Martin Flöser 2014-06-01 15:46:14 UTC
in addition also:

diff --git a/events.cpp b/events.cpp
index e7185e0..d2d5819 100644
--- a/events.cpp
+++ b/events.cpp
@@ -606,12 +606,12 @@ bool Client::windowEvent(xcb_generic_event_t *e)
         propertyNotifyEvent(reinterpret_cast<xcb_property_notify_event_t*>(e));
         break;
     case XCB_KEY_PRESS:
-        updateUserTime();
+        updateUserTime(reinterpret_cast<xcb_key_press_event_t*>(e)->time);
         workspace()->setWasUserInteraction();
         break;
     case XCB_BUTTON_PRESS: {
         const auto *event = reinterpret_cast<xcb_button_press_event_t*>(e);
-        updateUserTime();
+        updateUserTime(event->time);
         workspace()->setWasUserInteraction();
         buttonPressEvent(event->event, event->detail, event->state,
                          event->event_x, event->event_y, event->root_x, 
event->root_y);
Comment 13 Martin Flöser 2014-06-01 16:05:16 UTC
@Eike: please give a try to the patch in https://git.reviewboard.kde.org/r/118456
Comment 14 Eike Hein 2014-06-01 16:59:50 UTC
I'm running it now - so far it seems good, but I'll need to use it for a while again.
Comment 15 Eike Hein 2014-06-02 13:35:52 UTC
I've been running both patches all day and so far it seems to all behave nicely.
Comment 16 Martin Flöser 2014-06-03 12:04:50 UTC
Git commit 7910fed659b2b0c5d2d7c15880b290fd444e7135 by Martin Gräßlin.
Committed on 01/06/2014 at 15:57.
Pushed by graesslin into branch 'master'.

Improve updating user timestamp

Use the timestamp from the xcb event which triggers the update whenever
possible. If we don't have access to the latest event, let's at least
update our own xTime prior to using it.

Slightly unrelated change included: Group switches the userTime from
XLib datatype to xcb datatype.
REVIEW: 118456

M  +8    -4    activation.cpp
M  +2    -2    client.h
M  +10   -9    events.cpp
M  +4    -4    group.h

http://commits.kde.org/kwin/7910fed659b2b0c5d2d7c15880b290fd444e7135