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

Round 2
=======

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?
 * Line 80 comment "usefull" should be "useful" (English is silly, I know. :)
 * _set_selected_index comment needs cleanup (start with "#start select..."). Still not fixed.
 * 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.
 * I caught one more items_anchor_west still in the init.

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.
 * 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.

image_menu_item.py:
 * Glad you condensed this class into the image menu file.

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

menu.py:
 * One more unneeded widget down!

menu_item.py:
 * No comment.

scroll_menu.py:
 * No comment.

selector.py:
 * No comment.

special_behaviours.py:
 * No comment.

tab_group.py:
 * No comment.

text_menu.py:
 * "self.extra_label.position = ( \" Is the continuation character helpful here?
 * 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?
 * self.timeline is only used by ArtistsMenuItem, not the parent TextMenuItem. Could you please move it to the subclass?
 * add_item also calls a mysterious pack item. A comment might help.
 * TextMenu also has the context specific async_add_artists method. I'll let this land too, but please add an XXX comment here too.

text_menu_item.py:
 * 1 less file, nice.

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.

mock.py:
 * No comment.

test_base.py:
 * No comment.

test_gridmenu.py:
 * Thanks, these tests look pretty valuable.

test_imagemenuitem.py:
 * No comment.

test_label.py:
 * No comment.

test_menuitem.py:
 * No comment.

test_textmenuitem.py:
 * No comment.

OK, widgets and tests are looked at. Now, I'm going to look at the screens and tabs to see how the widgets are used.

album.py:
 * No comment.

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.

feed.py:
 * No comment.

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

movie.py:
 * Trailing spaces after TextMenu declaration.

photo.py:
 * No comment.

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

photographs.py:
 * No comment.

question.py:
 * No comment.

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.

screen.py:
 * No comment.

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.
 * Why was FIRST PAGE, LAST PAGE, PREVIOUS PAGE, and NEXT PAGE removed? Were those not working features?

tv_series.py:
 * No comment.

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

artists_tab.py:
 * No comment.

movies_tab.py:
 * No comment.

series_tab.py:
 * No comment.

tab.py:
 * No comment.

tracks_tab.py:
 * No comment.

video_clips_tab.py:
 * No comment.

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. I've 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,
Matt