Merge lp://qastaging/~mc-return/compiz/compiz.merge-fix1165198-horizontally-semimaximized-windows-do-not-snap-off-easily into lp://qastaging/compiz/0.9.10

128 + int x = wX + dx - w->border ().left;
129 + int y = wY + dy - w->border ().top;
130 + int width = wWidth + w->border ().left + w->border ().right;
131 + int height = w->border ().top ? w->border ().top : 1;
132 +
133 + int status = XRectInRegion (ms->region, x, y,
134 + (unsigned int) width,
135 + (unsigned int) height);

Can you align wX, wY, wWidth and w->border (), they've become de-aligned.

+ CompRect workArea = s->getWorkareaForOutput (w->outputDevice ());

This is better written as

CompRect workArea (s->getWorkareaForOutput (w->outputDevice ());

153 + if (w->saveMask ()& CWWidth)

You'll need a space between w->saveMask () and "&"

159 + ms->x = ms->y = 0;
160 +

Will that cause the window to be moved back to 0,0 ? I haven't got time to check at the moment.

> 128 + int x = wX + dx - w->border ().left;
> 129 + int y = wY + dy - w->border ().top;
> 130 + int width = wWidth + w->border ().left +
> w->border ().right;
> 131 + int height = w->border ().top ? w->border ().top :
> 1;
> 132 +
> 133 + int status = XRectInRegion (ms->region, x, y,
> 134 + (unsigned int) width,
> 135 + (unsigned int)
> height);
>
> Can you align wX, wY, wWidth and w->border (), they've become de-aligned.
>
Done.

> + CompRect workArea = s->getWorkareaForOutput (w->outputDevice ());
>
> This is better written as
>
> CompRect workArea (s->getWorkareaForOutput (w->outputDevice ());
>
Done.

> 153 + if (w->saveMask ()& CWWidth)
>
> You'll need a space between w->saveMask () and "&"
>
Done.

> 159 + ms->x = ms->y = 0;
> 160 +
>
> Will that cause the window to be moved back to 0,0 ? I haven't got time to
> check at the moment.
>

Effect is that the center of the window's titlebar gets moved to the mousepointer,
when it snaps off, no matter where the mousepointer is...
This behaviour is identical no matter if using Emerald and gtk-window-decorator.

So behaviour is quite okay as the user mostly pulls off the window somewhere along
the titlebar, it can feel a bit jumpy if the user decides to pull the window off by
pulling the left corner, but this is another problem for another day...
(the same happens when pulling off vertically maxed windows in trunk)

So this MP just imitates the successful method for pulling off vertically maximized
windows by using x instead of y.

Actually, now that I've looked at this in context, I can see why it is correct, but probably in need of some refactoring.

    if (w->state () & CompWindowStateMaximizedVertMask)
    {
        if (abs (yRoot - workArea.y () - ms->snapOffY) >= SNAP_OFF)
       if (!s->otherGrabExist ("move", NULL))
       {
         int width = w->serverGeometry ().width ();

         w->saveMask () |= CWX | CWY;

         if (w->saveMask () & CWWidth)
                width = w->saveWc ().width;

         w->saveWc ().x = xRoot - (width >> 1);
         w->saveWc ().y = yRoot + (w->border ().top >> 1);

         ms->x = ms->y = 0;

         w->maximize (0);

         ms->snapOffY = ms->snapBackY;

         return;
       }
    }
    else if (w->state () & CompWindowStateMaximizedHorzMask)
    {
        if (abs (xRoot - workArea.x () - ms->snapOffX) >= SNAP_OFF)
       if (!s->otherGrabExist ("move", NULL))
       {
         int width = w->serverGeometry ().width ();

         w->saveMask () |= CWX | CWY;

         if (w->saveMask () & CWWidth)
                width = w->saveWc ().width;

         w->saveWc ().x = xRoot - (width >> 1);
         w->saveWc ().y = yRoot + (w->border ().top >> 1);

         ms->x = ms->y = 0;

         w->maximize (0);

         ms->snapOffX = ms->snapBackX;

         return;
       }
    }

What the code will do in both cases is set the restore position such that the window is under the pointer upon restore, and then restore the window. Its effectively eight lines of repeated code, and could probably just be refactored into a separate function called "restoreWindowToCursorCentralTitlebarPosition"

> Actually, now that I've looked at this in context, I can see why it is
> correct, but probably in need of some refactoring.
>
> if (w->state () & CompWindowStateMaximizedVertMask)
> {
> if (abs (yRoot - workArea.y () - ms->snapOffY) >= SNAP_OFF)
> if (!s->otherGrabExist ("move", NULL))
> {
> int width = w->serverGeometry ().width ();
>
> w->saveMask () |= CWX | CWY;
>
> if (w->saveMask () & CWWidth)
> width = w->saveWc ().width;
>
> w->saveWc ().x = xRoot - (width >> 1);
> w->saveWc ().y = yRoot + (w->border ().top >> 1);
>
> ms->x = ms->y = 0;
>
> w->maximize (0);
>
> ms->snapOffY = ms->snapBackY;
>
> return;
> }
> }
> else if (w->state () & CompWindowStateMaximizedHorzMask)
> {
> if (abs (xRoot - workArea.x () - ms->snapOffX) >= SNAP_OFF)
> if (!s->otherGrabExist ("move", NULL))
> {
> int width = w->serverGeometry ().width ();
>
> w->saveMask () |= CWX | CWY;
>
> if (w->saveMask () & CWWidth)
> width = w->saveWc ().width;
>
> w->saveWc ().x = xRoot - (width >> 1);
> w->saveWc ().y = yRoot + (w->border ().top >> 1);
>
> ms->x = ms->y = 0;
>
> w->maximize (0);
>
> ms->snapOffX = ms->snapBackX;
>
> return;
> }
> }
>
> What the code will do in both cases is set the restore position such that the
> window is under the pointer upon restore, and then restore the window. Its
> effectively eight lines of repeated code, and could probably just be
> refactored into a separate function called
> "restoreWindowToCursorCentralTitlebarPosition"

Yes, agreed. I'll see what I can do to reduce this code duplication.

>
> What the code will do in both cases is set the restore position such that the
> window is under the pointer upon restore, and then restore the window. Its
> effectively eight lines of repeated code, and could probably just be
> refactored into a separate function called
> "restoreWindowToCursorCentralTitlebarPosition"

This might take a while as I've already found a ton of stuff in this code, that
could be improved and optimized.
The good thing is -> the move code is short :)

Examples:
There are a ton of

if (a)
    if (b)
        if (c)
        {
            do ()
        }

combinations, or things like:

 if (s->otherGrabExist ("move", NULL))
     return false;

 if (ms->w)
     return false;

 if (w->type () & (CompWindowTypeDesktopMask |
         CompWindowTypeDockMask |
         CompWindowTypeFullscreenMask))
     return false;

 if (w->overrideRedirect ())
     return false;

which can be better written as:

 if (s->otherGrabExist ("move", NULL) ||
     ms->w ||
     w->overrideRedirect () ||
     w->type () & (CompWindowTypeDesktopMask |
     CompWindowTypeDockMask |
     CompWindowTypeFullscreenMask))
     return false;

also this block above comes after these lines:

 unsigned int mods = CompOption::getIntOptionNamed (options, "modifiers", 0);

 int x = CompOption::getIntOptionNamed (options, "x", w->geometry ().x () +
            (w->size ().width () / 2));
 int y = CompOption::getIntOptionNamed (options, "y", w->geometry ().y () +
            (w->size ().height () / 2));

 int button = CompOption::getIntOptionNamed (options, "button", -1);

but all those calculations of local variables do not need to happen if we return false, so
those should be calculated after the return false check...

Also I noticed that we have a SNAP_BACK strategy for vertically maximized windows, but
none for horz maxed ones...
I have to see what it exactly does and if we still need that. From flying over the code it
seems to be responsible for snapping back into the vert maxed position it was snapped of from
if the mousepointer has not been released in the meantime, but I could be wrong...

Not even looked at moving stuff to separate functions yet as cleanup reveals a lot of potential
for optimization...

For the sake of a smaller diff size and easier review, I propose that this could be merged first, so I can build up from this version...

Okay, further investigation shows that the SNAP_BACK code has some purpose -
it is the one I speculated about above...

TODO: Although snapping back horizontally maxed windows does not make that much sense,
      it should be implemented as well to harmonize snap off/snap back behaviour of
      those window types...

TODO: SNAP_BACK and SNAP_OFF are constants, but should ideally be configurable also
      (distance in pixels, the mouse has to travel to snap off/snap back).

> Okay, further investigation shows that the SNAP_BACK code has some purpose -
> it is the one I speculated about above...
>
> TODO: Although snapping back horizontally maxed windows does not make that
> much sense,
> it should be implemented as well to harmonize snap off/snap back
> behaviour of
> those window types...
>
Done (in local branch).

> TODO: SNAP_BACK and SNAP_OFF are constants, but should ideally be configurable
> also
> (distance in pixels, the mouse has to travel to snap off/snap back).

Done (in local branch).

@Sam: There will be a new MP hopefully soonish. Got almost all move problems
      figured out now (finally)...
      The back-snapping of horizontally maxed wins still makes (very minor)
      problems - should work for left & right sides IMHO...
      In my local branch I made backsnapping configurable as well :)
      This is how it currently looks:
      http://uppix.net/1/1/e/06bbae1c05a85dc86ea0870b52ead.png

A few comments:

It is not perfect.
There was a ton of stuff broken, wrongly calculated and such.
This is now all fixed, but small problems (only snap-back) remain:

1. I implemented snapping back horizontal windows, but it will just work for the left side of your screen for now.

2. I corrected the mousepointer-coordinates when snapping back vertically resized windows and warping the pointer works there, but this function does not like to work with horizontally maxed wins :(
Although I have the correct coordinates, the timing somehow fails to work... but this is a rather minor issues and I've even implemented the possibility to turn off the backsnapping...