joerghoh commented on code in PR #17:
URL:
https://github.com/apache/sling-org-apache-sling-i18n/pull/17#discussion_r1807289083
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -566,14 +573,36 @@ private JcrResourceBundle createResourceBundle(
* </ol>
*/
private Locale getParentLocale(Locale locale) {
- if (locale.getVariant().length() != 0) {
+ if (locale.getScript().length() != 0 && locale.getVariant().length()
!= 0) {
Review Comment:
```suggestion
if (!locale.getScript().isEmpty() && !locale.getVariant().isEmpty())
{
```
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -566,14 +573,36 @@ private JcrResourceBundle createResourceBundle(
* </ol>
*/
private Locale getParentLocale(Locale locale) {
Review Comment:
Can you please provide an explicit test coverage for this method? As you
have a complete specification already in the comments, I think it should be
easy to convert that into a test method.
(Changing the scope from private to protected should not be a problem, as
this method is not exported publicly in OSGI.)
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -566,14 +573,36 @@ private JcrResourceBundle createResourceBundle(
* </ol>
*/
private Locale getParentLocale(Locale locale) {
- if (locale.getVariant().length() != 0) {
+ if (locale.getScript().length() != 0 && locale.getVariant().length()
!= 0) {
+ try {
+ return new Locale.Builder()
+ .setLanguage(locale.getLanguage())
+ .setRegion(locale.getCountry())
+ .setScript(locale.getScript())
+ .build();
+ } catch (IllformedLocaleException e) {
+ // fallback to previous implementation
+ return new Locale(locale.getLanguage(), locale.getCountry());
+ }
+ } else if (locale.getScript().length() != 0 &&
locale.getCountry().length() != 0) {
+ try {
+ return new Locale.Builder()
+ .setLanguage(locale.getLanguage())
+ .setScript(locale.getScript())
+ .build();
+ } catch (IllformedLocaleException e) {
+ // fallback to previous implementation
+ return new Locale(locale.getLanguage());
+ }
+ } else if (locale.getScript().length() != 0) {
Review Comment:
```suggestion
} else if (!locale.getScript().isEmpty() {
```
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -566,14 +573,36 @@ private JcrResourceBundle createResourceBundle(
* </ol>
*/
private Locale getParentLocale(Locale locale) {
- if (locale.getVariant().length() != 0) {
+ if (locale.getScript().length() != 0 && locale.getVariant().length()
!= 0) {
+ try {
+ return new Locale.Builder()
+ .setLanguage(locale.getLanguage())
+ .setRegion(locale.getCountry())
+ .setScript(locale.getScript())
+ .build();
+ } catch (IllformedLocaleException e) {
+ // fallback to previous implementation
+ return new Locale(locale.getLanguage(), locale.getCountry());
+ }
+ } else if (locale.getScript().length() != 0 &&
locale.getCountry().length() != 0) {
Review Comment:
```suggestion
} else if (! locale.getScript().isEmpty() &&
!locale.getCountry().isEmpty()) {
```
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -566,14 +573,36 @@ private JcrResourceBundle createResourceBundle(
* </ol>
*/
private Locale getParentLocale(Locale locale) {
- if (locale.getVariant().length() != 0) {
+ if (locale.getScript().length() != 0 && locale.getVariant().length()
!= 0) {
+ try {
+ return new Locale.Builder()
+ .setLanguage(locale.getLanguage())
+ .setRegion(locale.getCountry())
+ .setScript(locale.getScript())
+ .build();
+ } catch (IllformedLocaleException e) {
+ // fallback to previous implementation
+ return new Locale(locale.getLanguage(), locale.getCountry());
+ }
+ } else if (locale.getScript().length() != 0 &&
locale.getCountry().length() != 0) {
+ try {
+ return new Locale.Builder()
+ .setLanguage(locale.getLanguage())
+ .setScript(locale.getScript())
+ .build();
+ } catch (IllformedLocaleException e) {
+ // fallback to previous implementation
Review Comment:
same as above
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -566,14 +573,36 @@ private JcrResourceBundle createResourceBundle(
* </ol>
*/
private Locale getParentLocale(Locale locale) {
- if (locale.getVariant().length() != 0) {
+ if (locale.getScript().length() != 0 && locale.getVariant().length()
!= 0) {
+ try {
+ return new Locale.Builder()
+ .setLanguage(locale.getLanguage())
+ .setRegion(locale.getCountry())
+ .setScript(locale.getScript())
+ .build();
+ } catch (IllformedLocaleException e) {
+ // fallback to previous implementation
Review Comment:
nitpick: Can this even happen? As you construct the parent locale from an
already existing and valid locale object, all elements should be valid.
```suggestion
// should never happen, as all elements already come from a
valid locale object
```
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -646,43 +675,95 @@ private void preloadBundles() {
* languages and countries provided by the platform. Any unsupported
* language or country is replaced by the platform default language and
* country.
+ * Locale string is also parsed for script tag. Any unsupported script is
ignored.
* @param localeString the locale as string
* @return the {@link Locale} being generated from the {@code localeString}
*/
static Locale toLocale(String localeString) {
if (localeString == null || localeString.length() == 0) {
return Locale.getDefault();
}
+
// support BCP 47 compliant strings as well (using a different
separator "-" instead of "_")
localeString = localeString.replaceAll("-", "_");
-
// check language and country
final String[] parts = localeString.split("_");
if (parts.length == 0) {
return Locale.getDefault();
}
// at least language is available
- String lang = parts[0];
- boolean isValidLanguageCode = false;
- String[] langs = Locale.getISOLanguages();
- for (int i = 0; i < langs.length; i++) {
- if (langs[i].equalsIgnoreCase(lang)) {
- isValidLanguageCode = true;
- break;
+ String lang = getValidLanguage(parts[0]);
+ if (parts.length == 1) {
+ return new Locale(lang);
+ }
+
+ Locale localeWithBuilder = createLocaleWithBuilder(parts, lang);
+ if (localeWithBuilder != null) {
+ return localeWithBuilder;
+ }
+
+ return createLocaleWithConstructor(lang, parts);
+ }
+
+ private static Locale createLocaleWithBuilder(String[] parts, String lang)
{
Review Comment:
I think it would make sense for documentation purposes to add an assert
statement, that ``parts.length >=2`` can be always assumed. WDYT?
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -696,17 +777,26 @@ static Locale toLocale(String localeString) {
}
}
}
- if (!isValidCountryCode) {
- country = Locale.getDefault().getCountry();
- }
+ return isValidCountryCode;
+ }
- // language and country
- if (parts.length == 2) {
- return new Locale(lang, country);
+ private static boolean isAlpha(char c) {
Review Comment:
isn't it possible to use ``StringUtils.isAlpha()`` here? Or would that
require to add a new dependency on OSGI level?
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -646,43 +675,95 @@ private void preloadBundles() {
* languages and countries provided by the platform. Any unsupported
* language or country is replaced by the platform default language and
* country.
+ * Locale string is also parsed for script tag. Any unsupported script is
ignored.
* @param localeString the locale as string
* @return the {@link Locale} being generated from the {@code localeString}
*/
static Locale toLocale(String localeString) {
if (localeString == null || localeString.length() == 0) {
return Locale.getDefault();
}
+
// support BCP 47 compliant strings as well (using a different
separator "-" instead of "_")
localeString = localeString.replaceAll("-", "_");
-
// check language and country
final String[] parts = localeString.split("_");
if (parts.length == 0) {
return Locale.getDefault();
}
// at least language is available
- String lang = parts[0];
- boolean isValidLanguageCode = false;
- String[] langs = Locale.getISOLanguages();
- for (int i = 0; i < langs.length; i++) {
- if (langs[i].equalsIgnoreCase(lang)) {
- isValidLanguageCode = true;
- break;
+ String lang = getValidLanguage(parts[0]);
+ if (parts.length == 1) {
+ return new Locale(lang);
+ }
+
+ Locale localeWithBuilder = createLocaleWithBuilder(parts, lang);
+ if (localeWithBuilder != null) {
+ return localeWithBuilder;
+ }
+
+ return createLocaleWithConstructor(lang, parts);
+ }
+
+ private static Locale createLocaleWithBuilder(String[] parts, String lang)
{
+ if (isScript(parts[1])) {
+ try {
+ switch (parts.length) {
+ case 2:
+ return new Locale.Builder()
+ .setLanguage(lang)
+ .setScript(parts[1])
+ .build();
+ case 3:
+ return new Locale.Builder()
+ .setLanguage(lang)
+ .setScript(parts[1])
+ .setRegion(getValidCountry(parts[2]))
+ .build();
+ default: // case >= 4
+ return processMultipleParts(parts, lang);
+ }
+ } catch (IllformedLocaleException e) {
+ LoggerFactory.getLogger(JcrResourceBundleProvider.class)
+ .warn("Failed to create locale with LocaleBuilder", e);
Review Comment:
In this log message can you also provide the input parameters (``parts`` and
``lang``)? I am not sure that all exception messages contain that information,
and it can be really beneficial when debugging issues.
##########
src/main/java/org/apache/sling/i18n/impl/JcrResourceBundleProvider.java:
##########
@@ -646,43 +675,95 @@ private void preloadBundles() {
* languages and countries provided by the platform. Any unsupported
* language or country is replaced by the platform default language and
* country.
+ * Locale string is also parsed for script tag. Any unsupported script is
ignored.
* @param localeString the locale as string
* @return the {@link Locale} being generated from the {@code localeString}
*/
static Locale toLocale(String localeString) {
if (localeString == null || localeString.length() == 0) {
return Locale.getDefault();
}
+
// support BCP 47 compliant strings as well (using a different
separator "-" instead of "_")
localeString = localeString.replaceAll("-", "_");
-
// check language and country
final String[] parts = localeString.split("_");
if (parts.length == 0) {
return Locale.getDefault();
}
// at least language is available
- String lang = parts[0];
- boolean isValidLanguageCode = false;
- String[] langs = Locale.getISOLanguages();
- for (int i = 0; i < langs.length; i++) {
- if (langs[i].equalsIgnoreCase(lang)) {
- isValidLanguageCode = true;
- break;
+ String lang = getValidLanguage(parts[0]);
+ if (parts.length == 1) {
+ return new Locale(lang);
+ }
+
+ Locale localeWithBuilder = createLocaleWithBuilder(parts, lang);
+ if (localeWithBuilder != null) {
+ return localeWithBuilder;
+ }
+
+ return createLocaleWithConstructor(lang, parts);
+ }
+
+ private static Locale createLocaleWithBuilder(String[] parts, String lang)
{
+ if (isScript(parts[1])) {
+ try {
+ switch (parts.length) {
+ case 2:
+ return new Locale.Builder()
+ .setLanguage(lang)
+ .setScript(parts[1])
+ .build();
+ case 3:
+ return new Locale.Builder()
+ .setLanguage(lang)
+ .setScript(parts[1])
+ .setRegion(getValidCountry(parts[2]))
+ .build();
+ default: // case >= 4
+ return processMultipleParts(parts, lang);
+ }
+ } catch (IllformedLocaleException e) {
+ LoggerFactory.getLogger(JcrResourceBundleProvider.class)
+ .warn("Failed to create locale with LocaleBuilder", e);
}
}
- if (!isValidLanguageCode) {
- lang = Locale.getDefault().getLanguage();
+ return null;
+ }
+
+ private static Locale processMultipleParts(String[] parts, String lang) {
Review Comment:
Same as above, would it make sense to somehow document/enforce that
``parts.length() >= 4``?
(Then you don't need the context from the calling method, which already
checks for that. And for that case a simple assert is enough.)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
