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

Creating Locale with specified Language script #250

Merged
merged 2 commits into from
Apr 7, 2022
Merged

Creating Locale with specified Language script #250

merged 2 commits into from
Apr 7, 2022

Conversation

VeljkoMaksimovic
Copy link
Contributor

@VeljkoMaksimovic VeljkoMaksimovic commented Nov 1, 2021

What does this pull request do?

Adds a LocaleUtility class which expands LocaleUtil from lang3 package. LocaleUtil does not support creation of Locales with script specified (Latin, Cyrillic, etc.). LocaleUtility checks if the string that represents locale contains script. If so, it creates Locale using Locale.Builder class, otherwise calls the LocaleUtil.toLocale() as it was done before.

What's new?

Now we can specify locales like sr_Latn_RS or sr_Cyrl_RS, which was not the case before.

How should this be tested?

Only way to see the difference is to have Locales of same languages but in different scripts.

  • Does this change require documentation to be updated?
    • Yes. Localization documentation should contain example of Locale string representation which specifies the script.
  • Does this change add any new dependencies?
    • Not external

Interested parties

@VIVO-project/vivo-committers

@litvinovg
Copy link
Contributor

LocaleUtils from lang3 package support creation of locales with variants.
The first should be language in lower case + _ + country in upper case + _ + variant.
Try LocaleUtils.toLocale("sr_RS_Cyr")
and LocaleUtils.toLocale("sr_RS_Latn")

@VeljkoMaksimovic
Copy link
Contributor Author

Thanks for the reply.
I tried doing it that way before, but it has few problems. First, Locale does not treat script and variant the same, so although I created two languages, one in Latin and another in Cyrillic, like you suggested, this is the result I get in language selection drop-down menu:
SelectLanguageLang3
Both Languages are shown in Cyrillic since that is the default script for sr_RS locale. Also, since Latin and Cyrillic are not valid variants, I also get these warnings:
WarningLang3
On the other hand, if I use Locale.Builder() to set the script, this is the result:
SelectLanguageBuilder
and the warnings are also gone.

Looking at the source code of LocaleUtils, there seams to be no way to specify Script, although IETF BCP 47 standard https://tools.ietf.org/search/bcp47#section-2.1 , which Java documentation for Locale cites, specifies a way to denote a script within the language tag.

litvinovg
litvinovg previously approved these changes Dec 18, 2021
Copy link
Contributor

@litvinovg litvinovg left a comment

Choose a reason for hiding this comment

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

Works as described.

chenejac
chenejac previously approved these changes Dec 24, 2021
Copy link
Contributor

@chenejac chenejac left a comment

Choose a reason for hiding this comment

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

It works as described. The name of the LocaleUtility might introduce confusion, because the Utils suffix is used for some Vitro utility classes. However, naming this class LocaleUtils might introduce importing classes issues, i.e. mixing the newly added class with org.apache.commons.lang3.LocaleUtils. Therefore, there is some sense in keeping this name. I approve this PR.

@VeljkoMaksimovic VeljkoMaksimovic changed the base branch from main to rel-1.12.0-maint April 5, 2022 02:36
@VeljkoMaksimovic VeljkoMaksimovic changed the base branch from rel-1.12.0-maint to main April 5, 2022 02:36
@VeljkoMaksimovic VeljkoMaksimovic dismissed stale reviews from chenejac and litvinovg April 5, 2022 02:36

The base branch was changed.

@chenejac chenejac merged commit b8944fb into vivo-project:main Apr 7, 2022
ghost pushed a commit that referenced this pull request Feb 23, 2023
* Creating a util function to convert Locale string to Locale, but take language script into consideration

* emptz commit to trigger auto build
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
small PR A small pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants