Code review comment for lp:~mc-return/compiz/compiz.merge-fix1169353-screenshot-needs-color-options

> Looks mostly good, just a few tips:
>
> 115 +#ifndef USE_GLES
> 116 + glEnable (GL_BLEND);
> 117 +#endif
> 147 -#ifndef USE_GLES
> 148 - glEnable (GL_BLEND);
> 149 -#endif
>
> Its not necessary to move that as it only needs to be enabled when draw
> commands are sent to the GPU. So it can stay in the same place.
>

Question: Do we really need those #ifndefs ?
GLES should have no problem with glEnable (GL_BLEND); and glDisable (GL_BLEND);, no ?

> 119 + float alpha = ((float) optionGetFillColorAlpha () /
> 65535.0f);
>
> This can be replaced with
>
> float alpha = optionGetFillColorAlpha () / static_cast <float>
> (std::numeric_limits <unsigned short>::max ());
>
I removed the redundant (float) casts in r3659.

> Three things to note with that:
> 1. In order to get floating point division, you only need to have the
> denominator be a float and not the numerator.
> 2. std::numeric_limits <unsigned short::max () is the preferable way of
> saying 65536
> 3. static_cast should be preferred over c-style casts (eg (Type));
>

2. AFAIK an unsigned short can have a different size, depending on the
hardware.
I am not convinced this is the best way to say 65535...

Maybe we can find another way ? (see the Ezoom MP also)

>
> Finally, there are a few formatting changes scattered around here (though not
> as much as before). Try to split those two up as much as possible, as the
> areas where formatting changes were made were not really related to what this
> change is doing. They're fine to keep though, as they're correct.

Yeah, sorry about that (again) - but it will get better each time we touch a
specific file.
Once most stuff has been cleaned up, it won't get cleaned up again ;)