#8876 NORM 9.1.0: Make sugar control panel support selection of multiple languages
Zarro Boogs per Child
bugtracker at laptop.org
Mon Oct 27 13:50:32 EDT 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 | Spec_stage: unknown
Blockedby: | Blocking: 8875
Spec_reviewer: | Verified: 0
Spec_reviewed: 0 |
---------------------------+------------------------------------------------
Changes (by tomeu):
* keywords: localization => localization, r-
* spec_stage: => unknown
* spec_reviewed: => 0
Comment:
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()?
{{{
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):
}}}
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?
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).
{{{
+ self._avail_locales = self._model.read_all_languages()
}}}
Can we use self._available_locales instead?
{{{
+ 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])
}}}
{{{
+ combobox = gtk.ComboBox(model = store)
}}}
Named params shouldn't have spaces around the '='.
{{{
+ 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.
{{{
+ 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.
{{{
+ combobox.connect('changed', self._combobox_changed_cb)
}}}
Please prepend callbacks with __ so we avoid name clashes.
{{{
+ # 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)
}}}
{{{
+ # 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()
}}}
{{{
+ 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):
"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.
{{{
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.
{{{
+ 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?
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.
{{{
+ if lines[i][:5] == "LANG=" or lines[i][:9] == "LANGUAGE=":
}}}
Consider using startswith() for greater readability.
{{{
+ return 1
}}}
Why are we returning 1 sometimes and why in some cases None will be
silently returned?
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.
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.
{{{
+ label = gtk.Label(str = str(self._selected_lang_count))
}}}
Please remove spaces.
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.
--
Ticket URL: <http://dev.laptop.org/ticket/8876#comment:1>
One Laptop Per Child <http://laptop.org/>
OLPC bug tracking system
More information about the Bugs
mailing list