Code review comment for lp:~francesco-marella/entertainer/glade-less
Revision history for this message
| Francesco Marella (francesco-marella) wrote | # |
© 2004
Canonical Ltd.
•
Terms of use
•
Data privacy
•
Contact Launchpad Support
•
Blog
•
Careers
•
System status
•
73416ba
(Get the code!)
Hi Matt,
2009/11/11 Matt Layman <email address hidden>
> Review: Needs Fixing
> 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.
>
Peer review it's a good practice indeed and helped here to fix another bunch
of bugs. thanks for reviewing my code.
I'll give you a response for each point, excuse my conciseness.
>
> Now, on to the comments:
>
> translation_
> * 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.
>
fixed.
* Unless under special circumstances, exceptions that are caught must
> actually do something. The try/do something/
> 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.
>
I've added a comment, please refer to it for more info.
> 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.
>
fixed using `UI_DIR`, `uifile`, and `uis` folder.
* 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.
>
The tool used to convert is gtk-builder-
input and output a gtk+-2.12 compatible file.
Conversion was successful for all the glade files and my inspection provided
no evidence of regressions. Still I can't say if there are any (hidden)
regression, sorry.
* Please change the class variable GLADE_DIR to something like BUILDER_DIR.
>
done.
* 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.
>
done.
* Does set_translation
> doesn't belong in this branch, but I wonder if TranslationSetup could be
> called in some common Dialog class in the future.
>
Why you would call TranslationSetup in common dialogs?
From what i understand, TranslationSetup is supposed to set up locale and
gettext globally, right?
The method gtk.builder.
of builder and must be called *just after* gtk.Builder instance is created.
* A logger comment references a glade file. This text needs to be updated.
>
done.
> system_
> * Please change gladefile to something more gtk builder appropriate
> (perferably it should match whatever name you decide on in dialog.py).
>
done.
> 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.
>
done.
>
> translations_
> * I'm guessing with the change to gtk builder that the generate_header
> method is no longer correct because of the '--type=
> 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?
>
Old glade files and new ui files share the same type.
>
> 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.
>
Thanks to you guys to develop a such promising media center. ;)
>
> 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.
>
added my name to contributors.
> --
>
> https:/
> You are the owner of lp:~francesco-marella/entertainer/glade-less.
>