Merge lp:~brandontschaefer/compiz/lp.102246-remove-if-no-struts into lp:compiz/0.9.9

[Brandon Schaefer]

In the dtor for ~PlaceWindows we check if the window had struts, if so update the struts for that window. If the strut is set to NULL then remove it from the mStrutWindow list!

The mStrutWindows list is set before the signals are emitted that the screen has changed to unity. This means when the signals are emitted and in unity windows are destroyed they are still in the list of mStrutWindows. Another problem is the DestroyNotify event is not revived by compiz until after the monitor check is over. This means there isn't a good way to say, hey we have deleted this window, and should be removed from the list of mStrutWindows...

So in the dtor for ~PlaceWindows we update the strut for that window, if the strut is set to NULL then remove it from the mStrutWindow list!

[lp:~brandontschaefer/compiz/lp.102246-remove-if-no-struts]

3532. By Brandon Schaefer on 2012-12-19

* Only check if the list is not empty and struts are true for the window

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3532
http://jenkins.qa.ubuntu.com/job/compiz-ci/330/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/330/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/330//rebuild/?

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

As discussed, I'm happy with this, thanks for figuring it out.

11 + if (!window->struts())
12 + {

This check here is probably not necessary. updateStruts on a destroyed window that previous had struts should always clear the struts because the window is destroyed on the server. We should always seek to remove destroyed windows from that list - having destroyed windows in lists can cause later problems down the line, since we use raw pointers.

As for testing - it could be a tad tricky. One means of testing would be to encapsulate the strut window list and doHandleScreenSizeChange functions behind interfaces, and then make a class that encapsulates a PlaceWindow's ownership of a place in that list. That would be testing what the unit does at a unit level.

I was going to suggest using xorg-gtest to test integration with the server, however, xorg-gtest won't be able to handle this case. All attempts to reconfigure a root window automatically fail when using the protocol.

[lp:~brandontschaefer/compiz/lp.102246-remove-if-no-struts]

3533. By Brandon Schaefer on 2012-12-19

* If a PlaceWindow is in the dtor and it has a strut, and the mStrutWindows are not empty just remove it from the list

[Brandon Schaefer (brandontschaefer)]

Yes your right, and if we don't need to check for !window->struts() then there is no point in updating the struts...if we find a window that is in the dtor for PlaceWindows and it has struts and the list isn't empty we can assume it is in the list and just remove it. :)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:3533
http://jenkins.qa.ubuntu.com/job/compiz-ci/331/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-ci/./build=pbuilder,distribution=quantal,flavor=amd64/331/console

Click here to trigger a rebuild:
http://jenkins.qa.ubuntu.com/job/compiz-ci/331//rebuild/?

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

Given that there's a hole in xorg-gtest that prevents us from testing this using integration tests and that refactoring the code to get it under test would be a largely ineffective method of testing this, I'm happy to have this change without tests.

Stephen?

review: Approve

[Brandon Schaefer (brandontschaefer)]

Test here:
https://code.launchpad.net/~brandontschaefer/unity/lp.1002246-manual-test/+merge/140959

As it makes more sense from a unity point of view.

[Stephen M. Webb (bregma)]

Manual test is acceptable for this change.

review: Approve

=== modified file 'plugins/place/src/place.cpp'
--- plugins/place/src/place.cpp	2012-09-08 00:00:34 +0000
+++ plugins/place/src/place.cpp	2012-12-19 18:09:19 +0000
@@ -300,6 +300,14 @@
 
 PlaceWindow::~PlaceWindow ()
 {
+    if (!ps->mStrutWindows.empty() && window->struts())
+    {
+	ps->mStrutWindows.remove(window);
+	if (ps->mStrutWindows.empty())
+	{
+	    ps->doHandleScreenSizeChange(screen->width(), screen->height());
+	}
+    }
 }
 
 bool