Merge lp:~compiz-team/compiz/compiz.fix_1008020 into lp:compiz/0.9.8

[Sam Spilsbury]

Don't insert the window into the server list above the window it was
  created above.

  The server list might have been modified by the time that we process the
  create event, and as such there is a case where a window can be inserted
  into the server list above another window and not at the top (the default
  for where windows are created) if that window is pending a restack.

  When updateAttributes is called later, putting it above the correct window
  will silently fail, because it is already there in the server list, even
  though a restack was never issued to put it there.

  (LP: #1008020) (LP: #886605)

We really need to do something about the fact that the stacking code is not under test. An email was sent about smoke testing window stacking for now until we can get some kind of sensible unit testing story.

I tested this against lp:~smspillaz/+junk/stackingtest (using proc test-stacking-ops which creates tons of new windows and reshuffles them, excersizing this code). Nothing went above panels or screensavers after the fixes were applied.

Case from bug 1008020 that I know this fixed:

1. Log in to unity-2d or gnome-classic
2. kill unity-2d-shell (so just the panel remains)
3. Open nautilus and repeatedly hit Ctrl-N to create a new window really quickly
4. Windows will eventually go above the panel

[Daniel van Vugt (vanvugt)]

Using stackingtest in a loop, I find windows appear over the top of the lock screen even with this fix. But only when using the animation plugin. Is that still what's being fixed here or a different issue?

I see exactly the same stacking behaviour with and without this fix...

review: Needs Information

[Daniel van Vugt (vanvugt)]

Also, there's a trivial conflict in abiversion.h.

[Sam Spilsbury (smspillaz)]

Could be a different issue. There is no conflict in abiversion.h according to lp

[Sam Spilsbury (smspillaz)]

(Have a look at bug 1008020 for the case that I can absolutely confirm this fixes, I wasn't sure if the screen lock one and this one were related as I can't reproduce it anyways [I hate these race conditions])

[Sam Spilsbury (smspillaz)]

> Using stackingtest in a loop, I find windows appear over the top of the lock
> screen even with this fix. But only when using the animation plugin. Is that
> still what's being fixed here or a different issue?
>
> I see exactly the same stacking behaviour with and without this fix...

A small question here: Do the windows appear temporarily on top of the lockscreen, or do they go away quickly? I know that we're supposed to create windows on top of fullscreen windows and it is the job of the fullscreen window to request that it goes on top of the other windows in that case.

[Daniel van Vugt (vanvugt)]

As far as I can tell, only the open/close animations are visible over the lock screen.

[Sam Spilsbury (smspillaz)]

In that case, there is no stacking bug (but a bug in the animation plugin for sure)

[Sam Spilsbury (smspillaz)]

(What happens if you turn down the speed of the animations too)

[Sam Spilsbury (smspillaz)]

I updated the description with the case I know this fixes for sure. We can unlinkbug 886605 if appropriate. I'd like to get feedback from cimi and racarr on that since they have macbook airs that can reproduce this.

[Alan Griffiths (alan-griffiths)]

There is a trivial conflict in abiversion.h

Also, as list::size() can be O(N)

117 + if (serverWindows.size ())

Should be:

117 + if (!serverWindows.empty ())

[Daniel van Vugt (vanvugt)]

I think we should be making new functions as useful as possible. That means returning CompWindow* instead of Window (None --> NULL):
  Window cps::WindowManager::getTopServerWindow()

If you just return Window and actually want a CompWindow then you have to waste CPU and more lines of code looking it up again.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

There is no CPU wastage since it hits the cached last found window. Im amy case the function cab be changed

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

null

[Daniel van Vugt (vanvugt)]

I'm not talking about this proposal. I mean potential wastage for anyone else trying to use the function in future.

[Sam Spilsbury (smspillaz)]

To be honest, we should move getTopWindow and getTopServerWindow to a more private place too.

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

I'm not talking about this proposal. I mean potential wastage for anyone else trying to use the function in future.
--
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1008020/+merge/109758
Your team Compiz Maintainers is subscribed to branch lp:compiz.

[Daniel van Vugt (vanvugt)]

I don't see any problems running this branch. But I also still can't reproduce any of the bugs we're aiming to fix here. So can't really verify the fixes.

Just some notes on style:
You have inconsistent use of white space in function (calls). Sometimes there's a space, sometimes not. I think we should switch to no white space like most coding conventions do: function(calls).

review: Approve

[Sam Spilsbury (smspillaz)]

Compiz coding style has always had a space between the caller and the brackets. i'd prefer to keep it that way

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

Review: Approve

I don't see any problems running this branch. But I also still can't reproduce any of the bugs we're aiming to fix here. So can't really verify the fixes.

Just some notes on style:
You have inconsistent use of white space in function (calls). Sometimes there's a space, sometimes not. I think we should switch to no white space like most coding conventions do: function(calls).

--
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1008020/+merge/110724
Your team Compiz Maintainers is subscribed to branch lp:compiz.

[Tim Penhey (thumper)]

You have functions that may return null pointers, and call sites where you are dereferencing without checking.

Something should be done there.

review: Needs Fixing

[Tim Penhey (thumper)]

I suggest something like:

inline Window window_from_compwindow(CompWindow* win)
{
  return win ? win->id () : None;
}

[Sam Spilsbury (smspillaz)]

> I suggest something like:
>
> inline Window window_from_compwindow(CompWindow* win)
> {
> return win ? win->id () : None;
> }

Thanks, will do

[Tim Penhey (thumper)]

Line 71 also needs the function treatment.

> PrivateWindow::createCompWindow (i ? children[i - 1] : 0, i ? children[i - 1] : 0, attrib, children[i]);

This is fugly.

Please do something the equivalent of:

CompWindow* top_window_id = i ? children[i - 1] : 0;
PrivateWindow::createCompWindow (top_window_id, top_window_id, attrib, children[i]);

If you had the CompWindowToWindow function defined in a header, you could use it in src/window.cpp instead of implementing the logic again.

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

> Line 71 also needs the function treatment.
>
> > PrivateWindow::createCompWindow (i ? children[i - 1] : 0, i ? children[i -
> 1] : 0, attrib, children[i]);
>
> This is fugly.
>
> Please do something the equivalent of:
>
> CompWindow* top_window_id = i ? children[i - 1] : 0;
> PrivateWindow::createCompWindow (top_window_id, top_window_id, attrib,
> children[i]);
>

It is not a CompWindow*, but I will still do

Window topWindowId = i ? children[i - 1] : 0;

> If you had the CompWindowToWindow function defined in a header, you could use
> it in src/window.cpp instead of implementing the logic again.

I do not like functions in headers, changing them means rebuilding everything that includes that header.

[Daniel van Vugt (vanvugt)]

Remember to use "None" instead of "0". :)

[Sam Spilsbury (smspillaz)]

0 == none but I can change it.

Sent from Samsung Mobile

 Daniel van Vugt <email address hidden> wrote:

Remember to use "None" instead of "0". :)
--
https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1008020/+merge/110724
Your team Compiz Maintainers is subscribed to branch lp:compiz.

[Daniel van Vugt (vanvugt)]

Looks good, works well, though I still can't reproduce the bugs.

Ideally, CompWindowToWindow (CompWindow *window) would be CompWindowToWindow (const CompWindow *window). But I know that's not possible because CompWindow::id() is not const (!)

review: Approve

=== modified file 'include/core/abiversion.h'
--- include/core/abiversion.h	2012-05-26 06:07:20 +0000
+++ include/core/abiversion.h	2012-06-20 05:34:30 +0000
@@ -5,6 +5,6 @@
 #  error Conflicting definitions of CORE_ABIVERSION
 #endif
 
-#define CORE_ABIVERSION 20120526
+#define CORE_ABIVERSION 20120603
 
 #endif // COMPIZ_ABIVERSION_H

=== modified file 'include/core/screen.h'
--- include/core/screen.h	2012-06-06 05:12:28 +0000
+++ include/core/screen.h	2012-06-20 05:34:30 +0000
@@ -401,7 +401,8 @@
     virtual void updatePassiveKeyGrabs () const = 0;
     virtual void applyStartupProperties (CompWindow *window) = 0;
     virtual void updateClientList() = 0;
-    virtual Window getTopWindow() const = 0;
+    virtual CompWindow * getTopWindow() const = 0;
+    virtual CompWindow * getTopServerWindow() const = 0;
     virtual CoreOptions& getCoreOptions() = 0;
     virtual Colormap colormap() const = 0;
     virtual void setCurrentDesktop (unsigned int desktop) = 0;

=== modified file 'include/core/window.h'
--- include/core/window.h	2012-05-04 09:29:44 +0000
+++ include/core/window.h	2012-06-20 05:34:30 +0000
@@ -569,6 +569,7 @@
     private:
 
 	CompWindow (Window	      aboveId,
+		    Window	      aboveServerId,
 		    XWindowAttributes &wa,
 		    PrivateWindow     *priv);
 

=== modified file 'src/event.cpp'
--- src/event.cpp	2012-06-06 05:12:28 +0000
+++ src/event.cpp	2012-06-20 05:34:30 +0000
@@ -44,6 +44,14 @@
 
 namespace cps = compiz::private_screen;
 
+namespace
+{
+Window CompWindowToWindow (CompWindow *window)
+{
+    return window ? window->id () : None;
+}
+}
+
 
 bool
 PrivateWindow::handleSyncAlarm ()
@@ -1225,7 +1233,7 @@
 	     * that to wait until the map request */
 	    if (wa.root == privateScreen.rootWindow())
 	    {
-		PrivateWindow::createCompWindow (getTopWindow (), wa, event->xcreatewindow.window);
+		PrivateWindow::createCompWindow (CompWindowToWindow (getTopWindow ()), CompWindowToWindow (getTopServerWindow ()), wa, event->xcreatewindow.window);
             }
 	    else
 		XSelectInput (privateScreen.dpy, event->xcreatewindow.window,
@@ -1361,7 +1369,7 @@
 		if (!XGetWindowAttributes (privateScreen.dpy, event->xcreatewindow.window, &wa))
 		    privateScreen.setDefaultWindowAttributes (&wa);
 
-		PrivateWindow::createCompWindow (getTopWindow (), wa, event->xcreatewindow.window);
+		PrivateWindow::createCompWindow (getTopWindow ()->id (), getTopServerWindow ()->id (), wa, event->xcreatewindow.window);
 		break;
 	    }
 	    else

=== modified file 'src/privatescreen.h'
--- src/privatescreen.h	2012-06-06 05:12:28 +0000
+++ src/privatescreen.h	2012-06-20 05:34:30 +0000
@@ -183,7 +183,8 @@
 	    { return clientListStacking; }
 
 	CompWindow * findWindow (Window id) const;
-	Window getTopWindow() const;
+	CompWindow * getTopWindow() const;
+	CompWindow * getTopServerWindow() const;
 
 
 	void removeFromFindWindowCache(CompWindow* w)
@@ -1075,7 +1076,8 @@
 	virtual bool displayInitialised() const;
 	virtual void applyStartupProperties (CompWindow *window);
 	virtual void updateClientList();
-	virtual Window getTopWindow() const;
+	virtual CompWindow * getTopWindow() const;
+	virtual CompWindow * getTopServerWindow() const;
 	virtual CoreOptions& getCoreOptions();
 	virtual Colormap colormap() const;
 	virtual void setCurrentDesktop (unsigned int desktop);

=== modified file 'src/privatescreen/tests/test-privatescreen.cpp'
--- src/privatescreen/tests/test-privatescreen.cpp	2012-06-10 11:06:37 +0000
+++ src/privatescreen/tests/test-privatescreen.cpp	2012-06-20 05:34:30 +0000
@@ -185,7 +185,8 @@
     MOCK_CONST_METHOD0(displayInitialised, bool ());
     MOCK_METHOD1(applyStartupProperties, void (CompWindow *window));
     MOCK_METHOD0(updateClientList, void ());
-    MOCK_CONST_METHOD0(getTopWindow, Window ());
+    MOCK_CONST_METHOD0(getTopWindow, CompWindow * ());
+    MOCK_CONST_METHOD0(getTopServerWindow, CompWindow * ());
     MOCK_METHOD0(getCoreOptions, CoreOptions& ());
     MOCK_CONST_METHOD0(colormap, Colormap ());
     MOCK_METHOD1(setCurrentDesktop, void (unsigned int desktop));

=== modified file 'src/privatewindow.h'
--- src/privatewindow.h	2012-05-27 04:32:55 +0000
+++ src/privatewindow.h	2012-06-20 05:34:30 +0000
@@ -283,7 +283,7 @@
 
 	bool checkClear ();
 
-	static CompWindow* createCompWindow (Window aboveId, XWindowAttributes &wa, Window id);
+	static CompWindow* createCompWindow (Window aboveId, Window aboveServerId, XWindowAttributes &wa, Window id);
     public:
 
 	PrivateWindow *priv;

=== modified file 'src/screen.cpp'
--- src/screen.cpp	2012-06-18 06:22:29 +0000
+++ src/screen.cpp	2012-06-20 05:34:30 +0000
@@ -4097,14 +4097,23 @@
 	XUnmapWindow (dpy, screenEdge[edge].id);
 }
 
-Window
+CompWindow *
 cps::WindowManager::getTopWindow() const
 {
     /* return first window that has not been destroyed */
-    if (windows.size ())
-	return windows.back ()->id ();
-
-    return None;
+    if (!windows.empty ())
+	return windows.back ();
+
+    return NULL;
+}
+
+CompWindow *
+cps::WindowManager::getTopServerWindow () const
+{
+    if (!serverWindows.empty ())
+	return serverWindows.back ();
+
+    return NULL;
 }
 
 int
@@ -4858,12 +4867,18 @@
     privateScreen.updateClientList ();
 }
 
-Window
+CompWindow *
 CompScreenImpl::getTopWindow() const
 {
     return windowManager.getTopWindow();
 }
 
+CompWindow *
+CompScreenImpl::getTopServerWindow () const
+{
+    return windowManager.getTopServerWindow();
+}
+
 CoreOptions&
 CompScreenImpl::getCoreOptions()
 {
@@ -5292,7 +5307,9 @@
 	if (!XGetWindowAttributes (screen->dpy (), children[i], &attrib))
 	    setDefaultWindowAttributes(&attrib);
 
-	PrivateWindow::createCompWindow (i ? children[i - 1] : 0, attrib, children[i]);
+	Window topWindowInTree = i ? children[i - 1] : None;
+
+	PrivateWindow::createCompWindow (topWindowInTree, topWindowInTree, attrib, children[i]);
     }
 
     /* enforce restack on all windows

=== modified file 'src/window.cpp'
--- src/window.cpp	2012-05-27 04:32:55 +0000
+++ src/window.cpp	2012-06-20 05:34:30 +0000
@@ -1205,7 +1205,7 @@
 
 	    /* Put the frame window "above" the client window
 	     * in the stack */
-	    PrivateWindow::createCompWindow (priv->id, attrib, priv->serverFrame);
+	    PrivateWindow::createCompWindow (priv->id, priv->id, attrib, priv->serverFrame);
 	}
 
 	/* Immediately unhook the window once destroyed
@@ -1890,7 +1890,10 @@
     Window newAboveId;
 
     if (ce->place == PlaceOnTop)
-	newAboveId = screen->getTopWindow ();
+    {
+	CompWindow *newAbove = screen->getTopWindow ();
+	newAboveId = newAbove ? newAbove->id () : None;
+    }
     else
 	newAboveId = 0;
 
@@ -5969,19 +5972,20 @@
 }
 
 CompWindow *
-PrivateWindow::createCompWindow (Window aboveId, XWindowAttributes &wa, Window id)
+PrivateWindow::createCompWindow (Window aboveId, Window aboveServerId, XWindowAttributes &wa, Window id)
 {
     PrivateWindow* priv(new PrivateWindow ());
     priv->id = id;
     priv->serverId = id;
 
-    CompWindow *fw = new CompWindow (aboveId, wa, priv);
+    CompWindow *fw = new CompWindow (aboveId, aboveServerId, wa, priv);
 
     return fw;
 }
 
 
 CompWindow::CompWindow (Window aboveId,
+			Window aboveServerId,
 			XWindowAttributes &wa,
 			PrivateWindow *priv) :
     PluginClassStorage (windowPluginClassIndices),
@@ -5994,7 +5998,7 @@
     priv->window = this;
 
     screen->insertWindow (this, aboveId);
-    screen->insertServerWindow (this, aboveId);
+    screen->insertServerWindow (this, aboveServerId);
 
     /* We must immediately insert the window into the debugging
      * stack */
@@ -6896,7 +6900,7 @@
 
 	/* Put the frame window "above" the client window
 	 * in the stack */
-	PrivateWindow::createCompWindow (id, attrib, serverFrame);
+	PrivateWindow::createCompWindow (id, id, attrib, serverFrame);
     }
 
     /* Issue a DestroyNotify */