On Tue, 23 Dec 2025 09:33:03 GMT, Nir Lisker <[email protected]> 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 one additional > commit since the last revision: > > Formatting fixes I'm finally getting back to this. I'd like to move this forward, and try to get it in before too long (along with a few others that have been languishing), ideally early in JavaFX 28 so we don't find ourselves out of time yet again. Can you merge master into your branch? I did a preliminary look at the API, including generated docs, looking for compatibility issues. I am not finished, but I did spot one behavioral change and one binary compatibility issue. I left an inline question about whether the behavioral change caused by caching the default Locale was intentional or accidental. The one big problem I discovered is that this PR breaks binary compatibility in an unintended way due to the removal of the specialized versions of `String toString(SUBTYPE object)` and the covariant `SUBTYPE fromString(String string)` methods from the sub-classes. The following simple example will show this: var conv = new IntegerStringConverter(); System.out.println(conv.toString(1234)); Compiling against jfx master and running against this PR branch produces: Exception in thread "main" java.lang.NoSuchMethodError: 'java.lang.String javafx.util.converter.IntegerStringConverter.toString(java.lang.Integer)' Similarly the following will fail: var conv = new IntegerStringConverter(); Integer i = conv.fromString("567"); System.out.println("i=" + i); Compiling against jfx master and running against this PR branch produces: java.lang.NoSuchMethodError: 'java.lang.Integer javafx.util.converter.IntegerStringConverter.fromString(java.lang.String)' The other converters have the same problem. This must be addressed, probably by restoring the specialized methods and calling the superclass implementation. modules/javafx.base/src/main/java/javafx/util/converter/BaseTemporalStringConverter.java line 42: > 40: abstract class BaseTemporalStringConverter<T extends Temporal> extends > BaseStringConverter<T> { > 41: > 42: private static final Locale DEFAULT_LOCALE = > Locale.getDefault(Locale.Category.FORMAT); Currently, the default format Locale is read every time a converter is constructed. Caching it in a static final means that a change by an application to the default Locale after this class has been initialized will be ignored. If that's unintentional, then it should be fixed. If intentional, then it should be discussed and documented in the CSR as a (fairly minor) behavioral change. modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 42: > 40: public class DateTimeStringConverter extends BaseStringConverter<Date> { > 41: > 42: private static final Locale DEFAULT_LOCALE = > Locale.getDefault(Locale.Category.FORMAT); Same comment as above regarding caching the default Locale. ------------- Changes requested by kcr (Lead). PR Review: https://git.openjdk.org/jfx/pull/1880#pullrequestreview-4678070902 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r3564397406 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r3564401837
