RFR: 8347715: RichTextArea Follow-up: Minor Bugs

[Kevin Rushforth]

On Tue, 14 Jan 2025 15:41:38 GMT, Andy Goryachev <angorya at 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