Code review comment for lp:~smspillaz/compiz-core/compiz-core.work_923683

> GOOD IN THEORY:
>
> * Most of the description sounds like steps in the right direction.
>
> * Simplified: Replacing 559 lines of code with 247.
>
>
> UNSURE ABOUT THE THEORY:
>
> * "composite and opengl now use the geometry last sent to the server"
> This should give maximum performance, sure, but what if (for some reason) the
> server rejects the configure request as being somehow invalid? Would you ever
> encounter that, like moving a window to an invalid location?
> For stability and correctness (which should be the primary goal right now)
> don't we want "composite and opengl now use the geometry last received from
> the server"?

See http://tronche.com/gui/x/xlib/window/XConfigureWindow.html

Since we have SubstructureRedirectMask, it is guarunteed that any ConfigureWindow request we make on a child of the root window is guarunteed to end up in that position (hence the event tracker).

The only error cases I can think of is where you ask for a window to be of a size < 0 or > MAXSHORT. Ideally we should check for that case.

>
> * Caveat #1 is not really a caveat. It's the right (intended) design. But if
> you want to see a (buggy) example of how to solve the chattiness look at the
> timer code here:
> https://code.launchpad.net/~vanvugt/compiz-core/fix-764330-trunk/+merge/86497
> This was slightly buggy though as it broke keyboatrd-initiated moves
> (incorrect pointer warp). Simple to fix, probably.
>

There are some optimizations that can be done here - for example, if a window is grabbed, you can withold sending positions to the server until it is ungrabbed, or there is some other reason why you would want to do that. When that happens you can send it all at once.

> * Still no XFlush's to ensure XConfigureWindow happens immediately. Or are
> there adequate XSyncs already?

The GLib mainloop work introduced an XFlush as soon as we have finished dispatching the X event source.

>
>
> BAD IN THEORY:
>
> * The diff is too big for a mere mortal (who hasn't worked on all the code
> before) to fully load into their mental interpreter and evaluate its
> theoretical correctness. Please read that sentence again; it's important.

Indeed. The good thing is that its mostly all deletion and consolidation.

>
>
> GOOD IN PRACTICE:
>
> * Window movement is smooth and predictable, but not really better than in
> oneiric.
>
>
> BAD IN PRACTICE:
>
> * The bug introduced is worse than the bug being fixed; Window contents offset
> from the frame/decorations whenever a window is created or maximized.

Alright. I haven't seen this yet, but I have an idea of what might cause it. Will have a look into it when I get some time.

>
> * All that code, but we still haven't reduced the lag below oneiric-levels.
> Still not as good as Gnome Shell.

This will be fixed once the chattyness goes away. (as above). Watch your X.org CPU usage as you move windows - it skyrockets because at the moment we flood it with requests.

>
>
> CONCLUSION:
>
> The theory is mostly good but there is at least one serious bug that needs
> fixing. We probably shouldn't risk 0.9.7.0 any more with large changes like
> this. We should aim to get this code into a later release when it's more
> stable, not 0.9.7.0.

Indeed, not for 0.9.7.0