<i18n dev> [11] RFR: 8181157: CLDR Timezone name fallback implementation

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

<i18n dev> [11] RFR: 8181157: CLDR Timezone name fallback implementation

Naoto Sato-2
Hello,

Please review the changes to the following issue:

https://bugs.openjdk.java.net/browse/JDK-8181157

The proposed fix is located at:

http://cr.openjdk.java.net/~naoto/8181157/webrev.05/

This RFE is to implement CLDR time zone names fallback mechanism [1].
Prior to this fix, time zone names are substituted with English names.
With this change, it is generated according to the logic defined in TR
35. Here are the highlight of the changes:

CLDRConverter:
- CLDRConverter has changed to not substitute/invent time zone display
names, except English bundle for compatibility & runtime performance.
- For English bundle, copy over COMPAT's names as before.
- CLDRConverer now parses regionFormat/gmtZeroFormat/exemplarCity

CLDR provider:
- CLDRTimeZoneNameProviderImpl is introduced to generate fallback names
at runtime.
- If COMPAT provider is present, use it also as a fallback, before the
last resort (GMT offset format)

Naoto

[1] http://www.unicode.org/reports/tr35/tr35-dates.html#Time_Zone_Names
Reply | Threaded
Open this post in threaded view
|

Re: <i18n dev> [11] RFR: 8181157: CLDR Timezone name fallback implementation

Xueming Shen
Naoto,

Here some comments

(1) CLDRTimeZoneNameProviderImpl.java:
      Ln#58:  to use Stream.toArray(String[]::new) ? no sure which one is faster

      Ln#155-160:
          is it worth considering to check all possible empty slots in "names" here
          (from index_std_long to index_std_short?) to avoid check/load "compact" multiple
          times?  it appears doable for deriveFallbackNames() but I'm not sure about
          the invocation at #ln100, so a "?".

(2) CLDRConverter.java:
      Ln#674: (not in updated code) why update the "local" jreMetaMap?
                    just wonder it'd better to use Optional.ifPresentOrElse(...) here

      Ln:707:  (don't know which one is better though :-))
      .toMap(Map.Entry::getKey, Map.Entry::getValue);

(3) LocaleResources.java
      just wanted to confirm the "locale.resources.debug" is something we wanted to
      be a public interface or not?

Thanks,
Sherman


On 04/17/2018 02:28 PM, Naoto Sato wrote:

> Hello,
>
> Please review the changes to the following issue:
>
> https://bugs.openjdk.java.net/browse/JDK-8181157
>
> The proposed fix is located at:
>
> http://cr.openjdk.java.net/~naoto/8181157/webrev.05/
>
> This RFE is to implement CLDR time zone names fallback mechanism [1]. Prior to this fix, time zone names are substituted with English names. With this change, it is generated according to the logic defined in TR 35. Here are the highlight of the changes:
>
> CLDRConverter:
> - CLDRConverter has changed to not substitute/invent time zone display
> names, except English bundle for compatibility & runtime performance.
> - For English bundle, copy over COMPAT's names as before.
> - CLDRConverer now parses regionFormat/gmtZeroFormat/exemplarCity
>
> CLDR provider:
> - CLDRTimeZoneNameProviderImpl is introduced to generate fallback names
> at runtime.
> - If COMPAT provider is present, use it also as a fallback, before the
> last resort (GMT offset format)
>
> Naoto
>
> [1] http://www.unicode.org/reports/tr35/tr35-dates.html#Time_Zone_Names

Reply | Threaded
Open this post in threaded view
|

Re: <i18n dev> [11] RFR: 8181157: CLDR Timezone name fallback implementation

Naoto Sato-2
Hi Sherman, thanks for the review.

On 4/23/18 1:06 PM, Xueming Shen wrote:

> Naoto,
>
> Here some comments
>
> (1) CLDRTimeZoneNameProviderImpl.java:
>       Ln#58:  to use Stream.toArray(String[]::new) ? no sure which one
> is faster
>
>       Ln#155-160:
>           is it worth considering to check all possible empty slots in
> "names" here
>           (from index_std_long to index_std_short?) to avoid check/load
> "compact" multiple
>           times?  it appears doable for deriveFallbackNames() but I'm
> not sure about
>           the invocation at #ln100, so a "?".
>
> (2) CLDRConverter.java:
>       Ln#674: (not in updated code) why update the "local" jreMetaMap?
>                     just wonder it'd better to use
> Optional.ifPresentOrElse(...) here
>
>       Ln:707:  (don't know which one is better though :-))
>       .toMap(Map.Entry::getKey, Map.Entry::getValue);

Addressed all of the above suggestions. Unnecessary jreMetaMap update
was a good catch!

>
> (3) LocaleResources.java
>       just wanted to confirm the "locale.resources.debug" is something
> we wanted to
>       be a public interface or not?

Eventually I'd be public (8057075), but it is a private use for the time
being.

Here's the updated webrev:

http://cr.openjdk.java.net/~naoto/8181157/webrev.06/

Naoto

>
> Thanks,
> Sherman
>
>
> On 04/17/2018 02:28 PM, Naoto Sato wrote:
>> Hello,
>>
>> Please review the changes to the following issue:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8181157
>>
>> The proposed fix is located at:
>>
>> http://cr.openjdk.java.net/~naoto/8181157/webrev.05/
>>
>> This RFE is to implement CLDR time zone names fallback mechanism [1].
>> Prior to this fix, time zone names are substituted with English names.
>> With this change, it is generated according to the logic defined in TR
>> 35. Here are the highlight of the changes:
>>
>> CLDRConverter:
>> - CLDRConverter has changed to not substitute/invent time zone display
>> names, except English bundle for compatibility & runtime performance.
>> - For English bundle, copy over COMPAT's names as before.
>> - CLDRConverer now parses regionFormat/gmtZeroFormat/exemplarCity
>>
>> CLDR provider:
>> - CLDRTimeZoneNameProviderImpl is introduced to generate fallback names
>> at runtime.
>> - If COMPAT provider is present, use it also as a fallback, before the
>> last resort (GMT offset format)
>>
>> Naoto
>>
>> [1] http://www.unicode.org/reports/tr35/tr35-dates.html#Time_Zone_Names
>
Reply | Threaded
Open this post in threaded view
|

Re: <i18n dev> [11] RFR: 8181157: CLDR Timezone name fallback implementation

Xueming Shen
+1

On 4/23/18, 6:25 PM, Naoto Sato wrote:

> Hi Sherman, thanks for the review.
>
> On 4/23/18 1:06 PM, Xueming Shen wrote:
>> Naoto,
>>
>> Here some comments
>>
>> (1) CLDRTimeZoneNameProviderImpl.java:
>>       Ln#58:  to use Stream.toArray(String[]::new) ? no sure which
>> one is faster
>>
>>       Ln#155-160:
>>           is it worth considering to check all possible empty slots
>> in "names" here
>>           (from index_std_long to index_std_short?) to avoid
>> check/load "compact" multiple
>>           times?  it appears doable for deriveFallbackNames() but I'm
>> not sure about
>>           the invocation at #ln100, so a "?".
>>
>> (2) CLDRConverter.java:
>>       Ln#674: (not in updated code) why update the "local" jreMetaMap?
>>                     just wonder it'd better to use
>> Optional.ifPresentOrElse(...) here
>>
>>       Ln:707:  (don't know which one is better though :-))
>>       .toMap(Map.Entry::getKey, Map.Entry::getValue);
>
> Addressed all of the above suggestions. Unnecessary jreMetaMap update
> was a good catch!
>
>>
>> (3) LocaleResources.java
>>       just wanted to confirm the "locale.resources.debug" is
>> something we wanted to
>>       be a public interface or not?
>
> Eventually I'd be public (8057075), but it is a private use for the
> time being.
>
> Here's the updated webrev:
>
> http://cr.openjdk.java.net/~naoto/8181157/webrev.06/
>
> Naoto
>
>>
>> Thanks,
>> Sherman
>>
>>
>> On 04/17/2018 02:28 PM, Naoto Sato wrote:
>>> Hello,
>>>
>>> Please review the changes to the following issue:
>>>
>>> https://bugs.openjdk.java.net/browse/JDK-8181157
>>>
>>> The proposed fix is located at:
>>>
>>> http://cr.openjdk.java.net/~naoto/8181157/webrev.05/
>>>
>>> This RFE is to implement CLDR time zone names fallback mechanism
>>> [1]. Prior to this fix, time zone names are substituted with English
>>> names. With this change, it is generated according to the logic
>>> defined in TR 35. Here are the highlight of the changes:
>>>
>>> CLDRConverter:
>>> - CLDRConverter has changed to not substitute/invent time zone display
>>> names, except English bundle for compatibility & runtime performance.
>>> - For English bundle, copy over COMPAT's names as before.
>>> - CLDRConverer now parses regionFormat/gmtZeroFormat/exemplarCity
>>>
>>> CLDR provider:
>>> - CLDRTimeZoneNameProviderImpl is introduced to generate fallback names
>>> at runtime.
>>> - If COMPAT provider is present, use it also as a fallback, before the
>>> last resort (GMT offset format)
>>>
>>> Naoto
>>>
>>> [1] http://www.unicode.org/reports/tr35/tr35-dates.html#Time_Zone_Names
>>