Merge lp://qastaging/~jassmith/unity/unity.sticky-edges into lp://qastaging/unity

Proposed by Jason Smith
Status: Merged
Approved by: Jason Smith
Approved revision: no longer in the source branch.
Merged at revision: 2222
Proposed branch: lp://qastaging/~jassmith/unity/unity.sticky-edges
Merge into: lp://qastaging/unity
Diff against target: 606 lines (+394/-50)
8 files modified
plugins/unityshell/src/EdgeBarrierController.cpp (+189/-0)
plugins/unityshell/src/EdgeBarrierController.h (+57/-0)
plugins/unityshell/src/Launcher.cpp (+12/-44)
plugins/unityshell/src/Launcher.h (+3/-5)
plugins/unityshell/src/LauncherController.cpp (+12/-0)
plugins/unityshell/src/PointerBarrier.cpp (+2/-1)
plugins/unityshell/src/PointerBarrier.h (+13/-0)
tests/autopilot/autopilot/tests/test_launcher.py (+106/-0)
To merge this branch: bzr merge lp://qastaging/~jassmith/unity/unity.sticky-edges
Reviewer Review Type Date Requested Status
Thomi Richards (community) Approve
Review via email: mp+100844@code.qastaging.launchpad.net

Commit message

Implement sticky edge behavior as requested by design. Add EdgeBarrierController class to handle complexity of edge barriers.

Description of the change

== Problem ==
Launcher behavior with edge stickiness is inconsisten with design. Further details available with bug #961285

== Solution ==
Implement behavior consisten with design. A new class was created to control all barrier edges and launchers are subscribed to that class as needed.

== Testing ==
Autopilot tests are in place to test new behavior.

To post a comment you must log in.
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

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.

review: Needs Fixing
Revision history for this message
Thomi Richards (thomir-deactivatedaccount) wrote :

Looks good to me.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.