Merge lp:~smspillaz/compiz-core/compiz-core.fix_878934 into lp:compiz-core/0.9.5

[Sam Spilsbury]

Fixes incorrect _NET_REQUEST_FRAME_EXTENTS handling.

Previously, we would not allow any window to be decorated if it was not yet reparented, eg if a window as pending a map. However, clients are supposed to request frame extents before the window is mapped, so we need to generate a decoration (even if it is a default one) for the window as soon as we get a _NET_REQUEST_FRAME_EXTENTS client message so we can set the _NET_FRAME_EXTENTS property on the window immediately.

Also, we should always set the _NET_FRAME_EXTENTS property on the window, even if the window does not receive a frame (but a plugin has indicated that it is *capable* of receiving one, eg an initially undecorated window in the decor plugin). Some clients require that the property is set in response to _NET_REQUEST_FRAME_EXTENTS, even if they are 0, 0, 0, 0

Fixes bug 878934

[Sam Spilsbury (smspillaz)]

Testcase (using lp:xwmqa) - lp:~smspillaz/+junk/wait-frame-extents-test

[Sam Spilsbury (smspillaz)]

> Testcase (using lp:xwmqa) - lp:~smspillaz/+junk/wait-frame-extents-test

lp:~smspillaz/+junk/wait-frame-extents

[Gord Allott (gordallott)]

On 15/12/11 08:06, Sam Spilsbury wrote:
> Sam Spilsbury has proposed merging lp:~smspillaz/compiz-core/compiz-core.fix_878934 into lp:compiz-core.
>
> Requested reviews:
> Compiz Maintainers (compiz-team)
> Related bugs:
> Bug #878934 in compiz (Ubuntu): "Menu selection is wrong in Java apps"
> https://bugs.launchpad.net/ubuntu/+source/compiz/+bug/878934
>
> For more details, see:
> https://code.launchpad.net/~smspillaz/compiz-core/compiz-core.fix_878934/+merge/85806
>
> Fixes incorrect _NET_REQUEST_FRAME_EXTENTS handling.
>
> Previously, we would not allow any window to be decorated if it was not yet reparented, eg if a window as pending a map. However, clients are supposed to request frame extents before the window is mapped, so we need to generate a decoration (even if it is a default one) for the window as soon as we get a _NET_REQUEST_FRAME_EXTENTS client message so we can set the _NET_FRAME_EXTENTS property on the window immediately.
>
> Also, we should always set the _NET_FRAME_EXTENTS property on the window, even if the window does not receive a frame (but a plugin has indicated that it is *capable* of receiving one, eg an initially undecorated window in the decor plugin). Some clients require that the property is set in response to _NET_REQUEST_FRAME_EXTENTS, even if they are 0, 0, 0, 0
>
> Fixes bug 878934

Code seems fine, but untested

review approve

--
Gordon Allott
Canonical Ltd.
27 Floor, Millbank Tower
London SW1P 4QP
www.canonical.com

[Gord Allott (gordallott)]

review: Approve

=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2011-10-26 23:18:47 +0000
+++ plugins/decor/src/decor.cpp	2011-12-15 08:04:24 +0000
@@ -1384,6 +1384,7 @@
 {
     Decoration	     *old, *decoration = NULL;
     bool	     decorate = false;
+    bool	     shadowOnly = true;
     int		     moveDx, moveDy;
     int		     oldShiftX = 0;
     int		     oldShiftY  = 0;
@@ -1409,28 +1410,31 @@
 	    case CompWindowTypeMenuMask:
 	    case CompWindowTypeNormalMask:
 		if (window->mwmDecor () & (MwmDecorAll | MwmDecorTitle))
-		    decorate = (window->frame () ||
-				window->hasUnmapReference ()) ? true : false;
+		    shadowOnly = false;
 	    default:
 		break;
 	}
 
 	if (window->overrideRedirect ())
-	    decorate = false;
+	    shadowOnly = true;
 
 	if (window->wmType () & (CompWindowTypeDockMask | CompWindowTypeDesktopMask))
-	    decorate = false;
+	    shadowOnly = true;
 
-	if (decorate)
+	if (!shadowOnly)
 	{
 	    if (!dScreen->optionGetDecorationMatch ().evaluate (window))
-		decorate = false;
+		shadowOnly = true;
 	}
     }
     else
-	decorate = true;
-
-    if (decorate)
+	shadowOnly = false;
+
+    decorate = ((window->frame () ||
+		 window->hasUnmapReference ()) && !shadowOnly) ||
+		 isSwitcher;
+
+    if (decorate || frameExtentsRequested)
     {
         /* Attempt to find a matching decoration */
         decoration = decor.findMatchingDecoration (window, true);
@@ -1451,6 +1455,11 @@
 	    else if (dScreen->dmSupports & WINDOW_DECORATION_TYPE_WINDOW)
 		decoration = &dScreen->windowDefault;
 	}
+
+	/* Do not allow windows which are later undecorated
+	 * to have a set _NET_FRAME_EXTENTS */
+	if (decorate)
+	    frameExtentsRequested = false;
     }
     else
     {
@@ -1531,7 +1540,8 @@
 	moveDy = shiftY () - oldShiftY;
 
 	/* Update the input and output frame */
-	updateFrame ();
+	if (decorate)
+	    updateFrame ();
 	window->updateWindowOutputExtents ();
 	if (dScreen->cmActive)
 	    cWindow->damageOutputExtents ();
@@ -1545,7 +1555,8 @@
 	/* Undecorated windows need to have the
 	 * input and output frame removed and the
 	 * frame window geometry reset */
-	updateFrame ();
+	if (decorate)
+	    updateFrame ();
 
 	memset (&emptyExtents, 0, sizeof (CompWindowExtents));
 
@@ -2056,6 +2067,8 @@
 	this->dmSupports = dmSupports;
 
 	/* Create new default decorations */
+	screen->updateSupportedWmHints ();
+
 	if (dmWin)
 	{
 	    for (i = 0; i < DECOR_NUM; i++)
@@ -2849,7 +2862,10 @@
 {
     screen->addSupportedAtoms (atoms);
 
-    atoms.push_back (requestFrameExtentsAtom);
+    /* Don't support _NET_REQUEST_FRAME_EXTENTS
+     * where there is no decorator running yet */
+    if (dmWin)
+	atoms.push_back (requestFrameExtentsAtom);
 }
 
 void
@@ -3024,7 +3040,8 @@
     updateReg (true),
     unshading (false),
     shading (false),
-    isSwitcher (false)
+    isSwitcher (false),
+    frameExtentsRequested (false)
 {
     WindowInterface::setHandler (window);
 

=== modified file 'plugins/decor/src/decor.h'
--- plugins/decor/src/decor.h	2011-10-15 11:00:51 +0000
+++ plugins/decor/src/decor.h	2011-12-15 08:04:24 +0000
@@ -274,6 +274,8 @@
 	bool	  unshading;
 	bool	  shading;
 	bool	  isSwitcher;
+
+	bool      frameExtentsRequested;
 };
 
 class DecorPluginVTable :

=== modified file 'po/POTFILES.in'
--- po/POTFILES.in	2009-10-18 20:23:24 +0000
+++ po/POTFILES.in	2011-12-15 08:04:24 +0000
@@ -1,7 +1,4 @@
-gtk/gnome/50-compiz-desktop-key.xml.in
-gtk/gnome/50-compiz-key.xml.in
 gtk/gnome/compiz.desktop.in
-gtk/gnome/compiz-wm.desktop.in
 gtk/window-decorator/gwd.schemas.in
 gtk/window-decorator/gtk-window-decorator.c
 metadata/core.xml.in

=== modified file 'src/window.cpp'
--- src/window.cpp	2011-11-07 02:27:18 +0000
+++ src/window.cpp	2011-12-15 08:04:24 +0000
@@ -6966,31 +6966,42 @@
 	priv->border.top    != b->top ||
 	priv->border.bottom != b->bottom)
     {
-	unsigned long data[4];
-
 	priv->serverInput = *i;
 	priv->border = *b;
 
 	recalcActions ();
 
-	/* Use b for _NET_WM_FRAME_EXTENTS here because
-	 * that is the representation of the actual decoration
-	 * around the window that the user sees and should
-	 * be used for placement and such */
-
-	data[0] = b->left;
-	data[1] = b->right;
-	data[2] = b->top;
-	data[3] = b->bottom;
-
-	XChangeProperty (screen->dpy (), priv->id,
-			 Atoms::frameExtents,
-			 XA_CARDINAL, 32, PropModeReplace,
-			 (unsigned char *) data, 4);
-
 	priv->updateSize ();
 	priv->updateFrameWindow ();
     }
+
+    /* Use b for _NET_WM_FRAME_EXTENTS here because
+     * that is the representation of the actual decoration
+     * around the window that the user sees and should
+     * be used for placement and such */
+
+    /* Also update frame extents regardless of whether or not
+     * the frame extents actually changed, eg, a plugin could
+     * suggest that a window has no frame extents and that it
+     * might later get frame extents - this is mandatory if we
+     * say that we support it, so set them
+     * additionaly some applications might request frame extents
+     * and we must respond by setting the property - ideally
+     * this should only ever be done when some plugin actually
+     * need to change the frame extents or the applications
+     * requested it */
+
+    unsigned long data[4];
+
+    data[0] = b->left;
+    data[1] = b->right;
+    data[2] = b->top;
+    data[3] = b->bottom;
+
+    XChangeProperty (screen->dpy (), priv->id,
+		     Atoms::frameExtents,
+		     XA_CARDINAL, 32, PropModeReplace,
+		     (unsigned char *) data, 4);
 }
 
 bool