Merge lp://qastaging/~dandrader/compiz/resize_vert_max into lp://qastaging/compiz/0.9.8

Looks great, thanks for splitting up handleMotionEvent such that its more like a template function now - the code is much more readable.

The pedantic stuff:
 * 5 can be replaced with a magic number

1000 + if (grabWindowWorkArea->bottom() - yRoot <= 5
1001 + && !maximized_vertically)

vs

985 + if (yRoot - grabWindowWorkArea->top() <= max_edge_distance
986 + && !maximized_vertically)

 * max_edge_distance can be made const.
 * I'm not sure why max_geometry is an XRectangle, it can probably be a CompRect.
 * Prefer camel case variable names to underscored ones, eg fooBar not foo_bar.
 * Slight style issues, please add a space between the item and the bracket, eg foo () not foo()
 * 1071 + XRectangle geometryWithoutVertMax;
   1072 + bool maximized_vertically;

    - I *think* maximized_vertically is indented incorrectly here, its one tab and not 8 spaces.
 * Quick info on our indentation style as its almost impossible to get right unless through muscle memory :( [X11 style, and no, its not changing]
      : 1 indent, 4 spaces.
      : 2 indents, 1 tab,
      : 3 indents, 1 tab, 4 spaces
      : 4 indents, 2 tabs etc

 * This is very strange, as you seem to have gotten the indentation scheme right on the function that you added, (enableOrDisableVerticalMaximization), however your editor seems to have screwed up the bits that that were refactored, so if you can autocorrect it to the correct indentation scheme that would be great.

More important:
 * No tests, and I don't like merging code that doesn't have tests. Sorry :( . Unfortunately, compiz code is made more annoying to test by the fact that we have several objects in core that are not instantiable in a test harness. Also its not possible to link to plugins. Generally speaking I get around this in two ways:
 - break the dependencies of between CompWindow/CompScreen and my code by using interfaces
 - split out the thing one wants to test into a separate static library and link to that

  Fortunately I've had to do this enough times that there are a couple of examples littered around the source tree, see wall, composite, decor, place
 * I think we only need to worry about getting enableOrDisableMaximization under test, the rest is just cut'n'pasted refactoring where signatures are preserved
 * I have a bit of concern that this code doesn't entirely belong here, it would probably belong in the bit of unity/somewhere else where we actually deliver this feature. However, in case we don't manage to deliver the complete feature on time, having it here is good enough, and getting it under test has the added bonus of making it portable enough so that we can move it around if need be.

> [...]
> * I'm not sure why max_geometry is an XRectangle, it can probably be a
> CompRect.
> [...]

That's out of consistency. ResizeScreen::geometry and ResizeScreen::savedGeometry are also XRectangles.

>> [...]
>> * I'm not sure why max_geometry is an XRectangle, it can probably be a
>> CompRect.
>> [...]

> That's out of consistency. ResizeScreen::geometry and ResizeScreen::savedGeometry are also
> XRectangles.

Okay, that's fine then :)

I'm confused. This is almost exactly what the grid plugin does. You'd only need to change grid very slightly to get the same behaviour as this. So why not do it in grid?

On Fri, 10 Aug 2012, Daniel van Vugt wrote:

> Review: Needs Information
>
> I'm confused. This is almost exactly what the grid plugin does. You'd only need to change grid very slightly to get the same behaviour as this. So why not do it in grid?

I thought the same, although this belongs here for different reasons. The
window is supposed to vertically maximize when you resize it to the top or
bottom (not left/right). We are planning to replace the grid plugin in
quantal, but until that happens this seems like a better place to keep it.

> --
> https://code.launchpad.net/~dandrader/compiz/resize_vert_max/+merge/118950
> You are reviewing the proposed merge of lp:~dandrader/compiz/resize_vert_max into lp:compiz.
>

Updated according to comments.

Made the entire resizing logic testable instead of only enableOrDisableMaximization().

> Updated according to comments.
>
> Made the entire resizing logic testable instead of only
> enableOrDisableMaximization().

Hold on. There are some missing bits.

Thanks for refactoring the code! Just some things I discovered:

1181 + if (grabIndex && w)

Probably good to rename "w" to currentlyResizedWindow, "grab index and w" doesn't mean much.

1490 + /* if window and work-area intersect in x axis */
1491 + if (wX + wWidth > workArea.x () &&
1492 + wX < workArea.x

This expression can probably be done in terms of temporaries:

const int wX2 = wX + wWidth;

4760 + bool result = rs->logic.terminateResize(action, state, options);
4761 +
4762 + if (rs->logic.mode != ResizeOptions::ModeNormal)
4763 + {

You can probably inject logic into this function, so that there is no dependency on ResizeScreen. boost::bind lets you do that pretty easily, eg

boost::bind (resizeInitiate, _1, _2, _3, logic);

with

resizeInitiate (... , ResizeLogic &)

Looks okay otherwise even if the diff is on the large side [but that's fine, as I can tell its mostly all copy-and-paste + extract interface anyways]

> > Updated according to comments.
> >
> > Made the entire resizing logic testable instead of only
> > enableOrDisableMaximization().
>
> Hold on. There are some missing bits.

Ok. Now it's finally ready to go.

> Thanks for refactoring the code! Just some things I discovered:
>
> 1181 + if (grabIndex && w)
>
> Probably good to rename "w" to currentlyResizedWindow, "grab index and w"
> doesn't mean much.
>
> 1490 + /* if window and work-area intersect in x axis */
> 1491 + if (wX + wWidth > workArea.x () &&
> 1492 + wX < workArea.x
>
> This expression can probably be done in terms of temporaries:
>
> const int wX2 = wX + wWidth;
>
> 4760 + bool result = rs->logic.terminateResize(action, state, options);
> 4761 +
> 4762 + if (rs->logic.mode != ResizeOptions::ModeNormal)
> 4763 + {
>
> You can probably inject logic into this function, so that there is no
> dependency on ResizeScreen. boost::bind lets you do that pretty easily, eg
>
> boost::bind (resizeInitiate, _1, _2, _3, logic);
>
> with
>
> resizeInitiate (... , ResizeLogic &)
>
> Looks okay otherwise even if the diff is on the large side [but that's fine,
> as I can tell its mostly all copy-and-paste + extract interface anyways]

I'm sure there are many many ways to improve the original code, but that's not really the focus of this patch. I think we got far enough already for now. Could we please have it as it is, now that the needed test is in place?

Agreed.

No commit message specified.