Evening Matt,


> grid_menu.py:
>  * I counted 32 instance variables in use declared by the init. I know that
> GridMenu does a ton of stuff and I have no expectations for major refactorings
> of this branch, but maybe there are some class abstractions that can be
> extracted out somewhere else (because there's a lot of stuff to think about in
> this class). Maybe you have some thoughts on this subject. What do you think?
> Is there future common classes that could be pulled out from GridMenu?

Short answer Yes.
Indeed this is something I have in mind since a long time. I'm deeply convinced that there's a lot of line of code we can save and a much better readability we can reach doing a widget API refactoring which would take better usage of inheritance.
>>> http://wiki.entertainer-project.com/wiki/BluePrints/widget-API-changes
This is still in my mind (a few updates needed though).
 

>  * Line 80 comment "usefull" should be "useful" (English is silly, I know. :)

Fixed.


>  * _set_selected_index comment needs cleanup (start with "#start select...").
> Still not fixed.

Was fixed in a standing alone commit.


>  * I think I may have push you too far with the comments for the series of if
> statements in _set_selected_index. Rather than a textual description of what
> the logic is saying for each if statement, I think what I meant to ask for is
> "What are the if statements doing as a whole?" Once I have an idea of what the
> if statements are supposed to accomplish, I should have a good idea of how to
> read the code. In all of these if statements, it looks like you're adjusting
> xm, ym, xc, or yc. Maybe these if statements could be extracted into a method
> to help explain what the section is supposed to do. As a good rule of thumb
> that I use personally, if you have to write more lines of comments than code,
> something probably needs to be changed.

Agreed, so what I propose to you is that I'll create a wiki page with drawings and text to better explain that. And when it will be totally clear for you then maybe you'll be able to help be to find better words. I'll try to do that this week.


>  * I caught one more items_anchor_west still in the init.

Yep, fixed now.

 
> image_menu.py:
>  * You've already seen my comment about the specific types of async_add. I'll
> let it land, but please add an XXX comment so that we remember to come back to
> it.

Done.


>  * async_add items are a pack of information, but there is no comment
> explaining what an item is (texture and data). I think that this could use a
> clarifying comment.

Done.


> label.py:
>  * Can you add a color property test?

Indeed that was missing. I've added a test for this property.


> text_menu.py:
>  * "self.extra_label.position = ( \" Is the continuation character helpful
> here?

Absolutely not. That's fixed.


>  * ArtistsMenuItem. I don't think this needs to be specific to artists (even
> though it may be the only place it is used right now). How about
> AnimatingMenuItem instead?

Yes you're right. I've renamed this Class.


>  * self.timeline is only used by ArtistsMenuItem, not the parent TextMenuItem.
> Could you please move it to the subclass?

Agreed with you (one more time). That's also fixed.


>  * add_item also calls a mysterious pack item. A comment might help.

I don't understand what you mean here because I only see a TextMenuItem with its creation parameters.


>  * TextMenu also has the context specific async_add_artists method. I'll let
> this land too, but please add an XXX comment here too.

Done.


> music.py:
>  * I did this by mistake too in a different branch that I was working on (and
> should hopefully finish some day), but get_length_string should not zfill the
> first value. It should just zfill the second (otherwise it means there is a
> leading zero on the minute when we don't need there to be one). The places
> that in-lined this code only used one zfill as expected.


Yeah that's right. I've fixed that.



> disc.py:
>  * In _create_list_indicator, get_compact_disc_information is a pretty
> expensive call, especially when it was just called a few lines before the call
> to _create_list_indicator. Could you just get rid of _create_list_indicator
> and add the lines to the _get_disc_information method that it was called from?
> If you don't want to do that, please pass in the number of tracks as a
> parameter to _create_list_indicator instead.

Changed.

 
> main.py:
>  * I caught one little odd behavior. Start Entertainer, then click somewhere
> outside the main menu. It dims. But this is the only time it happens, and you
> can make it active again by clicking the menu or pressing right. I guess I
> found the one edge case though. :)

Oh man, you're really an expert to see those things ;). That's fixed.

 
> movie.py:
>  * Trailing spaces after TextMenu declaration.

Fixed.


> photo_albums.py:
>  * For some reason, I had one more album in my list indicator than actual
> albums (4 when there were supposed to be 3). I'm guessing this is something
> flaky about the backend since it is just a call to image_library.get_albums()

This was a bug (fixed) in fact. Because the menu is build with the async_add method from a list which as a condition statement.

 
> rss.py:
>  * list indicator is broken. Only reads 1 | 1 for me. I'm guessing async_add
> is what bit you here. By the time list indicator checks for the count in the
> menu, my guess is that there is only one.

Perfectly right. I've fixed that.

 
> tv_episodes.py:
>  * Is self.menu = None in the init really needed?
>  * I have nothing to actually test this screen with, but if TV episodes are
> added asynchronously, I'm not sure I would trust list indicator being set to
> self.menu.count.

Yeah same than above indeed. Fixed.


>  * Why was FIRST PAGE, LAST PAGE, PREVIOUS PAGE, and NEXT PAGE removed? Were
> those not working features?

Because that does not make sense in this case. The "plot" (that's the word we use in the code for that but I don't understand it in this context) are very small.


> albums_tab.py:
>  * If you've fixed my XXX comment, then there should be a corresponding bug to
> mark off once this lands.

Yep, will file bugs for XXX and the "rect" thing in the grid menu when I'll get the branch landed.

 
> Great work Samuel. This new grid widget has greatly cleaned up the screens and
> provided a ton of new functionality. Most of your code is really solid, and I
> think we are well aware of the rough edges. Therefore, I'm going to approve
> this branch so we don't waste too much time polishing solid work.

Thanks a LOT Matt. Considering the number of hours I've spent with this branch, your comment makes me very happy.


> reviewed every line of the diff so I hope that my comments will provide some
> value. I trust that you can address everything that I've listed here or at
> least give me an explanation about some of the bullets.

Thanks a lot for this review. I know it was a bit big. And as usual, your comments were very useful and made a LOT of sense for me.

Thanks again.
Samuel-