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.

Sign up for GitHub

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

[20+] fix warning 'The constructor Locale is deprecated since version 20' #1406

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

[20+] fix warning 'The constructor Locale is deprecated since version 20' #1406

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions ...e/org.eclipse.update.configurator/src/org/eclipse/update/internal/configurator/Utils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,21 +289,21 @@ private static boolean isMatchingLocale(String candidateValues, String locale) {
}
return false;
}

public static Locale getDefaultLocale() {
String nl = getNL();
// sanity test
if (nl == null)
return Locale.getDefault();

// break the string into tokens to get the Locale object
Copy link
Contributor

@mihnita mihnita Aug 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I would suggest a more correct way of creating a locale:
Locale.forLanguageTag(localeId.replace('_', '-'))

See here for some details:
https://errorprone.info/bugpattern/UnsafeLocaleUsage

Should never use the Locale.toString(), the Locale constructor and Locale.of now (that I didn't know about).

  • They don't support standard BCP 47 locale identifiers, including very common ones that have scripts. For example zh-Hant, or sr-Latn-RS.
  • No Unicode extensions (en-US-u-hc-h23, or ar-u-nu-latn)
  • No "real variant" (for example de-CH-1996 or ca_ES_VALENCIA)

https://www.rfc-editor.org/rfc/bcp/bcp47.txt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • They don't support standard BCP 47 locale identifiers

Hi @mihnita, thanks for the qualified comment. However introducing BCP 47 would change the documented behavior. That is not what this PR is about. As far as i understand this code reflects the Localization defined by OSGi in https://docs.osgi.org/specification/osgi.core/8.0.0/framework.module.html#framework.module.localization chapter 3.11, which defines only language, country, variant
See also https://github.com/eclipse-platform/eclipse.platform.releng.aggregator/blob/72743b4569d5f8d522fd93f4a8875077d32e1fc3/eclipse.platform.common/bundles/org.eclipse.platform.doc.isv/reference/misc/runtime-options.html#L721 ff
NL values should follow the standard Java locale naming conventions. i could not find any reference that BCP 47 encoding can or should be used or that anybody missed a certain local feature beside language, country, variant.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are right, if this applies to resource bundles.

It is really unfortunate that OSGi went with language+region+variant.
This system cannot represent certain locales that are currently in use. For example sr-Cyrl-RS vs sr-Latn-RS (Serbian in Cyrillic / Latin script, in Serbia).
I was not aware of this, and I am quite disappointed to see it. :-(

NL values should follow the standard Java locale naming conventions. i could not find any reference that BCP 47 encoding can or should be used or that anybody missed a certain local feature beside language, country, variant.

I think the Java locales are very much about BCP 47 these days:

The Locale class implements IETF BCP 47 which is composed of RFC 4647 "Matching of Language Tags" and RFC 5646 "Tags for Identifying Languages" with support for the LDML (UTS#35, "Unicode Locale Data Markup Language") BCP 47-compatible extensions for locale data exchange.
...
A Locale object logically consists of the fields described below:
... language, script, country (region), variant, extensions
...
Because of these issues, it is recommended that clients migrate away from constructing non-conforming locales and use the forLanguageTag and Locale.Builder APIs instead. Clients desiring a string representation of the complete locale can then always rely on toLanguageTag for this purpose.
...
Java's default resource bundle lookup mechanism also implements this mapping, so that resources can be named using either convention, see ResourceBundle.Control.
...
https://docs.oracle.com/en/java/javase/21/docs/api/java.base/java/util/Locale.html

Note especially "it is recommended that clients migrate away from constructing non-conforming locales and use the forLanguageTag and Locale.Builder APIs instead."

I have no idea why Java would add a factory method (of) that takes the legacy style lang + region + variant.

And why, even if the Java Locale says "resources can be named using either convention", OSGi went "the legacy way" :-(

And if it is indeed about OSGi resources, there might be problems in some cases, as there is no way to cleanly map between BCP 47 and lang+region+variant.
For example if my default locale is sr-Latn-RS, what will be the lang+region+variant equivalent? (see the "Compatibility" section in the Locale documentation.)

TLDR: if this is strictly about resources, and more precise about OSGi resources, then you change is OK as is.
Maybe add a comment.

If it is about something else, then probably BCP 47 is still the best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OSGi went "the legacy way"

I am sure OSGI is open for improvements. If there is any backward compatible way to support both lang+region+variant and BCP47 we could also just apply it here.

StringTokenizer locales = new StringTokenizer(nl,"_"); //$NON-NLS-1$
StringTokenizer locales = new StringTokenizer(nl, "_"); //$NON-NLS-1$
if (locales.countTokens() == 1)
return new Locale(locales.nextToken(), ""); //$NON-NLS-1$
return Locale.of(locales.nextToken());
else if (locales.countTokens() == 2)
return new Locale(locales.nextToken(), locales.nextToken());
return Locale.of(locales.nextToken(), locales.nextToken());
else if (locales.countTokens() == 3)
return new Locale(locales.nextToken(), locales.nextToken(), locales.nextToken());
return Locale.of(locales.nextToken(), locales.nextToken(), locales.nextToken());
else
return Locale.getDefault();
}
Expand Down
Loading