-
Notifications
You must be signed in to change notification settings - Fork 117
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
base: main
Are you sure you want to change the base?
Conversation
bca3988
to
3bf28f4
Compare
"type": "message", | ||
"conditions": { | ||
"min_client_version": "2.25.1", | ||
"enabled_features": ["webExtension"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++localProxy
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.)
"message": { | ||
"date": 1733157651, | ||
"usesSharedStrings": true, | ||
"shortVersion": "N/A", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this be "" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated this. It doesn't change anything for the user, as it's never shown - it's used for update/what's new messages (to drop a version number in the "Update to ___" slot).
Potentially this was a bad design choice of mine to always require it (messages using shared strings don't pass validation without it). I'm not inclined to update it right now, given how rarely we have messages like this - though let me know if you think it's needed now.
src/mozillavpn.cpp
Outdated
}); | ||
|
||
uo->registerUrlLabel("downloadFirefoxWindows", []() -> QString { | ||
return "https://www.mozilla.org/firefox/windows/"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do https://www.mozilla.org/firefox/new/
it will take them directly to the desktop install page and bedrock will automagically determine the platform. We probably want to attach some UTM parameters to this link too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, updated the URL (and name). I left a message for Santiago on the associated ticket to see if he wants UTM params on either of these links.
This is reviewable - I've put it on draft as a reminder to not merge it before #10065. |
This PR cannot be merged into
main
before #10065 is merged intomain
, to ensure we don't add bad translation IDs.Description
This adds the in-app message and notification for the web extension.
Notes:
strings.yaml
file, as we now have enough strings that we don't need those samples.Testing
This code will not be seen until 2.25.1. Thus, you need to change the
conditions
block. Given that QA will not be able to test this after merging tomain
, we need at least one other VPN engineer to test this code. To test:To test:
./3rdparty/i18n/en/addons/strings.xliff
:How to implement and test add-ons
section of./docs/Components/Addons/index.md
.webExtension
feature. Hard quit and relaunch the app.Reference
FXVPN-32
Checklist