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

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

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

« Back to merge proposal