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

[naoto.sato at oracle.com]

Hi Joe,

Thanks again for the review. The reason the way it is is all historical. 
percent/perMill/minusSign all had public APIs for the 'char' version 
since inception, and text version APIs were added later (JDK13). Thus 
they had to be in sync (both fields are accessible through API). On the 
other hand, exponential was private till JDK6, and at that time I guess 
the engineer decided only to expose public access to its text version, 
i.e., effectively deprecate 'char' version interface and field. I guess 
that's why he/she did not bother make them in sync, IMO. So there seems 
to be no explicit reason (to be noted in the source code) for not syncing.

My $.02

Naoto

On 1/3/20 11:40 AM, Joe Wang wrote:
> Hi Naoto,
> 
> The change looks fine to me as only monetaryGroupingSeparator was added 
> to equals.
> 
> I can't help to note though that, all fields participated in the equals 
> calculation except exponential. Some of the other fields are in similar 
> situations (one is public and the other not), e.g. percent and 
> percentText, perMill and perMillText, minusSign and minusSignText, and 
> also the currency related fields, but they all are included. It looks 
> like exponential was never publicly accessible, but the (1.6) added 
> exponentialSeparator became public. It's probably not necessary to 
> include all of them in the first place as they are in sync. that is, 
> changing one would change the other -- and in this regard, exponential 
> is an exception: setExponentialSymbol won't change exponential.
> 
> I understand this is all historical and it doesn't affect your 
> changeset. If the reason is known, it won't hurt to add some notes as 
> the other setXxxText clearly stated the relationship with their non-Text 
> representation. If not, it's fine to me to not have to spend the extra 
> time.
> 
> Best,
> Joe
> 
> On 1/3/20 9:23 AM, naoto.sato at oracle.com wrote:
>> 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
>>>>>
>>>
>