Code review comment for lp:~samuel-buffet/entertainer/new-scrollmenu

Samuel,

I'm finally getting a chance to look at some of this code. I'm going to do this review in a couple of stages because of some functional regressions that I found while doing user testing. First, I'll let you know what those problems are so that you'll get a chance to fix them, then I'll come back and review the new widget code in more detail.

But let me start by saying this is really cool stuff. Sometimes it is easy to be critical and just point out faults in code, but I just want to let you know in advance that I think this work is pretty awesome. I'm sure you put quite a bit of effort into this code, and it shows.

Okay, now for the comments:

1) The scroll menu no longer remembers its last item when you return to a screen. Every time I hit back, it started on Music again.

2) The major problem that I have so far is that the keyboard interface is mostly broken if you return to the main screen after being on a different screen. For some reason, the menu doesn't think that it is active so it won't react until the user presses the right arrow key, then it becomes active.

Now for the minor comments (keep in mind that in this stage of the review, I've only looked at the Main screen and UserInterface files):

3) I'm not a fan of variable abbreviations most of the time. I noticed that you have the menu property "visible_nb" and I understand what it is supposed to mean, but since ScrollMenu uses the term "item," I think that "visible_items" would be a much clearer property name.

4) You created some new methods in Main and UserInterface (which is fine, of course), but I don't think that they match the conventions that we use for "private" methods. Since the methods you made aren't meant to be used outside the class, I would recommend that you prepend an underscore character to make that clear (e.g., preview_activation becomes _preview_activation). This isn't directly stated in PEP8, but there is some discussion about having private portions start with _.

As soon as these regressions are addressed, I'll come back and look specifically at the underlying widget code.

Thanks.