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: use user's preferred language on first launch if available #2490

Closed
wants to merge 8 commits into from

Conversation

ddfreiling
Copy link
Contributor

@ddfreiling ddfreiling commented Jul 11, 2024

Hi EDRLab.
During our testing of our LCP implementation in Nota, our users complained that Thorium Reader wasn't using the same language as their OS when first launching it. I have tried to implement this, but do not have much experience with using Electron and saga.

Please feel free to recommend a more optimal way to do this. The logic around doing this on first launch, using a dummy file, feels a little rudimentary.

@ddfreiling
Copy link
Contributor Author

@danielweck any feedback on this? Our users would love to have this feature in a future release.

@HadrienGardeur
Copy link
Contributor

I agree with @ddfreiling that this is very useful, but I would advise against using just the first language found through app.getPreferredSystemLanguages().

It's important to check for all BCP 47 language tags returned by app.getPreferredSystemLanguages() by order of priority and to make sure that they can be matched against a locale:

  • if the region is missing (Windows may simply return fr instead of fr-FR)
  • and if there's no match for the region but a match for the language itself

Since Thorium 3.1 will introduce improvements to voice selection based on system languages, this would be a nice addition for this release as well.

…launch

* upstream/develop:
  feat (reader): save/apply/reset preferred reading settings (Fixes edrlab#2530 Fixes edrlab#2532 PR edrlab#2533)
  fix (main): initialize missing keys from reader.defaultConfig in memory.ts preloadedState
  chore (refactor): comment session.state unused and disable the force reset of the global defaultConfig at start
prevents edge-case issue with 3-letter language lang codes
@ddfreiling
Copy link
Contributor Author

@danielweck this should be ready for review

@panaC
Copy link
Member

panaC commented Oct 2, 2024

I think we need to check the double default locale initialization set to "en" value in first. Maybe I'm wrong, I will check an eye quickly on your PR if you allows me. Or Daniel will do, if he prefers.

here :

private locale = "en";

and here :

@ddfreiling
Copy link
Contributor Author

ddfreiling commented Oct 2, 2024

Yes, there are definitely some optimizations, so that "en" is not set just before another locale match is. This PR was meant to get the ball rolling on that. I'm not a saga wiz :)

@danielweck
Copy link
Member

good point Pierre, we need to check the initialisation lifecycle.

@ddfreiling
Copy link
Contributor Author

I think we need to check the double default locale initialization set to "en" value in first. Maybe I'm wrong, I will check an eye quickly on your PR if you allows me. Or Daniel will do, if he prefers.

here :

private locale = "en";

and here :

Should I just check that a matched locale is not the default at start-up? So that we don't set en locale twice.

@panaC
Copy link
Member

panaC commented Oct 7, 2024

Hello ddfreiling, I refactor the translator to rely mostly on the redux locale value to update/trigger the translator function and refresh all the react components if needed.

I've setup the PR in draft mode to review it. I took advantage of it to refactor the initialization of both renderer processes (library and reader).

FYI: This is the chunk of code to initialize the locale value with the preferred system language :

https://github.com/edrlab/thorium-reader/pull/2592/files#diff-b053d334bb59cdb8e933a84a5787acc7478dd227b984534b3612046abf574144R223-R239

@ddfreiling
Copy link
Contributor Author

ddfreiling commented Oct 8, 2024

Hello ddfreiling, I refactor the translator to rely mostly on the redux locale value to update/trigger the translator function and refresh all the react components if needed.

I've setup the PR in draft mode to review it. I took advantage of it to refactor the initialization of both renderer processes (library and reader).

FYI: This is the chunk of code to initialize the locale value with the preferred system language :

https://github.com/edrlab/thorium-reader/pull/2592/files#diff-b053d334bb59cdb8e933a84a5787acc7478dd227b984534b3612046abf574144R223-R239

Seems like a good refactor, but I don't see how my locale will be matched if my system returns da-DK, and the availableLanguage code is da. That's why I had a more complex GetMatchingAvailableLanguage implementation, which first checks for full match, and then just the lang-code.

@panaC
Copy link
Member

panaC commented Oct 15, 2024

Hello ddfreiling, I refactor the translator to rely mostly on the redux locale value to update/trigger the translator function and refresh all the react components if needed.
I've setup the PR in draft mode to review it. I took advantage of it to refactor the initialization of both renderer processes (library and reader).
FYI: This is the chunk of code to initialize the locale value with the preferred system language :
https://github.com/edrlab/thorium-reader/pull/2592/files#diff-b053d334bb59cdb8e933a84a5787acc7478dd227b984534b3612046abf574144R223-R239

Seems like a good refactor, but I don't see how my locale will be matched if my system returns da-DK, and the availableLanguage code is da. That's why I had a more complex GetMatchingAvailableLanguage implementation, which first checks for full match, and then just the lang-code.

On Mac os :

locale: en-GB
systemeLocale: en-FR
preferred: [ 'en-FR' ]

same on Linux and I cannot test on windows machine

So like you suggest I propose to check from the array for the first match of the entire bcp47 code and otherwise the 2 digits code lang only.

@ddfreiling
Copy link
Contributor Author

Yes I agree this would be the way to go 👍

@panaC panaC closed this in #2592 Oct 15, 2024
@panaC panaC closed this in 7a5cbc8 Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants