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

Revision history for this message
Francesco Marella (francesco-marella) 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".
Oops... sorry.
>
> 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.
Done.
>
> 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)".
Done.

« Back to merge proposal