#1684 closed enhancement (fixed)
Add support for an alternate localedir
Reported by: | sayamindu | Owned by: | erikos |
---|---|---|---|
Priority: | Immediate | Milestone: | |
Component: | Sugar | Version: | Unspecified |
Severity: | Unspecified | Keywords: | r? |
Cc: | Distribution/OS: | Unspecified | |
Bug Status: | Assigned |
Description
This is an implementation for http://wiki.sugarlabs.org/go/Features/Enhanced_Gettext without breaking/adding to the API.
The idea is pretty simple - before doing a bindtextdomain(), the candidate locale directories are scanned, and the one with the latest translations is chosen. (this is due to the fact that an activity may be updated, through which it may get newer translations than what is there in the langpackdir).
It is less invasive than what I originally proposed, and seems to do what it needs to. It would need some testing though, since there may be a few corner cases I might have missed.
The relevant patches are in:
- http://git.sugarlabs.org/projects/sugar-toolkit/repos/gettext-enhancements/commits/ef093d395e3b682a4f1cc603b385fecb09298cff
- http://git.sugarlabs.org/projects/sugar-toolkit/repos/gettext-enhancements/commits/173a4027019e0adff04534176167ac24a0b303cd
It also needs an extra GConf key (added via the Sugar package): /desktop/sugar/i18n/langpackdir
One problem with the code is that it adds a dependency to python-dateutils (for parsing the date). Python-dateutils is however, packaged for most distros. I could not figure out how to parse ISO 8601 formatted strings with the time.strptime().
Attachments (4)
Change History (17)
comment:1 follow-up: ↓ 2 Changed 14 years ago by erikos
comment:2 in reply to: ↑ 1 Changed 14 years ago by sayamindu
Replying to erikos:
Thanks for the detailed description of your review request.
Two first comments:
from sugar.activity import activityhandle, i18n
- please use two lines
Please use the docstring format like the one in sugar/datastore/datastore.py (with arguments and return values).
Done :-).
comment:3 Changed 14 years ago by erikos
- Keywords r! added; r? removed
This is the path that gets generated by the script:
/home/erikos/Activities/Memorize.activity/locale/de_DE/LC_MESSAGES/org.laptop.Memorize.mo
However on my system the path is:
/home/erikos/Activities/Memorize.activity/locale/de/LC_MESSAGES/org.laptop.Memorize.mo
This should be an 'and'
if packdir is not None or packdir is not '':
Smaller ones:
packdir - can you change that member to be packagedir (abbreviations in general)
Watch out for spaces:
ValueError ('Could not find revision date')
I would try to avoid such constructs if possible. At least they are easier to read when splitting them in a few lines.
sorted(candidate_dirs.iteritems(), key=lambda (k, v): (v, k), \ reverse = True)[0][0]
Unused variables should be with trailing underscore.
version_, numofstrings = _readbin(handle, fmt, 8)
Please run scripts/data/pep8.py and pylint (included in jhbuild). More: http://wiki.sugarlabs.org/go/Development_Team/Code_guidelines#Tools
comment:4 Changed 14 years ago by erikos
Ping, sayamindu, when do you think we can land this one?
comment:5 Changed 14 years ago by sayamindu
Sorry - I was away travelling and my 3G provider let me down by refusing to roam. Give me another day and I'll try to make this into landable stuff
comment:6 Changed 14 years ago by sayamindu
- Keywords r? added; r! removed
I fixed the style and pep8 issues.
Regarding the final return statement, I added a comment about what it does, so there should be less confusion for future hackers.
Regarding the confusion about de_DE and de, I'll need to do a bit more thinking about it, but it is not very critical, and I think we can land the code as it is for now.
comment:7 Changed 14 years ago by erikos
- Keywords r! added; r? removed
About the de_DE and de confusion: I am not sure (testing is a few days ago) it worked for me with this error. Please verify that.
Style: There are still quite some abbreviations in the code. And sometimes you do 'filepath' and then magic_number. Please use the underscore format consistently. As well the argument 'fmt' is not very descriptive.
Comments: The comment for get_locale_path should contain the return value. You might want to add a comment for the module, too. And as this is inner API we should reflect that in some way (not sure yet how to do that).
# Fancy way to sort the dictionary by value
I fear this is redundant information :) The code should be self explanatory if possible. For example you could assign the outcome of
sorted(candidate_dirs.iteritems(), key=lambda (k, v): (v, k), reverse=True)
to variables and return that. In addition with the comment in the docstring this would already make it a lot clearer.
Thanks!
comment:8 Changed 14 years ago by erikos
- Bug Status changed from Unconfirmed to Assigned
- Priority changed from Unspecified by Maintainer to Immediate
Btw, please attach a new patch next time. This makes it easier for the reviewer :D
comment:9 Changed 14 years ago by sayamindu
- Keywords r? added; r! removed
Patches attached.
The problem with de_DE is interesting. For many languages, it is OK when you use de instead of de_DE. However, certain languages are different, for example, pt_BR is different from pt, and bn_IN is different from bn (or bn_BD).
In such a situation, I wonder what the best approach would be. I am trying out a slightly enhanced weighing scheme which may work, but I need a few more days to properly test this out.
Changed 14 years ago by sayamindu
Unified version of the previously attached patches, as discussed on IRC
comment:10 Changed 14 years ago by sayamindu
Merged with mainline. Thanks for the thorough review. Closing this ticket.
comment:11 Changed 14 years ago by sayamindu
- Resolution set to fixed
- Status changed from new to closed
comment:12 Changed 10 years ago by dnarvaez
- Component changed from sugar-toolkit to Sugar
Thanks for the detailed description of your review request.
Two first comments:
from sugar.activity import activityhandle, i18n
Please use the docstring format like the one in sugar/datastore/datastore.py (with arguments and return values).
A deeper review will follow tomorrow.