Code review comment for lp:~mc-return/compiz/compiz.merge-ezoom-cleanup

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

>
> > 1. The numerator is being cast in floating point division at 433. This is
> not
> > necessary.
>
> Very good catch. Fixed in r3667.
>
> > 2. Replace "65535.0f" with static_cast <float> (std::numeric_limits
> <unsigned
> > short>::max ()). That is a bit verbose, so you can assign it to a const if
> > necessary, eg:
> >
> > const float MaxUShortFloat = std::numeric_limits <unsigned short>::max ();
>
> Don't we have global constants for things like that ?
> I also noticed PI getting defined by several plugins - we should have global
> constants for values like these, which get used several times throughout our
> code...
>
> In composite.h I found:
>
> #define OPAQUE 0xffff
> #define COLOR 0xffff
> #define BRIGHT 0xffff
>
> for example...
>
> Maybe use COLOR in our case ?

So 0xffff is basically MAX_USHRT. It would be better to switch those over to be actual constants one day, but not here. The better place to use those #defines is where you're modifying a CompWindowPaintAttrib as opposed to expressing an alpha value as a fraction (eg "what does 34362 divided by COLOR mean?")

« Back to merge proposal