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

FEATURE: Setting preferred locale and fallback manually #5026

Draft
wants to merge 13 commits into
base: release/v2.5.x
Choose a base branch
from

Conversation

Sharrnah
Copy link
Contributor

@Sharrnah Sharrnah commented Jul 24, 2024

Description:

Sometimes you might want the user to load a different language file, other than the systems current locale.

This adds a SetPreferredLocale() that you can call before or after for example AddTranslationsFS() and will set a custom locale.
This way, the preferred language is looked up for translations.

Also added a SetLanguageOrder() function that reorders the added languages, in which case the first language is used as fallback when no locale is found.

with this, on a foreign language OS system, you can call

lang.AddTranslationsFS(Resources.Translations, "translations")
lang.SetLanguageOrder([]string{"fr"}) // sets fr as fallback language
lang.SetPreferredLocale("en-US") // use english instead of system locale

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Public APIs match existing style and have Since: line.
  • Any breaking changes have a deprecation path or have been discussed.
  • Check for binary size increases when importing new modules.

@coveralls
Copy link

coveralls commented Jul 24, 2024

Coverage Status

coverage: 66.077% (+0.03%) from 66.046%
when pulling a28b575 on Sharrnah:feature/setting-preferred-language
into 49ad989 on fyne-io:release/v2.5.x.

lang/lang.go Outdated
@@ -159,6 +161,15 @@ func AddTranslationsFS(fs embed.FS, dir string) (retErr error) {
return retErr
}

// SetPreferredLanguage allows an app to set the preferred language for translations, overwriting the System Locale.
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this adds a new public API, it will need to be targeted for Fyne 2.6 and should have a Since: 2.6 line in the comment (see other APIs for an example). Also the PR should be based against develop instead of the release branch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This builds on top of this bugfix #5016
which is i think not yet merged back into develop.

Once that is done, i am happy to base it against develop.
Or should i merge #5016 into develop myself?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will leave it to @andydotxyz to manage the branches since I'm not sure what our usual process is for keeping develop in sync with release/v2.x.x branches

Copy link
Member

Choose a reason for hiding this comment

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

Release branch goes back into develop after each point release, so it should have been done a few weeks back.

@andydotxyz
Copy link
Member

I see there is a note about branch merging which I will look at shortly.
In the meantime I think it's important to note that the function refers to language but the parameter passed looks like a locale. We need to be consistent.
Look at how it is referred to in other lang package APIs

Also an open question - is this a lang package feature or is it a setting (like the font size or colour variant.

If you could articulate the use-case it will help us to understand the right place for the feature.

@dweymouth
Copy link
Contributor

The use case is for apps to be able to have a user-configurable setting to change the language for just that app. One reason this could be desirable is for apps with incomplete translations, which can happen often with open source projects with volunteer translators. If the system locale for some user is set to German, for example, and they are using an app that has incomplete German translations, but they can speak English well and the app does have complete English translations (since it is the developer's native language), then that user might want to be able to set just that app to use English despite system locale being German.

@andydotxyz
Copy link
Member

Is this impacted by the resolution of #5040 ?

@andydotxyz
Copy link
Member

The use case is for apps to be able to have a user-configurable setting to change the language for just that app.

As far as I can see the API does not match the use-case description then.
It talks about preference order not a single language preference.

I know it sounds boring but we have to get right what is expected and have naming that matches that perfectly, otherwise we will get surprising or unfortunate side-effects and follow-on bug reports.

Copy link
Member

@andydotxyz andydotxyz left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. As the CI shows, you have to stick to APIs that were in Go 1.19 to maintain support with existing Fyne usage.

@Sharrnah
Copy link
Contributor Author

Sharrnah commented Jul 31, 2024

Is this impacted by the resolution of #5040 ?

Sorry. haven't seen there is already a PR for it as i added the fallback option.
Its probably not impacted because i am doing the same thing just a bit differently.

Meaning, i sort the translated array instead of adding an english entry, so not sure if we need #5040 then.

i guess mine has the benefit that the user can set his own fallback?
Which should help in case someone has an app without english translation.

in any case, in my PR english is the default as well which is tried first as fallback.

@Sharrnah Sharrnah changed the title FEATURE: Setting preferred language manually FEATURE: Setting preferred locale and fallback manually Jul 31, 2024
@andydotxyz
Copy link
Member

Its probably not impacted because i am doing the same thing just a bit differently.

My point was really that the fix was landed already - so is no longer required to make this feature land.
I see the use-case for being able to set the preferred locale, but a full re-ordering of locale seems to be overkill.

According to the title "Setting preferred locale and fallback manually" it implies that the fallback could be set - replacing the current default of "en". This re-uses more of the code and is a smaller change.


// SetPreferredLocale allows an app to set the preferred locale for translations, overwriting the System Locale.
// locale can be in format en_US_someVariant, en_US, en-US-someVariant, en-US, en
// Since 2.6
Copy link
Member

Choose a reason for hiding this comment

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

This isn't the right format for a since line, missing : and the blank comment line above - see https://github.com/fyne-io/fyne/wiki/Contributing (or other Since: lines in the codebase).

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