Roundup Tracker - Issues

Message7469

Author marcus.priesch
Recipients marcus.priesch, rouilj, schlatterbeck
Date 2022-04-27.15:50:43
Message-id <e7db32a6-a129-2d01-6b15-2d6de82d0359@priesch.co.at>
In-reply-to <1639701686.92.0.643750938582.issue2551184@roundup.psfhosted.org>
Hi John,

finally i have a patch to test with lots of changes ;)

but first some comments on your mail:

> What is struct.unpack?

it is used internally in parsing of the .mo files - i am not sure why it
is doing this ... but thats for another story.

> This generates a new translation object correct? IIRC it has
> disk access every time to scan the locale directories. So
> yeah that can eat time.
> 
> Is Ralf doing a:
> 
>    from roundup.i18n import _

no, he was doing a

_ = roundup.cgi.TranslationService.get_translation \
     (db.config.TRACKER_LANGUAGE, db.config.TRACKER_HOME).gettext

which does what you state above and was run in every detector which got
loaded - and there are a lot ;)

if you want to have translated error messages from during a web request,
than you cant simply do a

	from roundup.i18n import _

because the language can be different for every request see
"use_browser_language"

you also dont get the locales from tracker_home by just doing

	from roundup.i18n import _

thats also the reason why we want the "translator" attached to the db so
that we always have the correct translator at hand (including 
tracker_home locales and users lang preferences)

>> mail_gw: the setting in config.ini (mailgw.language) is
>> ignored, uses ENV setting if found, default "en"
> 
> Hmm according to config.ini if empty it should use
> tracker.language.
> 
>      Default locale name for the tracker mail gateway.  If
>      this option is not set, mail gateway will use the
>      language of the tracker instance.
> 
> Is tracker.language set? If tracker.language is not set,
> tracker.language is set from the environment (see
> below). Maybe that is what you are seeing?
> 
> Of course it could be broken.

yes it was broken - nobody used MAIL_LANGUAGE in the code ;)

> I don't know why you would have a different locale for the
> mail gateway. Perhaps a German speaking country that does
> most of it's work with English speaking counties via email?
> I don't know if there is a way to determine what language
> email is in. If there was it would be possible to implement
> a "use_mailer_language" similar to "use_browser_language".
> But this is a bit off the point.

i am also not aware of such a machanism, but maybe you set up the
mailgw on an "english" system and want the mailgw (and the tracker)
run for german users - without having to change the locale of the
whole system - i dont know ;)

it's documented, but it behaves wrong, so i fixed it ;)

> tracker.language is the trackers base language. The web
> interface, email etc. all inherit it (or are supposed to).
[...]

>> for the rest api it could be optional if one wants
>> translated error messages or speed - using the
>> NullTranslatior i gain another 2ms in speed per request
> 
> That seems a reasonable tradeoff. Also it could be enabled
> by a query parameter "?@language=code" so the REST user has
> the ability to change that (say for debugging or something).
> (Not sure what an equivalent method would be for xmlrpc.)

it is implemented exactly the same for rest and xmlrpc (the form needs
to have a @language field) i am not in xmlrpc, therefore i cannot say
much about it's behaviour in xmlrpc.

> I would attach it to the hyperdb in client.py rather than
> making it part of the hyperdb class.

so here is what i have done:

actually a NullTranslator is attached to the db in
roundupdb.Database.__init__ which gets called in rbms_common and back_anydbm

i am not sure if the "global _" thing is correct as i implemented it so
this needs check and test.

in Client.py this gets overridden when TRANSLATE_REST|XMLRPC is set,
considering "USE_BROWSER_LANGUAGE" and TRACKER_LANGUAGE/Environment.

in the MailGW it gets set either to MAIL_LANGUAGE or TRACKER_LANGUAGE
or defaults to Env

in cgi.engine_zopetal i removed all the GlobalTranslationService stuff
as i18n gets injected in cgi.TranslationService - needs testing, classic
tracker works.

removed translationService from cgi.templating.py and cgi.form_parser.py

form_parser.py now uses cgi.TranslationService directly as noone else
needed the globally set translationService in templating.py

in wsgi_handler i removed the preload stuff and keep the tracker
instance over the requests - only the db gets opened again on every
request, templates and detectors only get computed once - as they cant 
change during running.

i removed the translate method in TALES.py as it gets injected as stated
above in cgi.TranslationService

> However what would be better is to create and cache the
> translator in i18n. So it's set up on the first call to:

caching would be nice, but as it just gets called once per request i
think we can go along with the current behaviour.

caching would also need to be thread safe ...

> 
>    from roundup.i18n import _
> 
> _ should be initialized on the first import and just returned
> afterwards. I can't think of a reason that is not the right thing to
> do. That would benefit code all over the roundup base that uses that
> idiom. From a glance at the code it seems to do that now but I may be
> misinterpreting it or not understand how this stuff works (likely).

the problem is that i18n can change during processing of web requests,
therefore this does not work.

>>    initialise it with the NullTranslator (as this merely
>>    costs nothing) and change this if needed down the way
>>    (be it in the mail_gw, cli or cgi handler)
>>
>>    add config item rest_no_translation and
>>    xmlrpc_no_translation (or maybe own sections for rest
>>    and xmlrpc)
> 
> I wold prefer two booleans:
> 
>    translate_rest = yes
>    translate_xmlrpc = yes

done.

> Otherwise you have a double negative which makes my head
> hurt.

yeah, you are definitely right - my head hurts all the time ;)

> If they are set to no, then does it make sense to have the
> web code in client.py override db.i18n before it calls the
> rest and xmlrpc handlers?

no, if they are not set, the NullTranslator - which gets set on db
instantiation, doesnt get changed.

for using i18n in the detectors you have to use db.i18n.gettext et al
like this:

def statusfail(db, cl, nodeid, newvalues):
     _ = db.i18n.gettext
     raise ValueError(_("this does not work"))

def init(db):
     # fire before changes are made
     db.status.audit('create', statusfail)

i have added a module "pygettext.py" which i am using in other projects
too which can extract msgid's from python code.

i have documented this a littte bit ;)

i extended roundup_gettext.py to use this module and to generate one
messages.pot file with both template strings and detector/extension strings.

this however depends on https://polib.readthedocs.io to merge the two
sources which i am not sure if you a) want a dependency in roundup, as i
have not seen any deps in setup.py or b) i have not found the correct
place to add it ... or c) just to document that it is a dependency if
you want to hack on your own tracker ...

attached is the patch against current tip, ralf will test it and modify
our tracker to use the db.i18n.gettext inside his detectors and then,
hopefully we have blazing fast rest API :)

... and hopefully it doesnt crash anything ...

regards,
marcus.
Files
File name Uploaded
improve_i18n.patch marcus.priesch, 2022-04-27.15:50:41
History
Date User Action Args
2022-04-27 15:50:43marcus.prieschsetrecipients: + marcus.priesch, schlatterbeck, rouilj
2022-04-27 15:50:43marcus.prieschlinkissue2551184 messages
2022-04-27 15:50:43marcus.prieschcreate