Merge lp:~vanvugt/compiz/fix-1084401 into lp:compiz/0.9.9

[Daniel van Vugt]

Fixed: unredirected fullscreen windows sliding offscreen were staying
unredirected (always visible), because compiz thought they were fullscreen
on a different monitor. Add extra smarts to tell the difference between
fullscreen and offscreen. (LP: #1084401)

[Sam Spilsbury (smspillaz)]

This looks correct, a few questions:

+FullscreenRegion::FullscreenRegion (const CompRect &output) :
+ untouched (output),
+ orig (output),
+ allOutputs (output)
+{
+}
+

Is that necessary? It seems like FullscreenRegion cares about the current monitor and all monitors, having this constructor might be misleading

75 + CompRect offscreen1 (2048, 0, 1024, 768);
76 + CompRect offscreen2 (-1024, 0, 1024, 768);

...

88 + EXPECT_TRUE (monitor.allowRedirection (offscreen1));
89 + EXPECT_TRUE (monitor.allowRedirection (offscreen2));

There's not a whole lot of dependency here - can it be moved into a separate test?

[PS Jenkins bot (ps-jenkins)]

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

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

review: Approve (continuous-integration)

[Daniel van Vugt (vanvugt)]

The old constructor is not necessary right now. It's only used to simplify existing test cases. But if you don't like it I can update all the test cases and remove it.

And no; I would prefer to not create a separate test. Doing so only generates more (redundant) code to achieve exactly the same thing. It's perfectly normal for a single test to be comprised of multiple assertions. And for each test to represent a single real-world configuration. As it does right now.

[Sam Spilsbury (smspillaz)]

On Mon, Dec 3, 2012 at 2:36 PM, Daniel van Vugt
<email address hidden> wrote:
> The old constructor is not necessary right now. It's only used to simplify existing test cases. But if you don't like it I can update all the test cases and remove it.

OK, lets keep it, didn't realize it was still in use.

>
> And no; I would prefer to not create a separate test. Doing so only generates more (redundant) code to achieve exactly the same thing. It's perfectly normal for a single test to be comprised of multiple assertions. And for each test to represent a single real-world configuration. As it does right now.

That would be functional testing yes, but not unit testing. Unit
testing is when each test only tests one particular aspect of a
program state. Then when that test fails, it fails only at that
particular level. Having one test that tests-the-world or a larger
subset than just a unit has less resolution for problem diagnosis.

Redundant code was what the test fixtures were designed to solve:

class TestFixture
{
    public:

        virtual void SetUp ()
        {
            ... set up the test "problem"
        }
};

TEST_F (TestFixture, HasStateX)
{
    ASSERT...
}

TEST_F (TestFixture, HasStateY)
{
    ASSERT....
}

I do note that compiz has a mix between unit and functional testing in
our testing areas, so its not that big a deal, just keep it in mind in
light of that.

Otherwise if you really don't want to change it, just say so and I'll
put this in.

> --
> https://code.launchpad.net/~vanvugt/compiz/fix-1084401/+merge/136901
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~vanvugt/compiz/fix-1084401 into lp:compiz.

--
Sam Spilsbury

[Daniel van Vugt (vanvugt)]

On principle I will not change it. You're taking something simple, making it more complex and adding zero value. That's very anti-canonical :)

[Sam Spilsbury (smspillaz)]

On Mon, Dec 3, 2012 at 5:32 PM, Daniel van Vugt
<email address hidden> wrote:
> On principle I will not change it. You're taking something simple, making it more complex and adding zero value. That's very anti-canonical :)

Questionable, but okay, not worth arguing over.

> --
> https://code.launchpad.net/~vanvugt/compiz/fix-1084401/+merge/136901
> Your team Compiz Maintainers is requested to review the proposed merge of lp:~vanvugt/compiz/fix-1084401 into lp:compiz.

--
Sam Spilsbury

[Sam Spilsbury (smspillaz)]

review: Approve

=== modified file 'plugins/opengl/src/fsregion/fsregion.cpp'
--- plugins/opengl/src/fsregion/fsregion.cpp	2012-09-21 09:45:00 +0000
+++ plugins/opengl/src/fsregion/fsregion.cpp	2012-11-29 10:55:23 +0000
@@ -28,9 +28,18 @@
 namespace compiz {
 namespace opengl {
 
-FullscreenRegion::FullscreenRegion (const CompRect &rect) :
-    untouched (rect),
-    orig (untouched)
+FullscreenRegion::FullscreenRegion (const CompRect &output) :
+    untouched (output),
+    orig (output),
+    allOutputs (output)
+{
+}
+
+FullscreenRegion::FullscreenRegion (const CompRect &output,
+                                    const CompRegion &all) :
+    untouched (output),
+    orig (output),
+    allOutputs (all)
 {
 }
 
@@ -56,8 +65,11 @@
 {
     /* Don't allow existing unredirected windows that cover this
      * region to be redirected again as they were probably unredirected
-     * on another monitor */
-    return region.intersects (orig);
+     * on another monitor
+     * Also be careful to not allow unredirection of offscreen windows
+     * (outside of allOutputs).
+     */
+    return region.intersects (orig) || !region.intersects (allOutputs);
 }
 
 } // namespace opengl

=== modified file 'plugins/opengl/src/fsregion/fsregion.h'
--- plugins/opengl/src/fsregion/fsregion.h	2012-09-21 09:45:00 +0000
+++ plugins/opengl/src/fsregion/fsregion.h	2012-11-29 10:55:23 +0000
@@ -42,7 +42,8 @@
 
     typedef unsigned int WinFlags;
 
-    FullscreenRegion (const CompRect &rect);
+    FullscreenRegion (const CompRect &output);
+    FullscreenRegion (const CompRect &output, const CompRegion &all);
 
     // isCoveredBy is called for windows from TOP to BOTTOM
     bool isCoveredBy (const CompRegion &region, WinFlags flags = 0);
@@ -51,6 +52,7 @@
 private:
     CompRegion untouched;
     CompRegion orig;
+    CompRegion allOutputs;
 };
 
 } // namespace opengl

=== modified file 'plugins/opengl/src/fsregion/tests/test-fsregion.cpp'
--- plugins/opengl/src/fsregion/tests/test-fsregion.cpp	2012-09-21 09:45:00 +0000
+++ plugins/opengl/src/fsregion/tests/test-fsregion.cpp	2012-11-29 10:55:23 +0000
@@ -139,13 +139,24 @@
 
 TEST (OpenGLFullscreenRegion, KeepUnredirectedStateIfNotOnMonitor)
 {
-    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
-    CompRegion       window (1025, 0, 1024, 768);
+    CompRect left (0, 0, 1024, 768);
+    CompRect right (1025, 0, 1024, 768);
+    CompRegion all (0, 0, 2048, 768);
+    CompRect offscreen1 (2048, 0, 1024, 768);
+    CompRect offscreen2 (-1024, 0, 1024, 768);
+    CompRegion window (right);
+    FullscreenRegion monitor (left, all);
+
     /* Eg, not covering the monitor, should be redirected */
     EXPECT_FALSE (monitor.isCoveredBy (window));
+
     /* Don't allow the redirection however, because we weren't
      * covering the monitor at all. */
     EXPECT_FALSE (monitor.allowRedirection (window));
+
+    /* Verify off-screen windows are always redirected (eg. other viewports) */
+    EXPECT_TRUE (monitor.allowRedirection (offscreen1));
+    EXPECT_TRUE (monitor.allowRedirection (offscreen2));
 }
 
 TEST (OpenGLFullscreenRegion, MaximizedWithDocks)  // LP: #1053902

=== modified file 'plugins/opengl/src/paint.cpp'
--- plugins/opengl/src/paint.cpp	2012-10-04 09:22:29 +0000
+++ plugins/opengl/src/paint.cpp	2012-11-29 10:55:23 +0000
@@ -276,7 +276,7 @@
 
     if (!(mask & PAINT_SCREEN_NO_OCCLUSION_DETECTION_MASK))
     {
-	FullscreenRegion fs (*output);
+	FullscreenRegion fs (*output, screen->region ());
 
 	/* detect occlusions */
 	for (rit = pl.rbegin (); rit != pl.rend (); ++rit)