Merge lp:~mar-kolya/compiz/fix-for-1158267 into lp:compiz/0.9.10

[kolya]

DecorWindow::update: always initialize wd field if decoration is known. This partly reverts change occured in rev 3633. This seems to fix LP #1158267

It looks like rev 3363 introduced a regression described in LP #1158267. The problem seems to be that DecorWindow::update doesn't always initialize DecorWindow::wd and this seems to be breaking decoration header initialization. This patch fixes the problem.

[Strelok (who-)]

This patch seems to fix LP #1158267, but after that menu shadow disappears in gtk3 applications (Rhythmbox, Zim, gnome-terminal, gedit...)

review: Needs Fixing

[Sam Spilsbury (smspillaz)]

At the moment, I'd prefer to hold any further fixes to the decor plugin until we can get a proper testsuite in place. Unfortunately this code has proven to be ... tricky and regression prone because of very tight behavioral coupling with the rest of compiz. Once the testsuite is in place we'll be able to fix stuff and not worry about missing some other corner case (of which there appear to be *plenty*).

I'm guessing its going to take me about a week before I'm able to start writing it, but I should be able to finish it the day I start it.

[Sam Spilsbury (smspillaz)]

(So watch this space, but to the other maintainers - please wait about a week before merging this, thanks).

[Brandon Schaefer (brandontschaefer)]

Will do :), but I think you just got some decor testing into place. Lets revisit this once you've some time and hopefully get some testing around this!

[Sam Spilsbury (smspillaz)]

Hey kolya,

Thanks for the hints with this one. I figured out that this problem actually occurs for a variety of reasons:

1. Where the WindowDecoration member is reset for windows that already have one set but are temporarily unable to have a WindowDecoration on update for whatever reason (perhaps due to a frame extents request but I'm not sure). The fix for that was similar - just throw out the old WindowDecoration as soon as we figure out that we can allocate a new one or we really want to get rid of it (eg, a null decoration)
2. Where the window was just decorated and the border shape is effectively the same as the
input frame shape. In that case we should just update the frame shape immediately as opposed to going through the server which may not send an event back if there was no effective change.

You can follow the process here:

https://code.launchpad.net/~compiz-team/compiz/compiz.fix_1158267.update-tests/+merge/174661

[Christopher Townsend (townsend)]

Hi koyla,

Thanks for the work you did on this. However, Sam's branch listed in the last comment is what we are going to use for this bug.

That said, I'm going to reject this branch, but if you ever feel you want to contribute to Compiz (or any other project for that matter), please don't hesitate.

review: Disapprove

[kolya (mar-kolya)]

I'm glad that this bug is finally getting proper fix! :)

Unmerged revisions

3734. By kolya on 2013-06-05

DecorWindow::update: always initialize wd field if decoration is known. This partly reverts change occured in rev 3633. This seems to fix LP #1158267

=== 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 16:31:41 +0000
@@ -1707,6 +1707,16 @@
      */
     if (decoration)
     {
+        wd = WindowDecoration::create (decoration);
+	if (!wd)
+	{
+	    /* Error condition, reset frame extents */
+	    CompWindowExtents emptyExtents;
+	    memset (&emptyExtents, 0, sizeof (CompWindowExtents));
+	    window->setWindowFrameExtents (&emptyExtents, &emptyExtents);
+	    return false;
+	}
+
 	/* Set extents based on maximize/unmaximize state
 	 * FIXME: With the new type system, this should be
 	 * removed */
@@ -1721,16 +1731,6 @@
 	if (decorate ||
 	    shadowOnly)
 	{
-	    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 = cwe::shift (window->border (), window->sizeHints ().win_gravity);
 
 	    sizeDelta = CompSize (-(window->border ().left +