VPN-5748: Use shared strings for addons #9763

mcleinman · 2024-08-02T18:07:58Z

Description

This adds a new translation file - ./addons/strings.yaml - where all future message addon strings will live. Bringing all (future) strings to one file allows us to reuse strings across messages, to reduce work on our translation team. My expectation is that the translation file will have this shared section (which may be added to), as well as sections specific to a single addon message (when appropriate). Additionally, there is one argument that can be substituted in - the version number. (I did this instead of something that could support a range of arguments to make the build.py changes simpler. I'm having trouble imaging anything other than a version number we'd want to drop in, and we can make this more generic in the future if it seems like that would be helpful.

All future message addons should include the usesSharedStrings boolean (set to true), as well as the shortVersion (used to insert into common strings). Documentation has been updated.

After this is merged, we'll need to merge mozilla-l10n/mozilla-vpn-client-l10n#466 so that translations are pulled into the i18n repo.

The bulk of the changes are in build.py. (In order to support older clients, all the changes need to be in the addons build system - as a solution that required the clients to do part of the workflow would not work on older clients.) This changes nothing for older clients, but when usesSharedStrings is present and true, it changes the flow:

The retrieve_strings_message function is changed to do some checks and then return the results of retrieve_legacy_strings_message or retrieve_shared_strings_message, depending if usesSharedStrings is present and true.
1. retrieve_legacy_strings_message has the logic that was moved out of retreive_strings_message.
2. retrieve_shared_strings_message returns strings in a similar fashion to the legacy function, but pulls the value and comments from the appropriate places (and look at the strings.yaml file to help construct this). It also includes a legacy_id to swap in during the final step.
3. retrieve_strings_blocks is updated to do similar things as retrieve_shared_strings_message for messages that use shared strings.
If useSharedStrings is present and true, the script confirms shortString is present and a string (and errors out if it is not present).
If useSharedStrings is present and true, it changes all the input xliff files before calling lconvert. Typically, it finds the shared strings.xliff file for the given language (this is the file that will exist in the i18n submodule), and calls transform_shared_strings on it. It creates an output xliff in the temp directory, and this output xliff file is what is used by the convert.
1. transform_shared_strings does 3 things before writing the temp xliff file: It filters the shared strings.xliff file (the input) to only include the strings relevant for this specific message addon. It substitutes the string ids for the legacy string IDs (saved much earlier in the process), so that the addon displays as expected on all clients. And finally, if it encounters %1 in any string, it substitutes in the shortVersion.

generate_shared_addon_xliff.py is used by the translation repo to pull these strings upstream. (Will be activated with a PR on that repo: mozilla-l10n/mozilla-vpn-client-l10n#466). It should be well commented enough to easily understand what it does; if not please let me know and I'll improve the comments.

Additionally, I've pulled some shared python code into ./utils/shared.py and removed some duplicated code around the repo. (We couldn't just import write_en_language from build.py because of the required arguments for that file.)

Testing

I tested this with the 2.24 update message addon (message_update_v2.24). To do this:

version.txt: change the current version to 2.23.0 (so that it shows the update message)
addons/strings.yaml: Uncomment out the tester string (as well as the 224updateMessage if you want). This is done to show us ignoring unneeded strings when creating the addon translation files, and still reporting accurate translation percentages.
addons/message_update_v2.24/manifest.json: In the message section, add the usesSharedString and shortVersion keys, and update the title, subtitle, and all content values to shared strings. That is, change the message section to:

     "date": 1722476590,
     "id": "message_update_v2.24",
     "usesSharedStrings": true,
     "shortVersion": "2.24",
     "title": "vpn.commonStrings.updateTitle",
     "subtitle": "vpn.commonStrings.subtitle",
     "badge": "new_update",
     "blocks": [
       {
         "id": "c_1",
         "type": "text",
         "content": "vpn.commonStrings.generalUpdateContent"
       },
       {
         "id": "c_3",
         "type": "button",
         "style": "primary",
         "content": "vpn.commonStrings.updateButton",
         "javascript": "update.js"
       },
       {
         "id": "c_4",
         "type": "button",
         "style": "primary",
         "content": "vpn.commonStrings.downloadButton",
         "javascript": "updateWeb.js"
       },
       {
         "id": "c_5",
         "type": "button",
         "style": "link",
         "content": "vpn.commonStrings.getHelpButton",
         "javascript": "getHelp.js"
       },
       {
         "id": "extra_1",
         "type": "text",
         "content": "vpn.commonStrings.downloadTitle"
       }
     ]
   }

After this is merged, we’ll get updated files from the translation repo. For now, I manually created two files in the i18n submodule to show how this works. Create a file ./3rdparty/i18n/en/addons/strings.xliff, and insert this text:

<?xml version='1.0' encoding='UTF-8'?>
<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" version="1.2">
  <file original="addon.qml" datatype="plaintext" source-language="en" target-language="en-US">
    <body>
      <trans-unit id="vpn.commonStrings.updateTitle">
        <note annotates="source" from="developer">Standard text in a composer block</note>
        <source>Update title %1</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.downloadTitle">
        <note annotates="source" from="developer">Standard text in a composer block</note>
        <source>Download title</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.subtitle">
        <note annotates="source" from="developer">Subtitle for a message view</note>
        <source>Subtitle</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.generalUpdateContent">
        <note annotates="source" from="developer">Title for a message view</note>
        <source>The typical content text</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.updateButton">
        <note annotates="source" from="developer">Title for a message view</note>
        <source>Update button</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.downloadButton">
        <note annotates="source" from="developer">Title for a message view</note>
        <source>Download button</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.getHelpButton">
        <note annotates="source" from="developer">Title for a message view</note>
        <source>Get help button</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.tester">
        <note annotates="source" from="developer">Title for a message view</note>
        <source>TESTER: This should never show up</source>
      </trans-unit>
    </body>
  </file>
</xliff>

Additionally, create ./3rdparty/i18n/fr/addons/strings.xliff:

<?xml version='1.0' encoding='UTF-8'?>
<xliff xmlns="urn:oasis:names:tc:xliff:document:1.2" version="1.2">
  <file original="addon.qml" datatype="plaintext" source-language="en" target-language="fr">
    <body>
      <trans-unit id="vpn.commonStrings.updateTitle">
        <note annotates="source" from="developer">Standard text in a composer block</note>
        <target>French: Update title %1</target>
        <source>Update title %1</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.downloadTitle">
        <note annotates="source" from="developer">Standard text in a composer block</note>
        <target>French: Download title</target>
        <source>Download title</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.subtitle">
        <note annotates="source" from="developer">Subtitle for a message view</note>
        <target>French: Subtitle</target>
        <source>Subtitle</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.generalUpdateContent">
        <note annotates="source" from="developer">Title for a message view</note>
        <target>French: The typical content text</target>
        <source>The typical content text</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.updateButton">
        <note annotates="source" from="developer">Title for a message view</note>
        <target>French: Update button</target>
        <source>Update button</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.downloadButton">
        <note annotates="source" from="developer">Title for a message view</note>
        <target>French: Download button</target>
        <source>Download button</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.getHelpButton">
        <note annotates="source" from="developer">Title for a message view</note>
        <target>French: Get help button</target>
        <source>Get help button</source>
      </trans-unit>
      <trans-unit id="vpn.commonStrings.tester">
        <note annotates="source" from="developer">Title for a message view</note>
        <source>TESTER: This should never show up</source>
      </trans-unit>
    </body>
  </file>
</xliff>

Then follow the instructions in the How to implement and test add-ons section of ./docs/Components/Addons/index.md, and compile a new version of the app to view the addons. (We need a new version of the app so that the version number is reported as 2.23, showing the update addon.)

Non-comprehensive list of things to do In testing:

Looked at the % translated (still shows as 100% for French, even though it doesn’t include a translation for commonStrings.tester)
French should show its translation
All Englishes should show English (demonstrating that language fallback works)
No other languages should show this addon, as it doesn’t have translations.
If shortVersion is missing, the build script will error out.
If the addon is changed to a string ID that is not in strings.yaml 9say, change commonStrings.title to commonStrings.fakeTitle, the build script will error out.

Reference

VPN-5748

Checklist

My code follows the style guidelines for this project
I have not added any packages that contain high risk or unknown licenses (GPL, LGPL, MPL, etc. consult with DevOps if in question)
I have performed a self review of my own code
I have commented my code PARTICULARLY in hard to understand areas
I have added thorough tests where needed

flodolo

(don't have the build set up locally, so can't really test this)

Note that there is also a side on the l10n repository to update
https://github.com/mozilla-l10n/mozilla-vpn-client-l10n/blob/main/.github/workflows/update.yaml#L85-L91
https://github.com/mozilla-l10n/mozilla-vpn-client-l10n/blob/main/.github/scripts/extract_source_strings.py

addons/strings.yaml

scripts/addon/build.py

mcleinman · 2024-08-07T11:26:36Z

(don't have the build set up locally, so can't really test this)

Note that there is also a side on the l10n repository to update https://github.com/mozilla-l10n/mozilla-vpn-client-l10n/blob/main/.github/workflows/update.yaml#L85-L91 https://github.com/mozilla-l10n/mozilla-vpn-client-l10n/blob/main/.github/scripts/extract_source_strings.py

@flodolo As mentioned at the top of this PR (in italics, somewhat hidden, sorry) - Should I put up the patch for the l10n repo, or is that something you'd want to do? I'm happy either way.

flodolo · 2024-08-07T12:27:00Z

Should I put up the patch for the l10n repo, or is that something you'd want to do?

It's much easier if it comes from your team, as it's hard for me to predict how much time I could dedicate to it.

scripts/addon/build.py

… repo

mcleinman · 2024-08-20T21:27:53Z

Should I put up the patch for the l10n repo, or is that something you'd want to do?

It's much easier if it comes from your team, as it's hard for me to predict how much time I could dedicate to it.

PR is up, but it shouldn't be merged until this one is: mozilla-l10n/mozilla-vpn-client-l10n#466

flodolo

Haven't looked much at build.py, because that's an area I'm not familiar with (and can't test locally).

addons/strings.yaml

scripts/utils/generate_shared_addon_xliff.py

flodolo · 2024-08-21T05:33:12Z

scripts/utils/import_languages.py

I haven't looked at these scripts in a while, but I'm confused: where's the i18n submodule located?

i18ndir = os.path.join('src', 'translations', 'i18n')

If I update submodule, i18n is created in 3rdparty/i18n, so I have no clue how can this work.
https://github.com/mozilla-mobile/mozilla-vpn-client/blob/main/.gitmodules#L35

Also, that path being relative doesn't seem very safe.

It seems like it would work by creating a fresh copy of the repo there.

That said, I can't find where import_languages.py is ever used. It seems like this work is now done in generate_translations_target.cmake.

oskirby

I think the python code is fine, but I feel that the approach isn't particularly flexible and it only really services one specific use case.

An example addon that uses this feature would be handy as well.

oskirby · 2024-08-22T17:53:51Z

scripts/addon/build.py

@@ -79,7 +86,7 @@ def retrieve_strings_tutorial(manifest, filename):
    return tutorial_strings


-def retrieve_strings_blocks(blocks, filename, strings, prefix):
+def retrieve_strings_blocks(blocks, filename, strings, prefix, shared_strings):


nit: optional arguments can be given a default value, eg:

def retrieve_strings_blocks(blocks, filename, strings, prefix, shared_strings=None):

oskirby · 2024-08-22T21:22:56Z

docs/Components/messages.md

@@ -17,11 +17,13 @@ property object with the following properties:
 | Property | Description  | Type | Required |
 | -- | -- | --| -- |
 | id | A unique identifier for the message | String | Yes |
+| usesSharedStrings | Whether it uses the translation file shared between all messages (default: false) | Boolean | No |


I don't want to blow the scope up too much, but it seems less than ideal for this to be a top-level config for the message addons. Consider for example, that at some point we want to release an update with some non-trivial messages. Having this exist as a top-level config means that we couldn't re-use any of the messages (even the boring stuff like the title).

Is it possible to have an approach where the addon generation script checks for translations in both the common and addon-specific locations, and chooses the appropriate one (defaulting to strings in the addon-specific location in the event of a conflict)?

I think I caused some confusion here - you can mix generic and one time strings in a message. Is that what you mean? I think the confusion is from my naming of usesSharedStrings, and perhaps this variable should be updated.

The strings.yaml file is meant to handle both reusable strings and one-time strings. While reusable strings will stay in there forever, you can also create some category for specificMessage, and then set vpn.specific225UpdateMessage.uniqueBody for the content but vpn.commonStrings.updateTitle for the title. The practice would be to delete the uniqueBody string from strings.yaml when it's no longer needed, but keep the commonStrings around forever. (I tried to give examples both the instructions in strings.yaml and in the commented out stuff in strings.yaml, but perhaps that needs to be improved as well.)

In that sense, usesSharedStrings is poorly named, as it's really a boolean of "do you use the old method or the new method of strings?". If this explanation was helpful and the issue is the naming of usesSharedStrings, let me know and I'll update it.

oskirby · 2024-08-22T21:28:42Z

docs/Components/messages.md

@@ -17,11 +17,13 @@ property object with the following properties:
 | Property | Description  | Type | Required |
 | -- | -- | --| -- |
 | id | A unique identifier for the message | String | Yes |
+| usesSharedStrings | Whether it uses the translation file shared between all messages (default: false) | Boolean | No |
+| shortVersion | A human-readable version number to insert into user-facing strings | Strings | Yes if usesSharedStrings is true, No otherwise |


nit: This PR is a lot of work to go through, and it leaves us only one variable which can be set in common strings.

Please note, I am not asking to change this... more just pondering about the extensibility of this feature.

Fully agreed... I started to draft out a generic variable system in these strings, but quickly realized I could not dream up a use case where we'd need it beyond "drop in the version". We wouldn't have a variable that is number (thus needing singular/plural) in a message, I don't think. In general, we never have variables in messages even.

If you can find an old message where this would be helpful, or have a potential use case, let me know and I'm happy to include it.

mcleinman · 2024-08-28T21:58:22Z

I believe I've addressed all comments on this, should be ready for another round of review.

flodolo

I think there is a problem here: this is currently extracting only strings from the main branch (also based on the WIP PR here).

I don't think that's enough, because we would lose any string that is removed from main, unless we manually keep them around.

One way to solve this is to include the script in generate_ts.sh, which goes through main and all the release branches.

mcleinman · 2024-09-04T18:25:46Z

Addons get built and deployed from main, so I think we should be okay, given that this is addons-specific. (Yes, this is confusing. And yes, addons should probably be their own repo.)

That's confusing… why are we extracting addon strings from all branches then?

I can't speak to why it was written that way historically, but for the last couple years addons have been exclusively built from main. The one potential issue with this is that there is a short period of time where we lose translations to the prod strings, and we wouldn't be able to redeploy without retranslation:

Several weeks into release of version 0.1 (for example), we update addons to prepare for version 0.2. As part of this, we remove the "update to 0.1" message strings, and add the "update to 0.2" message strings.
In between this point and when 0.2 goes out to full public release, we no longer have "update to 0.1" strings/translations. If we had to do a redeploy of addons for some reason, we'd need to get those strings retranslated.

Despite this, the team is comfortable with only pulling addon strings from main. While we occasionally do a re-release of addons within a day or 2 of a release, we can't think of any time where we've re-released addons weeks after the release went out - and can't think of a scenario where we'd need to do it.

(The older way pulling addons strings from release branches and main isn't hurting anything, after we move to this new system - Because if this new strings.yaml isn't covered, we won't be pulling anything relevant for addons from the release branches. That said, we can file a ticket to remove that outdated code if desired.)

flodolo · 2024-09-05T19:13:52Z

You don't need to import yaml in files like build.py or generate_shared_addon_xliff.py, since there is no code using it.

I assume you're trying to fix a test failure somewhere?

mcleinman added 2 commits August 1, 2024 16:38

VPN-5748 - reusable strings convert in the build process

2fd6fd2

move parameters in manifest file, add in shortVersion swapping

2211759

mcleinman requested review from flodolo, oskirby, strseb and lesleyjanenorton August 2, 2024 18:07

mcleinman marked this pull request as draft August 2, 2024 18:38

make string file only include relevant strings

a9eb258

flodolo reviewed Aug 5, 2024

View reviewed changes

addons/strings.yaml Outdated Show resolved Hide resolved

scripts/addon/build.py Outdated Show resolved Hide resolved

PR feedback

93cec33

update dictionary

23df4ba

flodolo reviewed Aug 7, 2024

View reviewed changes

scripts/addon/build.py Outdated Show resolved Hide resolved

share code

bc167dc

mcleinman mentioned this pull request Aug 20, 2024

Run script for shared addons file mozilla-l10n/mozilla-vpn-client-l10n#466

Merged

share python code, add script to pull shared strings into translation…

f2a76ae

… repo

mcleinman requested a review from flodolo August 20, 2024 21:27

mcleinman marked this pull request as ready for review August 20, 2024 21:27

flodolo reviewed Aug 21, 2024

View reviewed changes

mcleinman mentioned this pull request Aug 21, 2024

Addons: Fix fallback languages #9796

Merged

5 tasks

PR feedback

beb34c3

mcleinman force-pushed the vpn-5478-addon-shared-strings branch from 74f767e to beb34c3 Compare August 21, 2024 19:33

oskirby reviewed Aug 22, 2024

View reviewed changes

mcleinman requested review from flodolo and oskirby August 28, 2024 21:57

flodolo reviewed Sep 2, 2024

View reviewed changes

mcleinman changed the title ~~VPN-5478: Use shared strings for addons~~ VPN-5748: Use shared strings for addons Sep 3, 2024

oskirby approved these changes Sep 4, 2024

View reviewed changes

mcleinman added 2 commits September 4, 2024 11:59

Merge branch 'main' into vpn-5478-addon-shared-strings

c4177e4

PR feedback

0370e6a

mcleinman requested a review from flodolo September 4, 2024 21:33

flodolo approved these changes Sep 5, 2024

View reviewed changes

mcleinman force-pushed the vpn-5478-addon-shared-strings branch 2 times, most recently from bddb702 to 2935ce6 Compare September 5, 2024 19:05

mcleinman force-pushed the vpn-5478-addon-shared-strings branch 14 times, most recently from 9140018 to aa33f2b Compare September 6, 2024 17:30

use same shell, so pyyaml can be found (because conda is used)

31a70ef

mcleinman force-pushed the vpn-5478-addon-shared-strings branch from aa33f2b to 31a70ef Compare September 6, 2024 17:52

mcleinman merged commit 07a1c16 into main Sep 6, 2024
122 checks passed

mcleinman deleted the vpn-5478-addon-shared-strings branch September 6, 2024 18:41

mcleinman mentioned this pull request Nov 14, 2024

FXVPN-32 add message (and fix bug involving lists and shared strings) #10045

Draft

5 tasks

This was referenced Nov 22, 2024

VPN-6749: Fix shared addon ids #10065

Open

VPN-6749 - fix shared addon IDs mozilla-l10n/mozilla-vpn-client-l10n#494

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPN-5748: Use shared strings for addons #9763

VPN-5748: Use shared strings for addons #9763

mcleinman commented Aug 2, 2024 •

edited

Loading

flodolo left a comment

mcleinman commented Aug 7, 2024

flodolo commented Aug 7, 2024

mcleinman commented Aug 20, 2024

flodolo left a comment

flodolo Aug 21, 2024

mcleinman Aug 21, 2024

oskirby left a comment •

edited

Loading

oskirby Aug 22, 2024

oskirby Aug 22, 2024

mcleinman Aug 22, 2024 •

edited

Loading

oskirby Aug 22, 2024

mcleinman Aug 22, 2024 •

edited

Loading

mcleinman commented Aug 28, 2024

flodolo left a comment

mcleinman commented Sep 4, 2024

flodolo commented Sep 5, 2024

VPN-5748: Use shared strings for addons #9763

VPN-5748: Use shared strings for addons #9763

Conversation

mcleinman commented Aug 2, 2024 • edited Loading

Description

Testing

Reference

Checklist

flodolo left a comment

Choose a reason for hiding this comment

mcleinman commented Aug 7, 2024

flodolo commented Aug 7, 2024

mcleinman commented Aug 20, 2024

flodolo left a comment

Choose a reason for hiding this comment

flodolo Aug 21, 2024

Choose a reason for hiding this comment

mcleinman Aug 21, 2024

Choose a reason for hiding this comment

oskirby left a comment • edited Loading

Choose a reason for hiding this comment

oskirby Aug 22, 2024

Choose a reason for hiding this comment

oskirby Aug 22, 2024

Choose a reason for hiding this comment

mcleinman Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

oskirby Aug 22, 2024

Choose a reason for hiding this comment

mcleinman Aug 22, 2024 • edited Loading

Choose a reason for hiding this comment

mcleinman commented Aug 28, 2024

flodolo left a comment

Choose a reason for hiding this comment

mcleinman commented Sep 4, 2024

flodolo commented Sep 5, 2024

mcleinman commented Aug 2, 2024 •

edited

Loading

oskirby left a comment •

edited

Loading

mcleinman Aug 22, 2024 •

edited

Loading

mcleinman Aug 22, 2024 •

edited

Loading