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

Sam, first I want to say thanks for finding the time for reviewing this. +1

> 848 + float sx = (float) screen->width () / output->width ();
> 849 + float sy = (float) screen->height () / output->height ();
>
> The denominator should be casted and not the numerator.
>
AFAIR I kept what was there already, see:

825 - float sx = (float) screen->width () / output->width ();
826 - float sy = (float) screen->height () / output->height ();

...but I can change that -> no problem.

> 948 - gScreen->setTextureFilter (GL_LINEAR_MIPMAP_LINEAR);
> 949 + {
> 950 + /* check the actual filtering */
> 951 + oldFilter = gScreen->textureFilter ();
> 952 +
> 953 + /* just change the global filter if necessary */
> 954 + if (oldFilter != GL_LINEAR_MIPMAP_LINEAR)
> 955 + {
> 956 + gScreen->setTextureFilter (GL_LINEAR_MIPMAP_LINEAR);
> 957 + filterChanged = true;
> 958 + }
> 959 + }
>
> This change has no effect - setTextureFilter only changes the *next* texture
> filter any textures will be configured with the next time that they are bound
> to a texture unit, so checking if the filter is already
> GL_LINEAR_MIPMAP_LINEAR will not save any calls into OpenGL, as
> setTextureFilter doesn't make any.
>
Ah -> good to know, thanks for the info. I did not dive that deep into that
part of the code yet...
So setTextureFilter won't make any changes if the filter we want to set is
already set ?

> 1393 - zoomAnim = eScreen->optionGetExpoAnimation () ==
> 1394 - ExpoScreen::ExpoAnimationZoom;
> 1395 - hide = eScreen->optionGetHideDocks () &&
> 1396 - (window->wmType () & CompWindowTypeDockMask);
> 1397 -
> 1398 if (eScreen->expoCam > 0.0)
> 1399 mask |= PAINT_WINDOW_TRANSLUCENT_MASK;
> 1400
> 1401 + float opacity = 1.0;
> 1402 + bool zoomAnim = eScreen->optionGetExpoAnimation () ==
> ExpoScreen::ExpoAnimationZoom;
>
> 1412 - opacity = attrib.opacity *
> 1413 - (1 - sigmoidProgress (eScreen->expoCam));
> 1414 + opacity = attrib.opacity * (1 - sigmoidProgress
> (eScreen->expoCam));
>
> 1861 + int winRealWidth = w->width () + 2 * w->geometry
> ().border () + w->border ().left + w->border ().right;
> 1862 + int winRealHeight = w->height () + 2 * w->geometry
> ().border () + w->border ().top + w->border ().bottom;
> 1863 +
>
> The preference for compiz is to where possible keep line lengths under 80
> characters.
>
Yes, I know that of course ;)
Normally I use the rule to just extend over 80 characters, if it massively
improves readability.
Maybe I messed up here -> I'll investigate.

> It is a somewhat artificially small limit, but it assists those who are
> working on the code in text-based editors.
>
> 1407 - if (hide)
> 1408 + if (eScreen->optionGetHideDocks () &&
> 1409 + (window->wmType () & CompWindowTypeDockMask))
>
> Prefer to keep temporary booleans where they are there for the purpose of
> aiding readability. The compiler gets rid of them at the optimization phase.
>
I'll bring back hide.

> 1834 + /* TODO: Make glowSize configurable via CCSM */
> 1835 + int glowSize = 48;
>
> There is no good reason to make this configurable, especially where the
> texture itself is hardcoded and scales poorly.
>
Well, you are right about the hardcoded texture, but the size of 48 gets downscaled
to about 3 or 4 pixels in Expo, so a configuration value of 24-96 should be acceptable
in this case...

>
> 1861 + int winRealWidth = w->width () + 2 * w->geometry
> ().border ()
>
> compiz::window::Geometry has a helper function widthIncBorders () which does
> exactly this with less error prone typing.
>
I will use that function in this case. Thanks 4 the tip.

> The other changes seem fine so far. Keep in mind that that size of this review
> means that I might have missed things and that I might need to go through it
> again later.

Sure, I won't globally approve this until it gets your green light. :)
Thanks for the review and sorry about the length, it accumulated over time ;)