<i18n dev> [15] RFR: 8227313: Support monetary grouping separator in DecimalFormat/DecimalFormatSymbols

[naoto.sato at oracle.com]

Hi Joe,

I revised the changeset, as the cached hash code in DecimalFormatSymbols 
needs to be recalculated when any of the relevant fields is mutated. 
Here is the updated webrev:

http://cr.openjdk.java.net/~naoto/8227313/webrev.02/

Naoto

On 1/2/20 2:19 PM, Joe Wang wrote:
> Happy New Year, Naoto!
> 
> Thanks for the explanation and changes. The changeset looks good to me.
> 
> -Joe
> 
> On 1/2/20 12:50 PM, naoto.sato at oracle.com wrote:
>> Hi Joe,
>>
>> Happy new year and thanks for your comments. Please see my replies below:
>>
>> On 12/23/19 5:20 PM, Joe Wang wrote:
>>> Hi Naoto,
>>>
>>> Is there a need for an APINote to note the relationship between the 
>>> new get/setMonetaryGroupingSeparator and get/setGroupingSeparator 
>>> methods. The new method did state it "May be different from {@code 
>>> grouping separator} in some locales", but that may be insufficient. 
>>> For example, does setting one method affects the other (seems it 
>>> should since the monetaryGroupingSeparator may be initialized by the 
>>> groupingSeparator as the impl shows)? If yes, how it's affected?
>>
>> Setting the custom monetary grouping separator will not affect the 
>> existing normal grouping separator. I added the explanation in the 
>> method description.
>>
>>>
>>> If no, is there a compatibility concern? In the current impl, the new 
>>> get method is used when "isCurrencyFormat" is true while the new set 
>>> method doesn't affect the existing 'groupingSeparator'. For existing 
>>> applications that have a custom grouping separator set through 
>>> setGroupingSeparator, it seems to me the custom separator won't be 
>>> used moving onto the new JDK impl.
>>
>> Good point. Modified the compatibility risk from minimum to low with 
>> the explanation.
>>
>>>
>>>
>>> A minor comment about the definition statement in the following:
>>>
>>>      Add the following new serializable field 
>>> in|java.text.DecimalFormatSymbols|class:
>>>
>>> |/** * The grouping separator used when formatting currency values. * 
>>> * @serial * @since 15 */ private char monetaryGroupingSeparator;|
>>>
>>>
>>> and that for the new get method:
>>>          Gets the character used for thousands separator for currencies.
>>>
>>>
>>> While the meanings are clear, they were not as consistent as that 
>>> between the existing groupingSeparator and its get method, that is:
>>>          Character used for thousands separator.
>>>
>>>     and then the get method states:
>>>
>>>          Gets the character used for thousands separator.
>>>
>>>
>>> But this is minor, your call whether to change it or not.
>>
>> Consistency is important. I replaced all occurrences of "thousands 
>> separator(s)" in DecimalFormat/DecimalFormatSymbols with "grouping 
>> separator(s)."
>>
>> Here are the modified changeset:
>>
>> http://cr.openjdk.java.net/~naoto/8227313/webrev.01/
>>
>> as well as the modified CSR at the same URL.
>>
>> Naoto
>>
>>>
>>> Best,  and have a great Christmas! :-)
>>> Joe
>>>
>>> On 12/20/19 12:57 PM, naoto.sato at oracle.com wrote:
>>>> Hi,
>>>>
>>>> Please review the fix for the following issue:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8227313
>>>>
>>>> The proposed CSR and changeset are located at:
>>>>
>>>> https://bugs.openjdk.java.net/browse/JDK-8235942
>>>> https://cr.openjdk.java.net/~naoto/8227313/webrev.00/
>>>>
>>>> The change introduces the new monetary grouping separator in 
>>>> DecimalFormatSymbols, and it is used if a DecimalFormat instance 
>>>> designates currency formatting where its pattern includes the 
>>>> currency symbol (U+00A5). The change makes use of the CLDR data 
>>>> which provides two distinct grouping separators (one for generic, 
>>>> the other for currency) in some locales. It also addresses the 
>>>> similar cases for the decimal separator.
>>>>
>>>> Naoto
>>>
>