Closed
Bug 1237662
Opened 8 years ago
Closed 8 years ago
Set up a script to automatically import from the loop-client-l10n directory
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(Not tracked)
RESOLVED
FIXED
Iteration:
46.3 - Jan 25
People
(Reporter: standard8, Assigned: Mardak)
References
Details
User Story
The script should: - Import into locale/ab-CD/ the equivalent files from l10n/ab-CD/ from the loop-client-l10n repository. - It should update standalone/content/index.html with the locale list. - It should add locale lines to jar.mn for all the locales. - It should remove any obsolete locales. - It should NOT automatically commit the changes, as these should be reviewed before landing. - The locale files should be combined on build into one loop.properties for each locale.
Attachments
(2 files)
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
Reporter | ||
Updated•8 years ago
|
Rank: 15
Assignee | ||
Comment 1•8 years ago
|
||
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?
Reporter | ||
Comment 2•8 years ago
|
||
(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.
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → edilee
Reporter | ||
Updated•8 years ago
|
Rank: 15 → 8
Comment 3•8 years ago
|
||
Assignee | ||
Comment 4•8 years ago
|
||
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?
Attachment #8707807 -
Flags: feedback?(standard8)
Assignee | ||
Comment 5•8 years ago
|
||
Oh and should there be a make target to run ./bin/locale_update.py ?
Assignee | ||
Comment 6•8 years ago
|
||
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
Assignee | ||
Comment 7•8 years ago
|
||
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.
Attachment #8707807 -
Flags: feedback?(dmose)
Comment 8•8 years ago
|
||
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.
Attachment #8707807 -
Flags: feedback?(dmose) → feedback+
Reporter | ||
Comment 9•8 years ago
|
||
(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.
Assignee | ||
Comment 10•8 years ago
|
||
(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).
Flags: needinfo?(standard8)
Flags: needinfo?(francesco.lodolo)
Assignee | ||
Comment 11•8 years ago
|
||
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.
Assignee | ||
Comment 12•8 years ago
|
||
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•8 years ago
|
||
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.
Flags: needinfo?(francesco.lodolo)
Assignee | ||
Comment 14•8 years ago
|
||
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).
Assignee | ||
Comment 15•8 years ago
|
||
(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/
Assignee | ||
Comment 16•8 years ago
|
||
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.
Flags: needinfo?(standard8)
Attachment #8707807 -
Flags: feedback?(standard8) → review?(standard8)
Comment 17•8 years ago
|
||
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.
Reporter | ||
Comment 18•8 years ago
|
||
(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.
Reporter | ||
Comment 19•8 years ago
|
||
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.
Attachment #8707807 -
Flags: review?(standard8) → review+
Assignee | ||
Comment 20•8 years ago
|
||
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
Assignee | ||
Updated•8 years ago
|
Status: NEW → RESOLVED
Iteration: --- → 46.3 - Jan 25
Closed: 8 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 21•8 years ago
|
||
(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.
Blocks: 1240889
You need to log in
before you can comment on or make changes to this bug.
Description
•