Code review comment for lp:~jassmith/unity/unity.sticky-edges

Hi,

ResizeBarrierList should pass it's std::vector by const ref:

void ResizeBarrierList(std::vector<nux::Geometry> const& layout);

same with SetupBarriers:

void SetupBarriers(std::vector<nux::Geometry> const& layout);

Should probably fix this, just to bzr / lp don't complain:

194 \ No newline at end of file

...and definitely fix this one:

257 \ No newline at end of file

Is a lack of newline character at the end of header files can cause real problems.

Pro tip - this:

edge_barriers_(new ui::EdgeBarrierController())

Is better written as this:

edge_barriers_(std::make_shared<ui::EdgeBarrierController>())

std::make_shared returns a std::shared_ptr<T> constructed with the arguments you pass it, but it does it in only one memory allocation. Using "edge_barriers_(new ui::EdgeBarrierController())" requires at least two. Also, I believe that make_shared is exception safe, whereas the other method may throw if you cannot allocate an object (TBH though, if you get into that situation you're pretty screwed anyway). I don't mind if you choose to ignore this - it's something that I've started doing.

471 +enum BarrierDirection
472 +{
473 + BOTH = 0,
474 + LEFT = 1,
475 + RIGHT = 4,
476 +};

0, 1, 4? If you want this to be a bitmask, shouldn't RIGHT = 2?

500 +from autopilot.emulators.X11 import Mouse
512 + mouse = Mouse()

You don't need these two lines - all Autopilot test classes have access to 'self.mouse' already.

509 +class LauncherCaptureTests(ScenariodLauncherTests):

I don't think you want to derive from ScenariodLauncherTests - deriving from this class ensures that your tests get called several times with different sets of launcher options. It seems to me that your tests specify the options under which they'll work already? If that is in fact the case, just derive from AutopilotTestCase instead.

530 + if self.screen_geo.get_num_monitors() <= 1:
531 + return

Instead of returning, please do this:

self.skipTest("Cannot run this test with a single monitor configured.")

... obviously this applies to all the tests.

539 + self.assertThat(x_fin, NotEquals(x - width / 2))

Try not to do negative assertions - it's too easy to have the test pass when it shouldn't. Should the mouse value be less than, or greater than x_fin? Use GreaterThan / LessThen matchers instead (you may need to import them).

This code:

565 + self.set_unity_option('launcher_hide_mode', 1)
567 +
568 + launcher = self.get_launcher()
569 + for counter in range(10):
570 + sleep(1)
571 + if launcher.hidemode == 1:
572 + break
573 + self.assertThat(launcher.hidemode, Equals(1),
574 + "Launcher did not enter hidden mode.")

Should go in a method, so you can call it as often as you want from the setUp method and from within your tests.

Looking good though (apart from the issues above).

Cheers,

609 + self.assertThat(x_fin, NotEquals(x + width * 1.5))

Same thing as before with the negative assertions. Better to use Equals, or GreatherThan or LessTHan to test what you really mean.