RFR: 8250802: Refactor StringConverter and its subclasses [v2]

[Andy Goryachev]

On Sat, 30 Aug 2025 20:11:36 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Refactoring of all `StringConverter`s and their tests. General notes:
>> * The documentation language has been unified and `null` parameter rules have been documented.
>> *  Tests have been cleaned up in the vein of https://github.com/openjdk/jfx/pull/1759 and unneeded `@BeforeAll`s were removed.
>> * Internal fields were made `private final` to guarantee immutability.
>> 
>>  Incremental commits are provided for easier reviewing:
>> 
>> ### Parent classes
>> * `StringConverter`: updated documentation
>> * `BaseStringConverter`: a new internal class that implements repeated code from converter implementations and serves as an intermediate superclass. It does empty and `null` string checks that are handled uniformly, except for `DefaultStringConverter`, which has a different formatting mechanism.
>> 
>> ### Primitive-related converters
>> * All primitive (wrapper) converters also document their formatting and parsing mechanism since these are "well-established".
>> 
>> ### Format converter
>> * Checked for `null` during constriction time to avoid runtime NPEs.
>> * There is no test class for this converter. A followup might be desirable.
>> * A followup should deprecate for removal `protected Format getFormat()` (as in [JDK-8314597](https://bugs.openjdk.org/browse/JDK-8314597) and [JDK-8260475](https://bugs.openjdk.org/browse/JDK-8260475)).
>> 
>> ### Number and subclasses converters
>> * The intermediate `locale` and `pattern` fields were removed (along with their tests). The class generated a new formatter from these on each call. This only makes sense for mutable fields where the resulting formatter can change, but here the formatter can be computed once on construction and stored.
>> * The only difference between these classes is a single method for creating a format from a `null` pattern, which was encapsulated in the `getSpecializedNumberFormat` method.
>> * The terminally deprecated `protected NumberFormat getNumberFormat()` was removed. Can be split to its own issue if preferred. In my opinion, it shouldn't exist even internally since testing the internal formatter doesn't help. The only tests here should be for to/from strings, and these are lacking. A followup can be filed for adding more conversion tests.
>> 
>> ### Date/Time converters
>> * Added a documentation note advising users to use the `java.time` classes instead of the old `Date` class.
>> * As with Number converters, only the `dateFormat` field was kept, which is created once on construction instead of on each...
>
> Nir Lisker has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - review comments 2
>  - review comments 1

I think the change makes sense, despite a somewhat large footprint.

Requesting the following changes:
- sync up with the latest master
- revert javadoc markdown

modules/javafx.base/src/main/java/javafx/util/StringConverter.java line 38:

> 36: /// - Except for `DefaultStringConverter`, formatting `null` returns an empty string, otherwise the type's `toString` is
> 37: /// used if it is suitable; parsing `null` or an empty string returns `null`.
> 38: /// - Immutable (the same converter can be reused).

Is this true? 
I recall some formatters having a state, but I don't remember which...

modules/javafx.base/src/main/java/javafx/util/converter/BaseStringConverter.java line 30:

> 28: import javafx.util.StringConverter;
> 29: 
> 30: /// A base class containing common implementations for `StringConverter`s as noted in the @implNote of `StringConverter`.

In light of possible backporting, it's been decided earlier not to use markdown comments.

modules/javafx.base/src/main/java/javafx/util/converter/DateStringConverter.java line 99:

> 97: 
> 98:     @Override
> 99:     DateFormat getSpecialziedDateFormat(int dateStyle, int timeStyle, Locale locale) {

spelling

modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 115:

> 113:                 getSpecialziedDateFormat(dateStyle, timeStyle, locale) :
> 114:                 new SimpleDateFormat(pattern, locale);
> 115:         dateFormat.setLenient(false);

Would setting `lenient=false` constitute a change in functionality?

modules/javafx.base/src/main/java/javafx/util/converter/TimeStringConverter.java line 101:

> 99: 
> 100:     @Override
> 101:     DateFormat getSpecialziedDateFormat(int dateStyle, int timeStyle, Locale locale) {

spelling

modules/javafx.base/src/test/java/test/javafx/util/converter/CurrencyStringConverterTest.java line 58:

> 56:     void testConstructor_locale() {
> 57:         NumberFormat numberFormat = NumberFormat.getCurrencyInstance(Locale.CANADA);
> 58: 

very minor: extra newlines?

modules/javafx.base/src/test/java/test/javafx/util/converter/NumberStringConverterTest.java line 58:

> 56:     void testConstructor_locale() {
> 57:         NumberFormat numberFormat = NumberFormat.getNumberInstance(Locale.CANADA);
> 58: 

minor: weird extra newlines

-------------

Changes requested by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1880#pullrequestreview-3474802047
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535762634
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535714830
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535737237
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535744617
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535760564
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535779020
PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2535796857