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

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

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.

119 + float alpha = ((float) optionGetFillColorAlpha () / 65535.0f);

This can be replaced with

float alpha = optionGetFillColorAlpha () / static_cast <float> (std::numeric_limits <unsigned short>::max ());

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

also:

125 + colorData[3] = alpha * 65535.0f;
becomes

colorData[3] = alpha * std::numeric_limits <unsigned short>::max ();

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.

« Back to merge proposal