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

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.