Edgewall Software
Modify

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#7497 closed enhancement (fixed)

I18N/L10N support for plugins

Reported by: palgarvio Owned by: cboos
Priority: high Milestone: 0.12
Component: i18n Version: 0.12dev
Severity: critical Keywords: i18n
Cc: trac-ja@…
Release Notes:
API Changes:

Description

I'm opening this ticket to get some opinions on how the localization of trac plugins should be done.

There are several options:

  • Merge trac catalogs and plugin catalogs at runtime, but this might be a disadvantage because while merging catalogs, if 2 of them share the same msgid, the one being merged will override the existing one, ie, the trac one, allowing plugins to introduce translation errors which might make users create new ticket's for trac and not the plugin in question.
  • Make use of msgctxt. Each plugin will have it's name as it's context but this might be abusing a bit of the msgctxt. Defenition here.
  • Make use of domains. Each plugin will have it's own domain, and all localization calls must include the domain.

Now, some more problems:

  • Babel's msgctxt support is close to None currently and it's meant for 1.0, only some minor work has been done.
  • Genshi's msgctxt support is non-existant mostly because Babel does not yet support it.
  • Regarding the domain option, while this is easy inside python files, it won't be that easy regarding templates; perhaps a i18n:domain attribute should be added to genshi allowing a template to define it's domain and thus, allowing genshi to pass the domain when translating the message.

I'm tending towards the domain option, nothing will have to change regarding Babel and changes regarding genshi shouldn't be that much.

Also, regarding plugin developers, this would just mean using another translation function other than gettext or _ for example, and passing an extra arg; perhaps one could even do some code magic to ease this.

A new extension point would probably also be created so that plugins can register their domain and respective catalogs into trac's translator.

Attachments (5)

support_genshi_i18n_choose.patch (571 bytes) - added by palgarvio 6 years ago.
plugin_i18n_support.patch (7.2 KB) - added by palgarvio 6 years ago.
plugin_i18n_support-r7676.patch (7.4 KB) - added by cboos 6 years ago.
Updated patch - applies cleanly on current trunk, normal translations are working but I've not yet tested add_domain
plugin_i18n_support-r7678.patch (10.4 KB) - added by cboos 6 years ago.
Version améliorée, well, improved version of the i18n support for plugin
plugin_i18n_support-domain_functions.patch (11.0 KB) - added by cboos 6 years ago.
Same as previous patch, with a new helper function domain_functions to ease the setup of plugins.

Download all attachments as: .zip

Change History (35)

comment:1 Changed 6 years ago by palgarvio

The above patch allows plugins to provide their own translations.

Unlike other projects which just merge catalogs together possibly overriding a good translation for a bad one, this patch "detects" if the caller of the function has a domain variable defined on top of file, if it has, it switches to that domain, translates and then switches back to trac's domain.

So, plugin developers should start defining the domain. As an example, consider the following file my_module/web_ui.py:

from trac.core import *

domain = 'my_module_messages'

class MyModuleWebUi(Component):
    def __init__(self):
        print _("Foo")

So, imagine a request to the above component comes in using pt_PT for the locale. What happens is that the pt_PT translations on MyPackage/my_module/locale are loaded, "Foo" is translated and then the trac translations are loaded back.

comment:2 Changed 6 years ago by palgarvio

Oh, the only thing is that plugins must not have their catalog's domain be messages, they should be for the above example, my_module_messages.

comment:3 Changed 6 years ago by trac-ja@…

  • Cc trac-ja@… added

comment:4 Changed 6 years ago by palgarvio

Updated patch.

comment:5 Changed 6 years ago by palgarvio

This latest patch depends on ticket:137.

Changed 6 years ago by palgarvio

Changed 6 years ago by palgarvio

comment:6 Changed 6 years ago by palgarvio

  • Owner set to cmlenz

You can now disregard the support_genshi_i18n_choose.patch, that one would only work for python code.

The really good patch is the plugin_i18n_support.patch.

This one also depends on ticket:137 and ticket:129, both this tickets have patches not yet applied to their sources. Those patches reached good working point and are just waiting for cmlenz to review and check them in.

Do note that with all 3 patches applied to all sources, plugin developers will then have the change to provide translations on a separate domain for their plugins. This works for both python code and genshi templates.

comment:7 Changed 6 years ago by cboos

  • Keywords i18n added
  • Priority changed from normal to high
  • Severity changed from normal to critical

In plugin_i18n_support.patch, I don't like so much the sys._getframe(1).f_globals.get('domain') hack.

What about this alternative, e.g.:

(in myplugin/__init__.py)

from trac.util.translation import dgettext

def _(string, **kwargs):
    return dgettext('mydomain', string, **kwargs)

(in myplugin/backend.py)

from trac.core import Component
from myplugin import _

class MyPlugin(Component):
    def __init__(self):
        self.log.info(_("MyPlugin %(version)s loaded ", version="0.1"))

Also, what's the status of the dependencies? Is using Babel:milestone:0.9.4 and Genshi:r954 (trunk) enough for experimenting with the attachment:plugin_i18n_support.patch?

comment:8 Changed 6 years ago by cboos

Looks like Genshi:source:branches/experimental/advanced-i18n is needed.

I had to fix a few conflicts for the patch above, due to recent changes in trunk.

The add_domain could probably benefit from some locking, as it manipulates a global, otherwise this seems risky (IIUC, that add_domain has to be called from plugins in their __init__ method).

Finally, looking at Babel:r448, it seems that the way to add a domain to the translations has changed since you wrote the patch.

I'm posting a work-in-progress patch which at least applies cleanly, but I have yet to try the domains with a plugin.

Changed 6 years ago by cboos

Updated patch - applies cleanly on current trunk, normal translations are working but I've not yet tested add_domain

comment:9 Changed 6 years ago by cboos

Ah yes, the patch also introduce a bug by using translation.get_translations() when creating the Translator object:

  • trac/web/chrome.py

     
    660662        """
    661663        if not self.templates:
    662664            def _template_loaded(template):
    663                 template.filters.insert(0, Translator(translation.gettext))
     665                translator = Translator(translation.get_translations())
     666                setup_i18n(template, translator)
    664667
    665668            self.templates = TemplateLoader(self.get_all_templates_dirs(),
    666669                                            auto_reload=self.auto_reload,

That will associate permanently the currently activated Translations object with the template as the TemplateLoader will only call the callback on first load. From then on, the template will be retrieved from the cache and the filter will keep using the original Translations object, not the currently activated one.

This can be easily verified, e.g. on the preference page itself, if you start in German, then switch to French: the translations coming from the html template will stay in german, those obtained from the code will be in french.

Suggested fix: we should instead pass a Translations "proxy" that has its translation methods delegating the call to the currently active Translations instance.

comment:10 Changed 6 years ago by palgarvio

Sorry for not helping that much now, but I've started a clean install on my laptop yesterday and my development environment is not yet up and running. I also thought I had submited a reply which I'm now not seeing but I guess you did find the right branch :)

I'll test what you propose as soon as possible.

Changed 6 years ago by cboos

Version améliorée, well, improved version of the i18n support for plugin

comment:11 follow-up: Changed 6 years ago by cboos

  • Owner changed from cmlenz to cboos
  • Status changed from new to assigned

In attachment:plugin_i18n_support-r7678.patch, I did several refactorings:

  • fixed the bug explained in comment:9 by introducing a TranslationsProxy class.
  • removed the sys._getframe(1)... hack from the TranslationsProxy *gettext methods

Also, I tested the add_domain capability on the Mercurial plugin, along the way suggested in comment:7.

The tracext.hg.__init__.py contains the translation marker/substitution functions:

import pkg_resources

from trac.util.translation import dgettext, dtgettext, add_domain

domain = "tracmercurial"

def _(string, **kwargs):
    return dgettext(domain, string, **kwargs)

def tag_(string, **kwargs):
    return dtgettext(domain, string, **kwargs)

def N_(arg):
    return arg


def add_plugin_domain(env_path):
    locale_dir = pkg_resources.resource_filename(__name__, '../locale')
    add_domain(env_path, domain, locale_dir)

The adaptations to the tracext.hg.backend.py are minimal:

# ...

from tracext.hg import _, tag_, N_, add_plugin_domain

# only the add_plugin_domain() line is new:

class MercurialConnector(Component):

    implements(IRepositoryConnector, IWikiSyntaxProvider)

    def __init__(self):
        self._version = None
        self.ui = None
        add_plugin_domain(self.env.path)

Also the setup.cfg contains the domain spec:

[extract_messages]
add_comments = TRANSLATOR:
msgid_bugs_address = cboos@neuf.fr
output_file = tracext/locale/messages.pot
keywords = _ ngettext:1,2 N_ tag_
width = 72

[init_catalog]
input_file = tracext/locale/messages.pot
output_dir = tracext/locale
domain = tracmercurial

[compile_catalog]
directory = tracext/locale
domain = tracmercurial

[update_catalog]
input_file = tracext/locale/messages.pot
output_dir = tracext/locale
domain = tracmercurial

I have yet to test what happens for a conflict (i.e. when a translation in the tracmercurial catalog differs from one in the base trac message catalog), but other than that it seems to work fine.

comment:12 in reply to: ↑ 11 Changed 6 years ago by cboos

Follow-up to comment:11

I have yet to test what happens for a conflict (i.e. when a translation in the tracmercurial catalog differs from one in the base trac message catalog),

Conflicts are handled correctly, domain translation is preferred, falling back to the main catalog if not found in the domain catalog. Nicely done on the Babel level!

Now, I came up with a helper function that simplifies the setup for domain specific translations functions.

One has simply the following to do in his/her plugin, e.g. for tracext/hg/backend.py:

from trac.util.translation import domain_functions

_, tag_, N_, add_domain = domain_functions('tracmercurial', 
    '_', 'tag_', 'N_', 'add_domain')

# use _, tag_ and N_ as usual, e.g. _("this is a diff")

class MercurialConnector(Component):

    implements(IRepositoryConnector, IWikiSyntaxProvider)

    def __init__(self):
        self._version = None
        self.ui = None
        # bind the 'tracmercurial' catalog to the specified locale directory
        locale_dir = pkg_resources.resource_filename(__name__, '../locale')
        add_domain(self.env.path, locale_dir)

There are a few things left to be done on the patch:

  • add locking for add_domain (comment:8)
  • add a dummy domain_functions for the case Babel is not present

See attachment:plugin_i18n_support-domain_functions.patch

Changed 6 years ago by cboos

Same as previous patch, with a new helper function domain_functions to ease the setup of plugins.

comment:13 follow-ups: Changed 6 years ago by cboos

As a side-note, I wanted to extract the messages using Babel 0.9.4 and Genshi's advanced-i18n branch, but then I lost several strings (e.g. from the macros.html).

Looking a bit around, I saw the uncomment below to extract messages without adding directives comment in the i18n.py Genshi filter. Can you tell us more about that? Are we supposed to add i18n:msg attributes everywhere in Trac?

In the meantime, I'm using Genshi 0.5.1 for the extraction.

comment:14 in reply to: ↑ 13 ; follow-up: Changed 6 years ago by palgarvio

Replying to cboos:

As a side-note, I wanted to extract the messages using Babel 0.9.4 and Genshi's advanced-i18n branch, but then I lost several strings (e.g. from the macros.html).

Looking a bit around, I saw the uncomment below to extract messages without adding directives comment in the i18n.py Genshi filter. Can you tell us more about that? Are we supposed to add i18n:msg attributes everywhere in Trac?

In the meantime, I'm using Genshi 0.5.1 for the extraction.

Well, when setting up Genshi I18N(on the advanced-i18n branch) one should now also install/setup the i18n directives which will handle perfectly the messages extraction, like those messages you've lost. Got any public test code to see what/how you're doin it?

You should now use genshi.filters.i18n.setup_i18n. Ie, trac should be doin' something like this.

comment:15 in reply to: ↑ 13 Changed 6 years ago by palgarvio

Replying to cboos:

Are we supposed to add i18n:msg attributes everywhere in Trac?

Forgot to answer this one, No! Only if needed to handle html markup(<p i18n:msg="">Foo<b>Bar</b></p>) and/or to handle plurals(i18n:choose).

comment:16 in reply to: ↑ 14 ; follow-up: Changed 6 years ago by cboos

Replying to palgarvio:

… when setting up Genshi I18N(on the advanced-i18n branch) one should now also install/setup the i18n directives which will handle perfectly the messages extraction, like those messages you've lost. Got any public test code to see what/how you're doin it?

Well, I'm talking about the Trac codebase. I use latest trunk + the patch above, which seems to call setup_i18n as appropriate (but maybe not, I didn't touch that part of the original patch). That's on the usage side, though.

But I was talking about the message extraction phase and the content of the catalog, i.e. when one runs python setup.py extract_messages, with Babel 0.9.4 and Genshi's advanced-i18n branch. Then, the messages.pot file is updated, some new messages are effectively added, but also some others are lost, notably all the strings coming from trac/templates/macros.html (e.g. look for Case changes).

The tmpl.add_directives(Translator.NAMESPACE, translator) call seems to be done in extract, the Genshi plugin for Babel, so maybe it's another problem/bug at work here. Maybe you could have a look?

comment:17 in reply to: ↑ 16 Changed 6 years ago by palgarvio

Replying to cboos:

But I was talking about the message extraction phase and the content of the catalog, i.e. when one runs python setup.py extract_messages, with Babel 0.9.4 and Genshi's advanced-i18n branch. Then, the messages.pot file is updated, some new messages are effectively added, but also some others are lost, notably all the strings coming from trac/templates/macros.html (e.g. look for Case changes).

The tmpl.add_directives(Translator.NAMESPACE, translator) call seems to be done in extract, the Genshi plugin for Babel, so maybe it's another problem/bug at work here. Maybe you could have a look?

I've added a testcase to genshi's advanced i18n branch(have a look)with a code snippet grabbed from macros.html which succeeds, I'll see if I can catch the bug(lack of code/i18n setup) somehere else.

comment:18 Changed 6 years ago by palgarvio

heh, failed the macros.html link :)

comment:19 Changed 6 years ago by palgarvio

Ok, found the bug!!!

The issue happens because of the py:strip used, must be something failing on the genshi's advanced i18n branch.

Here's the failing testcase which reproduces the problem you noticed.

comment:20 Changed 6 years ago by palgarvio

The issue is happening because currently I'm pop'ing the strip directive while extracting, something that was needed by some reason which I know don't remember now, but has something to do with the 3 failing testcases if I don't pop it ;)

I'll see what I can do regarding this issue.

comment:21 Changed 6 years ago by palgarvio

Please update your genshi advanced i18n branch and see if latest changes fix the problems you found.

comment:22 follow-up: Changed 6 years ago by cboos

Works fine with Genshi:r966, thanks!

Now, any suggestion about the patch itself and domain_functions or can I finalize it as presented in comment:12?

comment:23 in reply to: ↑ 22 Changed 6 years ago by palgarvio

Replying to cboos:

Works fine with Genshi:r966, thanks!

Now, any suggestion about the patch itself and domain_functions or can I finalize it as presented in comment:12?

Nope, no suggestions, your solution is much cleaner, go for it!

comment:24 follow-up: Changed 6 years ago by cboos

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

Finalized version committed as [7705]. Thanks pedro for doing all the ground work on this!

comment:25 follow-ups: Changed 6 years ago by rblank

  • Resolution fixed deleted
  • Status changed from closed to reopened

There's a problem with [7705], as it tries to import setup_i18n from genshi.filters, but this symbol is present neither in 0.5.1 nor in Genshi trunk, only in the advanced-i18n branch.

Maybe setup_i18n should be imported conditionally?

comment:26 in reply to: ↑ 25 Changed 6 years ago by palgarvio

Replying to rblank:

There's a problem with [7705], as it tries to import setup_i18n from genshi.filters, but this symbol is present neither in 0.5.1 nor in Genshi trunk, only in the advanced-i18n branch.

Maybe setup_i18n should be imported conditionally?

Push cmlenz to review the branch and include those changes into trunk ;)

comment:27 in reply to: ↑ 24 Changed 6 years ago by palgarvio

Replying to cboos:

Finalized version committed as [7705]. Thanks pedro for doing all the ground work on this!

Anytime!

comment:28 in reply to: ↑ 25 Changed 6 years ago by cboos

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

Replying to rblank:

Maybe setup_i18n should be imported conditionally?

Right, done so in [7707].

comment:29 Changed 6 years ago by rblank

Unfortunately, this was not enough, in the case where Babel is not installed:

  • The various d(|n|t|tn)gettext() functions are missing.
  • There's a typo in domain_functions().
  • activate() is missing the second argument.
  • There was a regression of the fix from [7480].

I fixed these in [7713]. All unit tests pass here, but it looks like Bitten is still choking on something. Ah, got it, the functools module was introduced in 2.5. Fixed in [7714].

comment:30 Changed 6 years ago by cboos

Thanks for fixing the breakage, I'm focusing on the Python 2.5/Babel-0.9.4/Genshi advanced-i18n combination for now…

About partial: we have it in compat, i.e. from trac.util.compat import partial.

For the i18n ns error, I was looking in the wrong direction, thought we had to tweak something at the lxml level, I probably missed [7480].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain cboos.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from cboos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.