Merge lp:~azzar1/compiz/fix-781931 into lp:compiz/0.9.9

[Andrea Azzarone]

== Problem ==
Bug #781931: New windows are moved to front but don't take focus.

When a window is not allowed to get the focus Compiz should stack it below the active window. Due to avoidStackingRelativeTo Compiz fails to do it:

bool
PrivateWindow::avoidStackingRelativeTo (CompWindow *w)
{
    if (w->overrideRedirect ())
 return true;

    if (w->destroyed ())
 return true;

    if (!w->priv->shaded && !w->priv->pendingMaps)
    {
 if (!w->isViewable () || !w->isMapped ())
     return true;
    }

    return false;
}

This is because avoidStackingRelativeTo is called before the window get mapped, so it returns TRUE.

== Fix ==
This branch adds receivedMapRequestAndAwaitingMap.

== Test ==
Xorg-gtest added.

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3486
http://jenkins.qa.ubuntu.com/job/compiz-ci/272/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/272/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/272//rebuild/?

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

This feels a bit weird in its current state and could unintentionally cause other problems. Give me a day to look through some of the surrounding code before we merge this.

review: Abstain

[Sam Spilsbury (smspillaz)]

OK, I just need a little bit of clarification here:

avoidStackingRelativeTo is used to determine whether or not a sibling is not a good candidate to put a window above, so for newly mapped windows we won't put those above windows that are:

1. override redirect
2. pending destruction
3. unmapped, where those windows are also not shaded or waiting the event from an XMapWindow

It shouldn't ever be called on the window that is being restacked.

Now one situation where you might have broken behaviour is where you've got one window that is not yet mapped (because show () was never called on it), and then a new window is created and is put underneath it (because it isn't yet mapped). Then show () is called on the first window a little later

The only place where avoidStackingRelativeTo with the window as the "sibling" parameter is called is here:

CompWindow *
PrivateWindow::findValidStackSiblingBelow (CompWindow *w,
        CompWindow *sibling)
{
    CompWindow *lowest, *last, *p;

    /* check whether we're allowed to stack under a sibling by finding
     * the above 'sibling' and checking whether or not we're allowed
     * to stack under that - if not, then there is no valid sibling
     * underneath it */

    for (p = sibling; p; p = p->serverNext)
    {
 if (!avoidStackingRelativeTo (p))
 {
     if (!validSiblingBelow (p, w))
  return NULL;
     break;
 }
    }

And that would probably be called from here in updateAttributes:

 if (sibling &&
     (stackingMode == CompStackingUpdateModeInitialMapDeniedFocus))
 {
     CompWindow *p;

     for (p = sibling; p; p = p->serverPrev)
  if (p->priv->id == screen->activeWindow ())
      break;

     /* window is above active window so we should lower it,
      * assuing that is allowed (if, for example, our window has
      * the "above" state, then lowering beneath the active
      * window may not be allowed). */
     if (p && PrivateWindow::validSiblingBelow (p, this))

Where we try and find /a/ valid sibling below the current window that we can go above, and then try and go underneath the active window by finding a window beneath that which we can go above.

I still feel like this is strange:

    for (p = sibling; p; p = p->serverNext)
    {
 if (!avoidStackingRelativeTo (p))
 {
     if (!validSiblingBelow (p, w))
  return NULL;
     break;
 }
    }

That was introduced here:

------------------------------------------------------------
revno: 2609.6.31
committer: Sam Spilsbury <email address hidden>
timestamp: Tue 2010-12-07 17:08:11 +0800
message:
  Make sure findValidStackSibling below doesn't return a sibling that
  we're not allowed to stack under.

  Forward port 1baf514fefb9177764af99cf508596686583244e to master

Which is a port of:

commit 1baf514fefb9177764af99cf508596686583244e
Author: Danny Baumann <email address hidden>
Date: Mon Aug 31 09:31:43 2009 +0200

    Make sure findValidStackSibling below doesn't return a sibling that
    we're not allowed to stack under.
    If the passed sibling window is not allowed to stack under the passed
    window, returning any window under sibling would result in an invalid
    stacking order.

diff --git a/src/window.c b/src/window.c
...

[Sam Spilsbury (smspillaz)]

(Also I forgot to mention - thank you very much for looking into this, its been on my list for a while and I've been stumped every time I looked at it, so we're on the right track).

[Sam Spilsbury (smspillaz)]

Ah okay, now that I've had a look into this, this comment here is misleading:

    /* check whether we're allowed to stack under a sibling by finding
     * the above 'sibling' and checking whether or not we're allowed
     * to stack under that - if not, then there is no valid sibling
     * underneath it */

It really should say

    /* check whether we're allowed to stack under a sibling checking if
     * this window, or any other window above it is permitted to stack
     * above this window */

So I think you're definitely on the right track.

My only concern is that changing pendingMaps might have some unwanted side effects. Code that relies on pendingMaps could assume that show () can been called on the window, which has a few other invariants too (eg, not hidden, not shaded etc).

What might be better is to add a new variable to track whether or not the window is currently in map processing (eg, receivedMapRequestAndAwaitingMap). Then you can check that variable in avoidStackingRelativeTo in order to disregard the fact that the window is actually unmapped. Ideally receivedMapRequestAndAwaitingMap should only be set if we were to actually call show () on the window later (eg, !initiallyMinimized && !(priv->state & CompWindowStateHiddenMask))

[Sam Spilsbury (smspillaz)]

(I note that writing tests with xorg-gtest won't work right now until https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1084096/+merge/136676 is merged. CMake appears to have changed the behaviour of execute_process to require the caller to strip whitespace from the command output, and its build is currently broken)

[Andrea Azzarone (azzar1)]

> Ah okay, now that I've had a look into this, this comment here is misleading:
>
> /* check whether we're allowed to stack under a sibling by finding
> * the above 'sibling' and checking whether or not we're allowed
> * to stack under that - if not, then there is no valid sibling
> * underneath it */
>
> It really should say
>
> /* check whether we're allowed to stack under a sibling checking if
> * this window, or any other window above it is permitted to stack
> * above this window */
>

I agree. I will update the comment too.

>
> So I think you're definitely on the right track.
>
> My only concern is that changing pendingMaps might have some unwanted side
> effects. Code that relies on pendingMaps could assume that show () can been
> called on the window, which has a few other invariants too (eg, not hidden,
> not shaded etc).
>
> What might be better is to add a new variable to track whether or not the
> window is currently in map processing (eg, receivedMapRequestAndAwaitingMap).
> Then you can check that variable in avoidStackingRelativeTo in order to
> disregard the fact that the window is actually unmapped. Ideally
> receivedMapRequestAndAwaitingMap should only be set if we were to actually
> call show () on the window later (eg, !initiallyMinimized && !(priv->state &
> CompWindowStateHiddenMask))

Thank you for the review/suggestion. It shounds good to me and make the code more understandable.

Btw I'm working on an xorg-test.

[Sam Spilsbury (smspillaz)]

I am liking this a lot :)

21 + if (!w->priv->receivedMapRequestAndAwaitingMap && !w->priv->shaded && !w->priv->pendingMaps)
22 {

You can probably put those on separate lines, or even

bool allowRelativeToUnmmaped = w->priv->receivedMapRequestAndAwaitingMap ||
                               w->priv->shaded ||
                               w->priv->pendingMaps;

if (!allowRelativeToUnmapped)
...

78 + void SetUserTime (Display *dpy, Window w, Time time)
79 + {
80 + Atom _NET_WM_USER_TIME = XInternAtom (dpy, "_NET_WM_USER_TIME", false);
81 + unsigned int value = (unsigned int) time;
82 +
83 + XChangeProperty (dpy, w,
84 + _NET_WM_USER_TIME,
85 + XA_CARDINAL, 32, PropModeReplace,
86 + (unsigned char *) &value, 1);
87 + }
88 +
89 + void SetClientLeader (Display *dpy, Window w, Window leader)
90 + {
91 + Atom WM_CLIENT_LEADER = XInternAtom (dpy, "WM_CLIENT_LEADER", false);
92 +
93 + XChangeProperty (dpy, w,
94 + WM_CLIENT_LEADER,
95 + XA_WINDOW, 32, PropModeReplace,
96 + (unsigned char *) &leader, 1);
97 + }
98 +

Thanks for providing these :)

Good use of ASSERT_TRUE (ct::WaitForEventOfTypeOnWindowMatching (dpy, DefaultRootWindow (dpy), PropertyNotify, -1, -1, matcher));

All in all I am liking this a lot :)

review: Approve

[Sam Spilsbury (smspillaz)]

As above, nailed it, well done.

review: Approve

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3490
http://jenkins.qa.ubuntu.com/job/compiz-ci/276/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/276/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/276//rebuild/?

review: Approve (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3490
http://jenkins.qa.ubuntu.com/job/compiz-ci/277/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/277/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/277//rebuild/?

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

(A note that Andrea mentioned to me on IRC that he is going to convert the compound if statement into a bool, so probably don't merge this until then).

[lp:~azzar1/compiz/fix-781931]

3491. By Andrea Azzarone on 2012-11-29

Minor change.

=== modified file 'src/privatewindow.h'
--- src/privatewindow.h	2012-09-10 15:17:57 +0000
+++ src/privatewindow.h	2012-11-29 08:18:24 +0000
@@ -360,6 +360,7 @@
 	typedef std::pair <XWindowChanges, unsigned int> XWCValueMask;
 
 	compiz::X11::PendingEventQueue pendingConfigures;
+	bool receivedMapRequestAndAwaitingMap;
 
 	char *startupId;
 	char *resName;

=== modified file 'src/window.cpp'
--- src/window.cpp	2012-11-15 05:47:13 +0000
+++ src/window.cpp	2012-11-29 08:18:24 +0000
@@ -2611,7 +2611,11 @@
     if (w->destroyed ())
 	return true;
 
-    if (!w->priv->shaded && !w->priv->pendingMaps)
+    bool allowRelativeToUnmmaped = w->priv->receivedMapRequestAndAwaitingMap ||
+				   w->priv->shaded ||
+				   w->priv->pendingMaps;
+
+    if (!allowRelativeToUnmmaped)
     {
 	if (!w->isViewable () || !w->isMapped ())
 	    return true;
@@ -5424,6 +5428,9 @@
 
     priv->managed = true;
 
+    if (!initiallyMinimized && !(priv->state & CompWindowStateHiddenMask))
+	receivedMapRequestAndAwaitingMap = true;
+
     if (!priv->placed)
     {
 	int            gravity = priv->sizeHints.win_gravity;
@@ -5474,7 +5481,10 @@
 	    screen->setCurrentDesktop (priv->desktop);
 
 	if (!(priv->state & CompWindowStateHiddenMask))
+	{
 	    show ();
+	    receivedMapRequestAndAwaitingMap = false;
+	 }
 
 	if (allowFocus)
 	{
@@ -6244,6 +6254,7 @@
     pendingUnmaps (0),
     pendingMaps (0),
     pendingConfigures (screen->dpy ()),
+    receivedMapRequestAndAwaitingMap (false),
 
     startupId (0),
     resName (0),

=== modified file 'tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp'
--- tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp	2012-09-18 16:53:59 +0000
+++ tests/system/xorg-gtest/tests/compiz_xorg_gtest_test_window_stacking.cpp	2012-11-29 08:18:24 +0000
@@ -91,6 +91,27 @@
 	return w;
     }
 
+    void SetUserTime (Display *dpy, Window w, Time time)
+    {
+	Atom _NET_WM_USER_TIME = XInternAtom (dpy, "_NET_WM_USER_TIME", false);
+	unsigned int value = (unsigned int) time;
+
+	XChangeProperty (dpy, w,
+			 _NET_WM_USER_TIME,
+			 XA_CARDINAL, 32, PropModeReplace,
+			 (unsigned char *) &value, 1);
+    }
+
+    void SetClientLeader (Display *dpy, Window w, Window leader)
+    {
+	Atom WM_CLIENT_LEADER = XInternAtom (dpy, "WM_CLIENT_LEADER", false);
+
+	XChangeProperty (dpy, w,
+			 WM_CLIENT_LEADER,
+			 XA_WINDOW, 32, PropModeReplace,
+			 (unsigned char *) &leader, 1);
+    }
+
     class PropertyNotifyXEventMatcher :
 	public ct::XEventMatcher
     {
@@ -215,3 +236,69 @@
     EXPECT_EQ (*it++, w2); /* second should be the top normal window */
     EXPECT_EQ (*it++, dock); /* dock must always be on top */
 }
+
+TEST_F (CompizXorgSystemStackingTest, TestMapWindowWithOldUserTime)
+{
+    ::Display *dpy = Display ();
+    PropertyNotifyXEventMatcher matcher (dpy, "_NET_CLIENT_LIST_STACKING");
+
+    Window w1 = CreateNormalWindow (dpy);
+    Window w2 = CreateNormalWindow (dpy);
+    Window w3 = CreateNormalWindow (dpy);
+
+    XMapRaised (dpy, w1);
+    XMapRaised (dpy, w2);
+
+    /* Wait for property change notify on the root window to happen twice */
+    ASSERT_TRUE (ct::WaitForEventOfTypeOnWindowMatching (dpy, DefaultRootWindow (dpy), PropertyNotify, -1, -1, matcher));
+    ASSERT_TRUE (ct::WaitForEventOfTypeOnWindowMatching (dpy, DefaultRootWindow (dpy), PropertyNotify, -1, -1, matcher));
+
+    SetUserTime (dpy, w2, 200);
+    SetClientLeader (dpy, w2, w2);
+    SetUserTime (dpy, w3, 100);
+    SetClientLeader (dpy, w3, w3);
+
+    XMapRaised (dpy, w3);
+    ASSERT_TRUE (ct::WaitForEventOfTypeOnWindowMatching (dpy, DefaultRootWindow (dpy), PropertyNotify, -1, -1, matcher));
+
+    /* Check the client list to see that w2 > w3 > w1 */
+    std::list <Window> clientList = ct::NET_CLIENT_LIST_STACKING (dpy);
+    ASSERT_EQ (clientList.size (), 3);
+
+    std::list <Window>::iterator it = clientList.begin ();
+    EXPECT_EQ (*it++, w1);
+    EXPECT_EQ (*it++, w3);
+    EXPECT_EQ (*it++, w2);
+}
+
+TEST_F (CompizXorgSystemStackingTest, TestMapWindowAndDenyFocus)
+{
+    ::Display *dpy = Display ();
+    PropertyNotifyXEventMatcher matcher (dpy, "_NET_CLIENT_LIST_STACKING");
+
+    Window w1 = CreateNormalWindow (dpy);
+    Window w2 = CreateNormalWindow (dpy);
+    Window w3 = CreateNormalWindow (dpy);
+
+    XMapRaised (dpy, w1);
+    XMapRaised (dpy, w2);
+
+    /* Wait for property change notify on the root window to happen twice */
+    ASSERT_TRUE (ct::WaitForEventOfTypeOnWindowMatching (dpy, DefaultRootWindow (dpy), PropertyNotify, -1, -1, matcher));
+    ASSERT_TRUE (ct::WaitForEventOfTypeOnWindowMatching (dpy, DefaultRootWindow (dpy), PropertyNotify, -1, -1, matcher));
+
+    SetUserTime (dpy, w3, 0);
+    SetClientLeader (dpy, w3, w3);
+
+    XMapRaised (dpy, w3);
+    ASSERT_TRUE (ct::WaitForEventOfTypeOnWindowMatching (dpy, DefaultRootWindow (dpy), PropertyNotify, -1, -1, matcher));
+
+    /* Check the client list to see that w2 > w3 > w1 */
+    std::list <Window> clientList = ct::NET_CLIENT_LIST_STACKING (dpy);
+    ASSERT_EQ (clientList.size (), 3);
+
+    std::list <Window>::iterator it = clientList.begin ();
+    EXPECT_EQ (*it++, w1);
+    EXPECT_EQ (*it++, w3);
+    EXPECT_EQ (*it++, w2);
+}