Code review comment for lp:~compiz-team/compiz/compiz.performance_1027211.2

Revision history for this message
Sam Spilsbury (smspillaz) wrote :

> It will take me a while to get the code compiled 32-bit and running on the
> Atom where I tested all the previous lazy-mode regressions. Until then, some
> style issues:
>
> 1. This is hard to read. I recommend (but don't require) all on one line...
> 48 +namespace compiz
> 49 +{
> 50 +namespace window
> 51 +{
> 52 +namespace configure_buffers
> 53 +{
>
> 2. MOVE_WINDOW (w) - Using macros to declare local variables via side-effect
> is dangerous and very hard to understand if you're new to compiz. I suggest
> not doing this in future code. Instead use an explicit:
> MoveWindow *mw = MoveWindow::get(w);

+1

>
> 3. From a high-level perspective, even if the implementation turns out to be
> right then I think maybe more analysis was required. This fix is an example of
> where the medicine could be worse than the disease. It's only useful to nvidia
> because nvidia is buggy IMHO. And the price to pay is more abstraction and
> complication that hinders maintainability of the code. We might be better off
> not trying to "fix" window movement any further. Though resizing needs work
> still.

It is a little more abstract and complex, yes, however having an unusable system on nvidia is not an acceptable alternative either.

I don't think the code is "unmaintainable", I think it just works in a way that is a tad more complex and different to the way that you would expect. Its fully tested and encapsulated from the other code, so future maintainers don't need to worry about inadvertently breaking it (except for their usage of the client hooks).

In addition, if there are any problems you can just "turn it off" - just don't take a lock on the configure request queue in the opengl plugin.

Speaking of medicine worse than the disease, you just named one of my favourite pieces of music :P (http://www.youtube.com/watch?v=htGmTJNsOr0)

« Back to merge proposal