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

feat: add app.getPreferredSystemLanguages() API #36035

Merged
merged 12 commits into from Nov 9, 2022

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Oct 14, 2022

Description of Change

This PR provides an API that retrieves the user's preferred system languages.

It turns out that retrieving the user's locale often ends up retrieving "actual locale" data, which isn't what I originally wanted with PR #35697. However, upon discussing with @deepak1556, returning the "actual locale" data makes sense for a method named app.getSystemLocale, so this PR adds a new method, app.getPreferredSytemLanguages, that returns GetPreferredLanguages().

Downstream: microsoft/vscode#159958.

Checklist

Release Notes

Notes: Added an app.getPreferredSystemLanguages() API to return the user's system languages.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 14, 2022
@rzhao271
Copy link
Contributor Author

Windows 11 screenshots

A screenshot showing that the strings returned by app.getSystemLanguage(), app.getSystemLocale(), and app.getLocale() can all be different. Only the last one is affected by the --lang flag.

A screenshot showing that French (Canada) is the preferred language, whereas Chinese (Simplified, China) is the region. app.getSystemLocale() returns zh-CN whereas app.getSystemLanguage() returns fr-CA.
systemLocaleEnglishConfirm1

@rzhao271
Copy link
Contributor Author

macOS Monterey screenshots

A screenshot showing that the strings returned by app.getLocale(), app.getSystemLocale(), and app.getSystemLanguage() are different.

Canadian French (fr-CA) is the preferred language, whereas Finland (FI) is the region, and Swiss German (de-CH) is the language passed in through the --lang flag. app.getLocale() returns en-US, app.getSystemLocale() returns fr-FI, and app.getSystemLanguage() returns fr-CA.

I didn't expect app.getLocale() to return en-US, but I guess de-CH isn't a registered locale. If I pass in just German (de) through the --lang flag, then app.getLocale() returns de.

@rzhao271 rzhao271 changed the title feat: add app.getSystemLanguage() API feat: add app.getPreferredSystemLanguages() API Oct 17, 2022
@rzhao271
Copy link
Contributor Author

Sample results on Windows 11

app.getPreferredSystemLanguages() fr-CA,en-US,fa,zh-Hans-CN,ko,ja,pt-PT,pt-BR
app.getSystemLocale() zh-CN
app.getLocale() fr

@rzhao271 rzhao271 marked this pull request as ready for review October 18, 2022 00:06
@deepak1556
Copy link
Member

I didn't expect app.getLocale() to return en-US, but I guess de-CH isn't a registered locale. If I pass in just German (de) through the --lang flag, then app.getLocale() returns de.

Yes this is expected, it depends on what Cocoa locale the application supports. It is the list of *.lproj files shipped with the application.

docs/api/app.md Outdated Show resolved Hide resolved
Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Linux implementation can return the list from g_get_language_names instead of empty result. NOTE: l10n_util::GetApplicationLocaleInternal also uses it but we cannot call into this base API because it eventually returns the locale for which translatable strings are available in the application.

@deepak1556 deepak1556 added api-review/requested 🗳 target/21-x-y PR should also be added to the "21-x-y" branch. target/22-x-y PR should also be added to the "22-x-y" branch. semver/minor backwards-compatible functionality labels Oct 18, 2022
docs/api/app.md Outdated Show resolved Hide resolved
shell/common/language_util_linux.cc Outdated Show resolved Hide resolved
docs/api/app.md Outdated Show resolved Hide resolved
docs/api/app.md Outdated Show resolved Hide resolved
docs/api/app.md Outdated Show resolved Hide resolved
@rzhao271
Copy link
Contributor Author

I wasn't able to add tables as examples due to the error message

× npm run lint:docs:
-
- Checking argv
× Generating API in directory: "C:\Users\raymondzhao\work\electron\src\electron"
An error occurred while processing: "C:\Users\raymondzhao\work\electron\src\electron\docs\api\app.md"  
AssertionError: We only support plain text, links, softbreaks, inline code, strong tags and paragraphs inside joinable tokens: expected 'table_open' to be one of [ Array(22) ]
    at Object.exports.safelyJoinTokens (C:\Users\raymondzhao\work\electron\src\electron\node_modules\@electron\docs-parser\dist\markdown-helpers.js:386:48)
    at Object.exports.extractReturnType (C:\Users\raymondzhao\work\electron\src\electron\node_modules\@electron\docs-parser\dist\markdown-helpers.js:329:36)
    at Object.exports._headingToMethodBlock (C:\Users\raymondzhao\work\electron\src\electron\node_modules\@electron\docs-parser\dist\block-parsers.js:81:72)
> electron-docs-parser --dir=./

I have added nested lists for now.

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

Getting close! Implementation LGTM 👍

spec/api-app-spec.ts Outdated Show resolved Hide resolved
Copy link
Member

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

API LGTM

shell/browser/api/electron_api_app.cc Outdated Show resolved Hide resolved
@zcbenz
Copy link
Member

zcbenz commented Oct 27, 2022

We need a few more API LGTMs from @electron/wg-api.

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

(sorry x3)

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

x4

@codebytere
Copy link
Member

x5

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

x6

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

x7

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

x8

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Nov 9, 2022
Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

x9

Copy link
Member

@codebytere codebytere left a comment

Choose a reason for hiding this comment

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

x10

@release-clerk
Copy link

release-clerk bot commented Nov 9, 2022

Release Notes Persisted

Added an app.getPreferredSystemLanguages() API to return the user's system languages.

trop bot added a commit that referenced this pull request Nov 9, 2022
* feat: add app.getSystemLanguage() API

* Change the API to getPreferredSystemLanguages

* Fix test

* Clarify docs and add Linux impl

* Remove USE_GLIB

* Don't add C to list

* Remove examples since there's a lot of edge cases

* Fix lint

* Add examples

* Fix compile error

* Apply PR feedback

* Update the example

Co-authored-by: Raymond Zhao <7199958+rzhao271@users.noreply.github.com>
@trop
Copy link
Contributor

trop bot commented Nov 9, 2022

I have automatically backported this PR to "21-x-y", please check out #36290

@trop trop bot added in-flight/21-x-y and removed target/21-x-y PR should also be added to the "21-x-y" branch. labels Nov 9, 2022
@trop
Copy link
Contributor

trop bot commented Nov 9, 2022

I have automatically backported this PR to "22-x-y", please check out #36291

@trop trop bot added in-flight/22-x-y and removed target/22-x-y PR should also be added to the "22-x-y" branch. labels Nov 9, 2022
@rzhao271 rzhao271 deleted the rzhao271/system-languages branch November 9, 2022 16:03
@trop trop bot added merged/22-x-y PR was merged to the "22-x-y" branch. merged/21-x-y PR was merged to the "21-x-y" branch. and removed in-flight/22-x-y labels Nov 9, 2022
jkleinsc pushed a commit that referenced this pull request Nov 9, 2022
feat: add app.getPreferredSystemLanguages() API (#36035)

* feat: add app.getSystemLanguage() API

* Change the API to getPreferredSystemLanguages

* Fix test

* Clarify docs and add Linux impl

* Remove USE_GLIB

* Don't add C to list

* Remove examples since there's a lot of edge cases

* Fix lint

* Add examples

* Fix compile error

* Apply PR feedback

* Update the example

Co-authored-by: Raymond Zhao <7199958+rzhao271@users.noreply.github.com>

Co-authored-by: trop[bot] <37223003+trop[bot]@users.noreply.github.com>
Co-authored-by: Raymond Zhao <7199958+rzhao271@users.noreply.github.com>
@trop trop bot removed the in-flight/21-x-y label Nov 9, 2022
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* feat: add app.getSystemLanguage() API

* Change the API to getPreferredSystemLanguages

* Fix test

* Clarify docs and add Linux impl

* Remove USE_GLIB

* Don't add C to list

* Remove examples since there's a lot of edge cases

* Fix lint

* Add examples

* Fix compile error

* Apply PR feedback

* Update the example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/approved ✅ merged/21-x-y PR was merged to the "21-x-y" branch. merged/22-x-y PR was merged to the "22-x-y" branch. semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants