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

Determine language based on the OS' language [Rework] #262

Merged
merged 6 commits into from
Feb 27, 2021
Merged

Determine language based on the OS' language [Rework] #262

merged 6 commits into from
Feb 27, 2021

Conversation

midyh
Copy link
Contributor

@midyh midyh commented Feb 7, 2021

Hi there.

Hopefully, this should be better:

  • I removed the default.json and added an object in languages.ts so that detecting it being picked is easier.
  • Also, to keep the languages' names, I implemented a switch statement. The only drawback is that when adding a new language, a new case must be added.

Please feel free to make any corrections! Thanks.

// Get the language part from the locale (e.g. en-GB -> en)
const locale = globalAny.navigator.language.substr(0, globalAny.navigator.language.indexOf('-'))

switch (locale) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't really think this switch is needed, each language already provides a locale, example:

"locale": "en",

@@ -31,53 +31,19 @@ function setFallback(notFoundLang: string): void {
StaticConfig.keyChanged('appLanguage', (newLanguage: string) => {
if (Languages[newLanguage].name === 'Default') {
// Get the language part from the locale (e.g. en-GB -> en)
const locale = globalAny.navigator.language.substr(0, globalAny.navigator.language.indexOf('-'))
const newLocale = globalAny.navigator.language.substr(0, globalAny.navigator.language.indexOf('-'))
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to use this: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Locale

Example:

const { language } = new Intl.Locale(globalThis.navigator.language)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using Intl.Locale returns Property 'Locale' does not exist on type 'typeof Intl'. Do you have any solutions for this in mind?

Copy link
Member

Choose a reason for hiding this comment

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

Using Intl.Locale returns Property 'Locale' does not exist on type 'typeof Intl'. Do you have any solutions for this in mind?

Where did you test it? It worked fine for me.

Copy link
Contributor Author

@midyh midyh Feb 8, 2021

Choose a reason for hiding this comment

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

Where did you test it? It worked fine for me.

I am currently testing on Ubuntu 20.10.

Edit: I just found a workaround. Using new (Intl as any).Locale(...) actually worked, so it's probably an error in the .d.ts file of Intl.

Copy link
Member

Choose a reason for hiding this comment

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

Where did you test it? It worked fine for me.

I am currently testing on Ubuntu 20.10.

I mean, where are you executing that code? Node? Electron? Firefox?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mean, where are you executing that code? Node? Electron? Firefox?

Electron.

Copy link
Member

Choose a reason for hiding this comment

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

I mean, where are you executing that code? Node? Electron? Firefox?

Electron.

Show me the error, please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the console output:

↳✨ Watching in WatchWebpackRenderer
   ↳❌ [webpack] Error in: ./src/interface/core/lang_config.ts

 [tsl] ERROR in /home/user/Coding/Graviton-App/src/interface/core/lang_config.ts(33,33)
      TS2339: Property 'Locale' does not exist on type 'typeof Intl'.

And line 33:

const { language } = new Intl.Locale(globalThis.navigator.language)

Also, using (Intl as any).Locale(...) works normally.

Copy link
Member

Choose a reason for hiding this comment

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

Here is the console output:

↳✨ Watching in WatchWebpackRenderer
   ↳❌ [webpack] Error in: ./src/interface/core/lang_config.ts

 [tsl] ERROR in /home/user/Coding/Graviton-App/src/interface/core/lang_config.ts(33,33)
      TS2339: Property 'Locale' does not exist on type 'typeof Intl'.

And line 33:

const { language } = new Intl.Locale(globalThis.navigator.language)

Also, using (Intl as any).Locale(...) works normally.

Related to microsoft/TypeScript#37326 .

I think it would be better to just leave it as it is for now

@marc2332 marc2332 self-assigned this Feb 9, 2021
@marc2332 marc2332 added the need-to-review It needs to be fully reviewed label Feb 9, 2021
@marc2332
Copy link
Member

marc2332 commented Feb 10, 2021

I was thinking about if it would be better if it was a switch button rather than another radio button, what do you think?

It would say something like "Use System's preference"

@ghost
Copy link

ghost commented Feb 10, 2021

I was thinking about if it would be better if it was a switch button rather than another radio button, what do you think?

It would say something like "Use System's preference"

A switch would be too much, since if it is disabled, there would be another menu.

Maybe a "Reset language" button that resets the language to this?

@midyh
Copy link
Contributor Author

midyh commented Feb 11, 2021

A switch would be too much, since if it is disabled, there would be another menu.

I agree with @dragonDScript. If for example a user needed to switch languages they would have to turn off the switch and then choose. So yes I think a switch would complicate things.

Maybe a "Reset language" button that resets the language to this?

That's a good idea, but I think we should include the setting in the radio button so the user can know which one is in use.

@marc2332
Copy link
Member

I don't see any issue with something like:

image

Also, from the developer perspective it's better to don't include non-languages options in the settings 'appLanguage', think aboiut a plugin that wants to know what language is being used, that's easy to do by just reading StaticConfig.data.appLanguage, but, by doing that, you wouldn't be able to know what language is really being used.

@ghost
Copy link

ghost commented Feb 11, 2021

I don't see any issue with something like:

image

That is bad for UX. E.g. A user sets their language to russian. Then, he enables use system's preference. What language is being used? The system's or russian?

@marc2332
Copy link
Member

I don't see any issue with something like:
image

That is bad for UX. E.g. A user sets their language to russian. Then, he enables use system's preference. What language is being used? The system's or russian?

Enabling that switch would automatically change the selected radio button. And when you manually click on a radio button it would disable the switch.

@ghost
Copy link

ghost commented Feb 11, 2021

I don't see any issue with something like:
image

That is bad for UX. E.g. A user sets their language to russian. Then, he enables use system's preference. What language is being used? The system's or russian?

Enabling that switch would automatically change the selected radio button. And when you manually click on a radio button it would disable the switch.

or make the radio gray to denote that it is 'invalid' or disabled.

@ghost
Copy link

ghost commented Feb 11, 2021

Also, from the developer perspective it's better to don't include non-languages options in the settings 'appLanguage', think aboiut a plugin that wants to know what language is being used, that's easy to do by just reading StaticConfig.data.appLanguage, but, by doing that, you wouldn't be able to know what language is really being used.

And that's why I suggested the button way. This way, appLanguage wouldn't be affected and you wouldn't need an extra switch.
E.g. the button would set the current language to the system's and update the radio button.

@marc2332
Copy link
Member

Also, from the developer perspective it's better to don't include non-languages options in the settings 'appLanguage', think aboiut a plugin that wants to know what language is being used, that's easy to do by just reading StaticConfig.data.appLanguage, but, by doing that, you wouldn't be able to know what language is really being used.

And that's why I suggested the button way. This way, appLanguage wouldn't be affected and you wouldn't need an extra switch.
E.g. the button would set the current language to the system's and update the radio button.

Makes more sense to use a switch because its a thing you enable and disable. If you ever change your System's language it would change the app language too. You don't have to click the switch every time you change your system's language

@ghost
Copy link

ghost commented Feb 11, 2021

Also, from the developer perspective it's better to don't include non-languages options in the settings 'appLanguage', think aboiut a plugin that wants to know what language is being used, that's easy to do by just reading StaticConfig.data.appLanguage, but, by doing that, you wouldn't be able to know what language is really being used.

And that's why I suggested the button way. This way, appLanguage wouldn't be affected and you wouldn't need an extra switch.
E.g. the button would set the current language to the system's and update the radio button.

Makes more sense to use a switch because its a thing you enable and disable. If you ever change your System's language it would change the app language too. You don't have to click the switch every time you change your system's language

But this feature could detect the os' language on every launch and update it. In my way, the API is not affected.

@marc2332
Copy link
Member

Also, from the developer perspective it's better to don't include non-languages options in the settings 'appLanguage', think aboiut a plugin that wants to know what language is being used, that's easy to do by just reading StaticConfig.data.appLanguage, but, by doing that, you wouldn't be able to know what language is really being used.

And that's why I suggested the button way. This way, appLanguage wouldn't be affected and you wouldn't need an extra switch.
E.g. the button would set the current language to the system's and update the radio button.

Makes more sense to use a switch because its a thing you enable and disable. If you ever change your System's language it would change the app language too. You don't have to click the switch every time you change your system's language

But this feature could detect the os' language on every launch and update it. In my way, the API is not affected.

By adding an extra setting, which would be something like appUseSystemLanguage, we keep appLanguage behavior as it is now. This new setting would be a boolean, so it makes sense to use a switch.

@ghost
Copy link

ghost commented Feb 12, 2021

Also, from the developer perspective it's better to don't include non-languages options in the settings 'appLanguage', think aboiut a plugin that wants to know what language is being used, that's easy to do by just reading StaticConfig.data.appLanguage, but, by doing that, you wouldn't be able to know what language is really being used.

And that's why I suggested the button way. This way, appLanguage wouldn't be affected and you wouldn't need an extra switch.
E.g. the button would set the current language to the system's and update the radio button.

Makes more sense to use a switch because its a thing you enable and disable. If you ever change your System's language it would change the app language too. You don't have to click the switch every time you change your system's language

But this feature could detect the os' language on every launch and update it. In my way, the API is not affected.

By adding an extra setting, which would be something like appUseSystemLanguage, we keep appLanguage behavior as it is now. This new setting would be a boolean, so it makes sense to use a switch.

This would cause a lot of bugs. I can't really explain, I had similar issues in the past with this kind of two-settings layout.

@marc2332 marc2332 linked an issue Feb 13, 2021 that may be closed by this pull request
@marc2332 marc2332 merged commit e1c12d7 into Graviton-Code-Editor:master Feb 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need-to-review It needs to be fully reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Suggestion] Determine language based on the OS' language
2 participants