Merge lp://qastaging/~smspillaz/compiz-core/fix_894633_geometry_saver_class into lp://qastaging/compiz-core/0.9.5

Hi,

A few notes:

First, I'm not qualified to ensure that your changes actually do what they're supposed to - I know next to nothing about Compiz, and, to be honest, the sight of the compiz code fills me with trepidation. What I *can* do is check for stylistic consistency, and point out any places in the public API where I feel a comment or two would help. My point is that we really need to get someone else who understands compiz to look at these branches.

window.h:
 * ln 219 - comments above preprocessor macros need to be Doxygen-style comments, like the others on the lines above.

 * In your Geometry class, the border() and setBorder() methods should be declared next to each other.

 * The unit tests for this class need to be landed as part of this merge, not later in the pipeline.

As a general rule, #defining things makes me nervous, but it's obviously the "compiz way", so you'd better stick with the established coding standards. I'm nervous because #define ignores all scoping rules.

composite.h:
 * ln 318 - need comment.
 * is it called positionOffset or paintOffset? The comment and the method name should be the same, whichever you pick.

privates.h:
 * ln 125 - need comment.

move.cpp:
 * ln 451 - don't comment out code - delete it. This looks especially odd when compared to the previous revision.

privatewindow.h:
 * ln 82 - it's not obvious to me what the difference is between configureXWindow and reconfigureXWindow - perhaps a comment explaining what the difference is?

That's it!

Please ignore me - I posted my review to the wrong merge proposal.

include/core/windowgeometry.h:

Comments in GeometrySaver class should be doxygen comments - either start them with '///' or '/**' if you're using block comments.

plugins/place/src/place.cpp
You have two instances in this file of code being commented with with a 'XXX' label. If you don't need the code, delete it. At the very least tell us why it's commented out. If you're not sure whether you need the code or not, you need to find out before landing the merge.

src/windowgeometry.cpp
You have a comment that says:

/* XXX: Do not allow geometry to be saved if window
 * has not been placed */

Is this something that needs to be fixed before this lands?

You also have this interesting logic within the push(...) method:

unsigned int useMask = mask & ~mMask;
mMask |= useMask;

Perhaps I'm being dense, but surely this is the same as "mMask |= mask" ?

> include/core/windowgeometry.h:
>
> Comments in GeometrySaver class should be doxygen comments - either start them
> with '///' or '/**' if you're using block comments.
>

+1

> plugins/place/src/place.cpp
> You have two instances in this file of code being commented with with a 'XXX'
> label. If you don't need the code, delete it. At the very least tell us why
> it's commented out. If you're not sure whether you need the code or not, you
> need to find out before landing the merge.
>

+1

> src/windowgeometry.cpp
> You have a comment that says:
>
> /* XXX: Do not allow geometry to be saved if window
> * has not been placed */
>
> Is this something that needs to be fixed before this lands?
>

fixed

> You also have this interesting logic within the push(...) method:
>
> unsigned int useMask = mask & ~mMask;
> mMask |= useMask;
>
> Perhaps I'm being dense, but surely this is the same as "mMask |= mask" ?

useMask is the return value here to indicate which bits were actually pushed (eg the class doesn't allow you to overwrite some saved geometry with the push () method).

Looks good to me except for unnecessary custom c'tors and d'tors in the test fixtures.