Code review comment for lp:~mc-return/compiz/compiz.merge-fix1082001-gridded-windows-jump-workspaces

> Hey MCR1. Excellent work, here's some comments so far.
>
Hi Sam. First of all thanks for the review. It is very appreciated.

> 8 + CompOption::Vector o(0);
> 9 dndWindow->ungrabNotify ();
> 10
> 11 + screen->handleCompizEvent ("expo", "start_viewport_switch", o);
>
> I believe there is a constant somewhere, either as a static member of
> CompOption or just as a global called "noOptions". I'd suggest using that.
>
I am not sure where to find that, but I'll search...

> 43 - <_long>Warp and resize windows to fit an imaginary
> grid.</_long>
> 44 + <_long>Warp and resize windows to fit an imaginary
> grid</_long>
> 45 <category>Window Management</category>
> 46 <deps>
> 47 <requirement>
> 48 @@ -83,7 +83,7 @@
> 49 <_long>Window resize action</_long>
> 50 <option name="top_left_corner_action"
> type="int">
> 51 <_short>Upper Left Corner</_short>
> 52 - <_long>Action to be performed when window
> is dropped on the top left corner</_long>
> 53 + <_long>Action to be performed when window
> is dropped on the top left corner.</_long>
>
> Try to avoid mixing formatting changes with substantive changes.
>
I am sorry - I just can't help myself. I want to harmonize how plugins and their
tooltips look & feel, so if I am fixing stuff in the xml - I automatically just
have to fix those also...

> 319 - * computing what the 33% and 66% offsets would be
> 320 - */
> 321 + * computing what the 33% and 66% offsets would be
> 322 + */
>
> I think that's probably an undesired formatting change.
>
Not sure, have to check...

> Statements like this:
>
> 377 + if (where & GridLeft || where & GridRight ||
> 378 + where & GridTop || where & GridBottom)
>
> And this:
>
> 308 + (where & ~(GridMaximize) ||
> 309 + (where & ~(GridLeft | GridRight | GridTop | GridBottom)
> &&
>
> Can be combined, eg
>
> unsigned int VerticalGrid = GridLeft | GridRight;
> unsigned int HorizontalGrid = GridTop | GridBottom;
> unsigned int ConstrainedGrid = VerticalGrid | HorizontalGrid | GridMaximize;
>
> if (where & ~ConstrinedGrid)
>
> ...
>
> if (where & VerticalGrid)
> {
> }
> else if (where & HorizontalGrid)
> {
> }
>
Yeah, I did not touch that any further for the sake of the diffs' size and
readability, but if you want me to fix it in this MP, I'll do it...

> 530 + else if (!gw->isGridResized &&
> 531 + gw->isGridHorzMaximized &&
> 532 + !gw->isGridVertMaximized)
> 533 + {
> 534 + /* Window has been horizontally maximized by grid. We only
> need
> 535 + * to restore Y and height - core handles X and width. */
> 536 + if (gw->sizeHintsFlags)
> 537 + gw->window->sizeHints ().flags |= gw->sizeHintsFlags;
> 538 + xwcm |= CWY | CWHeight;
> 539 + }
> 540 + else if (!gw->isGridResized &&
> 541 + !gw->isGridHorzMaximized &&
> 542 + gw->isGridVertMaximized)
> 543 {
> 544 /* Window has been vertically maximized by grid. We only need
> 545 - * to restore the X and width - core handles Y and height. */
> 546 + * to restore X and width - core handles Y and height. */
> 547 if (gw->sizeHintsFlags)
> 548 gw->window->sizeHints ().flags |= gw->sizeHintsFlags;
> 549 xwcm |= CWX | CWWidth;
> 550 }
> 551 -
> 552 - else if (gw->isGridResized && !gw->isGridSemiMaximized)
> 553 - /* Window is just gridded (top, bottom, center, corners). We
> 554 - * need to handle everything. */
> 555 + else if (gw->isGridResized &&
> 556 + !gw->isGridHorzMaximized &&
> 557 + !gw->isGridVertMaximized)
> 558 + /* Window is just gridded (center, corners).
> 559 + * We need to handle everything. */
>
> It might make sense to nest these blocks.
>
> if (!isGridResized)
> {
> assert (isGridHorzMaximized != isGridVertMaximized);
>
> if (isGridHorzMaximized)
> {
> ...
> }
> else if (isGridVertMaximized)
> {
> ...
> }
> }
> else
> {
> assert (!isGridHorzMaximized);
> assert (!isGridVertMaximized);
> ...
> }
>
> Though the use of all these boolean flags has made me wonder if it would be
> better to use an enum instead:
>
> enum CurrentGridState
> {
> None = 0,
> Resized = 1,
> VerticallyMaximized = 2,
> HorizontallyMaximized = 3
> };

I'll see what I can do about that one as well.
Thanks a lot for the review. I am happy we have all currently known Grid bugs
eliminated finally...