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.