Code review comment for lp:~rockstar/entertainer/package-structure-apocalypse

Revision history for this message
Matt Layman (mblayman) wrote :

Paul,

Here are all my comments that I recorded as I went through the diff:

What was the rationale behind putting all the dialog classes into a single file? I would suggest that we at least call it dialogs if there are going to be multiple, but it doesn't make too much sense to me to put them all in the same file, especially if our dialogs might only grow more capabilities over time (like TV stuff).

entertainerlib/backend/core/message_type_priority.py
 * FRONTEND_OPENED and FRONTEND_CLOSED should probably change to CLIENT_*. The comments with those lines don't make sense because frontend was substituted with client.

entertainerlib/client/__init__.py
 * Initial comment isn't valid.
 * client_client is weird.

entertainerlib/client/client.py
 * import order
 * class doc string has Frontend
 * logger name isn't correct, would client.Client

entertainerlib/client/media_player.py
 * import order

entertainerlib/client/medialibrary/music.py
 * import order

entertainerlib/client/medialibrary/videos.py
 * import order

entertainerlib/dialog.py
 * import order

entertainerlib/gui/screens/artist.py
 * import order

entertainerlib/gui/screens/disc.py
 * import order

entertainerlib/gui/screens/feed.py
 * import order

entertainerlib/gui/screens/feed_entry.py
 * import order

entertainerlib/gui/screens/main.py
 * import order

entertainerlib/gui/screens/music.py
 * import order

entertainerlib/gui/screens/rss.py
 * import order

entertainerlib/gui/system_tray_icon.py
 * Switch from frontend to client revealed some code that must is broken. set_client_visible has no implementation.

entertainerlib/gui/transitions/factory.py
 * import order

entertainerlib/gui/transitions/slide.py
 * import order

entertainerlib/gui/user_interface.py
 * import order
 * Uses UserEvent.QUIT_FRONTEND. That event should change to QUIT_CLIENT.

entertainerlib/gui/widgets/image_menu.py
 * import order

entertainerlib/gui/widgets/list_indicator.py
 * import order

entertainerlib/gui/widgets/tab_group.py
 * import order

entertainerlib/gui/widgets/text_menu.py
 * import order

entertainerlib/gui/widgets/texture.py
 * logger name doesn't need to refer to client anymore

entertainerlib/tests/test_logger.py
 * import order

entertainerlib/tests/test_theme.py
 * import order

entertainerlib/thumbnailer.py
 * import order

entertainerlib/utils/cd_utils.py
 * Removed, so you can link to bug 313815

setup.py
 * There is no longer a client/glade directory so 'client/glade/*' shouldn't be needed anymore.

« Back to merge proposal