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

[Andy Goryachev]

On Wed, 5 Feb 2025 13:08:07 GMT, Jose Pereda <jpereda at openjdk.org> wrote:

>> Andy Goryachev has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   25 25
>
> Overall it looks really good, the new API is quite useful in some scenarios. I've done some successful tests and left some comments and questions about the API.

thank you for thoughtful comments, @jperedadnr !

> modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java line 91:
> 
>> 89:      * @return the immutable list of {@code Rectangle2D} objects
>> 90:      */
>> 91:     public abstract List<Rectangle2D> selectionShape(int start, int end, boolean includeLineSpacing);
> 
> See my comment below about using `javafx.scene.shape.Rectangle` instead of `javafx.geometry.Rectangle2D`: otherwise the API is misleading: `selectionShape` doesn't return a `Shape` subclass.

Good point.  The word 'shape' might be confusing, although it is still correct in human terms (it is a shape, after all).  I think it's ok, but we could use the word "geometry" for these methods `selectionGeometry` / `strikeThroughGeometry` / `underlineGeometry`, what do other people think?

As for using `javafx.scene.shape.Rectangle` - it is too heavy of an object, with properties and everything, it's not what we ask from this method.

> modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java line 91:
> 
>> 89:      * @return the immutable list of {@code Rectangle2D} objects
>> 90:      */
>> 91:     public abstract List<Rectangle2D> selectionShape(int start, int end, boolean includeLineSpacing);
> 
> I take that  `LayoutInfo::selectionShape` should match the existing API `TextFlow::rangeShape` for the same selection coordinates.
>  
> I wonder if you have tested this, with different insets. I take that with your current implementation, for `Rectangle2D` objects, it makes sense to have the insets of the TextFlow/Text node, but shapes don't include them.

We've got https://bugs.openjdk.org/browse/JDK-8341438 and a possibility of a regression if we change the existing methods.

I would very much like to get your thoughts on this.

> modules/javafx.graphics/src/main/java/javafx/scene/text/TextLineInfo.java line 56:
> 
>> 54:  * @since 25
>> 55:  */
>> 56: public record TextLineInfo(int start, int end, Rectangle2D bounds) {
> 
> I understand that you are using `javafx.geometry.Rectangle2D` for this PR to hold all caret/layout/line information related to bounds, and that seems to work fine so far.
> 
> However, given that the `TextFlow`/`Text` APIs already provide information using `javafx.scene.shape.PathElement`, and given that as a result of this PR users will be able to query either old and new APIs, I was wondering if another `Shape` subclass might be better for the new API instead of `Rectangle2D`: As is now, you can't easily combine `Rectangle2D` with `PathElement` objects (in fact you need to do a manual conversion in `TextUtils::getRange` or `TextUtils::toRectangle2D`).
> 
> Have you thought about using `javafx.scene.shape.Rectangle` instead? That would simplify your internal operations, and would be more easy to use for developers (for instance, drawing new shapes based the LayoutInfo, like in your MonkeyTester `LayoutInfoVisualizer` class).

it was intentional decision to use the light weight `Rectangle2D` class instead of a `Shape` (which extends `Node`).

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

PR Comment: https://git.openjdk.org/jfx/pull/1596#issuecomment-2638059384
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1943674678
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1943677479
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1943679700