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

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.