On Tue, 14 Jan 2025 15:41:38 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
> Found during writing the API tests:
>
> - setUseContentHeight/Width (true -> on)
> - remove unnecessary non-nullable limitation for caretBlinkProperty
The change to setUseContentWidth / Height is an obviously correct fix.
I am not in favor of the change to caretBlink period. Returning null as a
default is not how we do things for numeric values. See Tooltip::showDelay, for
example.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java
line 394:
> 392: *
> 393: * @return the caret blink period property
> 394: * @defaultValue null
I am not in favor of this change. From a user API perspective, the default
should be 1000 ms, regardless of whether or not null should be also treated as
valid, and treated as 1000 ms.
Separately, I'm not sure I see the need for allowing null, but there might be a
reason I'm missing.
modules/jfx.incubator.richtext/src/main/java/jfx/incubator/scene/control/richtext/RichTextArea.java
line 413:
> 411:
> 412: public final Duration getCaretBlinkPeriod() {
> 413: return caretBlinkPeriod == null ? null : caretBlinkPeriod.get();
This just makes the API harder to use. Why push the burden to the application?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1676#pullrequestreview-2550174512
PR Review Comment: https://git.openjdk.org/jfx/pull/1676#discussion_r1915095668
PR Review Comment: https://git.openjdk.org/jfx/pull/1676#discussion_r1915097291
