Merge lp://qastaging/~3v1n0/unity/quicklist-keynav-fixes into lp://qastaging/unity

Hi,

82 +bool
83 +QuicklistMenuItem::GetSelectable()

199 +bool
200 +QuicklistMenuItemSeparator::GetSelectable()

238 +void
239 +QuicklistView::SelectItem(int index)

263 bool
264 -QuicklistView::IsMenuItemSeperator(int index)
265 +QuicklistView::IsMenuItemSelectable(int index)

Coding style says these should be on the same name.

667 + def get_items(self, visible_only=True):
668 + """Returns the quicklist items"""
669 + if visible_only:
670 + return self.get_children_by_type(QuicklistMenuItem, visible=True)
671 + else:
672 + return self.get_children_by_type(QuicklistMenuItem)
673 +
674 @property
675 def items(self):
676 """Individual items in the quicklist."""
677 - return self.get_children_by_type(QuicklistMenuItem)
678 + return self.get_items()
679 +
680 + @property
681 + def selectable_items(self):
682 + """Items that can be selected in the quicklist"""
683 + return filter(lambda it: it.selectable == True, self.items)

This is a bit inconsistent - you have 'get_items' and 'selectable_items' - one a method, one a property. Please make them both the same (either methods or properties, I don't mind which). Also, please don't use 'filter' in autopilot. In most places, list comprehensions are clearer. However, in this case, you don't need either - just pass more keyword filters to get_children_by_type (like this):

@property
def selectable_items(self):
    return self.get_children_by_type(QuicklistMenuItem, visible=True, selectable=True)

704 + Mouse().move(target_x, target_y, animate=False)

Unless you have good reason to, please don't pass 'animate=false'
 - this is only intended to be used in exceptional circumstances.

856 + self.assertThat(self.quicklist, Not(Is(None)))
857 + self.assertThat(self.quicklist.selected_item, Not(Is(None)))

Please avoid negative assertions. Instead of saying "make sure foo is not None", say "Make sure foo is XYZ" - it makes your tests more specific and robust.

A few notes on docstrings:
 1. All your docstrings need a full-stop (period) at the end.
 2. Please try and make your docstrings PEP257 compliant: http://www.python.org/dev/peps/pep-0257/
 3. You can rewrite many of your docstrings to be shorter, and more forceful. For example, instead of writing this:

"""Tests that the quicklist Home key selects the first valid item when
no item is selected
"""

write this:

"""Home key MUST select the first selectable item in a quicklist."""

Note that this uses the word "must" which is better than "should", "might", or "will", and follows more closely the code of the tests themselves.

Other than that, these look good.

> Hi,
>
> 82 +bool
> 83 +QuicklistMenuItem::GetSelectable()
>
> Coding style says these should be on the same name.

Yes I know and I generally do that, but not when I would be inconsistent with the style currently used on the class I'm editing, so I'd prefer not to change it here not to include unneeded changes.
This is something that should be fixed globally on a different coding-style-fixes branch (for Q).

> 667 + def get_items(self, visible_only=True):
> 668 + """Returns the quicklist items"""
> 669 + if visible_only:
> 670 + return self.get_children_by_type(QuicklistMenuItem, visible=True)
> 671 + else:
> 672 + return self.get_children_by_type(QuicklistMenuItem)
> 673 +
> 674 @property
> 675 def items(self):
> 676 """Individual items in the quicklist."""
> 677 - return self.get_children_by_type(QuicklistMenuItem)
> 678 + return self.get_items()
> 679 +
> 680 + @property
> 681 + def selectable_items(self):
> 682 + """Items that can be selected in the quicklist"""
> 683 + return filter(lambda it: it.selectable == True, self.items)
>
>
> This is a bit inconsistent - you have 'get_items' and 'selectable_items' - one
> a method, one a property. Please make them both the same (either methods or
> properties, I don't mind which).

Well, I wanted to keep both for a reason: the get_items() also allow you to get the invisible items of a quicklist, while the property will give you only the "real" result.

> 704 + Mouse().move(target_x, target_y, animate=False)
>
>
> Unless you have good reason to, please don't pass 'animate=false'
> - this is only intended to be used in exceptional circumstances.

I used it to avoid that a quicklist item would have been selected when performing that, adding a possibility to get an invalid result.

> 856 + self.assertThat(self.quicklist, Not(Is(None)))
> 857 + self.assertThat(self.quicklist.selected_item, Not(Is(None)))
>
> Please avoid negative assertions. Instead of saying "make sure foo is not
> None", say "Make sure foo is XYZ" - it makes your tests more specific and
> robust.

I wouldn't have used, but it's just a copy and paste from your code of the same class that I used not to be wrong :D

Thanks for the review, I'll update the branch soon.

> > Hi,
> >
> > 82 +bool
> > 83 +QuicklistMenuItem::GetSelectable()
> >
> > Coding style says these should be on the same name.
>
> Yes I know and I generally do that, but not when I would be inconsistent with
> the style currently used on the class I'm editing, so I'd prefer not to change
> it here not to include unneeded changes.
> This is something that should be fixed globally on a different coding-style-
> fixes branch (for Q).

OK, fine with me.
> >
> > This is a bit inconsistent - you have 'get_items' and 'selectable_items' -
> one
> > a method, one a property. Please make them both the same (either methods or
> > properties, I don't mind which).
>
> Well, I wanted to keep both for a reason: the get_items() also allow you to
> get the invisible items of a quicklist, while the property will give you only
> the "real" result.
>

You can still have both - just either make them both properties, or both methods. Probably making them both properties makes more sense.

> > 704 + Mouse().move(target_x, target_y, animate=False)
> >
> >
> > Unless you have good reason to, please don't pass 'animate=false'
> > - this is only intended to be used in exceptional circumstances.
>
> I used it to avoid that a quicklist item would have been selected when
> performing that, adding a possibility to get an invalid result.
>

Fair enough.

> > 856 + self.assertThat(self.quicklist, Not(Is(None)))
> > 857 + self.assertThat(self.quicklist.selected_item, Not(Is(None)))
> >
> > Please avoid negative assertions. Instead of saying "make sure foo is not
> > None", say "Make sure foo is XYZ" - it makes your tests more specific and
> > robust.
>
> I wouldn't have used, but it's just a copy and paste from your code of the
> same class that I used not to be wrong :D
>

Yeah, my code's not perfect either. Negative assertions are something that I've come to realise are really bad very recently, so older code is likely to be wrong.

Cheers,

> This is a bit inconsistent - you have 'get_items' and 'selectable_items' - one
> a method, one a property. Please make them both the same (either methods or
> properties, I don't mind which).

Well, I did that because I also wanted to include a method to fetch the invisible items, but I'll remove it since no one uses it yet.

> Also, please don't use 'filter' in autopilot.

Yes you're right, I used it only because at the begginning selectable was a property that I defined only on the emulator, then I moved it to the introspection too, and I forgot to update the code.

I've updated the branch as well, I hope it's fine now ;)

Hi,

There are still some issues with this I'm afraid:

672 + """Items that can be selected in the quicklist"""

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

727 + def mouse_click(self, button=1):
728 + logger.debug("Clicking on quicklist item %r", self)
729 + self.mouse_move_to()
730 + sleep(.25)
731 + assert(self.visible)
732 + self._mouse.click(press_duration=.1)
733 + sleep(.25)

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()".

66 + for icon in self.launcher.model.get_launcher_icons():
867 + if (icon.tooltip_text != self.ql_app.name):
868 + self.ql_launcher.key_nav_next()
869 + else:
870 + self.keybinding("launcher/keynav/open-quicklist")
871 + self.addCleanup(self.keybinding, "launcher/keynav/close-quicklist")
872 + break

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(...)

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

885 + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id))
899 + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id))
908 + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id))
917 + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id))
931 + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id))
940 + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id))
949 + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id))
958 + self.assertTrue(item.selected)
959 + self.assertThat(self.quicklist.selected_item.id, Equals(item.id))
979 + self.assertTrue(mouse_item.selected)
984 + self.assertTrue(expected_item.selected)
985 + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id))
995 + self.assertTrue(mouse_item.selected)
1000 + self.assertTrue(expected_item.selected)
1001 + self.assertThat(self.quicklist.selected_item.id, Equals(expected_item.id))
1010 + self.assertTrue(mouse_item.selected)
1017 + self.assertTrue(key_item.selected)
1018 + self.assertThat(self.quicklist.selected_item.id, Equals(key_item.id))
1023 + self.assertThat(self.quicklist.selected_item.id, Equals(key_item.id))
1027 + self.assertThat(self.quicklist.selected_item.id, Equals(key_item.id))
1032 + self.assertTrue(mouse_item.selected)
1033 + self.assertThat(self.quicklist.selected_item.id, Equals(mouse_item.id))

Please use the new Eventually() matcher for all of these. To import it:

from autopilot.matchers import Eventually

to use it, take this:

self.assertThat(self.quicklist.selected_item.id, Equals(mouse_item.id))

and turn it into this:

self.assertThat(self.quicklist.selected_item.id, Eventually(Equals(mouse_item.id)))

Eventually takes any mater, so you can also do things like this:

self.assertThat(self.quicklist.selected_item.id, Eventually(LessThan(mouse_item.id)))

Finally, please remove the sleep() statements. Using the Eventually(...) matcher removes the n...

> 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 ;)

There are additional revisions which have not been approved in review. Please seek review and approval of these new revisions.