#8876 NORM 9.1.0: Make sugar control panel support selection of multiple languages
Zarro Boogs per Child
bugtracker at laptop.org
Tue Nov 11 10:34:36 EST 2008
#8876: Make sugar control panel support selection of multiple languages
---------------------------------+------------------------------------------
Reporter: sayamindu | Owner: marco
Type: defect | Status: new
Priority: normal | Milestone: 9.1.0
Component: sugar | Version: Git as of bug date
Resolution: | Keywords: localization, r?
Next_action: review | Verified: 0
Deployment_affected: | Blockedby:
Blocking: 8875 |
---------------------------------+------------------------------------------
Changes (by sayamindu):
* keywords: localization, r- => localization, r?
Comment:
Replying to [comment:1 tomeu]:
> Patch looks like good work to me, some nitpicks below:
>
> Subject: [PATCH 1/7] Fix sugardir
>
> Ok
>
> Subject: [PATCH 2/7] Make get_language return a list of language codes
>
> {{{
> @@ -86,22 +86,23 @@ def get_language():
> }}}
> Shouldn't be renamed to get_languages()?
>
Done
> {{{
> if os.access(path, os.R_OK) == 0:
> }}}
> Not your code, but os.access() doesn't return an int but a boolean, you
could change this to:
> {{{
> if not os.access(path, os.R_OK):
> }}}
Done
> Or even better, just change it to os.path.exists(path), because if the
file exists but is unreadable, we aren't fixing anything by writing the
default again.
>
> {{{
> + if line[:9] == "LANGUAGE=":
> }}}
> I would use
> {{{
> + if line.startswith("LANGUAGE="):
> }}}
>
> Also, what happens when we have LANG in .i18n? If I understand the code
correctly, we would be returning None in that case, right?
>
Done. The LANG part is handled in the latter part of the code
{{{
elif line.startswith("LANG="):
lang = line[9:].replace('"', '')
}}}
> If you could remove the gratuitous abbreviations in the code you modify,
would be great.
>
> Subject: [PATCH 3/7] Implement new UI
>
> {{{
> +L_ = lambda msg: gettext.dgettext('iso_639', msg)
> +C_ = lambda msg: gettext.dgettext('iso_3166', msg)
> }}}
>
> Can we use something more descriptive than L_ and C_? I would name them
as any other private module-level function (start with underscore, no
capitals).
>
Done.
{{{
_translate_language = lambda msg: gettext.dgettext('iso_639', msg)
_translate_country = lambda msg: gettext.dgettext('iso_3166', msg)
}}}
> {{{
> + self._avail_locales = self._model.read_all_languages()
> }}}
>
> Can we use self._available_locales instead?
>
Done.
> {{{
> + for locale in self._avail_locales:
> + store.append([locale[2], '%s (%s)' % (L_(locale[0]),
C_(locale[1]))])
> }}}
>
> Cannot figure out what this does without looking at code in other files,
perhaps could be rewritten as this (just guessing):
>
> {{{
> + for code, name_native, name_english in self._avail_locales:
> + description = '%s (%s)' % (L_(name_native),
C_(name_english))
> + store.append([code, description])
> }}}
>
Done
> {{{
> + combobox = gtk.ComboBox(model = store)
> }}}
> Named params shouldn't have spaces around the '='.
>
Done
> {{{
> + combobox.pack_start(cell, True)
> }}}
> If possible, set the name of the param if it's a boolean:
> {{{
> + combobox.pack_start(cell, expand=True)
> }}}
> Though in this case, True is the default so no need to specify it.
>
Done
> {{{
> + lang = locale_code.split('.')[0]
> + lang_column = row[0].split('.')[0]
> }}}
> May be good to split the ".split('.')[0]" part in a separate function
named _get_language_code_from_locale(row) so people know what it does.
>
This is done only once in the code. Don't you think a method would be a
overkill ? I put in a comment.
> {{{
> + combobox.connect('changed', self._combobox_changed_cb)
> }}}
> Please prepend callbacks with __ so we avoid name clashes.
>
Done.
> {{{
> + # Add to last row, first column
> + self._table.attach(combobox, 0, 1, \
> + self._selected_lang_count-1, self._selected_lang_count, \
> + xoptions=gtk.FILL, yoptions=gtk.FILL, xpadding=20,
ypadding=20)
> }}}
> Wonder if could be made clearer by refactoring out to a method like
this:
>
> {{{
> + self._attach_to_table(combobox, row=0, column=1)
> +
> + def _attach_to_table(self, widget, row, column):
> + self._table.attach(widget, row, column,
> + self._selected_lang_count - 1, self._selected_lang_count,
> + xoptions=gtk.FILL, yoptions=gtk.FILL, xpadding=20,
ypadding=20)
> }}}
>
Done.
> {{{
> + # Get stuff in the last row
> + add_remove_box = self._add_remove_boxes.pop()
> + combobox = self._comboboxes.pop()
> + store = self._stores.pop()
> }}}
> For the price of two more lines, you can take out that comment by doing
this:
> {{{
> + def _get_last_row(self):
> + add_remove_box = self._add_remove_boxes.pop()
> + combobox = self._comboboxes.pop()
> + store = self._stores.pop()
> + return add_remove_box, combobox, store
> ...
> add_remove_box, combobox, store = self._get_last_row()
> }}}
>
Done
> {{{
> + if self._selected_lang_count > 1:
> + self._add_remove_boxes[-2].hide_all() # Hide the previous
add/removes
> }}}
> That comment makes the line wider than 80 chars, which some other
developer in the project hates. I suggest doing this instead of just
splitting the line:
> {{{
> + if self._selected_lang_count > 1:
> + previous_add_removes = self._add_remove_boxes[-2]
> + previous_add_removes.hide_all()
> }}}
> In that way, you conform to the max of 80 chars while removing a comment
with implementation details, which are pretty bad in my opinion and of the
GNOME coding style (http://developer.gnome.org/doc/guides/programming-
guidelines/code-style.html):
>
Done
> "Comment your code. Please put a comment before each function that says
what it does. Do not say how it does it unless it is absolutely necessary;
this should be obvious from reading the code. If it is not, you may want
to rework it until the code is easy to understand."
>
> {{{
> + self._add_row(locale_code = locale)
> }}}
> Please remove spaces around '=' to make clearer that it's not an
assignment.
>
Done
> {{{
> def __realize_cb(self, widget):
> ...
> + pass
> + #self._entry.grab_focus()
> }}}
> What's going on here? We should avoid having dead code around. At least
should have a TODO marker.
>
Gah - that's a leftover from the older code. Removed the entire callback.
> {{{
> + self._changed = (selected_langs == self._selected_locales)
> }}}
> I must be reading wrong the code, because seems me that it should be the
opposite (!=). What's confusing here?
>
Gah - it was fixed in a latter commit.
> Subject: [PATCH 4/7] Add multilanguage support to model
>
> {{{
> if os.access(path, os.W_OK) == 0:
> }}}
> Not your code again, but I guess this should be os.path.exists()
instead.
>
Done
> {{{
> + if lines[i][:5] == "LANG=" or lines[i][:9] == "LANGUAGE=":
> }}}
> Consider using startswith() for greater readability.
>
Done
> {{{
> + return 1
> }}}
> Why are we returning 1 sometimes and why in some cases None will be
silently returned?
>
That's from the old code. I'm not sure if it was intentional or not, so I
left it as it is.
> Subject: [PATCH 5/7] Add multilanguage support to view
>
> {{{
> + try:
> + self.restart_alerts.remove('lang')
> + except:
> + pass
> }}}
> Silently eating all exceptions is dangerous and doesn't make clear your
intentions. I suggest to check if restart_alerts contain what you want to
remove first.
>
Done.
> Subject: [PATCH 6/7] Add a numeral column to the table, fix a typo
>
> {{{
> + self._labels = [] # List to store the labels
> }}}
> This comment sounds as pretty much redundant to me.
>
Removed
> {{{
> + label = gtk.Label(str = str(self._selected_lang_count))
> }}}
> Please remove spaces.
>
Done
> Subject: [PATCH 7/7] We need to keep LANG around as well, as per
http://www.gnu.org/software/gettext/manual/gettext.html#The-LANGUAGE-
variable
>
> Ok
>
> As I read further into the changeset, I realized that you had zipped
together several consecutive commits. I recommend that you submit to
review a single resulting patch and only a zipped dir if all the files are
new.
Thanks for the awesome review. I'll attach a new (single, unified) patch
as per your suggestions.
--
Ticket URL: <http://dev.laptop.org/ticket/8876#comment:2>
One Laptop Per Child <http://laptop.org/>
OLPC bug tracking system
More information about the Bugs
mailing list