Entertainer Media Center

Code review comment for lp:~francesco-marella/entertainer/glade-less

glade-less
Merge into entertainer-future

Revision history for this message

Matt Layman (mblayman) wrote on 2009-11-11:

Francesco, thanks for this branch.

Since this is is my first time reviewing your work, I should point out some things about my review style. I'm very exhaustive and will point out everything that I see. Most points will just be about helping the code base keep a consistent style, but I will sometimes ask questions if necessary. If I seem critical, please know that it is not any statement about you personally, it just my style to be, some might say, *picky*. We do peer reviews to get the best code so I find it best to be exhaustive so we can have a good discussion about the code. Also, I noticed on IRC that you're Italian. If there is anything unclear about my English, please let me know and I can rephrase. This has been helpful in the past for reviews of non-native English speaker. Please don't be intimidated by the number of comments. This work is great and I really appreciate it.

Now, on to the comments:

translation_setup.py:
* Import order does not match Entertainer conventions. The order is standard modules, third party modules, entertainerlib modules. Within each grouping, the modules are alphabetical. Thus, in this case, the "import locale" belongs before "import os" because locale is a standard library module.
* Unless under special circumstances, exceptions that are caught must actually do something. The try/do something/except/pass paradigm is not good because it typically just masks bugs. If you have a rationale for the "except locale.Error, e: pass", please let me know what it is.

dialog.py:
* I've noticed that you've renamed the glade files to *.ui. I'm assuming that these are converted from glade to GTK build UI definition files. In order to make this branch truly "gladeless", please rename instances of "gladefile" to "builderfile" or something similar.
* Is there some conversion tool to convert glade files to gtk builder ui files? How do I know that the dialogs haven't regressed in functionality if you redid your own? I would guess there is some tool, but I'm hoping you could enlighten me.
* Please change the class variable GLADE_DIR to something like BUILDER_DIR.
* Please rename the "glade" directory to something more appropriate (perhaps "gtkbuilder"). These requests sound trivial, but it just helps us to truly purge old methodologies so that the code doesn't get crufty. The last thing we need is someone looking at the source code, seeing "gladefile", and then scratching their head in confusion when it says it's actually using gtk.Builder.
* Does set_translation_domain('entertainer') need to happen? Maybe it doesn't belong in this branch, but I wonder if TranslationSetup could be called in some common Dialog class in the future.
* A logger comment references a glade file. This text needs to be updated.

system_tray_icon.py:
* Please change gladefile to something more gtk builder appropriate (perferably it should match whatever name you decide on in dialog.py).

po files:
* Please revert all the po files. The way we have Entertainer configured with Launchpad for translations is as follows:
   1. We run `make pot` in a branch. This Makefile commands call the translation generator tool and updates the pot file accordingly.
   2. The pot file update (typically in its own branch so it can be approved without contention) is committed to the trunk (so this may not do anything with on the future branch right now, which we should correct).
   3. Launchpad detects the updated pot file and automatically updates the po files *in the Launchpad translation section*.
   4. The awesome Launchpad translators update their strings based on the update source strings in the po files.
   5. A developer eventually downloads the po files from Launchpad after the translators have had the time to work on them.
   6. The po files are committed to the trunk and everyone gets to be happy. :)
* Unfortunately, your po file updates cut the Launchpad translators out of the process. I'll accept the pot file update on this occasion (because the file is in desparate need of an update), but the po files need to be updated through their normal process.

translations_generator.py:
* I'm guessing with the change to gtk builder that the generate_header method is no longer correct because of the '--type=gettext/glade'. I'm not sure what this should be updated to (or if it needs to be updated). Could you please check what the correct type is supposed to be and update the method to not refer to glade?

Overall, great work Francesco! I make it a habit to look over *every* line of the branch diff and your code is pretty solid (okay, I'll admit that I don't look over all the po file and XML-ish lines of code :P). Once some of these issues are addressed, I'll be happy to approve this work.

LEGAL NOTE: For ease of maintenance (like if we ever wanted to move to a newer version of the GPL), we have decided to use copyright assignment (assigning copyright to the Entertainer Developers). Attribution of code is listed in the docs/COPYING file which is where Entertainer Developers is defined. At this point in time, if you're not comfortable with assigning your copyright to Entertainer Developers, then I'm afraid we won't be able to merge your branch (which would be very sad). If that is okay with you, please add your name to the contributors (alphabetical by first name, you'd be first! :) to the docs/COPYING file as your way of approving that you are okay with the copyright assignment. The list of Entertainer Developers is at the bottom of the file. Sorry for this legal stuff. I personally dislike this aspect, but it's something that we have to deal with.

Francesco, thanks for this branch.

Now, on to the comments:

translation_setup.py:
 * Import order does not match Entertainer conventions. The order is standard modules, third party modules, entertainerlib modules. Within each grouping, the modules are alphabetical. Thus, in this case, the "import locale" belongs before "import os" because locale is a standard library module.
 * Unless under special circumstances, exceptions that are caught must actually do something. The try/do something/except/pass paradigm is not good because it typically just masks bugs. If you have a rationale for the "except locale.Error, e: pass", please let me know what it is.

dialog.py:
 * I've noticed that you've renamed the glade files to *.ui. I'm assuming that these are converted from glade to GTK build UI definition files. In order to make this branch truly "gladeless", please rename instances of "gladefile" to "builderfile" or something similar.
 * Is there some conversion tool to convert glade files to gtk builder ui files? How do I know that the dialogs haven't regressed in functionality if you redid your own? I would guess there is some tool, but I'm hoping you could enlighten me.
 * Please change the class variable GLADE_DIR to something like BUILDER_DIR.
 * Please rename the "glade" directory to something more appropriate (perhaps "gtkbuilder"). These requests sound trivial, but it just helps us to truly purge old methodologies so that the code doesn't get crufty. The last thing we need is someone looking at the source code, seeing "gladefile", and then scratching their head in confusion when it says it's actually using gtk.Builder.
 * Does set_translation_domain('entertainer') need to happen? Maybe it doesn't belong in this branch, but I wonder if TranslationSetup could be called in some common Dialog class in the future.
 * A logger comment references a glade file. This text needs to be updated.

system_tray_icon.py:
 * Please change gladefile to something more gtk builder appropriate (perferably it should match whatever name you decide on in dialog.py).

po files:
 * Please revert all the po files. The way we have Entertainer configured with Launchpad for translations is as follows:
   1. We run `make pot` in a branch. This Makefile commands call the translation generator tool and updates the pot file accordingly.
   2. The pot file update (typically in its own branch so it can be approved without contention) is committed to the trunk (so this may not do anything with on the future branch right now, which we should correct).
   3. Launchpad detects the updated pot file and automatically updates the po files *in the Launchpad translation section*.
   4. The awesome Launchpad translators update their strings based on the update source strings in the po files.
   5. A developer eventually downloads the po files from Launchpad after the translators have had the time to work on them.
   6. The po files are committed to the trunk and everyone gets to be happy. :)
 * Unfortunately, your po file updates cut the Launchpad translators out of the process. I'll accept the pot file update on this occasion (because the file is in desparate need of an update), but the po files need to be updated through their normal process.

translations_generator.py:
 * I'm guessing with the change to gtk builder that the generate_header method is no longer correct because of the '--type=gettext/glade'. I'm not sure what this should be updated to (or if it needs to be updated). Could you please check what the correct type is supposed to be and update the method to not refer to glade?

review: Needs Fixing

« Back to merge proposal