On Wed, 19 Nov 2025 22:01:01 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: > > Clarify thread safety Overall this looks like good cleanup. I think it will take more time to fully review the doc changes than we have for JavaFX 26, so we might target it to 27. I spot checked some of the methods and asked a couple questions, but I'll have to look at it more closely after the holidays. modules/javafx.base/src/main/java/javafx/util/converter/BooleanStringConverter.java line 28: > 26: package javafx.util.converter; > 27: > 28: /// A `StringConverter` implementation for `Boolean` (and `boolean`) > values. Formatting is done by [Boolean#toString()] The `Boolean` reference is currently a hyperlink. Did you remove it intentionally? modules/javafx.base/src/main/java/javafx/util/converter/CharacterStringConverter.java line 28: > 26: package javafx.util.converter; > 27: > 28: /// A `StringConverter` implementation for `Character` (and `character`) > values. Formatting is done by Shouldn't that be "and `char`" since there is no primitive type name `character`? modules/javafx.base/src/main/java/javafx/util/converter/DateTimeStringConverter.java line 37: > 35: /// > 36: /// Note that using `Date` is not recommended in JDK versions where > [java.time.LocalDateTime] is available, in which > 37: /// case [LocalDateTimeStringConverter] should be used. Should we consider deprecating this class (not for removal)? ------------- PR Review: https://git.openjdk.org/jfx/pull/1880#pullrequestreview-3606090074 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2641532126 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2641539007 PR Review Comment: https://git.openjdk.org/jfx/pull/1880#discussion_r2641543171
