Opened 11 years ago

Closed 11 years ago

Last modified 8 years ago

#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:

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)

0002-Consistent-variable-names.patch (3.6 KB) - added by sayamindu 11 years ago.
Consistent variable name underscoring
0003-Clean-up-docstring.patch (1.2 KB) - added by sayamindu 11 years ago.
Docstring cleanup
0004-Make-the-dictionary-sorting-a-bit-more-sensible.patch (932 bytes) - added by sayamindu 11 years ago.
Make the return section (dictionary sorting part) make more sense
unified_patch.patch (4.3 KB) - added by sayamindu 11 years ago.
Unified version of the previously attached patches, as discussed on IRC

Download all attachments as: .zip

Change History (17)

comment:1 follow-up: Changed 11 years ago by 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).

A deeper review will follow tomorrow.

comment:2 in reply to: ↑ 1 Changed 11 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 11 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 11 years ago by erikos

Ping, sayamindu, when do you think we can land this one?

comment:5 Changed 11 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 11 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 11 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 11 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

Changed 11 years ago by sayamindu

Consistent variable name underscoring

Changed 11 years ago by sayamindu

Docstring cleanup

Changed 11 years ago by sayamindu

Make the return section (dictionary sorting part) make more sense

comment:9 Changed 11 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 11 years ago by sayamindu

Unified version of the previously attached patches, as discussed on IRC

comment:10 Changed 11 years ago by sayamindu

Merged with mainline. Thanks for the thorough review. Closing this ticket.

comment:11 Changed 11 years ago by sayamindu

  • Resolution set to fixed
  • Status changed from new to closed

comment:12 Changed 8 years ago by dnarvaez

  • Component changed from sugar-toolkit to Sugar

comment:13 Changed 8 years ago by dnarvaez

  • Milestone 0.88 deleted

Milestone 0.88 deleted

Note: See TracTickets for help on using tickets.