1237662 - Set up a script to automatically import from the loop-client-l10n directory

Status: RESOLVED FIXED

(Hello (Loop) :: Client, defect, P1)

Description

[Mark Banner (:standard8)]

We need to set up imports of L10n from their repo at:

https://github.com/mozilla/loop-client-l10n

into the new loop repo.

For this, I think we should re-use the existing script as it gives us a large proportion of the user story already:

https://github.com/mozilla/loop-client/blob/master/locale_update.py

Comment 1

[Ed Lee :Mardak]

Is it intended to have the locale/ab-CD checked in to the loop repository? So there'll be locale/en-US as well as all the other ones from l10n/ab_CD from loop-client-l10n?

Or is this script supposed to run only for a xpi build or dist?

Comment 2

[Mark Banner (:standard8)]

(In reply to Ed Lee :Mardak from comment #1)
> Is it intended to have the locale/ab-CD checked in to the loop repository?
> So there'll be locale/en-US as well as all the other ones from l10n/ab_CD
> from loop-client-l10n?

Yes, its intended to be checked in. However, I don't want the script automatically checking in, as we should be doing a visual check of the changes - at least to begin with (we might change that later).

We'll also need to export these to m-c, hence the need for checking in.

Comment 3

[GitHub Autolander Bot]

Attachment: [loop] Mardak:bug-1237662-import > mozilla:master

Comment 4

[Ed Lee :Mardak]

Comment on attachment 8707807 [details] [review]
[loop] Mardak:bug-1237662-import > mozilla:master

https://github.com/Mardak/loop/commit/f8c505b4ccf767738a07496f373c139aed499a84 has the interesting changes to Makefile and bin/locale_update.py


f? as there's still some outstanding issues/questions:

- should we be detecting partially translated locales (missing strings and/or files)
-- completely missing should just use en-US, but partial results in errors
- should .keep files from loop-client-l10n be ignored/removed?

Comment 5

[Ed Lee :Mardak]

Oh and should there be a make target to run ./bin/locale_update.py ?

Comment 6

[Ed Lee :Mardak]

A related issue of detecting partially translated in that some strings aren't actually translated, e.g., zh_TW's legal_text_and_links

legal_text_and_links=By using {{clientShortname}} you agree to the {{terms_of_use_url}} and {{privacy_notice_url}}.

https://github.com/mozilla/loop-client-l10n/blob/master/l10n/zh_TW/standalone.properties#L20

Comment 7

[Ed Lee :Mardak]

Comment on attachment 8707807 [details] [review]
[loop] Mardak:bug-1237662-import > mozilla:master

f?dmose for Makefile changes that generate each of the loop.properties files for add-on as well as standalone.

From my testing, `make dist_xpi` correctly packages up all the locales at chrome/locale/* with the correct output chrome.manifest. And installing the xpi shows the translated strings.

`make standalone_dist` doesn't seem to be quite hooked up yet, but `make standalone` looks to generate the appropriate loop.properties and index.html files.

Comment 8

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8707807 [details] [review]
[loop] Mardak:bug-1237662-import > mozilla:master

Makefile parts look good; I've put a couple of minor comments in the PR for your consideration.

Comment 9

[Mark Banner (:standard8)]

(In reply to Ed Lee :Mardak from comment #4)
> - should we be detecting partially translated locales (missing strings
> and/or files)
> -- completely missing should just use en-US, but partial results in errors

For the add-on side of things, we're going to need a "complete" locale. Ideally for a partial locale, we'll copy the en-US versions of the strings. Alternately we'll have to look at replacing the desktop l10n system.

For the standalone, we've got a fallback system so we're ok there.

If we want to land something that just manages the complete locales for now, that might be useful and deal with the partial/fallback system in a follow-up maybe.

> - should .keep files from loop-client-l10n be ignored/removed?

We should not have those in the loop repo if possible.

Comment 10

[Ed Lee :Mardak]

Attachment: screenshot append translated strings to en-US strings

(In reply to Mark Banner (:standard8) from comment #9)
> (In reply to Ed Lee :Mardak from comment #4)
> > - should we be detecting partially translated locales (missing strings
> > and/or files)
> > -- completely missing should just use en-US, but partial results in errors
> For the add-on side of things, we're going to need a "complete" locale.
> Ideally for a partial locale, we'll copy the en-US versions of the strings.
> Alternately we'll have to look at replacing the desktop l10n system.
We could generate a loop.properties file that first has en-US strings then translated strings if available. The latter strings in the file are used. This won't affect l10n process as these are the files to be packaged in the xpi.

>-$(BUILT)/add-on/chrome/locale/%/loop.properties: locale/%/add-on.properties locale/%/shared.properties
>+$(BUILT)/add-on/chrome/locale/%/loop.properties: locale/en-US/add-on.properties locale/en-US/shared.properties locale/%/add-on.properties locale/%/shared.properties
>        cat $^ > $@

Where the cat expands to something like...
> cat locale/en-US/add-on.properties locale/en-US/shared.properties locale/bn-BD/add-on.properties locale/bn-BD/shared.properties > built/add-on/chrome/locale/bn-BD/loop.properties

I've attached a screenshot of what bn-BD looks like with the add-on installed with the above change.

Does this seem reasonable to package as many locales as possible in the add-on?

Otherwise, if having mixed strings is worse than not having any strings, we could try to only package locales that are fully translated (although some locales that seem to be 100% still have some en-US strings).

Comment 11

[Ed Lee :Mardak]

Comment on attachment 8708443 [details]
screenshot append translated strings to en-US strings

To be explicit: "Browse this page with a friend" and "Recently browsed" are falling back to en-US strings while the edit menu and footer are translated.

Comment 12

[Ed Lee :Mardak]

Without combining en-US and bn-BD strings into a single loop.properties file, clicking on the loop button results in an empty panel with: `No string found for key:  panel_browse_with_friend_button` in the console.

Comment 13

[Francesco Lodolo [:flod]]

I think I'm lacking a bit of context: is the add-on using l10n.js (I assume so)? You're creating a file that contains both en-US strings (first) and localized strings (after), how deterministic is the fact that we're going to pick the last version of a string available in the file?

The safe way would be to have a proper merge system that creates a file with localized strings, plus English strings when missing.

CCing also Pike in case he has ideas.

Comment 14

[Ed Lee :Mardak]

The strings eventually get passed into l10n.js:

https://github.com/mozilla/loop/blob/master/add-on/panels/vendor/l10n.js

The bundle given to l10n.js is handled by the service:

https://github.com/mozilla/loop/blob/master/add-on/chrome/modules/MozLoopService.jsm#L776

That enumerates the file in order and does a Map.set() with the string key/value, so that's why the latter ones get used.

To flod's comment 13 of having a proper merge system, MozLoopService could first read in en-US version of the strings (perhaps chrome://loop-en-US/locale/loop.properties) then process the localized version (chrome://loop/locale/loop.properties).

Comment 15

[Ed Lee :Mardak]

(In reply to Ed Lee :Mardak from comment #14)
> perhaps chrome://loop-en-US/locale/loop.properties
Actually, looks like we want to treat it as content as the locale entry in chrome.manifest will try to match locales, so:

% content loop-locale-fallback %locale/en-US/

Comment 16

[Ed Lee :Mardak]

Comment on attachment 8707807 [details] [review]
[loop] Mardak:bug-1237662-import > mozilla:master

Updated PR implementing flod's suggestion by updating MozLoopService to first load strings from chrome://loop-locale-fallback/content/loop.properties then overwrite with chrome://loop/locale/loop.properties if possible.

https://github.com/Mardak/loop/commit/f18d76297090aff183034fc37ca4186c3ca146aa#diff-1

Also got rid of the .keep files. I did have to touch locale/rm/shared.properties because it doesn't exist in loop-client-l10n yet.

Comment 17

[Axel Hecht [:Pike]]

I like that we're not modifying the l10n files. We also should get rid of the one-off l10n library here and get towards something shared with other folks.

Is there anything that's watching for locale changes at runtime? As you're generating your own cache, and language packs are restartless.

Comment 18

[Mark Banner (:standard8)]

(In reply to Axel Hecht [:Pike] from comment #17)
> I like that we're not modifying the l10n files. We also should get rid of
> the one-off l10n library here and get towards something shared with other
> folks.

We'll work on that in other bugs. 

> Is there anything that's watching for locale changes at runtime? As you're
> generating your own cache, and language packs are restartless.

This is one case I hadn't thought about. I suspect when we move to a different l10n library it may be easier to do, but we'll have to revisit it then.

Comment 19

[Mark Banner (:standard8)]

Comment on attachment 8707807 [details] [review]
[loop] Mardak:bug-1237662-import > mozilla:master

This looks good. I think there's some improvements that we can probably make (like not copying .keep files, or maybe adding to .gitignore), but lets see how this performs in use.

Comment 20

[Ed Lee :Mardak]

Added script:
https://github.com/Mardak/loop/commit/f6843220dc0e15833523b7584a39b3f1745093ed

Ran script `./bin/locale_update.py; rm -f locale/*/.keep`:
https://github.com/mozilla/loop/commit/a152d173d79528934ea0e25a1a2aa4d7c4f03178

Comment 21

[Ed Lee :Mardak]

(In reply to Axel Hecht [:Pike] from comment #17)
> Is there anything that's watching for locale changes at runtime? As you're
> generating your own cache, and language packs are restartless.
Indeed, we should be flushing the string cache on locale change. Filed bug 1240889.