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:
> 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>
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 kAreaAndGeoemtr y
> method could be renamed to assignStrutsWor
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 TestScreenSizeC hangePreviousVi ewport:
> 506 + /* Deal with the case where the position is negative, which means :window: :Geometry (-300, 200, 300, 400, 0);
> 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:
> 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.
-- www.debian. org/
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://
<email address hidden> <email address hidden>