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

> > 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,