Code review comment for lp:~vorlon/compiz/lp.763148

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>