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

>
> "One big wall" is currently the default behavior which makes some sense so
> long as the screens have exactly the same resolution. Its *completely* broken
> for multimonitor configurations with different resolutions.

This is not true, just look at this screenshot, which is 3200x1200 downscaled.
It shows Expo in dual-screen mode with 1920x1200 and 1280x1024 screens:
https://bugs.launchpad.net/compiz/+bug/1009592/+attachment/3687163/+files/Expo-2screens-different-resolution-very-usable_scaled_down.png

> There's just no
> sane way to line the views up so that they look correctly when the wall is
> only rendered in one pass. This change should be reverted.
>
Well, here everything lines up fine, while otherwise since r3320 I am hitting
bug #1041822 (video: https://bugs.launchpad.net/compiz/+bug/1041822/+attachment/3370316/+files/Expo2ndDisplayBlack.mp4 ),
which makes Expo completely unusable in "One wall per output" mode.

> I even have misgivings about the way that it works on multimonitor
> configurations where you have the same resolution and a number of viewports
> that is not evenly divisible amongst the monitors. It results in confusing
> situations where viewports end up partially on one monitor and partially on
> another and does not make for a good user experience.
>
Well, this is no problem here at all, see the screenshot here again:
https://bugs.launchpad.net/compiz/+bug/1009592/+attachment/3687163/+files/Expo-2screens-different-resolution-very-usable_scaled_down.png
I am not promoting to remove the "one wall per output" mode, but letting
the user choose the prefered method. Here this is "One big wall".

> 328 + if (event->xkey.keycode == leftKey)
> 329 + moveFocusViewport (-1, 0);
> 330 + else if (event->xkey.keycode == rightKey)
> 331 + moveFocusViewport (1, 0);
>
> This is alignment-formatting taken to its extreme. Alignment can sometimes aid
> readability but it should be used judiciously and not inflexibly. This just
> makes the code unreadable. There is no reason why there should be that much
> whitespace between an if and brackets.
>
Ok, no problem to remove the few spaces...

> 358 + (unsigned int)optionGetDoubleClickTime ()
>
> There should be a space between the cast or better yet a static_cast <unsigned
> int> ()
>
Ok, I'll convert all the casts to static_casts.

> 368 + prevClickPoint = CompPoint(event->xbutton.x,
> event->xbutton.y);
>
> Extra whitespace required.
>
Eagle-eye ;)

> 395 + /* TODO: What action to take if expo_key is not
> defined ? */
>
> An action is always bound to it. That's different from whether or not a
> keybinding is assigned. This todo is inaccurate.
>
I am not sure here -> but I'll re-investigate.
AFAIR removing the Expo Initiate key results in not being able to exit Expo...

> 449 + const int scw = screen->width ();
>
> That variable should be renamed to "screenWidth" - "scw" is ambiguous.
>
ok.

> 448 + const float mpi = M_PI / 180.0f;
>
> This should be renamed degreesToRadians
>
ok.

> 475 + screen->handleEventSetEnabled (this, enable);
> 476 + cScreen->preparePaintSetEnabled (this, enable);
> 477 + cScreen->paintSetEnabled (this, enable);
> 478 + cScreen->donePaintSetEnabled (this, enable);
> 479 + gScreen->glPaintOutputSetEnabled (this, enable);
> 480 gScreen->glPaintTransformedOutputSetEnabled (this, enable);
>
> Whitespace alignment should be applied judiciously.
>
I'll remove the whitespaces here...

> 640 + CompRect input (w->inputRect ());
>
> That can be taken by const & while you're at it.
>
ok.

> 775 + const float sw = (float)screen->width (); //
> screen-width
> 776 + const float sws = sw * sw; //
> screen-width-squared
> 777 + const float rs = curveDistance * curveDistance + 0.25;
> 778 + const float pcd = p1[2] - curveDistance;
> 779 + const float vzs = v[0] * v[0]; // v
> -zero-squared
> 780 + const float vts = v[2] * v[2]; // v
> -two-squared
> 781 + const float vsv = vts * sws + vzs;
>
> Instead of using comments consider using more descriptive variable names.
>
ok.

> Note: This isn't a full review yet, but its what I've found so far.

Thanks.
I did not expect that much TBH ;) Quite some stuff to improve here ;)