Merge lp:~townsend/compiz/fix-minimize-unredirect-window into lp:compiz/0.9.10

[Christopher Townsend]

Fixed issue where minimizing an unredirect full screen window would repaint the full screen window after minimizing it even though it isn't really there.

= Issue =
Minimizing an unredirect full screen window will display the full screen window after the minimization even though it isn't really there.

= Fix =
Do not unredirect a full screen window if no occlusion is detected meaning that it has been minimized.

= Test =
Added some unit test coverage with Sam's guidance.

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3685
http://jenkins.qa.ubuntu.com/job/compiz-ci/166/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/204/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-amd64-ci/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-armhf-ci/21
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-i386-ci/21

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

review: Needs Fixing (continuous-integration)

[Sam Spilsbury (smspillaz)]

This is more of a strange interaction with the unity plugin's fake minimization workaround than it is a compiz issue. I think the best way to handle this would be to simply skip painting any window marked minimized - eg over here:

 /* detect occlusions */
 for (rit = pl.rbegin (); rit != pl.rend (); ++rit)
 {
     w = (*rit);
     gw = GLWindow::get (w);

...

 /* detect occlusions */
 for (rit = pl.rbegin (); rit != pl.rend (); ++rit)
 {
     w = (*rit);
     gw = GLWindow::get (w);

     if (w->destroyed ())
  continue;

     if (!w->shaded ())
     {
  /* Non-damaged windows don't have valid pixmap
   * contents and we aren't displaying them yet
   * so don't factor them into occlusion detection */
  if (!gw->priv->cWindow->damaged ())
  {
      gw->priv->clip = region;
      continue;
  }

                /* Some plugins may explicitly inhibit paint of minimized
                 * windows, just don't paint them at all if they're viewable
                 * but also minimized */
  if (!w->isViewable () ||
+ w->minimized ())
      continue;
     }

[Sam Spilsbury (smspillaz)]

review: Needs Fixing

[Christopher Townsend (townsend)]

Sam,

Thanks for reviewing this. I tried what you suggested and it does not fix the bug I'm trying to fix. In fact, the behavior is even worse in that the full screen image will not go away no matter what I do.

I still think my suggested fix is good since this issue is only caused by unredirect full screen windows and not setting a full screen window to unredirect when it is minimized seems to make sense.

review: Needs Resubmitting

[Christopher Townsend (townsend)]

Oops, didn't mean to set my comment as a resubmit...

[Sam Spilsbury (smspillaz)]

Hey Chris, no problem.

I've only had a chance to have a passing look so far, but I'm doing a full test + look now. I still feel like putting the check for w->minimized () after we've built the unredirected windows list feels a little weird, but perhaps my previous approach was too overzealous. I'm just going to try something else now and see how that goes.

[Sam Spilsbury (smspillaz)]

Hey Chris,

So I had a bit more of a look into this - I think the more general problem was that windows which plugins explicitly did not want to paint (eg, they set PAINT_WINDOW_NO_CORE_INSTANCE_MASK) were being factored into the undredirection algorithm when they shouldn't have been. Can you try out the following diff to see if it fixes the minimization problem? It should fix all of the other problems more generally too. Also has some automated test coverage.

=== modified file 'plugins/opengl/src/fsregion/fsregion.cpp'
--- plugins/opengl/src/fsregion/fsregion.cpp 2012-11-29 10:51:38 +0000
+++ plugins/opengl/src/fsregion/fsregion.cpp 2013-05-08 01:42:37 +0000
@@ -48,7 +48,7 @@
 {
     bool fullscreen = false;

- if (!(flags & (Desktop | Alpha)) &&
+ if (!(flags & (Desktop | Alpha | NoOcclusionDetection)) &&
         region == untouched &&
         region == orig)
     {

=== modified file 'plugins/opengl/src/fsregion/fsregion.h'
--- plugins/opengl/src/fsregion/fsregion.h 2012-11-29 10:51:38 +0000
+++ plugins/opengl/src/fsregion/fsregion.h 2013-05-08 01:42:34 +0000
@@ -37,7 +37,8 @@
     typedef enum
     {
  Desktop = 1,
- Alpha = 2
+ Alpha = 2,
+ NoOcclusionDetection = 3
     } WinFlag;

     typedef unsigned int WinFlags;

=== modified file 'plugins/opengl/src/fsregion/tests/test-fsregion.cpp'
--- plugins/opengl/src/fsregion/tests/test-fsregion.cpp 2012-11-29 10:51:38 +0000
+++ plugins/opengl/src/fsregion/tests/test-fsregion.cpp 2013-05-08 02:33:58 +0000
@@ -26,7 +26,7 @@
 #include "gtest/gtest.h"
 #include "fsregion.h"

-using namespace compiz::opengl;
+using namespace compiz::opengl;

 TEST (OpenGLFullscreenRegion, NoWindows)
 {
@@ -68,6 +68,25 @@
                                        FullscreenRegion::Desktop));
 }

+TEST (OpenGLFullscreenRegion, NoOcclusionFullscreen)
+{
+ FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
+ FullscreenRegion::NoOcclusionDetection));
+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
+ FullscreenRegion::Desktop));
+}
+
+TEST (OpenGLFullscreenRegion, NoOcclusionOverFullscreen)
+{
+ FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (50, 60, 70, 80),
+ FullscreenRegion::NoOcclusionDetection));
+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
+ EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
+ FullscreenRegion::Desktop));
+}
+
 TEST (OpenGLFullscreenRegion, NormalWindows)
 {
     FullscreenRegion monitor (CompRect (0, 0, 1024, 768));

=== modified file 'plugins/opengl/src/paint.cpp'
--- plugins/opengl/src/paint.cpp 2013-04-25 17:01:46 +0000
+++ plugins/opengl/src/paint.cpp 2013-05-08 02:31:48 +0000
@@ -353,6 +353,11 @@
          flags |= FullscreenRegion::Desktop;
      if (w->alpha ())
   flags |= FullscreenRegion::Alpha;
+
+ /* Anything which was not occlusion detected is not a suitable
+ * candidate for unredirection either */
+ if (...

[Sam Spilsbury (smspillaz)]

Oops, seems like launchpad mangled that one. Try here: http://paste.ubuntu.com/5643479/

I should also note that unity's fullscreen window detection seems to detect such "non painted" windows as well causing the launcher and panel to be invisible when a fullscreen window gets minimized. I think the fullscreen window detection code can be adjusted to handle this case. The relevant bits can be found in unityshell.cpp:UnityWindow::glPaint

[Christopher Townsend (townsend)]

Wow, thanks Sam!

I tried it and it works. I'll get my branch updated soon with the changes you proposed.

As far as the Launcher and Panel not repainting, I have a separate MP in Unity already to fix that. Just waiting on a review.

[Sam Spilsbury (smspillaz)]

Excellent - I've reviewed your unity branch too - similar comment as to here.

[Sam Spilsbury (smspillaz)]

Excellent, thanks.

review: Approve

[Christopher Townsend (townsend)]

Ok, this is ready for review again.

Thanks!

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:3688
http://jenkins.qa.ubuntu.com/job/compiz-ci/169/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/209
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-amd64-ci/26
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-armhf-ci/26/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-i386-ci/24

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

review: Needs Fixing (continuous-integration)

[PS Jenkins bot (ps-jenkins)]

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/compiz-autolanding/33/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-amd64-autolanding/33
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-raring-armhf-autolanding/33/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/compiz-raring-i386-autolanding/33

review: Needs Fixing (continuous-integration)

[Christopher Townsend (townsend)]

Re-approving since the armhf build failed due to some transient failure.

[PS Jenkins bot (ps-jenkins)]

review: Approve (continuous-integration)

=== modified file 'plugins/opengl/src/fsregion/fsregion.cpp'
--- plugins/opengl/src/fsregion/fsregion.cpp	2012-11-29 10:51:38 +0000
+++ plugins/opengl/src/fsregion/fsregion.cpp	2013-05-08 15:16:22 +0000
@@ -48,7 +48,7 @@
 {
     bool fullscreen = false;
 
-    if (!(flags & (Desktop | Alpha)) &&
+    if (!(flags & (Desktop | Alpha | NoOcclusionDetection)) &&
         region == untouched &&
         region == orig)
     {

=== modified file 'plugins/opengl/src/fsregion/fsregion.h'
--- plugins/opengl/src/fsregion/fsregion.h	2012-11-29 10:51:38 +0000
+++ plugins/opengl/src/fsregion/fsregion.h	2013-05-08 15:16:22 +0000
@@ -37,7 +37,8 @@
     typedef enum
     {
 	Desktop = 1,
-	Alpha = 2
+	Alpha = 2,
+	NoOcclusionDetection = 3
     } WinFlag;
 
     typedef unsigned int WinFlags;

=== modified file 'plugins/opengl/src/fsregion/tests/test-fsregion.cpp'
--- plugins/opengl/src/fsregion/tests/test-fsregion.cpp	2012-11-29 10:51:38 +0000
+++ plugins/opengl/src/fsregion/tests/test-fsregion.cpp	2013-05-08 15:16:22 +0000
@@ -68,6 +68,25 @@
                                        FullscreenRegion::Desktop));
 }
 
+TEST (OpenGLFullscreenRegion, NoOcclusionFullscreen)
+{
+    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
+    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
+                                       FullscreenRegion::NoOcclusionDetection));
+    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
+                                       FullscreenRegion::Desktop));
+}
+
+TEST (OpenGLFullscreenRegion, NoOcclusionOverFullscreen)
+{
+    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
+    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (50, 60, 70, 80),
+                                       FullscreenRegion::NoOcclusionDetection));
+    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
+    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
+                                       FullscreenRegion::Desktop));
+}
+
 TEST (OpenGLFullscreenRegion, NormalWindows)
 {
     FullscreenRegion monitor (CompRect (0, 0, 1024, 768));

=== modified file 'plugins/opengl/src/paint.cpp'
--- plugins/opengl/src/paint.cpp	2013-01-03 16:05:26 +0000
+++ plugins/opengl/src/paint.cpp	2013-05-08 15:16:22 +0000
@@ -354,6 +354,11 @@
 	    if (w->alpha ())
 		flags |= FullscreenRegion::Alpha;
 	    
+	    /* Anything which was not occlusion detected is not a suitable
+	     * candidate for unredirection either */
+	    if (!status)
+		flags |= FullscreenRegion::NoOcclusionDetection;
+
 	    CompositeWindow *cw = CompositeWindow::get (w);
 
 	    /*