Code review comment for lp:~3v1n0/unity/quicklist-keynav-fixes

Marco Trevisan (TreviƱo) (3v1n0) wrote :

> Needs a full-stop at the end of the docstring.

Ouch!

> 727 + def mouse_click(self, button=1):
> Several issues here:
> 1) The assert should be at the start of this method, not half way through.
> 2) You don't need the sleep() statements, please remove them.
> 3) You don't need to specify the click duration. Please just use
> "mouse.click()".

Done

> 66 + for icon in self.launcher.model.get_launcher_icons():

> Instead of iterating over the model icons, please use the method on the
> launcher model, like this:
>
> icon = self.launcher.model.get_icon_by_desktop_id(...)

I can't... That was a keyboard selection... I must do that.

> If you use the desktop Id then we won't have issues in didfferent locales.

Using the bamf name we don't have these issue... Tested here, with Italian locale ;)

> 885 + self.assertThat(self.quicklist.selected_item.id,
> Equals(expected_item.id))

> Please use the new Eventually() matcher for all of these. To import it:
> Finally, please remove the sleep() statements. Using the Eventually(...)
> matcher removes the need for random sleep statements in your tests.

Done ;)

« Back to merge proposal