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.

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.

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.

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.

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)
{
}

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