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

Pass display language as a locale to Electron #159958

Merged
merged 10 commits into from
Nov 14, 2022
Merged

Conversation

rzhao271
Copy link
Contributor

@rzhao271 rzhao271 commented Sep 2, 2022

Ref #159813
Passing the locale to Electron fixes how the WCO is rendered, otherwise, Electron decides the locale based on the system language. The WCO should be rendered based on the display language, not the system language.

Ref #161218
Fixes #163458

TODO:

  • Determine what other regressions this change could cause.
  • Do we want to limit this change to just Windows?

The screenshot below shows that the WCO has been moved out of the way with the fix.
Screenshot

@rzhao271 rzhao271 added this to the September 2022 milestone Sep 2, 2022
@rzhao271 rzhao271 self-assigned this Sep 2, 2022
Copy link
Collaborator

@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.

I like this change but I have small concern for this fix being shipped as candidate.

The change impacts ICU's default locale for both chromium and node, also affects Accept-language header. Although I agree that these values should match the display language set in VSCode, is there a history for why we haven't done this since the beginning ? Did we have any bugs when we configured the locale of Electron ? @bpasero do you know anything on this ?

src/main.js Outdated Show resolved Hide resolved
@bpasero
Copy link
Member

bpasero commented Sep 3, 2022

I am not aware of the reasoning why we never did this, maybe @dbaeumer, @joaomoreno or @TylerLeonhardt know of a reason. However, we do set the lang attribute in the DOM quite early for a long time here and I would have expected that to have an impact, but. maybe @deepak1556 should compare how the --locale does things different from the lang attribute`:

let locale = nlsConfig.availableLanguages['*'] || 'en';
if (locale === 'zh-tw') {
locale = 'zh-Hant';
} else if (locale === 'zh-cn') {
locale = 'zh-Hans';
}
window.document.documentElement.setAttribute('lang', locale);

One thing to keep in mind is that locale will only be in argv.json if the user explicitly picked a language in VSCode. We do not automatically write to argv.json otherwise.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I feel this is a bit too heavy for a candidate and would rather suggest to do this on insiders and disable WCO on stable as the candidate fix.

@deepak1556
Copy link
Collaborator

deepak1556 commented Sep 3, 2022

However, we do set the lang attribute in the DOM quite early for a long time here and I would have expected that to have an impact

This would only impact the render process in which it was changed, main process would remain unaffected. WCO rendering depends on the locale of the main process. Ideally, to change the application locale of chromium, application has to configure the --lang switch so that main process first picks up the change and it will also take care of propagating the changes to all of the child processes automatically.

should compare how the --locale does things different from the lang attribute`:

--locale is a switch implemented for VSCode, the plan is to not alter the behavior of this switch but additionally make sure this switch value also gets reflected for --lang which is a switch dedicated to Electron/Chromium.

One thing to keep in mind is that locale will only be in argv.json if the user explicitly picked a language in VSCode. We do not automatically write to argv.json otherwise.

Thanks for the pointer, Do we use system locale for any scenarios ? Other cases we can default to same value as locale when user does not pick a language.

@deepak1556
Copy link
Collaborator

I feel this is a bit too heavy for a candidate and would rather suggest to do this on insiders and disable WCO on stable as the candidate fix.

Yup this sounds better 💯

@bpasero
Copy link
Member

bpasero commented Sep 3, 2022

I am not sure I understand the difference between --locale and --lang and suggest to connect with devs owning our language support (Tyler, Dirk).

Yes we use app.getLocale to know which language pack to resolve unless this was explicitly defined by the user. I think in the 99% case, the locale will not be in the argv.json:

vscode/src/main.js

Lines 552 to 568 in 4f69cdf

// Try to use the app locale. Please note that the app locale is only
// valid after we have received the app ready event. This is why the
// code is here.
let appLocale = app.getLocale();
if (!appLocale) {
nlsConfiguration = { locale: 'en', availableLanguages: {} };
} else {
// See above the comment about the loader and case sensitiveness
appLocale = appLocale.toLowerCase();
const { getNLSConfiguration } = require('./vs/base/node/languagePacks');
nlsConfiguration = await getNLSConfiguration(product.commit, userDataPath, metaDataFile, appLocale);
if (!nlsConfiguration) {
nlsConfiguration = { locale: appLocale, availableLanguages: {} };
}
}

@deepak1556
Copy link
Collaborator

@rzhao271 we can continue this PR to address the locale of the runtime after connecting with @TylerLeonhardt and @dbaeumer .

But for resolving #159813 as a candidate, can you open a separate PR that disables WCO. Thanks!

@dbaeumer
Copy link
Member

dbaeumer commented Sep 5, 2022

Here is what I can remember:

  • app.commandLine.appendSwitch('lang', argvValue ?? 'en'): I simply wasn't aware of this
  • locale versus language in VS Code: locale is either the OS local or the locale configured for VS Code. language is the language the UI is rendered in. This doesn't necessarily need to be the locale. Usual case is that the locale points to a language for which we don't have a language pack.

@deepak1556
Copy link
Collaborator

locale is either the OS local or the locale configured for VS Code.

@dbaeumer which components of the UI or core does this value affect ? Especially in cases where language pack and locale value are different.

@dbaeumer
Copy link
Member

dbaeumer commented Sep 6, 2022

The locale value does only affect the picking of the language pack. The UI should always be rendered using the language value.

@rzhao271
Copy link
Contributor Author

rzhao271 commented Sep 9, 2022

For #159958 (comment), the reason that locale often points to a language for which there is no language pack installed is because locale often takes the OS locale if not explicitly passed in. This PR changes it so that it takes whatever is in argv.json for the locale field. If that value happens to be where the language used by the language pack is stored, then we have locale = Electron locale = VS Code language and I think that's what we want.

One issue with this approach is I'm unsure what happens when the language pack doesn't represent a valid locale, such as with the pseudo-language extension pack. In that case, I'd hope that the locale would default back to en through the getLocale call, but I still have to check.

@deepak1556
Copy link
Collaborator

deepak1556 commented Sep 15, 2022

Based on the locale and language distinctive usage in VSCode I have heard from @dbaeumer and @rzhao271, using --lang flag to change Electron's locale will be a disruptive change because app.getLocale reflects the value passed to --lang.

Given we want locale to reflect either a user defined locale or default to OS locale, I suggest exposing another api in Electron app.getOSLocale so that we have the following contracts,

  1. --lang will reflect the value of language if a language pack was installed and app.getLocale will reflect the same.
  2. When language pack is not available --lang and app.getLocale will point to en as default.
  3. VSCode locale can point to user defined locale or can get OS locale value from app.getOSLocale

@dbaeumer @TylerLeonhardt does this make sense ?

@TylerLeonhardt
Copy link
Member

Sounds great! I was just saying to @rzhao271 that we needed an API for "give me whatever the OS says, not anything else"

@dbaeumer
Copy link
Member

Makes perfect sense to me. It is basically locale and lang in out model.

@deepak1556
Copy link
Collaborator

Great, thanks for the feedback. @rzhao271 lets start with adding the Electron API as next step.

TylerLeonhardt
TylerLeonhardt previously approved these changes Sep 24, 2022
src/main.js Outdated Show resolved Hide resolved
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I thought we wanted to exclude this in stable? If so, please use product.quality !== stable around the new code.

src/main.js Outdated Show resolved Hide resolved
@rzhao271
Copy link
Contributor Author

rzhao271 commented Nov 7, 2022

I did some sanity checking on Windows and noticed that when I had no locale key in argv.json, and had the Windows language set to Farsi, that the WCO would once again render to the left even when the application was in English. It turned out I wasn't handling the case where the locale key wasn't in argv.json at all.

@rzhao271
Copy link
Contributor Author

rzhao271 commented Nov 8, 2022

Another round of sanity testing:

  • Windows
  • macOS
  • Linux

@rzhao271 rzhao271 requested a review from bpasero November 8, 2022 17:40
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

What does this change actually mean for our support to run code --locale? I just now found out that we advertise for it when you run code --help. This probably should then override whatever is in argv.json?

src/main.js Outdated Show resolved Hide resolved
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

I am a bit confused by your last change, I was not suggesting to remove the locale check, just consolidate it in one place. Why is it removed now? Do we no longer have to check for argv.json and locale?

@rzhao271
Copy link
Contributor Author

I consolidated the checks—the check for the locale arg already happens at

function getUserDefinedLocale(argvConfig) {

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

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

Your last change makes good sense 👍

Can we also test what happens when you pass a bogus value to the lang switch, will the startup continue normally or do we have to validate the input?

Copy link
Collaborator

@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.

LGTM on the Electron changes.

@rzhao271
Copy link
Contributor Author

rzhao271 commented Nov 14, 2022

Your last change makes good sense 👍

Can we also test what happens when you pass a bogus value to the lang switch, will the startup continue normally or do we have to validate the input?

I just tested this with electron-v22.0.0-beta.5 which has my API change merged in.

On Windows 11, it seems to default to the system language which I currently have set to Chinese (first preferred language: fr-CA, regional format: fi-FI, system language: zh-CN)
On macOS Big Sur, the console emits an error message that ICU was not able to set the default locale to the bogus value, and falls back to en-US (first preferred language: fr-CA, region: FI)
On CentOS 7, the locale falls back to en-US (locale: fr-FR, format: ru-RU)

@rzhao271 rzhao271 merged commit 5f67407 into main Nov 14, 2022
@rzhao271 rzhao271 deleted the rzhao271/pass-locale branch November 14, 2022 19:55
@github-actions github-actions bot locked and limited conversation to collaborators Dec 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Spanish language pack recommender doesn't work on macOS
5 participants