Merge lp://qastaging/~vorlon/compiz/lp.763148 into lp://qastaging/compiz/0.9.10

FAILED: Continuous integration, rev:3647
No commit message was specified in the merge proposal. Click on the following link and set the commit message (if you want a jenkins rebuild you need to trigger it yourself):
https://code.launchpad.net/~vorlon/compiz/lp.763148/+merge/157534/+edit-commit-message

http://jenkins.qa.ubuntu.com/job/compiz-ci/122/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-gles-ci/./build=pbuilder,distribution=raring,flavor=amd64/159/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/compiz-pbuilder/./build=pbuilder,distribution=raring,flavor=amd64/511/console

Click here to trigger a rebuild:
http://s-jenkins:8080/job/compiz-ci/122/rebuild

Hey, thanks so much for doing this, and also adding test cases.

A quick note: I'm marking this as "resubmit" as there are merge conflicts. It appears that you've taken lp:compiz/raring, made changes there, and then submitted it for merging against lp:compiz . The two branches differ slightly (distro is cherry-picking fixes from lp:compiz to lp:compiz/0.9.9). Also - changes to debian/changelog are unnecessary and probably not wanted either. The automerger will automatically update the debian changelog, and updating it manually will probably confuse it.

As for some code review, here are some suggestions I have.

1. Thanks for refactoring some of the common setup code. I think the method could be renamed to assignStrutsWorkAreaAndGeoemtry
2. In TestScreenSizeChangePreviousViewport:

506 + /* Deal with the case where the position is negative, which means
507 + * it's actually wrapped around to the rightmost viewport
508 + */
509 + g.setPos (CompPoint (-300, 200));
510 + ms.setGeometry (g);
511 +
512 + expected = compiz::window::Geometry (-300, 200, 300, 400, 0);

It appears that the relevant assertion here is that the window will have the same geometry if wrapped to the rightmost viewport when the rightmost viewport is unplugged. In that case, it might be better to just set "expected" to "g", as they are both the same.

Everything else seems mostly fine to me.

Doing this review reminded me that both the tests and adjustForSize need some serious refactoring, because even in their simplified versions, there is way too much going on in both of them. I'm not going to block this review on it, however, because:
 a) They were monsters of my own creation, and I should have the responsibility to deal with them.
 b) I was going to suggest doing the refactoring in this review, but decided against it because articulating what needed to be done was so detailed and complicated that it would just cause miscommunication and lots of unrelated change.

What you'll need to do from here is:

1. Branch lp:compiz
2. Cherry pick revisions 3643..3646 from your old branch into that one
3. Submit that branch.

(Or at least 3646 from this branch)

\o/
Stopping windows from jumping around weirdly should be numbero uno priority for us...
But I do not like this diff either...

On Sun, Apr 07, 2013 at 05:59:18AM -0000, Sam Spilsbury wrote:
> Hey, thanks so much for doing this, and also adding test cases.

> A quick note: I'm marking this as "resubmit" as there are merge conflicts.
> It appears that you've taken lp:compiz/raring, made changes there, and
> then submitted it for merging against lp:compiz

Yep, was betrayed by the default behavior of bzr lp-propose-merge here.
I've rebased now and resubmitted:

  https://code.launchpad.net/~vorlon/compiz/lp.763148/+merge/157537

> As for some code review, here are some suggestions I have.

> 1. Thanks for refactoring some of the common setup code. I think the
> method could be renamed to assignStrutsWorkAreaAndGeoemtry

I don't find that a very clear description of what the method does, since
it's also what calls adjustForSize(). But maybe I'm misunderstanding?

> 2. In TestScreenSizeChangePreviousViewport:

> 506 + /* Deal with the case where the position is negative, which means
> 507 + * it's actually wrapped around to the rightmost viewport
> 508 + */
> 509 + g.setPos (CompPoint (-300, 200));
> 510 + ms.setGeometry (g);
> 511 +
> 512 + expected = compiz::window::Geometry (-300, 200, 300, 400, 0);

> It appears that the relevant assertion here is that the window will have
> the same geometry if wrapped to the rightmost viewport when the rightmost
> viewport is unplugged. In that case, it might be better to just set
> "expected" to "g", as they are both the same.

Right, changed - thanks.

--
Steve Langasek Give me a lever long enough and a Free OS
Debian Developer to set it on, and I can move the world.
Ubuntu Developer http://www.debian.org/
<email address hidden> <email address hidden>