RFR: 8341670: [Text, TextFlow] Public API for Text Layout Info [v8]

[Kevin Rushforth]

On Thu, 24 Oct 2024 22:45:28 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> The RichTextArea control ([JDK-8301121](https://bugs.openjdk.org/browse/JDK-8301121)), or any custom control that needs non-trivial navigation within complex or wrapped text needs a public API to get information about text layout.
>> 
>> This change fixes the missing functionality by adding a new public method to the `Text` and `TextFlow` classes.:
>> 
>> 
>>     /**
>>      * Obtains the snapshot of the current text layout information.
>>      * @return the layout information
>>      * @since 24
>>      */
>>     public final LayoutInfo getLayoutInfo()
>> 
>> 
>> The `LayoutInfo` provides a view into the text layout within `Text`/`TextFlow` nodes such as:
>> 
>> - text lines: offsets and bounds
>> - overall layout bounds
>> - text selection geometry
>> - strike-through geometry
>> - underline geometry
>> - caret information
>> 
>> This PR also adds the missing `strikeThroughShape()` method to complement existing `underlineShape()` and `rangeShape()` for consistency sake:
>> 
>> 
>>     /**
>>      * Returns the shape for the strike-through in local coordinates.
>>      *
>>      * @param start the beginning character index for the range
>>      * @param end the end character index (non-inclusive) for the range
>>      * @return an array of {@code PathElement} which can be used to create a {@code Shape}
>>      * @since 24
>>      */
>>     public final PathElement[] strikeThroughShape(int start, int end)
>> 
>> 
>> ## WARNING
>> 
>> Presently, information obtained via certain Text/TextField methods is incorrect with non-zero padding and borders, see [JDK-8341438](https://bugs.openjdk.org/browse/JDK-8341438).
>> 
>> It is not clear whether this PR should correct the computation at least for the new APIs and keep the existing APIs as is to be fixed later, or if the fix should be applied to both sets of APIs at the same time.
>> 
>> 
>> ## See Also
>> 
>> https://github.com/FXMisc/RichTextFX/pull/1246
>
> Andy Goryachev has updated the pull request with a new target base due to a merge or a rebase. The incremental webrev excludes the unrelated changes brought in by the merge/rebase. The pull request contains 14 additional commits since the last revision:
> 
>  - Merge remote-tracking branch 'origin/master' into ag.text.layout.api
>  - Merge remote-tracking branch 'origin/master' into ag.text.layout.api
>  - remove line spacing
>  - tests
>  - whitespace
>  - caret info
>  - text line info
>  - Merge remote-tracking branch 'origin/master' into ag.text.layout.api
>  - convert to wrapper
>  - clarify
>  - ... and 4 more: https://git.openjdk.org/jfx/compare/a528a56c...eb990081

Finished the first pass through the API, with a few more questions.

modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java line 42:

> 40: 
> 41:     /**
> 42:      * Returns the number of lines representing the caret.

Since this is in the text class, the term "lines" is ambiguous. You mean geometric lines, not text lines, right?

modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java line 44:

> 42:      * Returns the number of lines representing the caret.
> 43:      *
> 44:      * @return the number of parts representing the caret

I wouldn't mix the terms "lines" and "parts". Pick one of them and use that.

modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java line 52:

> 50:      * <p>
> 51:      * The geometry is encoded in an array of [x, ymin, ymax] values which
> 52:      * represent a line drawn from (x, ymin) to (x, ymax).

This seems overly restrictive, unless I don't understand what this does. How would this work for caret that is "caret" shaped -- `^` -- or shaped line an "I" with serifs? Presuming that specifying it with a series of geometric lines is sufficient, should it be defined as `[xmin, xmax, ymin, ymax]` instead?

modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java line 57:

> 55:      * @return the array of [x, ymin, ymax] values
> 56:      */
> 57:     public abstract double[] getLineAt(int index);

Have you considered returning a record for this?

If you did that, it might be easier for apps to consume. It would also make it easy for you to add a convenient `List<CaretInfo.Line> getLines()` method.

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java line 54:

> 52:      * <li>{@code minY} is the ascent of the first line (negative)
> 53:      * <li>{@code width} the width of the widest line
> 54:      * <li>{@code height} the sum of all lines height

Is this correct ? What about spacing between lines, or is that included in the line height of each line?

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java line 94:

> 92: 
> 93:     /**
> 94:      * Returns the geometry of the strike-through shape, as an array of {@code Rectangle2D} objects,

Same comment here (and below) as above: this is a list not an array; it should ideally be immutable.

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

PR Review: https://git.openjdk.org/jfx/pull/1596#pullrequestreview-2395417389
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816764790
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816765714
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816768652
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816758792
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816790045
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816793085