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

VPN-5175 - Get languages, currencies and server name translations from the l10n repository strings #9448

Merged
merged 19 commits into from
May 7, 2024

Conversation

brizental
Copy link
Contributor

Ok, this is a large one.

I considered separating each commit into a separate PR, but 🤷‍♀️ Let me know if you'd prefer I do that.

Regardless, I recomend reviewing this per commit. Each commit handles a different part of the task.

We had the languages.json and servers.json files that included translations for: language name, currency symbols and server names. This PR moves away from that to actually use translations from Pontoon for these.

On previous PRs (#9411, mozilla-l10n/mozilla-vpn-client-l10n#435) I have added the extras.xliff file with language and server name strings to take place of the JSON files.

1. Currency symbols (221aa63)

After discussions with @flodolo and Santiago, we decided not to have translations for these anymore. It seems pointless, because all translations are the same.

On the first commit of this pull request, I have update localizer.cpp to either grap currency symbol translations from Qt translations or from a hardcoded list of currencies that we support.

2. Using extras.xliff (3afc2ed)

On this commit I updated the build to include strings from extas.xliff into i18nstrings_p.cpp and friends.

3. Laguage names (721effb)

On this commit I remove code that generates and consumes languages.json and add code to get language names from I18nStrings. I also had to include some new code generation in order to have a list of language names in all supported locales, the alternative to that would be to load a QTranslator just to grab a string than unload it -- not good. That means there is also some build changes to generate the new string and well as to update the I18nStrings class to be able to get strings by id.

4. Server names (d6830e1)

Finally, this commit adds logic to get city and country names. All of the infrastrcuture that makes this possible was done already in the previous commits so this is mechanical work.

I also removed the code that would get the server names form servers.json.

scripts/utils/generate_language_names_map.py Outdated Show resolved Hide resolved
scripts/utils/generate_strings.py Show resolved Hide resolved
scripts/utils/generate_strings.py Outdated Show resolved Hide resolved
scripts/utils/generate_strings.py Outdated Show resolved Hide resolved
scripts/utils/generate_strings.py Outdated Show resolved Hide resolved
exit("No source argument.")

# If no output directory was provided, use the current directory.
if args.output is None:
args.output = os.getcwd()

# Parse the inputs for their sweet juicy strings.
Copy link
Collaborator

Choose a reason for hiding this comment

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

🙄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, I was hoping I could just quietly remove this.

@flodolo
Copy link
Collaborator

flodolo commented Apr 29, 2024

Looks like GitHub ate one of my comment. Don't know much about C++, but is there a switch/case structure to use instead of so many "if else"?

@brizental
Copy link
Contributor Author

Looks like GitHub ate one of my comment. Don't know much about C++, but is there a switch/case structure to use instead of so many "if else"?

Can´t do a switch statement with strings unfortunately, only integers :/

@brizental brizental force-pushed the 5175-do-the-work branch 2 times, most recently from b1a0b94 to d60967a Compare April 29, 2024 14:31
@oskirby
Copy link
Collaborator

oskirby commented Apr 29, 2024

Regarding the Linux build failures. The package sources for RPM-based distributions maintain their dependency information in linux/mozillavpn.spec instead of linux/debian/control. That file will need to be updated as well if we need to add lxml as a dependency.

However! Unless we need to use lxml specifically, for XML parsing we have generally gotten away with import xml.etree.ElementTree as etree as a suitable replacement. It's a got the same basic API and nearly the same functionality but it's a python built-in and generally does the job just as good as lxml without needing to bring in extra dependencies.

@oskirby
Copy link
Collaborator

oskirby commented Apr 29, 2024

Also: The last time we made use of lxml in our build tooling we encountered some friction when resolving python deps (see: #5225) we might want to keep an eye out on that to make sure we don't cause a regression.

@brizental
Copy link
Contributor Author

brizental commented Apr 29, 2024

Looks like GitHub ate one of my comment. Don't know much about C++, but is there a switch/case structure to use instead of so many "if else"?

Plot twist: the Windows C++ compiler spits out a fatal error when there are more than 128 nesting blocks 🅰️ 🅰️ 🅰️ 🅰️ Going to need to rethink this.

@brizental
Copy link
Contributor Author

Also: The last time we made use of lxml in our build tooling we encountered some friction when resolving python deps (see: #5225) we might want to keep an eye out on that to make sure we don't cause a regression.

No worries, I just removed that dependency.

Copy link
Collaborator

@flodolo flodolo left a comment

Choose a reason for hiding this comment

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

Python scripts look good to me 👍🏼

Copy link
Member

@lesleyjanenorton lesleyjanenorton left a comment

Choose a reason for hiding this comment

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

👍

@brizental brizental force-pushed the 5175-do-the-work branch from 36fc2a6 to 319d45e Compare May 6, 2024 14:32
@brizental brizental force-pushed the 5175-do-the-work branch from bc2b3d4 to 6a88e62 Compare May 6, 2024 17:24
@brizental brizental enabled auto-merge (squash) May 6, 2024 17:26
@github-actions github-actions bot added the 🛬 Landing This PR is marked as "auto-merge" label May 6, 2024
@brizental brizental disabled auto-merge May 7, 2024 13:32
@brizental brizental merged commit 241f580 into main May 7, 2024
124 of 126 checks passed
@brizental brizental deleted the 5175-do-the-work branch May 7, 2024 13:49
@brizental
Copy link
Contributor Author

Test failure was unrelated and being worked on in #9499. Therefore I force merged.

@mcleinman mcleinman mentioned this pull request Oct 10, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🛬 Landing This PR is marked as "auto-merge"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants