Merge lp:~compiz-team/compiz/compiz.fix_1186560 into lp:compiz/0.9.10

[Sam Spilsbury]

Don't cause maximized windows to be double adjusted upon decoration change.

This is effectively a bundle of fixes in one:

1. Don't adjust maximized windows after their decorations change with
   addWindowSizeChanges. That only needs to be done on initial maximization
   and then any other decoration size change can handle itself
2. Don't cause the saved co-ordinates to be updated upon unmaximization
3. Don't put the decoration adjustment on a timer. Do it immediately.
   The calling WindowInterface::moveNotify from within itself is no
   longer a problem.

(LP: #1186560)

Don't cause maximized windows to be double adjusted upon decoration change.

This is effectively a bundle of fixes in one:

1. Don't adjust maximized windows after their decorations change with
   addWindowSizeChanges. That only needs to be done on initial maximization
   and then any other decoration size change can handle itself
2. Don't cause the saved co-ordinates to be updated upon unmaximization
3. Don't put the decoration adjustment on a timer. Do it immediately.
   The calling WindowInterface::moveNotify from within itself is no
   longer a problem.

(LP: #1186560)

Please test this, but don't merge it yet. I'd prefer to wait until we have a testsuite for the decor plugin.

[MC Return (mc-return)]

You forgot:

4. Fixed int dwidth = sizeDelta.width () - lastSizeDelta.width (); typo.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3736
http://jenkins.qa.ubuntu.com/job/compiz-ci/191/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/232/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-amd64-ci/4
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-saucy-armhf-ci/4/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-saucy-i386-ci/4

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/191/rebuild

review: Needs Fixing (continuous-integration)

[Sam Spilsbury (smspillaz)]

I imagine that this will be folded into an unrevert of the relevant revision.

=== modified file 'gtk/window-decorator/tests/test_gwd_settings.cpp'
--- gtk/window-decorator/tests/test_gwd_settings.cpp	2013-03-06 13:44:06 +0000
+++ gtk/window-decorator/tests/test_gwd_settings.cpp	2013-06-05 08:25:46 +0000
@@ -258,8 +258,6 @@
 {
 };
 
-const GValue referenceGValue = G_VALUE_INIT;
-
 namespace
 {
     void gwd_settings_storage_unref (GWDSettingsStorage *storage)
@@ -287,7 +285,11 @@
 
 	    AutoUnsetGValue (GType type)
 	    {
-		memcpy (&mValue, &referenceGValue, sizeof (GValue));
+		/* This is effectively G_VALUE_INIT, we can't use that here
+		 * because this is not a C++11 project */
+		mValue.g_type = 0;
+		mValue.data[0].v_int = 0;
+		mValue.data[1].v_int = 0;
 		g_value_init (&mValue, type);
 	    }
 
@@ -423,109 +425,154 @@
     EXPECT_CALL (settingsGMock, dispose ());
     EXPECT_CALL (settingsGMock, finalize ());
 
+    /* The order of evaluation of matchers in Google Mock appears to be undefined and
+     * the way GValueMatch is written makes it particularly unsafe when used with
+     * matchers of multiple types on the same function, since there's no guaruntee
+     * that the matchers will be traversed in any order. If a type is passed to
+     * any of the matchers that it doesn't know how to handle then it will
+     * call directly through to GValueCmp which will run into undefined behaviour
+     * in itself.
+     *
+     * In reality, the API for GValueMatch is probably a little bit broken in this
+     * sense, but just satisfying each expectation as soon as its set seems to do
+     * the job here
+     */
+
     /* calling g_object_get_property actually resets
      * the value so expecting 0x0 is correct */
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_ACTIVE_SHADOW,
 					     GValueMatch <gpointer> (0x0, g_value_get_pointer),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "active-shadow",
+			   &pointerGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_INACTIVE_SHADOW,
 					     GValueMatch <gpointer> (0x0, g_value_get_pointer),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "inactive-shadow",
+			   &pointerGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_USE_TOOLTIPS,
 					     GValueMatch <gboolean> (FALSE, g_value_get_boolean),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "use-tooltips",
+			   &booleanGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_DRAGGABLE_BORDER_WIDTH,
 					     GValueMatch <gint> (0, g_value_get_int),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "draggable-border-width",
+			   &integerGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_ATTACH_MODAL_DIALOGS,
 					     GValueMatch <gboolean> (FALSE, g_value_get_boolean),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "attach-modal-dialogs",
+			   &booleanGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_BLUR_CHANGED,
 					     GValueMatch <gint> (0, g_value_get_int),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "blur",
+			   &integerGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_METACITY_THEME,
 					     GValueMatch <const gchar *> (NULL, g_value_get_string),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "metacity-theme",
+			   &stringGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_ACTIVE_OPACITY,
 					     GValueMatch <gdouble> (0.0, g_value_get_double),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "metacity-active-opacity",
+			   &doubleGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_INACTIVE_OPACITY,
 					     GValueMatch <gdouble> (0.0, g_value_get_double),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "metacity-inactive-opacity",
+			   &doubleGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_ACTIVE_SHADE_OPACITY,
 					     GValueMatch <gboolean> (FALSE, g_value_get_boolean),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "metacity-active-shade-opacity",
+			   &booleanGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_INACTIVE_SHADE_OPACITY,
 					     GValueMatch <gboolean> (FALSE, g_value_get_boolean),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "metacity-inactive-shade-opacity",
+			   &booleanGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_BUTTON_LAYOUT,
 					     GValueMatch <const gchar *> (NULL, g_value_get_string),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "metacity-button-layout",
+			   &stringGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_TITLEBAR_ACTION_DOUBLE_CLICK,
 					     GValueMatch <gint> (0, g_value_get_int),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "titlebar-double-click-action",
+			   &integerGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_TITLEBAR_ACTION_MIDDLE_CLICK,
 					     GValueMatch <gint> (0, g_value_get_int),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "titlebar-middle-click-action",
+			   &integerGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_TITLEBAR_ACTION_RIGHT_CLICK,
 					     GValueMatch <gint> (0, g_value_get_int),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "titlebar-right-click-action",
+			   &integerGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_MOUSE_WHEEL_ACTION,
 					     GValueMatch <gint> (0, g_value_get_int),
 					     _));
+
+    g_object_get_property (G_OBJECT (settingsMock.get ()),
+			   "mouse-wheel-action",
+			   &integerGValue);
+
     EXPECT_CALL (settingsGMock, getProperty (GWD_MOCK_SETTINGS_PROPERTY_TITLEBAR_FONT,
 					     GValueMatch <const gchar *> (NULL, g_value_get_string),
 					     _));
 
     g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "active-shadow",
-			   &pointerGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "inactive-shadow",
-			   &pointerGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "use-tooltips",
-			   &booleanGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "draggable-border-width",
-			   &integerGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "attach-modal-dialogs",
-			   &booleanGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "blur",
-			   &integerGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "metacity-theme",
-			   &stringGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "metacity-active-opacity",
-			   &doubleGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "metacity-inactive-opacity",
-			   &doubleGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "metacity-active-shade-opacity",
-			   &booleanGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "metacity-inactive-shade-opacity",
-			   &booleanGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "metacity-button-layout",
-			   &stringGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "titlebar-double-click-action",
-			   &integerGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "titlebar-middle-click-action",
-			   &integerGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "titlebar-right-click-action",
-			   &integerGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
-			   "mouse-wheel-action",
-			   &integerGValue);
-    g_object_get_property (G_OBJECT (settingsMock.get ()),
 			   "titlebar-font",
 			   &stringGValue);
 }
 
=== modified file 'plugins/decor/src/decor.cpp'
--- plugins/decor/src/decor.cpp	2013-05-20 15:26:11 +0000
+++ plugins/decor/src/decor.cpp	2013-06-05 08:25:46 +0000
@@ -1149,9 +1149,8 @@
 /*
  * decorOffsetMove
  *
- * Function called on a timer (to avoid calling configureXWindow from
- * within a ::moveNotify) which actually moves the window by the offset
- * specified in the xwc. Also sends a notification that the window
+ * Moves the window by the offset specified in the xwc.
+ * Also sends a notification that the window
  * was decorated
  *
  */
@@ -1522,7 +1521,7 @@
 
     int dx = movement.x () - lastShift.x ();
     int dy = movement.y () - lastShift.y ();
-    int dwidth = sizeDelta.width () - lastSizeDelta.height ();
+    int dwidth = sizeDelta.width () - lastSizeDelta.width ();
     int dheight = sizeDelta.height () - lastSizeDelta.height ();
 
     /* We don't apply these rules to override-redirect windows
@@ -1557,16 +1556,18 @@
 	if (window->state () & CompWindowStateMaximizedVertMask)
 	    mask &= ~(CWY | CWHeight);
 
-	if (window->saveMask () & CWX)
+	/* We don't want to make changes to the saved co-ordinates
+	 * if we're unmaximizing */
+	if ((window->saveMask () & ~mask) & CWX)
 	    window->saveWc ().x += xwc.x;
 
-	if (window->saveMask () & CWY)
+	if ((window->saveMask () & ~mask) & CWY)
 	    window->saveWc ().y += xwc.y;
 
-	if (window->saveMask () & CWWidth)
+	if ((window->saveMask () & ~mask) & CWWidth)
 	    window->saveWc ().width += xwc.width;
 
-	if (window->saveMask () & CWHeight)
+	if ((window->saveMask () & ~mask) & CWHeight)
 	    window->saveWc ().height += xwc.height;
 
 	/* If the window has not been placed, do not move it this time
@@ -1585,10 +1586,7 @@
 	     * FIXME: CompTimer should really be PIMPL and allow
 	     * refcounting in case we need to keep it alive
 	     */
-	    if (instant)
-		decorOffsetMove (window, xwc, mask);
-	    else
-		moveUpdate.start (boost::bind (decorOffsetMove, window, xwc, mask), 0);
+	    decorOffsetMove (window, xwc, mask);
 	}
 
 	/* Even if the window has not yet been placed, we still
@@ -1667,6 +1665,7 @@
 
     bool shadowOnly = bareDecorationOnly ();
     bool decorate = shouldDecorateWindow (window, shadowOnly, isSwitcher);
+    unsigned int decorMaximizeState = window->state () & MAXIMIZE_STATE;
 
     if (decorate || frameExtentsRequested)
     {
@@ -1689,7 +1688,8 @@
     /* Don't bother going any further if
      * this window is going to get the same
      * decoration, just use the old one */
-    if (decoration == old)
+    if (decoration == old &&
+        lastMaximizedStateDecorated == decorMaximizeState)
 	return false;
 
     /* Destroy the old WindowDecoration */
@@ -1710,12 +1710,15 @@
 	/* Set extents based on maximize/unmaximize state
 	 * FIXME: With the new type system, this should be
 	 * removed */
-	if ((window->state () & MAXIMIZE_STATE))
+	if (decorMaximizeState)
 	    window->setWindowFrameExtents (&decoration->maxBorder,
 					   &decoration->maxInput);
 	else if (!window->hasUnmapReference ())
 	    window->setWindowFrameExtents (&decoration->border,
 					   &decoration->input);
+
+	lastMaximizedStateDecorated = decorMaximizeState;
+
 	/* This window actually needs its decoration contents updated
 	 * as it was actually visible */
 	if (decorate ||
@@ -2978,13 +2981,7 @@
 	shading = false;
 	unshading = false;
     }
-    /* FIXME: we should not need a timer for calling decorWindowUpdate,
-       and only call updateWindowDecorationScale if decorWindowUpdate
-       returns false. Unfortunately, decorWindowUpdate may call
-       updateWindowOutputExtents, which may call WindowResizeNotify. As
-       we never should call a wrapped function that's currently
-       processed, we need the timer for the moment. updateWindowOutputExtents
-       should be fixed so that it does not emit a resize notification. */
+
     updateMatrix = true;
     updateReg = true;
 
@@ -3009,19 +3006,7 @@
 void
 DecorWindow::stateChangeNotify (unsigned int lastState)
 {
-    if (wd && wd->decor)
-    {
-	if ((window->state () & MAXIMIZE_STATE))
-	    window->setWindowFrameExtents (&wd->decor->maxBorder,
-					   &wd->decor->maxInput);
-	else
-	    window->setWindowFrameExtents (&wd->decor->border,
-					   &wd->decor->input);
-
-	/* The shift will occurr in decorOffsetMove */
-
-	updateFrame ();
-    }
+    update (true);
 
     window->stateChangeNotify (lastState);
 }
@@ -3258,7 +3243,8 @@
     mClipGroup (NULL),
     mOutputRegion (window->outputRect ()),
     mInputRegion (window->inputRect ()),
-    mRequestor (screen->dpy (), w->id (), &decor)
+    mRequestor (screen->dpy (), w->id (), &decor),
+    lastMaximizedStateDecorated (0)
 {
     WindowInterface::setHandler (window);
 
 
=== modified file 'plugins/decor/src/decor.h'
--- plugins/decor/src/decor.h	2013-04-17 23:41:09 +0000
+++ plugins/decor/src/decor.h	2013-06-05 08:25:46 +0000
@@ -385,6 +385,8 @@
 	CompPoint               lastShift;
 	CompSize                lastSizeDelta;
 
+	unsigned int              lastMaximizedStateDecorated;
+
     private:
 
 	bool            bareDecorationOnly ();
 
=== modified file 'src/window.cpp'
--- src/window.cpp	2013-05-27 16:19:59 +0000
+++ src/window.cpp	2013-06-05 08:25:46 +0000
@@ -6610,10 +6610,7 @@
 
 	recalcActions ();
 
-	bool sizeUpdated = false;
-
-	sizeUpdated |= priv->updateSize ();
-	sizeUpdated |= priv->updateFrameWindow ();
+	bool sizeUpdated = priv->updateFrameWindow ();
 
 	/* Always send a moveNotify
 	 * whenever the frame extents update