Bug 344671

Created attachment 91599 [details]
fixes multihead screen focus stealing

I created a multihead system with my Thinkpad X61 using an external monitor and managed to replicate the problem.
I have managed to create a patch for activation.cpp in kwin that fixes the problem on the laptop and my desktop machine.

Thanks for the patch - could you please upload it to git.reviewboard.kde.org and please provide it for the kwin5 repository. It's unlikely that we will do another update for 4.11

The patch makes it pointer driven, did you check whether

if (!Workspace::isOnCurrentHead())
   return false;

works?

I have rebuilt the code using "if (!Workspace::isOnCurrentHead())" and no, it does not work.

With it, I can't get focus on windows on screen 0 although I can click on say, Konsole's "Help->About Konsole" menu entry and it will pop up the About window but the window border stays in the inactive colour.  Clicking in the Konsole window body does not let me type anything in.


Martin,
I don't know how to get an account on git.reviewboard.kde.org so I will put the patch for kwin5 here too so at least you can see it (I'm new to all this - I've never created patches before).

The code I used is based on the "void X11Cursor::doGetPos()" function in "cursor.cpp".  Since that code has changed in kwin5, the patch for "activation.cpp" in kwin5 is using the new style although I am unable to build and test it.

Created attachment 91609 [details]
Similar patch but for kwin5

I updated my Fedora 21 system and kde-workspace-4.11.18.3.fc21.rpm was installed which includes kwin and the "multihead focus jump to the wrong screen" is still present.
I checked the source code for that rpm and my patch is not present.

Martin/Thomas - has my patch been rejected or has it not filtered through to the Fedora distribution yet?

I've obviously been running with the patch ever since I submitted it and I have seen no adverse side effects.

KDE workspace from SC4 is feature frozen, it'll only see security fixes (and "i could accidentally type into the wrong window" doesn't account here ;-)

So the patch could only be added for KF5.

Multihead is pretty much unmaintained, so we could just "look the other side", but as pointed out in comment #4, the patch is not entirely correct.

The original bug said:
"Close a program window or a sub-window and instead of the focus jumping to another window on the same screen or the parent window, focus instead jumps to the other screen."

So the goal is to ensure "If I close a client on HEAD #a, the next focus shall not end up on a client on HEAD #b" - this sounds reasonable to me, but what the patch *does* is to move the focus to the head that currently has the mouse.
While that may be equivalent in your present workflow, it is not actually.

The relevant context will (likely) be events.cpp, ::workspaceEvent(), XCB_FOCUS_IN branch.
It would be required to check the event attributes on either kwin instance for a pattern (eg. one has XCB_WINDOW_NONE  and the other XCB_INPUT_FOCUS_POINTER_ROOT)  to determine which head had the focus before.
In worst case, one would have to track "did I have the focus before" - you *might* be lucky in XCB_FOCUS_OUT here.

If you've no reservations, we usually process patches through http://git.reviewboard.kde.org - they easily got lost in bug reports (qed.)

I have spent some time learning about XCB and creating test programs to find which head has focus and ended up creating code using the same calls as your "Workspace::isOnCurrentHead()".  This test code worked fine until I inserted it into KWin at which point it blew up just like your function did when I tried to use it.

The function "xcb_get_input_focus" can also return "XCB_POINTER_ROOT" (or would if it was actually defined in xproto.h but it is missing) but "XCB_INPUT_FOCUS_POINTER_ROOT" has the same value which is "1".  If you attempt to do a "xcb_get_geometry" with a window ID of "1" that  causes the crash.
According to the man page of "xcb_set_input_focus", if you have a focus of "XCB_POINTER_ROOT" then that means that the "focus is on the root window of the screen on which the pointer is on currently".  Which means in this case, the focus really is pointer driven as comment 4 questions.
So at that point, I think it is appropriate to ask the pointer which screen it is on to determine the active head.

I have attached two patches.  The workspace.patch corrects the "isOnCurrentHead" function and the events.patch stops the XCB_FOCUS_IN event doing anything for the head that doesn't have focus thereby stopping the race condition that was causing the focus stealing.

Created attachment 95249 [details]
patch for events loop

Created attachment 95250 [details]
patch for isOnCurrentHead()

(In reply to Nicholas Redgrave from comment #9)
> The function "xcb_get_input_focus" can also return "XCB_POINTER_ROOT"

Yes, this is when the focus is returned from a closing window.
Workspace::workspaceEvent() / XCB_FOCUS_IN is then supposed to pass it somewhere useful.

This is the critical moment and your patch just repeats your former approach (sorry ;-) by looking at the pointer position here.

What still should happen is to not move away the focus from the head that had it.
(Handling the XCB_INPUT_FOCUS_POINTER_ROOT case in isOnCurrentHead() should be superfluous either since we do not want the focus to end up on the root window *ever* while KWin is up - it can only be the interim condition of XCB_FOCUS_IN situation)


> Which means in this case, the focus really is pointer driven as comment 4 questions.
No, does not. Wherever "some" client (including kwin, for that matter) may have redirected the focus in case of its death, does not determine the KWin focus policy at all.
And regardless of that, a client can easily close on the head your pointer is currently not on (just use click-to-focus, "sleep 5; pkill xterm" and move over the pointer...)

=> If you want the focus to not jump to the other head, stay away from the pointer =)


Instead, it will (likely) be required to track the head with "legal" focus in events, ie. in Workspace::workspaceEvent() / XCB_FOCUS_IN append

else {
   m_wasCurrentHead = isOnCurrentHead(); // true if !is_multihead
}

and use "if (!m_wasCurrentHead) return true;" in the focus reverting "if (event->event == rootWindow() ..." branch.

I was paying attention when you said my old patch could be fooled so I didn't submit this one until I thought I couldn't fool it anymore.

>Wherever "some" client may have redirected the focus in case of its death, does not determine the KWin focus policy at all.

KWin has a user-defined policy of how to treat windows on a particular head (click-to-focus, etc.) but there does not appear to be any defined policy on how to treat the focus between independent heads nor any way to set that policy.

> And regardless of that, a client can easily close on the head your pointer is currently not on (just use click-to-focus, "sleep 5; pkill xterm" and move over the pointer...) 

I have just tried this procedure ten times, both ways round with my patch:

An xterm on each head.
Run kcalc from xterm on head 1.
Click on the xterm on head 0 and run "sleep 5; pkill kcalc".
Move the pointer to head 1 before the pkill executes and don't click.
Result:  focus stays on xterm on head 0.

Reverse procedure.
Run kcalc from xterm on head 0.
Click on the xterm on head 1 and run "sleep 5; pkill kcalc".
Move the pointer to head 0 before the pkill executes and don't click.
Result:  focus stays on xterm on head 1.

Isn't this the behaviour you wanted?

(In reply to Nicholas Redgrave from comment #13)
> Isn't this the behaviour you wanted?

The test is uncritical.
The relevant condition is when the focused window disappears, since that will revert the focus - in this case to the root - and cause the unwanted activateNext() situation.
If you close a window that does not currently have the input focus, the focus will just stay where it is, there's also no XCB_FOCUS_IN event or anything.

> KWin has a user-defined policy of how to treat windows on a particular head (click-to-focus, 
> etc.) but there does not appear to be any defined policy on how to treat the focus between 
> independent heads nor any way to set that policy.

No, but that doesn't mean XCB_INPUT_FOCUS_POINTER_ROOT would indicate that "the focus really is pointer driven" at all - the value is simply unspecific.

As for the intended behavior, your very own demand was:
> Focus should return to the parent window or to another window on the same screen. 

Only as a resort you mentioned:
> At the very least, focus should remain on the screen where the mouse pointer is.

The latter however matches mouse driven focus policies.
Once there's an actual way to stay on the current head, we'll see whether additional code to honor mouse precedence is required on top (could be for FFM if the desktop doesn't gain focus, ie. you manage to move the mouse w/o moving input focus to the other head)

I really don't want to argue with you and maybe I'm reading these man pages completely wrong but you said:

>No, but that doesn't mean XCB_INPUT_FOCUS_POINTER_ROOT would indicate that "the focus really is pointer driven" at all - the value is simply unspecific.

I have to disagree with you saying the value is simply unspecific.  The XCB function "xcb_get_input_focus" essentially calls "XGetInputFocus":

int XGetInputFocus(Display *display, Window *focus_return, int *revert_to_return);
"focus_return" can have three values; focus window, PointerRoot, or None.

If the return value is "PointerRoot" then it does have a very specific value.  The man page says:
"If focus is PointerRoot, the focus window is dynamically taken to be the root window of whatever screen the pointer is on at each keyboard event."

The "isOnCurrentHead" function in workspace.cpp is trying to find which head has focus.  The reply "PointerRoot" is saying that the head that has focus is the one that the pointer is on.
To find out which head the pointer is on we call "xcb_query_pointer" with the root window of this head and see if it's on "same_screen".
If the reply to XGetInputFocus is an actual window number then we use the "xcb_get_geometry" to find out which root window that window belongs to.

Here's the whole function slightly tweaked in case it failed to get a valid "pointer":

/**
 * checks whether the X Window with the input focus is on our X11 screen
 * if the window cannot be determined or inspected, return depends on whether there's actually
 * more than one screen
 *
 * this is NOT in any way related to XRandR multiscreen
 *
 */
bool Workspace::isOnCurrentHead()
{
    if (!is_multihead) {
        return true;
    }

    Xcb::CurrentInput currentInput;
    if (currentInput.window() == XCB_WINDOW_NONE) {
        return !is_multihead;
    }
    // should be "XCB_POINTER_ROOT"(which should also equal "1") but is missing from xproto.h
    if (currentInput.window() == XCB_INPUT_FOCUS_POINTER_ROOT) {
        Xcb::Pointer pointer(rootWindow());
        if (pointer) {
            if ( pointer->same_screen ){
                return true;
            }
            else
            {
                return false;
            }
        }
        else
        {
            return !is_multihead;
        }
    }

    Xcb::WindowGeometry geometry(currentInput.window());
    if (geometry.isNull()) { // should not happen
        return !is_multihead;
    }

    return rootWindow() == geometry->root;
}

Clients (including kwin) just dump a value there like "when you loose the focus, put it somewhere else"

In a managed context, this has *no* meaning, the window managers will typically pick up the focus and move it somewhere else (according to their habits), so clients tend to say "put it onto the root window", notably for the alternatives (XCB_INPUT_FOCUS_PARENT is usually not even available in this context - or rather /is/ the root window)

KWin could just as much say XCB_INPUT_FOCUS_NONE here - with zero impact on the behavior.

=> ignore this value (but as indicator that the focus is presently "invalid")

I wasn't entirely convinced by your claim that "PointerRoot" is "simply unspecific" so I explained the context and pasted example code showing how I was trying to find which head had focus to the experts over on the XCB mailing list - figuring if anyone knew what the reply from "xcb_get_input_focus" really meant, in all contexts, then they would.

So Uli Schlachter, who incidentally works on the "awesome window manager", states that the answer is precisely what is in the X11 documentation:

http://www.x.org/releases/X11R7.7/doc/xproto/x11protocol.html#requests:SetInputFocus

and to quote him:
"So yes, this really does mean "focus follows the pointer"."

This means that my patch is doing what the X11 protocol intended.  If you are using "PointerRoot" to mean something else then it looks like you are mistaken.

(In reply to Nicholas Redgrave from comment #17)
> This means that my patch is doing what the X11 protocol intended. 

Ok, so we simply replace it by XCB_INPUT_FOCUS_NONE when setting the input focus and your patch breaks. Convinced now?

Okay.  So do it.  You'll need to edit:

main_wayland.cpp
main_x11.cpp
xcbutils.h (two entries)

You do seem weirdly attached to this race condition in your code.  You seem to prefer the random outcome depending on system load and number of cores to a patch that may not be everything you want it to be but does at least provide a deterministic outcome.
Obviously it's been a few years since I professionally programmed real-time multi-CPU systems in assembler so I expect the rules have changed since then.

(In reply to Nicholas Redgrave from comment #19)
> Okay.  So do it.  You'll need to edit:

No. Please feel encouraged to test the patch on the dupe (I gave it a brief test only)

> You do seem ...
>  You seem to prefer ...

We simply do not "fix" bugs with a broken workaround that will just cause other bug reports.
I tried to guide you but apparently you seem to prefer™ defending your incorrect approach thatn just fixing it up.

*** This bug has been marked as a duplicate of bug 324782 ***