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

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

Francesco, thanks for the updates, here's what I've found out about the new diff. Also, I don't know if this is something that you did, but there is no need to create a new merge proposal each time you update the branch based on feedback. We can use the same merge proposal for the review dialog, and when the branch is ready for approval, I'll change from "Needs Fixing" to "Approved".

translation_setup.py:
 * Thanks for the XXX comment. We typically file bugs for XXX comments if they make it onto the trunk. It's a good way to keep track of stuff that we want to revisit later. So after the branch is merged, please file a bug for that comment.

dialog.py:
 * The explanation about the conversion tool for glade to gtk builder was helpful. I'm confident that the tool did its job correctly. I just wanted to make sure all the conversion wasn't done by hand or something.

system_tray_icon.py:
 * Looks good to me now.

po files:
 * Thanks for reverting.

glade occurences:
 * I did a `grep -rI 'glade' *` to find where glade still existed and I found a couple of spots that still need to be fixed.
   1. setup.py needs to be updated to refer to uis (or ui) instead of glade.
   2. generate_headers function in translations_generator.py still refers to glade throughout. I know the type is still glade, but variable names and doc strings should be updated to be "ui" instead of glade.

As a note about the merge proposal, please add a commit message to the proposal (which can be found towards the top of the proposal page). When branches are merged onto the target branch (in this case "future"), we use the commit message from the proposal along with the user's name to identify who did the work. For example for this branch, the final message might look something like "Converted glade to gtk builder. (Francesco Marella)".

review: Needs Fixing

« Back to merge proposal