Entertainer Media Center

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

new-scrollmenu
Merge into trunk

Revision history for this message

Matt Layman (mblayman) wrote on 2009-04-20:

Samuel,

These are the comments that I had written down before you last push. I still haven't looked at MotionBuffer and I need to give all the files a second look through, but I think it would be a good start to address some of these things. I hope you find them helpful. Most of the comments are just an attempt to keep a unified style in the codebase. The comments might seem a little rough because they are basically just my notes. I hope that they're readable enough.

main.py:
* _create_main_menu should return the menu. And the main screen should add that menu to itself. This will ensure that the init is the place that self.menu is created. It would also allow testing of _create_main_menu in the future (if we ever figure out what we would need to test). I did this for _create_rss_preview_menu and I think that it's a nice separation of responsibility. After all, shouldn't it only be the init that adds instance variables to itself?
* docstrings need improvement for _on_menu_moved and _on_menu_selected. Don't use "We" and try to reduce to one line if you can.
* hide_cursor_timeout_callback and handle_motion_event seem private. They should probably start with an underscore.

scroll_menu.py:
* import order is wrong because math is a core library.
* docstring doesn't need "This is" and might fit on one line
* What is __gtype_name__ for?
* Could clean up docstrings for get_active, set_active, get_selected (no need for @return and @param).
* idx, xx, and yy aren't very intuitive variable names in _display_items_at_target
* In _set_selected_index, I don't understand "#compute selected index to be between 0 and nb." What is nb? number? What number if so.
* I'd suggest you remove the @author tag in get_index and just add Joshua Scotton as an author to the top of the file. I don't think we need to identify authors at the method level (that's what bzr annotate is for), but he still has the right to credit himself with something if that is what he wrote.

special_behaviours.py:
* docstring is wrong at top of file.
* import order is wrong. math is a core lib and clutter is not so math should be first and clutter should be below it separated by a newline.
* No need for the space between class name and parenthetical superclass.
* "custome" is actually spelled custom
* do_alpha_notify has a space before its parameter list
* Don't need "We" in the docstring for do_alpha_notify
* what are idx and idxx?
* Does path_length need to return an integer? (because it says length in pixels)

test_scrollmenu.py:
* has an empty line with spaces.
* testCreate should use a PEP8 method name. test_create (We use camel case for setUp and tearDown because that's what unittest does. Any existing test that we have that uses camel case is just because it's from a time before started being fully PEP8 compliant.)
* test ScrollMenu instead of object

Samuel,

main.py:
 * _create_main_menu should return the menu. And the main screen should add that menu to itself. This will ensure that the init is the place that self.menu is created. It would also allow testing of _create_main_menu in the future (if we ever figure out what we would need to test). I did this for _create_rss_preview_menu and I think that it's a nice separation of responsibility. After all, shouldn't it only be the init that adds instance variables to itself?
 * docstrings need improvement for _on_menu_moved and _on_menu_selected. Don't use "We" and try to reduce to one line if you can.
 * hide_cursor_timeout_callback and handle_motion_event seem private. They should probably start with an underscore.

scroll_menu.py:
 * import order is wrong because math is a core library.
 * docstring doesn't need "This is" and might fit on one line
 * What is __gtype_name__ for?
 * Could clean up docstrings for get_active, set_active, get_selected (no need for @return and @param).
 * idx, xx, and yy aren't very intuitive variable names in _display_items_at_target
 * In _set_selected_index, I don't understand "#compute selected index to be between 0 and nb." What is nb? number? What number if so.
 * I'd suggest you remove the @author tag in get_index and just add Joshua Scotton as an author to the top of the file. I don't think we need to identify authors at the method level (that's what bzr annotate is for), but he still has the right to credit himself with something if that is what he wrote.

special_behaviours.py:
 * docstring is wrong at top of file.
 * import order is wrong. math is a core lib and clutter is not so math should be first and clutter should be below it separated by a newline.
 * No need for the space between class name and parenthetical superclass.
 * "custome" is actually spelled custom
 * do_alpha_notify has a space before its parameter list
 * Don't need "We" in the docstring for do_alpha_notify
 * what are idx and idxx?
 * Does path_length need to return an integer? (because it says length in pixels)

test_scrollmenu.py:
 * has an empty line with spaces.
 * testCreate should use a PEP8 method name. test_create (We use camel case for setUp and tearDown because that's what unittest does. Any existing test that we have that uses camel case is just because it's from a time before started being fully PEP8 compliant.)
 * test ScrollMenu instead of object

« Back to merge proposal