Merge lp:~compiz-team/compiz/compiz.fix_1138517 into lp:compiz/0.9.9

[Sam Spilsbury]

Don't set lastFrameExtents unless the window geometry actually changed - as
that variable only exists to track changes in the actual geometry of
the window and not the apparant frame extents.

Added tests to verify that behaviour.

(LP: #1138517)

Don't set lastFrameExtents unless the window geometry actually changed - as
that variable only exists to track changes in the actual geometry of
the window and not the apparant frame extents.

Added tests to verify that behaviour.

(LP: #1138517)

[MC Return (mc-return)]

+1.

review: Approve

[Timo Jyrinki (timo-jyrinki)]

With this branch I couldn't resize or move Java windows (like jEdit) anymore. Reverting to the archive fixed the issue.

review: Disapprove

[Sam Spilsbury (smspillaz)]

I'm sure you meant "needs fixing" not disapprove. In any case I can have
another look into it. I didn't test with jedit but java is such a bloody
disaster that I'm not even sure I care anymore.
On 11/03/2013 1:39 PM, "Timo Jyrinki" <email address hidden> wrote:

> Review: Disapprove
>
> With this branch I couldn't resize or move Java windows (like jEdit)
> anymore. Reverting to the archive fixed the issue.
> --
>
> https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1138517/+merge/151941
> Your team Compiz Maintainers is subscribed to branch lp:compiz.
>

[Timo Jyrinki (timo-jyrinki)]

(or: Needs Fixing .. the problematic bzr 3616 was already reverted, but of course a proper fix could be nicer anyhow so that the 3616 could be resubmitted)

[Timo Jyrinki (timo-jyrinki)]

Sure, Needs Fixing it is.

review: Needs Fixing

[lp:~compiz-team/compiz/compiz.fix_1138517]

3632. By Sam Spilsbury on 2013-03-11

Only set the decoration if we actually meant to decorate the window.

This allows us to set frame extents and not set the whole decoration
property

[Andrea Azzarone (azzar1)]

review: Approve

[Andrea Azzarone (azzar1)]

Ops wrong branch.

[Andrea Azzarone (azzar1)]

review: Abstain

Unmerged revisions

=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2013-02-26 01:08:15 +0000
+++ plugins/decor/src/decor.cpp	2013-03-11 06:34:23 +0000
@@ -1585,35 +1585,44 @@
      */
     if (decoration)
     {
-	wd = WindowDecoration::create (decoration);
-	if (!wd)
-	    return false;
-
 	/* Set extents based on maximize/unmaximize state
 	 * FIXME: With the new type system, this should be
 	 * removed */
 	if ((window->state () & MAXIMIZE_STATE))
-	    window->setWindowFrameExtents (&wd->decor->maxBorder,
-					   &wd->decor->maxInput);
+	    window->setWindowFrameExtents (&decoration->maxBorder,
+					   &decoration->maxInput);
 	else if (!window->hasUnmapReference ())
-	    window->setWindowFrameExtents (&wd->decor->border,
-					   &wd->decor->input);
-
-	movement = compiz::window::extents::shift (window->border (), window->sizeHints ().win_gravity);
-	movement -= oldShift;
-
-	/* Update the input and output frame */
+	    window->setWindowFrameExtents (&decoration->border,
+					   &decoration->input);
+
+	/* We actually need to decorate this window */
 	if (decorate)
+	{
+	    wd = WindowDecoration::create (decoration);
+	    if (!wd)
+	    {
+		/* Error condition, reset frame extents */
+		CompWindowExtents emptyExtents;
+		memset (&emptyExtents, 0, sizeof (CompWindowExtents));
+		window->setWindowFrameExtents (&emptyExtents, &emptyExtents);
+		return false;
+	    }
+
+	    movement = compiz::window::extents::shift (window->border (), window->sizeHints ().win_gravity);
+	    movement -= oldShift;
+
+	    /* Update the input and output frame */
 	    updateFrame ();
-	window->updateWindowOutputExtents ();
+	    window->updateWindowOutputExtents ();
 
-	updateReg = true;
-	updateMatrix = true;
-	mOutputRegion = CompRegion (window->outputRect ());
-	updateGroupShadows ();
-	if (dScreen->cmActive)
-	    cWindow->damageOutputExtents ();
-	updateDecorationScale ();
+	    updateReg = true;
+	    updateMatrix = true;
+	    mOutputRegion = CompRegion (window->outputRect ());
+	    updateGroupShadows ();
+	    if (dScreen->cmActive)
+		cWindow->damageOutputExtents ();
+	    updateDecorationScale ();
+	}
     }
     else
     {

=== modified file 'src/privatewindow.h'
--- src/privatewindow.h	2013-02-10 05:01:50 +0000
+++ src/privatewindow.h	2013-03-11 06:34:23 +0000
@@ -155,7 +155,7 @@
 
 	void recalcNormalHints ();
 
-	void updateFrameWindow ();
+	bool updateFrameWindow ();
 
 	void setWindowMatrix ();
 
@@ -302,7 +302,7 @@
 						       int            gravity,
 						       int	      direction);
 
-	void updateSize ();
+	bool updateSize ();
 
 	bool getUserTime (Time& time);
 	void setUserTime (Time time);

=== modified file 'src/window.cpp'
--- src/window.cpp	2013-02-27 03:24:45 +0000
+++ src/window.cpp	2013-03-11 06:34:23 +0000
@@ -807,12 +807,11 @@
     priv->type = type;
 }
 
-
-void
+bool
 PrivateWindow::updateFrameWindow ()
 {
     if (!serverFrame)
-	return;
+	return false;
 
     XWindowChanges xwc = XWINDOWCHANGES_INIT;
     unsigned int   valueMask = CWX | CWY | CWWidth | CWHeight;
@@ -826,6 +825,8 @@
     window->configureXWindow (valueMask, &xwc);
     window->windowNotify (CompWindowNotifyFrameUpdate);
     window->recalcActions ();
+
+    return true;
 }
 
 
@@ -4109,11 +4110,11 @@
 	priv->placed = true;
 }
 
-void
+bool
 PrivateWindow::updateSize ()
 {
     if (window->overrideRedirect () || !managed)
-	return;
+	return false;
 
     XWindowChanges xwc = XWINDOWCHANGES_INIT;
 
@@ -4124,7 +4125,10 @@
 	    window->sendSyncRequest ();
 
 	window->configureXWindow (mask, &xwc);
+	return true;
     }
+
+    return false;
 }
 
 int
@@ -6780,8 +6784,10 @@
 
 	recalcActions ();
 
-	priv->updateSize ();
-	priv->updateFrameWindow ();
+	bool sizeUpdated = false;
+
+	sizeUpdated |= priv->updateSize ();
+	sizeUpdated |= priv->updateFrameWindow ();
 
 	/* Always send a moveNotify
 	 * whenever the frame extents update
@@ -6789,7 +6795,8 @@
 	moveNotify (0, 0, true);
 
 	/* Once we have updated everything, re-set lastServerInput */
-	priv->lastServerInput = priv->serverInput;
+	if (sizeUpdated)
+	    priv->lastServerInput = priv->serverInput;
     }
 
     /* Use b for _NET_WM_FRAME_EXTENTS here because
@@ -6909,6 +6916,9 @@
      * but that's all */
     XSelectInput (dpy, screen->root (), SubstructureNotifyMask);
 
+    /* Gravity here is assumed to be SouthEast, clients can update
+     * that if need be */
+
     /* Awaiting a new frame to be given to us */
     frame = None;
     serverFrame = XCreateWindow (dpy, screen->root (), 0, 0,
@@ -6918,10 +6928,17 @@
     /* Do not get any events from here on */
     XSelectInput (dpy, screen->root (), NoEventMask);
 
-    wrapper = XCreateWindow (dpy, serverFrame, 0, 0,
-			    wa.width, wa.height, 0, wa.depth,
+    /* If we have some frame extents, we should apply them here and
+     * set lastFrameExtents */
+    wrapper = XCreateWindow (dpy, serverFrame,
+			    serverInput.left, serverInput.top,
+			    wa.width - (serverInput.left + serverInput.right),
+			    wa.height - (serverInput.top + serverInput.bottom),
+			    0, wa.depth,
 			    InputOutput, visual, mask, &attr);
 
+    lastServerInput = serverInput;
+
     xwc.stack_mode = Above;
 
     /* Look for the client in the current server side stacking

=== modified file 'tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp'
--- tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp	2013-02-26 00:52:32 +0000
+++ tests/system/xorg-gtest/tests/compiz_xorg_gtest_configure_window.cpp	2013-03-11 06:34:23 +0000
@@ -55,27 +55,41 @@
     return ct::AdvanceToNextEventOnSuccess (d, r);
 }
 
+Window GetImmediateParent (Display *display,
+			   Window  w,
+			   Window  &rootReturn)
+{
+    Window parentReturn = w;
+    Window *childrenReturn;
+    unsigned int nChildrenReturn;
+
+    XQueryTree (display,
+		w,
+		&rootReturn,
+		&parentReturn,
+		&childrenReturn,
+		&nChildrenReturn);
+    XFree (childrenReturn);
+
+    return parentReturn;
+}
+
+
 Window GetTopParent (Display *display,
 		     Window w)
 {
-    Window rootReturn;
+    Window rootReturn = 0;
     Window parentReturn = w;
-    Window *childrenReturn;
-    unsigned int nChildrenReturn;
-
     Window lastParent = 0;
 
     do
     {
 	lastParent = parentReturn;
 
-	XQueryTree (display,
-		    lastParent,
-		    &rootReturn,
-		    &parentReturn,
-		    &childrenReturn,
-		    &nChildrenReturn);
-	XFree (childrenReturn);
+	parentReturn = GetImmediateParent (display,
+					   lastParent,
+					   rootReturn);
+	
     } while (parentReturn != rootReturn);
 
     return lastParent;
@@ -565,3 +579,138 @@
 				   currentWidth + (left + right),
 				   currentHeight + (top + bottom)));
 }
+
+TEST_F (CompizXorgSystemConfigureWindowTest, SetFrameExtentsUnmapped)
+{
+    ::Display *dpy = Display ();
+
+    Window client = ct::CreateNormalWindow (dpy);
+    WaitForWindowCreation (client);
+
+    /* Set frame extents and get a response */
+    int left = 1;
+    int right = 1;
+    int top = 1;
+    int bottom = 1;
+
+    int currentX, currentY;
+    unsigned int currentWidth, currentHeight;
+    ASSERT_TRUE (QueryGeometry (dpy,
+				client,
+				currentX,
+				currentY,
+				currentWidth,
+				currentHeight));
+
+    /* We should get a response with our frame extents but it shouldn't actually
+     * do anything to the client as it is unmapped */
+    SendSetFrameExtentsRequest (client, left, right, top, bottom);
+    ASSERT_TRUE (VerifySetFrameExtentsResponse (client, left, right, top, bottom));
+
+    ASSERT_TRUE (VerifyWindowSize (client,
+				   currentX,
+				   currentY,
+				   currentWidth,
+				   currentHeight));
+}
+
+TEST_F (CompizXorgSystemConfigureWindowTest, SetFrameExtentsCorrectMapBehaviour)
+{
+    ::Display *dpy = Display ();
+
+    Window client = ct::CreateNormalWindow (dpy);
+    WaitForWindowCreation (client);
+
+    /* Set frame extents and get a response */
+    int left = 1;
+    int right = 1;
+    int top = 1;
+    int bottom = 1;
+
+    int currentX, currentY;
+    unsigned int currentWidth, currentHeight;
+    ASSERT_TRUE (QueryGeometry (dpy,
+				client,
+				currentX,
+				currentY,
+				currentWidth,
+				currentHeight));
+
+    SendSetFrameExtentsRequest (client, left, right, top, bottom);
+    ASSERT_TRUE (VerifySetFrameExtentsResponse (client, left, right, top, bottom));
+
+    /* Map the window */
+    XMapRaised (dpy, client);
+    WaitForReparentAndMap (dpy, client);
+
+    /* Check the geometry of the frame */
+    Window frame = GetTopParent (dpy, client);
+    ASSERT_TRUE (VerifyWindowSize (frame,
+				   currentX,
+				   currentY,
+				   currentWidth + (left + right),
+				   currentHeight + (top + bottom)));
+
+    /* Wrapper geometry is extents.xy, size.wh */
+    Window root;
+    Window wrapper = GetImmediateParent (dpy, client, root);
+    ASSERT_TRUE (VerifyWindowSize (wrapper,
+				   left,
+				   top,
+				   currentWidth,
+				   currentHeight));
+}
+
+TEST_F (CompizXorgSystemConfigureWindowTest, SetFrameExtentsConsistentBehaviourAfterMap)
+{
+    ::Display *dpy = Display ();
+
+    Window client = ct::CreateNormalWindow (dpy);
+    WaitForWindowCreation (client);
+
+    /* Set frame extents and get a response */
+    int left = 1;
+    int right = 1;
+    int top = 1;
+    int bottom = 1;
+
+    int currentX, currentY;
+    unsigned int currentWidth, currentHeight;
+    ASSERT_TRUE (QueryGeometry (dpy,
+				client,
+				currentX,
+				currentY,
+				currentWidth,
+				currentHeight));
+
+    SendSetFrameExtentsRequest (client, left, right, top, bottom);
+    ASSERT_TRUE (VerifySetFrameExtentsResponse (client, left, right, top, bottom));
+
+    /* Map the window */
+    XMapRaised (dpy, client);
+    WaitForReparentAndMap (dpy, client);
+
+    /* Send it another frame extents request */
+    right = right + 1;
+    bottom = bottom + 1;
+
+    SendSetFrameExtentsRequest (client, left, right, top, bottom);
+    ASSERT_TRUE (VerifySetFrameExtentsResponse (client, left, right, top, bottom));
+
+    /* Check the geometry of the frame */
+    Window frame = GetTopParent (dpy, client);
+    ASSERT_TRUE (VerifyWindowSize (frame,
+				   currentX,
+				   currentY,
+				   currentWidth + (left + right),
+				   currentHeight + (top + bottom)));
+
+    /* Wrapper geometry is extents.xy, size.wh */
+    Window root;
+    Window wrapper = GetImmediateParent (dpy, client, root);
+    ASSERT_TRUE (VerifyWindowSize (wrapper,
+				   left,
+				   top,
+				   currentWidth,
+				   currentHeight));
+}