Merge lp:~vanvugt/compiz/fix-1046661 into lp:compiz/0.9.8

[Daniel van Vugt]

Fixed: Windows with an alpha-channel, like gnome-terminal, were not being
considered as possibly covering fullscreen windows. But they most certainly
can. This ensures such RGBA windows are visible if they're stacked above a
fullscreen window. (LP: #1046661)
.

Fixed: Windows with an alpha-channel, like gnome-terminal, were not being
considered as possibly covering fullscreen windows. But they most certainly
can. This ensures such RGBA windows are visible if they're stacked above a
fullscreen window. (LP: #1046661)

[jenkins (martin-mrazik+qa)]

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

review: Approve (continuous-integration)

[Sam Spilsbury (smspillaz)]

!(flags & (Desktop | Alpha)

That can be expressed as a temporary rather than in a compound if statement

const bool ignoreWindow = (flags & (Desktop | Alpha)

119 + /*
120 + * Alpha windows (status == false) can cover/cancel fullscreen
121 + * unredirected windows. But they can't be unredirected themselves.
122 + * I wonder if this is sensible as some games may use fullscreen
123 + * RGBA (alpha) windows that are fully opaque and need unredirect.
124 + */

This comment is confusing. We're checking whether or not the window has an rgba visual (eg, w->alpha () not whether or not it failed to paint (as status == false) would indicate to me.

Also, the default visual for most windows would be an RGB24 visual, so I don't think the concern is too much. (And we wouldn't be able to detect it anyways, as an RGBA visual just means a client is at liberty to have alpha values in its backing pixmap, which we must blend).

I'm also a bit confused as to why it matters that the window has an alpha channel as to whether or not a fullscreen window underneath it is unredirected. Shouldn't the fullscreen window underneath always be unredirected if there is some other window that is occluding it?

review: Needs Information

[Sam Spilsbury (smspillaz)]

> Shouldn't the fullscreen window underneath always be
> unredirected if there is some other window that is occluding it?

Correction: That should read

"Shouldn't the fullscreen window underneath always be redirected if there is some other window that is occluding it"

[Daniel van Vugt (vanvugt)]

!(flags & (Desktop | Alpha) is clearer, I think. Programmers think in logic, not words. We don't need to be translating clear logic into extraneous English. It complicates the thought process. Also, "ignoreWindow" is not accurate because it's not the only condition for ignoring a window.

Yeah, the comment is slightly out-dated still. It belongs next to some code that's not there any more. But technically it's still true. status is false for alpha windows (and other cases too).

Alpha "matters" for FullscreenRegion, only because we don't want to accidentally unredirect a translucent fullscreen window. You are right that alpha does not matter for the non-fullscreen windows.

[lp:~vanvugt/compiz/fix-1046661]

3349. By Daniel van Vugt on 2012-09-06

Clarify comment per smspillaz suggestion.

[Sam Spilsbury (smspillaz)]

Okay, I'm good with it now after discussing.

Noted that this proposal doesn't deal with the issue of "not counting" windows that are fully-transparent or not painted at all on top of fullscreen windows. This case should be dealt with later.

review: Approve

=== modified file 'plugins/opengl/src/fsregion/fsregion.cpp'
--- plugins/opengl/src/fsregion/fsregion.cpp	2012-09-04 10:19:19 +0000
+++ plugins/opengl/src/fsregion/fsregion.cpp	2012-09-06 09:57:22 +0000
@@ -35,11 +35,11 @@
 }
 
 bool
-FullscreenRegion::isCoveredBy (const CompRegion &region, WinType type)
+FullscreenRegion::isCoveredBy (const CompRegion &region, WinFlags flags)
 {
     bool fullscreen = false;
 
-    if (!covered && type == Normal && region == untouched)
+    if (!covered && !(flags & (Desktop | Alpha)) && region == untouched)
     {
 	covered = true;
 	fullscreen = true;
 
=== modified file 'plugins/opengl/src/fsregion/fsregion.h'
--- plugins/opengl/src/fsregion/fsregion.h	2012-09-04 10:19:19 +0000
+++ plugins/opengl/src/fsregion/fsregion.h	2012-09-06 09:57:22 +0000
@@ -36,14 +36,16 @@
 public:
     typedef enum
     {
-	Normal,
-	Desktop
-    } WinType;
+	Desktop = 1,
+	Alpha = 2
+    } WinFlag;
+
+    typedef unsigned int WinFlags;
 
     FullscreenRegion (const CompRect &rect);
 
     // isCoveredBy is called for windows from TOP to BOTTOM
-    bool isCoveredBy (const CompRegion &region, WinType type = Normal);
+    bool isCoveredBy (const CompRegion &region, WinFlags flags = 0);
 
 private:
     bool covered;
 
=== modified file 'plugins/opengl/src/fsregion/tests/test-fsregion.cpp'
--- plugins/opengl/src/fsregion/tests/test-fsregion.cpp	2012-09-04 09:51:05 +0000
+++ plugins/opengl/src/fsregion/tests/test-fsregion.cpp	2012-09-06 09:57:22 +0000
@@ -49,6 +49,25 @@
     EXPECT_TRUE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
 }
 
+TEST (OpenGLFullscreenRegion, AlphaFullscreen)
+{
+    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
+    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
+                                       FullscreenRegion::Alpha));
+    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
+                                       FullscreenRegion::Desktop));
+}
+
+TEST (OpenGLFullscreenRegion, AlphaOverFullscreen)
+{
+    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
+    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (50, 60, 70, 80),
+                                       FullscreenRegion::Alpha));
+    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	2012-09-05 10:20:20 +0000
+++ plugins/opengl/src/paint.cpp	2012-09-06 09:57:22 +0000
@@ -337,36 +337,40 @@
 		}
 		else
 		    tmpRegion -= w->region ();
-
-		/* unredirect top most fullscreen windows. */
-		FullscreenRegion::WinType type =
-		    w->type () & CompWindowTypeDesktopMask ?
-		    FullscreenRegion::Desktop :
-		    FullscreenRegion::Normal;
-		
-		if (unredirectFS &&
-		    !(mask & PAINT_SCREEN_TRANSFORMED_MASK) &&
-		    fs.isCoveredBy (w->region (), type))
-		{
-		    fullscreenWindow = w;
-		}
-		else
-		{
-		    CompositeWindow *cw = CompositeWindow::get (w);
-		    if (!cw->redirected ())
-		    {
-			// 1. GLWindow::release to force gw->priv->needsRebind
-			gw->release ();
-
-			// 2. GLWindow::bind, which redirects the window,
-			//    rebinds the pixmap, and then rebinds the pixmap
-			//    to a texture.
-			gw->bind ();
-
-			// 3. Your window is now redirected again with the
-			//    latest pixmap contents.
-		    }
-		}
+	    }
+
+	    FullscreenRegion::WinFlags flags = 0;
+	    if (w->type () & CompWindowTypeDesktopMask)
+	        flags |= FullscreenRegion::Desktop;
+	    if (w->alpha ())
+		flags |= FullscreenRegion::Alpha;
+	    
+	    /*
+	     * Windows with alpha channels can partially occlude windows
+	     * beneath them and so neither should be unredirected in that case.
+	     */
+	    if (unredirectFS &&
+	        !(mask & PAINT_SCREEN_TRANSFORMED_MASK) &&
+	        fs.isCoveredBy (w->region (), flags))
+	    {
+	        fullscreenWindow = w;
+	    }
+	    else
+	    {
+	        CompositeWindow *cw = CompositeWindow::get (w);
+	        if (!cw->redirected ())
+	        {
+		    // 1. GLWindow::release to force gw->priv->needsRebind
+		    gw->release ();
+
+		    // 2. GLWindow::bind, which redirects the window,
+		    //    rebinds the pixmap, and then rebinds the pixmap
+		    //    to a texture.
+		    gw->bind ();
+
+		    // 3. Your window is now redirected again with the
+		    //    latest pixmap contents.
+	        }
 	    }
 	}
     }
 
=== modified file 'tests/manual/Unredirect.txt'
--- tests/manual/Unredirect.txt	2012-09-05 10:20:20 +0000
+++ tests/manual/Unredirect.txt	2012-09-06 09:57:22 +0000
@@ -59,3 +59,19 @@
   Every time you triggered expo mode, expo mode should have become visible and
   you should see the video still playing in the expo view.
 
+
+Unredirect cancelation by alpha windows on top (LP: #1046661)
+-------------------------------------------------------------
+Setup:
+#. Install Chromium or Chrome browser.
+ 
+Actions:
+#. Open a Chrome/Chromium window.
+#. Open a gnome-terminal window (Ctrl+Alt+T)
+#. Move the gnome-terminal to the centre of the screen, above the Chrome
+   window.
+#. Click on the Chrome window and make it fullscreen (F11).
+#. Alt+Tab to the gnome-terminal window.
+
+Expected Result:
+  The gnome-terminal window should be visible on top.