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

Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

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!

review: Needs Resubmitting

« Back to merge proposal