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

[Joe Wang]

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
>>