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

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...

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...

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?

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.

Clean-ups all round.

"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...

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