Merge into 0.9.8 : fix-980663-1041066 : Code : Compiz

Reviewer	Date Requested	Status
Sam Spilsbury	2012-09-04	Approve on 2012-09-04
Compiz Maintainers	2012-09-04	Pending
Review via email: mp+122650@code.launchpad.net

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-08-28: Posted in a previous version of this proposal

#

Seems to work perfectly. But on hold until I figure out if automated testing is possible for this right now, without making the code totally unmaintainable...

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-08-28: Posted in a previous version of this proposal

#

Download full text (4.7 KiB)

115 +/*
116 + * Why mock locally? Because...
117 + * 1. Code is cleaner without typecasts, so we need to ensure that
118 + * Output::Window is-a CompWindow when not mocking.
119 + * 2. No polymorphism is required = fewer classes, less code, easier to follow.
120 + */
121 +#ifdef OUTPUTWINDOW
122 +class MockCompWindow
123 +{
124 +public:
125 + MockCompWindow (unsigned int type, int x, int y, int width, int height) :
126 + flags (type), reg (x, y, width, height) {}
127 + unsigned int type () const { return flags; }
128 + const CompRegion &region () const { return reg; }
129 +
130 +private:
131 + unsigned int flags;
132 + CompRegion reg;
133 +};
134 +#else
135 +#define OUTPUTWINDOW CompWindow
136 +#endif

Yeah, I disagree with this :)

1. It relies on a define for the code to compile the right way
   a. (Unity has code that does this and its not nice to work with)
2. Ifdefs make the code harder to follow in general. The code is doing two different things depending on how it was compiled. This could introduce subtle gotchas.
3. This code is relying on an implicit interface (eg, what the caller is doing with CompWindow). This means that
   when it breaks, it will be non-obvious as to why (eg, instead of being told that MockCompWindow does not
   implement the interface, you get a normal compile error saying that certain functions don't exist on this object
   that #defines itself into existence at compile time).
4. The Window definition conflicts with the Xlib one.

If you don't want to use an interface class (this is a point I agree on), here are some alternatives:

a. Output::occlude is only really interested in three things from CompWindow, firstly whether or not it can take focus (I will deal with this later), and secondly a means to get a CompositeWindow and thirdly its covering region. You can do something like this:

typedef enum _FocusableState
{
CanTakeFocus = 0,
NoFocusAllowed = 1
} FocusableState;

//
Output::occlude (FocusableState focusState,
                  CompRegion const & reg,
                  Window win) // Using Window in terms of its XLib meaning here ...
{
57 +
58 + if (!fullscreen &&
              focusState == CanTakeFocus && // Skip desktop etc
60 + reg == untouched)
61 + {
62 + fullscreen = win;
63 + }
64 + untouched -= reg;
}

Window
Output::fullscreenWindow ()
{
return Window;
}

// not under test
FocusableState
compiz::opengl::WindowTypeToFocusableState (CompWindow *window)
{
if (window->type () & NO_FOCUS_MASK)
return NoFocusAllowed;

return CanTakeFocus;
}

Use the code like this:

FocusableState focusState = WindowTypeToFocusableState (window);
output.occlude (focusState, window->id (), window->region ());

Window fullscreenWindow = output.fullscreenWindow ();
CompWindow *fullscreenCompWindow = screen->findWindow (fullscreenWindow);
CompositeWindow *compositeFullscreenWindow = CompositeWindow::get (fullscreenCompWindow);

compositeFullscreenWindow->unredirect ();

I personally don't like using the Xlib Window handle as a means to identify windows within compiz, and would personally prefer that compiz::opengl::Output used an interface like RedirectedWindow (with metho...

115	+/*
116	+ * Why mock locally? Because...
117	+ * 1. Code is cleaner without typecasts, so we need to ensure that
118	+ * Output::Window is-a CompWindow when not mocking.
119	+ * 2. No polymorphism is required = fewer classes, less code, easier to follow.
120	+ */
121	+#ifdef OUTPUTWINDOW
122	+class MockCompWindow
123	+{
124	+public:
125	+ MockCompWindow (unsigned int type, int x, int y, int width, int height) :
126	+ flags (type), reg (x, y, width, height) {}
127	+ unsigned int type () const { return flags; }
128	+ const CompRegion &region () const { return reg; }
129	+
130	+private:
131	+ unsigned int flags;
132	+ CompRegion reg;
133	+};
134	+#else
135	+#define OUTPUTWINDOW CompWindow
136	+#endif

Yeah, I disagree with this :)

1. It relies on a define for the code to compile the right way
   a. (Unity has code that does this and its not nice to work with)
2. Ifdefs make the code harder to follow in general. The code is doing two different things depending on how it was compiled. This could introduce subtle gotchas.
3. This code is relying on an implicit interface (eg, what the caller is doing with CompWindow). This means that
   when it breaks, it will be non-obvious as to why (eg, instead of being told that MockCompWindow does not
   implement the interface, you get a normal compile error saying that certain functions don't exist on this object
   that #defines itself into existence at compile time).
4. The Window definition conflicts with the Xlib one.

If you don't want to use an interface class (this is a point I agree on), here are some alternatives:

a. Output::occlude is only really interested in three things from CompWindow, firstly whether or not it can take focus (I will deal with this later), and secondly a means to get a CompositeWindow and thirdly its covering region. You can do something like this:

typedef enum _FocusableState
 {
     CanTakeFocus = 0,
     NoFocusAllowed = 1
 } FocusableState;

// 
 Output::occlude (FocusableState focusState,
                  CompRegion const & reg,
                  Window win) // Using Window in terms of its XLib meaning here ...
 {
57	+
58	+ if (!fullscreen &&
              focusState == CanTakeFocus && // Skip desktop etc
60	+     reg == untouched)
61	+ {
62	+     fullscreen = win;
63	+ }
64	+ untouched -= reg;
 }

Window
Output::fullscreenWindow ()
{
    return Window;
}

// not under test
FocusableState
compiz::opengl::WindowTypeToFocusableState (CompWindow *window)
{
    if (window->type () & NO_FOCUS_MASK)
        return NoFocusAllowed;

return CanTakeFocus;
}

Use the code like this:

FocusableState focusState = WindowTypeToFocusableState (window);
output.occlude (focusState, window->id (), window->region ());

Window fullscreenWindow = output.fullscreenWindow ();
CompWindow *fullscreenCompWindow = screen->findWindow (fullscreenWindow);
CompositeWindow *compositeFullscreenWindow = CompositeWindow::get (fullscreenCompWindow);

compositeFullscreenWindow->unredirect ();

I personally don't like using the Xlib Window handle as a means to identify windows within compiz, and would personally prefer that compiz::opengl::Output used an interface like RedirectedWindow (with methods redirect and unredirect) which CompositeWindow implements or somesuch in order to get a window that we can unredirect. That being said, an interface with two methods seems weird when we could just go all the way. Your call really.

Functionality wise:

Its not clear why a window that can't take focus has much to do with finding the topmost window to unredirect. In your initial version it says its there to ignore desktops, however I think it actually makes sense to call glPaint with PAINT_WINDOW_OCCLUSION_DETECTION_MASK to figure out what is on top of them (noting here that we can make the unity windows return true here), and unredirect the desktop window if its the top most window. No point wasting time compositing for the single-window case I say.

In any case, if you really don't want desktops to be unredirected, it might be better to explicitly not unredirect them by doing type () & CompWindowTypeDesktopMask

Syntax and Naming Wise:

Personally I think the name "Output" is not descriptive of what it does. In addition there's some room for confusion with what CompOutput does (which is different). May I suggest "NonOccludedScreenArea" ?

162	+#include "./output.h"

It would be better to do this:

include_directories (${CMAKE_CURRENT_SORUCE_DIR})

in the toplevel CMakeLists.txt

240	+add_executable(test-output test-output.cpp ../output.cpp)

Please use something like compiz_opengl_test_output . I prefer to have the executables namespaces.

242	+add_test(OutputTests test-output)

Please use compiz_discover_tests () here (see other test files for examples). We want the full results in CTest.

review: Needs Fixing

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-08-29: Posted in a previous version of this proposal

#

Actually, I thought of all of that already and disagree with almost all of it :)

I'll try to find a nicer way to hide and clean up the mocking. However changing a class' public interface is usually a mistake. Test cases can change how you do your implementation, but should never change the interfaces.

I'm not using compiz_discover_tests because I hate the fact that it necessitates excessively deep directories (plugins/NAME/src/LIBNAME/src/stuff.cpp). Also, what does compiz_discover_tests give me if I need to create more files, directories and complexity to use it?

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-08-29: Posted in a previous version of this proposal

#

Also, test-output is in a namespace already by virtue of its directory:
build/plugins/opengl/src/tests/test-output

However I will rename it internally at least so you can tell what and where it is just from the CTest output.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-08-29: Posted in a previous version of this proposal

#

Clean-ups all round.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-08-29: Posted in a previous version of this proposal

#

Download full text (3.2 KiB)

"Actually, I thought of all of that already and disagree with almost all of it :)"

Could you elaborate? I didn't say that you needed to change any public interfaces.

This is still very confusing:

380 +#ifdef MOCK_COMPWINDOW
381 + typedef Stackable Window;
382 +#else
383 + typedef CompWindow Window;
384 +#endif

The code is compiled twice, and you have an abstract base class in one version and a concrete class in another. What this essentially is, is a template implemented in terms of #define.

If you're going to create an abstract base class, why not just go all the way and make PrivateGLWindow inherit Stackable, put a redirect method in Stackable and make it call through to PrivateGLWindow? That way you don't need to rely on a define, and you only need to compile this code once.

96 +include_directories(${GTEST_INCLUDE_DIRS} ..)
97 +add_definitions(-DMOCK_COMPWINDOW)
98 +set(exe "compiz_opengl_test_windowstack")
99 +add_executable(${exe} test-windowstack.cpp ../windowstack.cpp)
100 +target_link_libraries(${exe} compiz_core ${GTEST_BOTH_LIBRARIES})
101 +add_test(OpenGLWindowStackTests ${exe})

"I'm not using compiz_discover_tests because I hate the fact that it necessitates excessively deep directories (plugins/NAME/src/LIBNAME/src/stuff.cpp). Also, what does compiz_discover_tests give me if I need to create more files, directories and complexity to use it?"

It doesn't necessitate that at all. The tests are only organized that way because this is the way I happened to organize them. (Also, when in doubt, its a good idea to be consistent rather than go against the grain - I'm happy to change the organization if we can agree on a better way).

Here's the syntax for compiz_discover_tests.

compiz_discover_tests (test_executable COVERAGE library_that_the_test_uses_1 library_2 ...)

The coverage argument is optional, but does require that the tested code goes into a separate library (which you should do anyways because currently the code is compiled twice, which is the exact problem that unity has which I am trying to avoid).

I strongly recommend using this function. It has the following purposes:

1. It adds every individual test to the CTest manifest, meaning that you always have a perfectly clean environment on setup and teardown (defend against subtle bugs where tests are stateful and rely on each other when they shouldn't).
2. You have per-test resolution when something fails from "make test"
3. Its the best way to get your code added to the coverage report, so that you can see if your tests are actually excersizing the code all round. (You can add to the coverage report yourself, however that's several hundred lines of CMake code).

Another good reason:
4. If someone else adds a test using compiz_discover_tests there is a race in CMake where the CTestFile for this test will be clobbered.

"WindowStack" also feels weird. Its not a window stack - its a class that accumulates uncovered screen regions to determine which windows are not occluded. Also "stackable" in the namespace compiz::opengl feels weird too. Its not a "stackable", in compiz terms "stackable" means something more like a window that sits in a stack and can be re-arranged. I...

"Actually, I thought of all of that already and disagree with almost all of it :)"

Could you elaborate? I didn't say that you needed to change any public interfaces.

This is still very confusing:

380	+#ifdef MOCK_COMPWINDOW
381	+ typedef Stackable Window;
382	+#else
383	+ typedef CompWindow Window;
384	+#endif

The code is compiled twice, and you have an abstract base class in one version and a concrete class in another. What this essentially is, is a template implemented in terms of #define.

If you're going to create an abstract base class, why not just go all the way and make PrivateGLWindow inherit Stackable, put a redirect method in Stackable and make it call through to PrivateGLWindow? That way you don't need to rely on a define, and you only need to compile this code once.

96	+include_directories(${GTEST_INCLUDE_DIRS} ..)
97	+add_definitions(-DMOCK_COMPWINDOW)
98	+set(exe "compiz_opengl_test_windowstack")
99	+add_executable(${exe} test-windowstack.cpp ../windowstack.cpp)
100	+target_link_libraries(${exe} compiz_core ${GTEST_BOTH_LIBRARIES})
101	+add_test(OpenGLWindowStackTests ${exe})

"I'm not using compiz_discover_tests because I hate the fact that it necessitates excessively deep directories (plugins/NAME/src/LIBNAME/src/stuff.cpp). Also, what does compiz_discover_tests give me if I need to create more files, directories and complexity to use it?"

It doesn't necessitate that at all. The tests are only organized that way because this is the way I happened to organize them. (Also, when in doubt, its a good idea to be consistent rather than go against the grain - I'm happy to change the organization if we can agree on a better way).

Here's the syntax for compiz_discover_tests.

compiz_discover_tests (test_executable COVERAGE library_that_the_test_uses_1 library_2 ...)

The coverage argument is optional, but does require that the tested code goes into a separate library (which you should do anyways because currently the code is compiled twice, which is the exact problem that unity has which I am trying to avoid).

I strongly recommend using this function. It has the following purposes:

1. It adds every individual test to the CTest manifest, meaning that you always have a perfectly clean environment on setup and teardown (defend against subtle bugs where tests are stateful and rely on each other when they shouldn't).
2. You have per-test resolution when something fails from "make test"
3. Its the best way to get your code added to the coverage report, so that you can see if your tests are actually excersizing the code all round. (You can add to the coverage report yourself, however that's several hundred lines of CMake code).

Another good reason:
4. If someone else adds a test using compiz_discover_tests there is a race in CMake where the CTestFile for this test will be clobbered.

"WindowStack" also feels weird. Its not a window stack - its a class that accumulates uncovered screen regions to determine which windows are not occluded. Also "stackable" in the namespace compiz::opengl feels weird too. Its not a "stackable", in compiz terms "stackable" means something more like a window that sits in a stack and can be re-arranged. In this part of the code, it is something that occludes another region, or can occlude it.

review: Needs Fixing

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-08-29: Posted in a previous version of this proposal

#

I apologize if I seem ... opinionated, I'm just trying to distill what the intention is and the best way of effecting that intention

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-08-31: Posted in a previous version of this proposal

#

Looking much better without the #define.

I think that Output can take the X11 definition of a "Window" (eg, typedef unsigned int Window) rather than using void *, it means the same thing in both instances and makes the code slightly more clear as to what is being used to identify the window. There's no need to link to xlib for this, all there is a header dependency on <X11/Xlib.h>

Now that Output is taking references to the data that it needs to use to identify which window is on top, there's not much need for MockCompWindow anymore. The boolean parameter in Output can be changed to an enum, eg

typedef enum _FocusAllowedState
{
NoFocusAllowed,
FocusAllowed
}

Output::occlude (FocusAllowed, region, window) is more expressive than Output::occlude (true, region, window).

Finally, it would be good if output.cpp was a static library that was linked into opengl. I've tried to avoid setting the trend of compiling any source file more than once in the tree (like Unity does), and would like to keep the source tree this way.

compiz_discover_tests(${exe})

If you are using another static library, you can create coverage targets as so

compiz_discover_tests(${exe} COVERAGE compiz_opengl_output_occlusion_detection)

That will ensure that you can get coverage reports from "make coverage"

The preferred method of addressing source files in a cmake tree is this:

${CMAKE_CURRENT_SOURCE_DIR}/../file.cpp not ../file.cpp

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-08-31: Posted in a previous version of this proposal

#

If possible by the way, see if you can find a better name for "Output". I still think there's some confusion between "Output" and "CompOutput" - the responsibility of this class seems to be finding a fullscreen window to unredirect based on which windows are not occluded.

I did mention it before, but I find that difficulty in finding a name for a class might indicate that its doing too many unrelated things. In this case, the class is checking for a window to unredirect and then offering a way to get the result. Perhaps it would be better if it stored an interface by which the window could be unredirected and then provided a method to call into the class to unredirect the saved fullscreen window. That way the class could be called "UnoccludedWindowUnredirector". Yes I know you don't like -er, but this is still a noun.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-09-04: Posted in a previous version of this proposal

#

Lots of clean-ups and renaming.

I disagree with the bi-state enumeration and think a boolean is clearer in cases like this where you can use it.

I disagree with the suggested approach to coverage where you have to put code in a static library to get coverage data. Getting coverage information should be transparently possible for all test cases. I'm surprised it's not done that way already, but we can improve that later. See also bug 1045665.

And I disagree with putting a single, single-use class in a separate library just to avoid recompiling it in the test cases. Clarity and a simpler source tree is more important.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-09-04: Posted in a previous version of this proposal

#

> Lots of clean-ups and renaming.
>
> I disagree with the bi-state enumeration and think a boolean is clearer in
> cases like this where you can use it.

How is:

isCoveredBy (region, true);

Any more clear than

isCoveredBy (region, FullscreenWindow); or isCoveredBy (region, NormalWindow);

Read it aloud.

"is covered by region true" vs "is covered by region, a FullscreenWindow" "is covered by region, a NormalWindow".

Not blocking on this, but please consider it.

>
> I disagree with the suggested approach to coverage where you have to put code
> in a static library to get coverage data. Getting coverage information should
> be transparently possible for all test cases. I'm surprised it's not done that
> way already, but we can improve that later. See also bug 1045665.

Right, the /current/ coverage system doesn't support this case. In fact, if you pass it the testcase as the coverage target, you should be able to get a coverage report on that file though:

>
> And I disagree with putting a single, single-use class in a separate library
> just to avoid recompiling it in the test cases. Clarity and a simpler source
> tree is more important.

add_library (compiz_opengl_fullscreen_region fullscreenregion.cpp)
target_link_libraries (compiz_opengl_fullscreen_region compiz_region)

...

compiz_plugin (opengl ... LIBRARIES compiz_opengl_fullscreen_region ...)

The source tree doesn't exactly have to be reorganized to make it work.

We aren't using autotools - any buildsystem that requires you to compile a file twice is a travesty and I won't allow this trend to be set in compiz. Unity does exactly this and it annoys me greatly - it takes at least fifteen minutes longer than it should to compile because nobody took the time to ensure that files were only compiled once.

Revision history for this message

Daniel van Vugt (vanvugt) wrote on 2012-09-04: Posted in a previous version of this proposal

#

I don't care any more. I'm willing to compromise any which way to get this merged. A week on a fix that took me 10 minutes is crazy...

WIP again.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-09-04:

#

All looking good now.

review: Approve

Revision history for this message

Unity Merger (unity-merger) wrote on 2012-09-04:

#

The Jenkins job https://jenkins.qa.ubuntu.com/job/automerge-compiz-core/109/console reported an error when processing this lp:~vanvugt/compiz/fix-980663-1041066 branch.
Not merging it.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-09-05:

#

So I can't believe this, but the automerge still has -DCOMPIZ_BUILD_TESTING=OFF. You'll need to do this:

if (COMPIZ_BUILD_TESTING)
add_subdirectory (tests)
endif (COMPIZ_BUILD_TESTING)

I note that we should probably develop a cmake file with macros for this stuff.

Revision history for this message

Sam Spilsbury (smspillaz) wrote on 2012-09-05:

#

Once that's fixed, just approve the branch again (though the chances are it won't merge anyways as jenkins is out of space)

lp:~vanvugt/compiz/fix-980663-1041066 updated on 2012-09-05

3346. By Daniel van Vugt on 2012-09-05: Building of tests is conditional, apparently. This fixes a Jenkins failure.

Compiz

Merge lp:~vanvugt/compiz/fix-980663-1041066 into lp:compiz/0.9.8

Commit message

Description of the change

Preview Diff

Subscribers

 === modified file 'plugins/opengl/CMakeLists.txt'
 --- plugins/opengl/CMakeLists.txt	2012-08-29 08:41:58 +0000
 +++ plugins/opengl/CMakeLists.txt	2012-09-05 03:21:22 +0000
@@ -3,9 +3,12 @@
  include (CompizPlugin)
  set (INTERNAL_LIBRARIES
--     compiz_opengl_double_buffer)
++    compiz_opengl_double_buffer
++    compiz_opengl_fsregion
++)
  add_subdirectory (src/doublebuffer)
++add_subdirectory (src/fsregion)
  if (USE_GLES)
      compiz_plugin(opengl PLUGINDEPS composite CFLAGSADD "-DUSE_GLES -std=c++0x" LIBRARIES ${OPENGLES2_LIBRARIES} ${INTERNAL_LIBRARIES} dl INCDIRS ${OPENGLES2_INCLUDE_DIR})
 === added directory 'plugins/opengl/src/fsregion'
 === added file 'plugins/opengl/src/fsregion/CMakeLists.txt'
 --- plugins/opengl/src/fsregion/CMakeLists.txt	1970-01-01 00:00:00 +0000
 +++ plugins/opengl/src/fsregion/CMakeLists.txt	2012-09-05 03:21:22 +0000
@@ -0,0 +1,7 @@
++if (COMPIZ_BUILD_TESTING)
++add_subdirectory (tests)
++endif ()
++
++add_library (compiz_opengl_fsregion STATIC fsregion.cpp)
++target_link_libraries (compiz_opengl_fsregion compiz_core)
++
 === added file 'plugins/opengl/src/fsregion/fsregion.cpp'
 --- plugins/opengl/src/fsregion/fsregion.cpp	1970-01-01 00:00:00 +0000
 +++ plugins/opengl/src/fsregion/fsregion.cpp	2012-09-05 03:21:22 +0000
@@ -0,0 +1,54 @@
++/*
++ * Compiz opengl plugin, FullscreenRegion class
++ *
++ * Copyright (c) 2012 Canonical Ltd.
++ * Author: Daniel van Vugt <daniel.van.vugt@canonical.com>
++ *
++ * Permission is hereby granted, free of charge, to any person obtaining a
++ * copy of this software and associated documentation files (the "Software"),
++ * to deal in the Software without restriction, including without limitation
++ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
++ * and/or sell copies of the Software, and to permit persons to whom the
++ * Software is furnished to do so, subject to the following conditions:
++ *
++ * The above copyright notice and this permission notice shall be included in
++ * all copies or substantial portions of the Software.
++ *
++ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
++ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
++ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
++ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
++ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
++ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
++ * DEALINGS IN THE SOFTWARE.
++ */
++
++#include "fsregion.h"
++
++namespace compiz {
++namespace opengl {
++
++FullscreenRegion::FullscreenRegion (const CompRect &rect) :
++    covered (false),
++    untouched (rect)
++{
++}
++
++bool
++FullscreenRegion::isCoveredBy (const CompRegion &region, WinType type)
++{
++    bool fullscreen = false;
++
++    if (!covered && type == Normal && region == untouched)
++    {
++	covered = true;
++	fullscreen = true;
++    }
++
++    untouched -= region;
++
++    return fullscreen;
++}
++
++} // namespace opengl
++} // namespace compiz
 === added file 'plugins/opengl/src/fsregion/fsregion.h'
 --- plugins/opengl/src/fsregion/fsregion.h	1970-01-01 00:00:00 +0000
 +++ plugins/opengl/src/fsregion/fsregion.h	2012-09-05 03:21:22 +0000
@@ -0,0 +1,55 @@
++/*
++ * Compiz opengl plugin, FullscreenRegion class
++ *
++ * Copyright (c) 2012 Canonical Ltd.
++ * Author: Daniel van Vugt <daniel.van.vugt@canonical.com>
++ *
++ * Permission is hereby granted, free of charge, to any person obtaining a
++ * copy of this software and associated documentation files (the "Software"),
++ * to deal in the Software without restriction, including without limitation
++ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
++ * and/or sell copies of the Software, and to permit persons to whom the
++ * Software is furnished to do so, subject to the following conditions:
++ *
++ * The above copyright notice and this permission notice shall be included in
++ * all copies or substantial portions of the Software.
++ *
++ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
++ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
++ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
++ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
++ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
++ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
++ * DEALINGS IN THE SOFTWARE.
++ */
++
++#ifndef __COMPIZ_OPENGL_FSREGION_H
++#define __COMPIZ_OPENGL_FSREGION_H
++#include "core/rect.h"
++#include "core/region.h"
++
++namespace compiz {
++namespace opengl {
++
++class FullscreenRegion
++{
++public:
++    typedef enum
++    {
++	Normal,
++	Desktop
++    } WinType;
++
++    FullscreenRegion (const CompRect &rect);
++
++    // isCoveredBy is called for windows from TOP to BOTTOM
++    bool isCoveredBy (const CompRegion &region, WinType type = Normal);
++
++private:
++    bool covered;
++    CompRegion untouched;
++};
++
++} // namespace opengl
++} // namespace compiz
++#endif
 === added directory 'plugins/opengl/src/fsregion/tests'
 === added file 'plugins/opengl/src/fsregion/tests/CMakeLists.txt'
 --- plugins/opengl/src/fsregion/tests/CMakeLists.txt	1970-01-01 00:00:00 +0000
 +++ plugins/opengl/src/fsregion/tests/CMakeLists.txt	2012-09-05 03:21:22 +0000
@@ -0,0 +1,10 @@
++include_directories (${GTEST_INCLUDE_DIRS} ..)
++set (exe "compiz_opengl_test_fsregion")
++add_executable (${exe} test-fsregion.cpp)
++target_link_libraries (${exe}
++    compiz_opengl_fsregion
++    compiz_core
++    ${GTEST_BOTH_LIBRARIES}
++)
++compiz_discover_tests(${exe} COVERAGE compiz_opengl_fsregion)
++
 === added file 'plugins/opengl/src/fsregion/tests/test-fsregion.cpp'
 --- plugins/opengl/src/fsregion/tests/test-fsregion.cpp	1970-01-01 00:00:00 +0000
 +++ plugins/opengl/src/fsregion/tests/test-fsregion.cpp	2012-09-05 03:21:22 +0000
@@ -0,0 +1,121 @@
++/*
++ * Compiz opengl plugin, FullscreenRegion class
++ *
++ * Copyright (c) 2012 Canonical Ltd.
++ * Author: Daniel van Vugt <daniel.van.vugt@canonical.com>
++ *
++ * Permission is hereby granted, free of charge, to any person obtaining a
++ * copy of this software and associated documentation files (the "Software"),
++ * to deal in the Software without restriction, including without limitation
++ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
++ * and/or sell copies of the Software, and to permit persons to whom the
++ * Software is furnished to do so, subject to the following conditions:
++ *
++ * The above copyright notice and this permission notice shall be included in
++ * all copies or substantial portions of the Software.
++ *
++ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
++ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
++ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
++ * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
++ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
++ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
++ * DEALINGS IN THE SOFTWARE.
++ */
++
++#include "gtest/gtest.h"
++#include "fsregion.h"
++
++using namespace compiz::opengl;
++
++TEST (OpenGLFullscreenRegion, NoWindows)
++{
++    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
++                                       FullscreenRegion::Desktop));
++}
++
++TEST (OpenGLFullscreenRegion, OneFullscreen)
++{
++    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
++    EXPECT_TRUE  (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
++                                       FullscreenRegion::Desktop));
++}
++
++TEST (OpenGLFullscreenRegion, FullscreenNoDesktop)
++{
++    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
++    EXPECT_TRUE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++}
++
++TEST (OpenGLFullscreenRegion, NormalWindows)
++{
++    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (10, 10, 40, 30)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (20, 20, 50, 20)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
++                                       FullscreenRegion::Desktop));
++}
++
++TEST (OpenGLFullscreenRegion, TwoFullscreen)
++{
++    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
++    EXPECT_TRUE  (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (10, 10, 40, 30)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (20, 20, 50, 20)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
++                                       FullscreenRegion::Desktop));
++}
++
++TEST (OpenGLFullscreenRegion, Offscreen)
++{
++    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (-100, -100, 1, 1)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (2000, 2000, 123, 456)));
++    EXPECT_TRUE  (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (10, 10, 40, 30)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (20, 20, 50, 20)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
++                                       FullscreenRegion::Desktop));
++}
++
++TEST (OpenGLFullscreenRegion, CancelFullscreen1)
++{
++    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (500, 500, 345, 234)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (10, 10, 40, 30)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (20, 20, 50, 20)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
++                                       FullscreenRegion::Desktop));
++}
++
++TEST (OpenGLFullscreenRegion, CancelFullscreen2)
++{
++    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (-100, -100, 1, 1)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (2000, 2000, 123, 456)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (500, 500, 345, 234)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (10, 10, 40, 30)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (20, 20, 50, 20)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
++                                       FullscreenRegion::Desktop));
++}
++
++TEST (OpenGLFullscreenRegion, Overflow)
++{
++    FullscreenRegion monitor (CompRect (0, 0, 1024, 768));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (10, 10, 40, 30)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (-10, -10, 1044, 788)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768)));
++    EXPECT_FALSE (monitor.isCoveredBy (CompRegion (0, 0, 1024, 768),
++                                       FullscreenRegion::Desktop));
++}
++
++
 === modified file 'plugins/opengl/src/paint.cpp'
 --- plugins/opengl/src/paint.cpp	2012-08-30 09:05:04 +0000
 +++ plugins/opengl/src/paint.cpp	2012-09-05 03:21:22 +0000
@@ -38,9 +38,12 @@
  #include <opengl/opengl.h>
  #include "privates.h"
++#include "fsregion/fsregion.h"
  #define DEG2RAD (M_PI / 180.0f)
++using namespace compiz::opengl;
++
  GLScreenPaintAttrib defaultScreenPaintAttrib = {
 .0f, 0.0f, 0.0f, 0.0f, 0.0f, 0.0f, -DEFAULT_Z_CAMERA
  };
@@ -241,7 +244,7 @@
      CompRegion    tmpRegion (region);
      CompWindow    *w;
      GLWindow      *gw;
--    int		  count, windowMask, odMask;
++    int           windowMask, odMask;
      CompWindow	  *fullscreenWindow = NULL;
      bool          status, unredirectFS;
      bool          withOffset = false;
@@ -257,12 +260,10 @@
      if (mask & PAINT_SCREEN_TRANSFORMED_MASK)
+     {
  	windowMask     = PAINT_WINDOW_ON_TRANSFORMED_SCREEN_MASK;
--	count	       = 1;
+     }
      else
+     {
  	windowMask     = 0;
--	count	       = 0;
+     }
      /*
@@ -274,6 +275,8 @@
      if (!(mask & PAINT_SCREEN_NO_OCCLUSION_DETECTION_MASK))
+     {
++	FullscreenRegion fs (*output);
++
  	/* detect occlusions */
  	for (rit = pl.rbegin (); rit != pl.rend (); ++rit)
+ 	{
@@ -336,23 +339,18 @@
  		    tmpRegion -= w->region ();
  		/* unredirect top most fullscreen windows. */
--		if (count == 0 && unredirectFS)
++		FullscreenRegion::WinType type =
++		    w->type () & CompWindowTypeDesktopMask ?
++		    FullscreenRegion::Desktop :
++		    FullscreenRegion::Normal;
++
++		if (unredirectFS &&
++		    !(mask & PAINT_SCREEN_TRANSFORMED_MASK) &&
++		    fs.isCoveredBy (w->region (), type))
+ 		{
--		    if (w->region () == screen->region () &&
--			tmpRegion.isEmpty ())
--		    {
--			fullscreenWindow = w;
--		    }
--		    else
--		    {
--			foreach (CompOutput &o, screen->outputDevs ())
--			    if (w->region () == CompRegion (o))
--				fullscreenWindow = w;
--		    }
++		    fullscreenWindow = w;
+ 		}
+ 	    }
--
--	    count++;
+ 	}
+     }