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 3 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('downloadFirefoxWindows');
});
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.1",
"enabled_features": ["webExtension"],
Copy link
Member

Choose a reason for hiding this comment

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

++localProxy

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, we just removed the localProxy feature in PR #10058 since it's no longer something that the client can toggle (it's just a service that happens to exist on Windows and Linux). The "feature" still gets reported through the web extension, but it's value just checks if the corresponding serivce is running.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, maybe that was a mistake and I should put it back as an un-toggle-able feature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lesleyjanenorton , were you suggesting adding localProxy in addition to webExtension?

(I'm looking for the name of a feature that will be the thing we "flip on" to make this go live for users - to be clear this is not a user-toggleable thing.)

Copy link
Member

Choose a reason for hiding this comment

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

we just removed the localProxy feature in PR #10058 since it's no longer something that the client can toggle

@oskirby Does this mean that the proxy will be available by default on Windows in v2.25? If yes, we should put it back behind the 'localProxy' feature flag.

I'm looking for the name of a feature that will be the thing we "flip on" to make this go live for users - to be clear this is not a user-toggleable thing.

@mcleinman Let's sort this out in the office hours meetings. Previously this was 'localProxy' and 'webExtension'.

Copy link
Collaborator

Choose a reason for hiding this comment

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

To summarize a zoom discussion: I'm not in favor of adding a localProxy feature flag back. The proxy, being a windows/systemd service required administrative privileges to start and stop and the client UI does not hold such privileges... to make this controllable by a feature flag it would have to be managed through the daemon and that is a lot of work for not a lot of gain.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated per our meeting

"trigger_time": 1209600,
"platforms": ["windows"]
},
"message": {
"date": 1733157651,
"usesSharedStrings": true,
"shortVersion": "N/A",
lesleyjanenorton marked this conversation as resolved.
Show resolved Hide resolved
"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 @@ -1282,6 +1282,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("downloadFirefoxWindows", []() -> QString {
return "https://www.mozilla.org/firefox/windows/";
lesleyjanenorton marked this conversation as resolved.
Show resolved Hide resolved
});

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