Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VPN-5748: Use shared strings for addons #9763

Merged
merged 11 commits into from
Sep 6, 2024
Merged

VPN-5748: Use shared strings for addons #9763

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2fd6fd2
VPN-5748 - reusable strings convert in the build process
mcleinman Aug 1, 2024
2211759
move parameters in manifest file, add in shortVersion swapping
mcleinman Aug 2, 2024
a9eb258
make string file only include relevant strings
mcleinman Aug 2, 2024
93cec33
PR feedback
mcleinman Aug 7, 2024
23df4ba
update dictionary
mcleinman Aug 7, 2024
bc167dc
share code
mcleinman Aug 20, 2024
f2a76ae
share python code, add script to pull shared strings into translation…
mcleinman Aug 20, 2024
beb34c3
PR feedback
mcleinman Aug 21, 2024
c4177e4
Merge branch 'main' into vpn-5478-addon-shared-strings
mcleinman Sep 4, 2024
0370e6a
PR feedback
mcleinman Sep 4, 2024
31a70ef
use same shell, so pyyaml can be found (because conda is used)
mcleinman Sep 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .dictionary
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -237,6 +237,7 @@ serverData
settingHolder
shader
shaders
shortVersion
sig
signup
src
Expand All @@ -258,6 +259,7 @@ unenrolled
uniffi
uploader
urls
usesSharedStrings
utils
uuid
vm
Expand Down
73 changes: 73 additions & 0 deletions addons/strings.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# This Source Code Form is subject to the terms of the Mozilla Public
# License, v. 2.0. If a copy of the MPL was not distributed with this
# file, You can obtain one at http://mozilla.org/MPL/2.0/.

# This file contains the strings for the MozillaVPN addons.
# At the top level of this document, you must specify the categories. For each
# category, there is a list of string IDs. Each string ID can be:
# - a string: this will be the English translation of that string ID
# - an object composed of a `value` key and, optional, a `comment` key. Both
# keys can be a string or an array of strings. The `value` contains the
# English translation; the comment is useful to describe what the string is
# and where it is used.
#
# Example:
#
#initialView:
# # This is a simple string
# getStartedButton: Get started
#
# # StringID with a value and a comment
# learnMore:
# value: Learn more
# comment: This is the `learn more` link shown in the initial view
#
# # Multiple line string (with a multiple line comment)
# subTitle:
# value:
# - A fast, secure and easy to use VPN. Built
# - by the makers of Firefox
# comment:
# - Also comments can be written using
# - multiple lines!
#
# String can contain just one specific type of argument, the short version number.
# Add '%1' where the version should go. This will get replaced with the addons message's
# 'shortVersion' (from manifeste.json), for instance '2.24'
#
#
# Strings that are no longer needed should be removed from this file.

commonStrings:
updateTitle:
value: Update to Mozilla VPN %1
comment: Title for update message. %1 is replaced by the version number, such as 2.24
downloadTitle:
value: Download the new Mozilla VPN %1
comment: Alternative title used for some versions. %1 is replaced by the version number, such as 2.24
subtitle:
value: We’ve released an updated version of Mozilla VPN! Update to the latest version for the best possible Mozilla VPN experience.
comment: Subtitle for default update message
generalUpdateContent:
value: This update includes minor bug fixes, UI adjustments and other performance improvements.
comment: Default update notes
updateButton:
value: Update now
comment: Label for `update now` button
flodolo marked this conversation as resolved.
Show resolved Hide resolved
downloadButton:
value: Download update
comment: Label for `Download update` button
getHelpButton:
value: Get help
comment: Label for `Get help` button
# tester:
# value: shouldn't show up in translation file
# comment: Label for `Get help` button

# 224updateMessage:
# somethingSpecificJustToThisMessage:
# value: This is just an example
# comment: Use this as as template for strings that we expect to only use in one message.

# These sample commented out translations can be used in testing, to ensure this is working as expected. Feel free to remove in
# the near future, when this file has more translations.
4 changes: 3 additions & 1 deletion docs/Components/messages.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Copy link
Collaborator Author

@mcleinman mcleinman Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| shortVersion | A human-readable version number to insert into user-facing strings | Strings | Yes if usesSharedStrings is true, No otherwise |
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@mcleinman mcleinman Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

| title | The subject of the message | String | Yes |
| subtitle | A brief description of the message | String | No |
| title_comment | An optional comment to describe the meaning of the title | String | No |
| subtitle_comment | An optional comment to describe the meaning of the subtitle | String | No |
| date | The date the message was received (using seconds since epoch time) | Number | No |
| badge | A label used to tag a message (options: `warning`, `critical`, `new_update`, `whats_new`, `survey`, `subscription` ) | String | No |
| notify | Should we notify the user about this message via system notifications (Default: true) | Boolean | No |
| blocks | An array of graphical blocks that compose the user interface of the message's contents (see more info [here](https://github.com/mozilla-mobile/mozilla-vpn-client/wiki/guides#block-object)) | Array of Block objects | Yes |
| blocks | An array of graphical blocks that compose the user interface of the message's contents (see more info [here](https://github.com/mozilla-mobile/mozilla-vpn-client/wiki/guides#block-object)) | Array of Block objects | Yes |
Loading
Loading