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.

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

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

Merged
merged 8 commits into from
Dec 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions addons/message_try_firefox_extension/downloadFirefox.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
((api) => {
api.urlOpener.openUrlLabel('downloadFirefox');
Copy link
Collaborator

Choose a reason for hiding this comment

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

As a minor nitpick requiring the URL to be defined by label in the client binary is probably unnecessary, we could just put it into this file as api.urlOpener.openUrl('https://foo.bar/baz/...')

Copy link
Collaborator

@oskirby oskirby Dec 2, 2024

Choose a reason for hiding this comment

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

I don't like that to change the URL we would have to change the client, which kind of makes the addon system unnecessarily brittle.

Copy link
Collaborator Author

@mcleinman mcleinman Dec 2, 2024

Choose a reason for hiding this comment

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

I was following convention here, though I don't love it either.

That said, nearly all addon changes take client work (for non-English languages), as we need to release new client versions to get updated translations - a brand new addon without a client update wouldn't be translated.

Since it's a nit, I'm going to leave this for now. If we want to change the convention, let's do it for all addons at once.

});
3 changes: 3 additions & 0 deletions addons/message_try_firefox_extension/getExtension.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
((api) => {
api.urlOpener.openUrlLabel('downloadExtension');
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comment as for downloadFirefox.js this could done directly as api.urlOpener.openUrl('https://foo.bar/baz/...')

});
60 changes: 60 additions & 0 deletions addons/message_try_firefox_extension/manifest.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
{
"api_version": "0.1",
"id": "message_try_firefox_extension",
"name": "Try the Firefox extension",
"type": "message",
"conditions": {
"min_client_version": "2.25.0",
"trigger_time": 1209600,
"env": "staging",
"platforms": ["windows"]
},
"message": {
"date": 1733157651,
"usesSharedStrings": true,
"shortVersion": "",
"id": "message_try_firefox_extension.24",
"title": "vpn.tryFirefoxExtension.title",
"subtitle": "vpn.tryFirefoxExtension.subtitle",
"badge": "new_update",
"blocks": [
{
"id": "c_1",
"type": "ulist",
"content": [
{
"id": "l_1",
"content": "vpn.tryFirefoxExtension.bullet1"
},
{
"id": "l_2",
"content": "vpn.tryFirefoxExtension.bullet2"
},
{
"id": "l_3",
"content": "vpn.tryFirefoxExtension.bullet3"
}
]
},
{
"id": "c_2",
"type": "text",
"content": "vpn.tryFirefoxExtension.finalLine"
},
{
"id": "c_3",
"type": "button",
"style": "primary",
"content": "vpn.tryFirefoxExtension.getExtension",
"javascript": "getExtension.js"
},
{
"id": "c_4",
"type": "button",
"style": "link",
"content": "vpn.tryFirefoxExtension.downloadFirefox",
"javascript": "downloadFirefox.js"
}
]
}
}
37 changes: 26 additions & 11 deletions addons/strings.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
#
# 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'
# 'shortVersion' (from manifest.json), for instance '2.24'
#
#
# Strings that are no longer needed should be removed from this file.
Expand All @@ -60,14 +60,29 @@ commonStrings:
getHelpButton:
value: Get help
comment: Button label
# 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.
tryFirefoxExtension:
title:
value: Try the Firefox extension
comment: Title for message
subtitle:
value: Personalize your VPN protections to meet your web browsing needs.
comment: Subtitle for message
bullet1:
value: Turn off VPN for specific websites
comment: First bullet point
bullet2:
value: Set different locations for different websites, to see the web as you prefer
comment: Second bullet point
bullet3:
value: Keep protection for Firefox on, even when the Mozilla VPN app is off
comment: Third bullet point
finalLine:
value: Not using Firefox to browse? Give it a try.
comment: Final 'call to action' line of message
getExtension:
value: Get the extension
comment: Button label
downloadFirefox:
value: Download Firefox
comment: Button label
4 changes: 2 additions & 2 deletions docs/Components/Addons/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,12 @@ least a manifest.json file. The properties of this JSON file are:
| id | The ID of the add-on. It must match the file name | String | Yes |
| name | The name of the add-on | String | Yes |
| api_version | The version of the add-on framework | String | Yes |
| type | One of the supported types (message, guide, ...) | String | Yes |
| type | One of the supported types (message, replacer, ...) | String | Yes |
| conditions | List of conditions to meet | Array of Condition objects | No |
| state | Object describing the state of the addon | Collection of state objects | No |

Based on the add-on type, extra properties can be in added. See the
[guide](https://github.com/mozilla-mobile/mozilla-vpn-client/blob/main/docs/guides.md), [message](https://github.com/mozilla-mobile/mozilla-vpn-client/blob/main/docs/message.md), and [replacer](https://github.com/mozilla-mobile/mozilla-vpn-client/blob/main/docs/replacer.md) documentation.
[message](./messages.md) and [replacer](./replacer.md) documentation.

## State Object

Expand Down
File renamed without changes.
File renamed without changes.
11 changes: 8 additions & 3 deletions scripts/addon/build.py
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,10 @@ def retrieve_strings_blocks(blocks, filename, strings, prefix, shared_strings):

block_id = block["id"]
legacy_block_string_id = f"{prefix}.block.{block_id}"
block_string_id = legacy_block_string_id if not shared_strings else block["content"]
if block["type"] in ["olist", "ulist"]:
block_string_id = legacy_block_string_id
else:
block_string_id = legacy_block_string_id if not shared_strings else block["content"]
block_default_comment = comment_types.get(block["type"], "")
if block_string_id in strings:
exit(f"Duplicate block enum {block_string_id} when parsing {filename}")
Expand Down Expand Up @@ -128,7 +131,8 @@ def retrieve_strings_blocks(blocks, filename, strings, prefix, shared_strings):
)

subblock_id = subblock["id"]
subblock_string_id = f"{prefix}.block.{block_id}.{subblock_id}" if not shared_strings else subblock["content"]
legacy_subblock_string_id = f"{prefix}.block.{block_id}.{subblock_id}"
subblock_string_id = legacy_subblock_string_id if not shared_strings else subblock["content"]
if subblock_string_id in strings:
exit(
f"Duplicate sub-block enum {subblock_string_id} when parsing {filename}"
Expand All @@ -138,7 +142,8 @@ def retrieve_strings_blocks(blocks, filename, strings, prefix, shared_strings):
translation_obj = find_translation_object(shared_strings, subblock_string_id)
strings[subblock_string_id] = {
"value": translation_obj['value'][0],
"comments": translation_comment(translation_obj)
"comments": translation_comment(translation_obj),
"legacy_id": legacy_subblock_string_id
}
else:
strings[subblock_string_id] = {
Expand Down
8 changes: 8 additions & 0 deletions scripts/ci/jsonSchemas/message.json
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,14 @@
"type": "boolean",
"description": "Determines whether we trigger a system notification. Default: true"
},
"usesSharedStrings": {
"type": "boolean",
"description": "Determines whether we use the new shared strings.yaml file. Default: false"
},
"shortVersion": {
"type": "string",
"description": "Version number, short form. Ex: `2.24`"
},
"blocks": {
"type": "array",
"description": "The list of text blocks",
Expand Down
9 changes: 9 additions & 0 deletions src/mozillavpn.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1267,6 +1267,15 @@ void MozillaVPN::registerUrlOpenerLabels() {
: settingsHolder->captivePortalIpv4Addresses().first());
});

uo->registerUrlLabel("downloadExtension", []() -> QString {
// TODO: This link MUST be updated with a link to our extension
return "https://addons.mozilla.org/en-US/firefox/";
});

uo->registerUrlLabel("downloadFirefox", []() -> QString {
return "https://www.mozilla.org/firefox/new/";
});

uo->registerUrlLabel("inspector", []() -> QString {
return "https://mozilla-mobile.github.io/mozilla-vpn-client/inspector/";
});
Expand Down
Loading