8341670: [Text,TextFlow] Public API for Text Layout Info #1596

andy-goryachev-oracle · 2024-10-08T16:07:54Z

Please refer to

https://github.com/andy-goryachev-oracle/Test/blob/main/doc/Text/LayoutInfo.md

The RichTextArea control (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 25
     */
    public final LayoutInfo getLayoutInfo()

The LayoutInfo provides a view into the text layout within Text/TextFlow nodes such as:

caret information
text lines: offsets and bounds
overall layout bounds
text selection geometry
strike-through geometry
underline geometry

This PR also adds the missing strikeThroughShape() method to complement the 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 25
     */
    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.

This PR provides correct answers in the new API, leaving the behavior of the existing methods unchanged (there is a compatibility risk associated with trying to fix JDK-8341438 ).

Testing

The new APIs can be visually tested using the Monkey Tester on this branch:
https://github.com/andy-goryachev-oracle/MonkeyTest/tree/text.layout.api

in the Text and TextFlow pages:

Two very basic headful tests have been added.

Why an interface ? It is easier to evolve a class when you want to add more info.
Also it should be sealed unless you can show why it is important that
applications can create instances.

It is being exposed for the benefit of RT but the class doc itself doesn't say when you might want to get one and why.
It might also benefit from a succinct description of what "the text layout" means and encompasses.

Oh and how do you know when it is no longer valid ?

good points, thanks! please let me know if the update addressed your concerns.

a couple of notes:

converted to an abstract class. I feel weird referring to an internal implementation at the API level (public sealed abstract class LayoutInfo permits com.sun.javafx.text.PrismLayoutInfo, yikes!)

why we get it: we don't explain why we need HitInfo or caret shapes or range shapes, do we?

prrace · 2024-10-09T19:30:39Z

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java

+/**
+ * Represents an immutable snapshot of certain aspects of the text layout
+ * in a {@code Text} or {@code TextFlow} node,
+ * such as break up of the text into lines and the their bounds.


"the their"

prrace · 2024-10-09T19:44:30Z

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java

+ * such as break up of the text into lines and the their bounds.
+ * <p>
+ * The snapshot is valid until the layout changes due to any change that
+ * triggers that, such as resizing of the container or modification of properties.


"until the layout changes due to any change that triggers that"

it's not wrong, but writing change twice reads a bit off.
How about

The layout snapshot is no longer valid after actions such as resizing of the container, or modification of certain properties.
For example updating the text or the font would invalidate the layout snapshot, but a change of color would not.

I still don't see how applications can KNOW this has happened.
There's nothing on the (immutable) layout which says its invalidated.
There's no property to monitor.
So people will in effect never be able to do anything but use it and toss it and get a new one every time.
Perhaps you can make it 'cheap' to do this.
Since it is immutable, the Text or TextFlow can cache it.
If it is invalidated, the Text or TextFlow should know and can flush its cached version.
So the SAME immutable instance can be returned over and over again until that happens.

People can then also use "==" or equals() to check this and save themselves validation too.
I notice you don't have equals() so probably that is needed anyway to save people from manual comparison .. also an evolution issue if the class is updated.

A consequence of that is then the text about invalidation can be updated with advice on doing a simple equals() call if they need to know if it changed. And equals will start with "==" so will be really fast to return true ..

thank you!

I am struggling with how to explain that it should neither be cached, nor compared to another snapshot.

Maybe an example would help. Use case is an editable rich text component based on TextFlow. The user presses HOME and the caret needs to go to the beginning of the line. We get the LayoutInfo, determine which line the caret is currently at and determine the start offset of that line to move the caret to.

Another example: in Text, to determine whether the mouse is clicked inside of the text or beyond the last character (see JDK-8091012 ).

We are not expecting the application to monitor the text layout, so no notifications or observable properties or equals() are needed.

(I've added some words to that effect in Text/TextFlow.getLayoutInfo() )

Clients will usually obtain a LayoutInfo in order to respond to some user action or an update of the target node.
LayoutInfo should be considered an ephemeral snapshot, to be used and immediately discarded.
Clients should always obtain a new one when they need to respond again as likely the trigger to respond again means the target node has changed.

Although I don't see the harm in being more efficient.

I hear a suggestion to create a think wrapper/proxy instead of the snapshot, right?

This might, in fact, be a better idea. The API stays the same.
Let me think.

Thanks!

andy-goryachev-oracle · 2024-10-10T17:16:57Z

@Jugen do you think this PR addresses the issue https://bugs.openjdk.org/browse/JDK-8091012 ?

andy-goryachev-oracle · 2024-10-10T19:23:47Z

One additional thought/question:

There is a need to extract a few more data points from the text layout in addition to the text lines, specifically:

bounds of the (selection) range, as rectangles
strikethrough range shape (as PathElement[] and/or rectangles)
underline range shape (as PathElement[] and/or rectangles)

We currently have Text/TextFlow.underlineShape(int,int), .rangeShape(int,int) which return PathElement[] in an undocumented way.

There is no corresponding .strikeThroughShape() method, or a way to retrieve the geometric bounds as lines/rectangles.

Granted, one can attempt to analyze the PathElement[] and convert it to geometry, but it seems we should avoid asking the clients to do that.

So the question are:

should we add these data points to LayoutInfo
if so, in what form? i.e. as individual methods or as an array of Line/Rectangle objects,
should we add the .strikeThroughShape() to Text/TextFlow, or simply adding it here should be sufficient?

Jugen · 2024-10-11T13:57:02Z

@andy-goryachev-oracle First off thanks for all your work on the richtext control you're working on.

@Jugen do you think this PR addresses the issue https://bugs.openjdk.org/browse/JDK-8091012 ?

Unfortunately I don't think this helps for the hit problem Tomas was having, see hit(x,y) in RichTextFX's TextFlowExt for the relevant code.

However I was able to use all the methods in the LayoutInfo interface of this PR in RichTextFX to replace the custom layout info code, which is great.

Jugen · 2024-10-11T14:22:27Z

Wrt to rangeShape & underlineShape: RichTextFX quite successfully uses the PathElement[] returned to it's advantage and so wouldn't gain from Rectangles.

.strikeThroughShape() should be added to Text/TextFlow for consistency sake.

andy-goryachev-oracle · 2024-10-11T17:03:58Z

thank you so much @Jugen for checking and the feedback!

I see that a lot of what is being done in TextFlowExt should become unnecessary with the new API (or maybe even the whole thing might become unnecessary).

Also, while the new API do not address JDK-8091012 directly, that is, there is no field added to HitInfo, it looks like the information provided by the LayoutInfo should be sufficient to implement what you guys need as a utility, since you will have the geometry of text lines in the layout, right?

Anyway, thank you. I agree with you that we should add T/TF.strikeThroughShape() for consistency sake.

andy-goryachev-oracle · 2024-10-11T17:15:43Z

Thank you you all @prrace @Jugen @kevinrushforth for the feedback!
I am going to take this PR back to draft to make the suggested changes.

andy-goryachev-oracle · 2024-10-11T22:27:46Z

/issue add JDK-8341672

openjdk · 2024-10-11T22:28:11Z

@andy-goryachev-oracle
Adding additional issue to issue list: 8341672: [Text/TextFlow] getRangeInfo.

bridgekeeper · 2024-11-22T21:43:20Z

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle · 2024-11-22T21:45:48Z

@prrace please take a look

bridgekeeper · 2024-12-20T22:07:35Z

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

openjdk · 2025-01-16T16:08:17Z

@andy-goryachev-oracle this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout ag.text.layout.api
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

bridgekeeper · 2025-01-17T23:43:41Z

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle · 2025-01-17T23:46:01Z

keep-alive

johanvos · 2025-02-04T18:19:58Z

@jperedadnr Can you review this?

jperedadnr

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.

jperedadnr · 2025-02-04T18:33:46Z

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java

+     * @param leading whether the caret is biased on the leading edge of the character
+     * @return the caret geometry
+     */
+    public float[] getCaretInf(int offset, boolean leading);


Is this a typo or is there a reason for naming this method getCaretInf instead of getCaretInfo?

jperedadnr · 2025-02-04T18:37:36Z

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java

+     *
+     * @param start the start offset
+     * @param end the end offset
+     * @param the type of the geometry


missing typeafter @param

jperedadnr · 2025-02-04T18:39:31Z

modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextUtils.java

+public final class TextUtils {
+    /**
+     * Queries the range geometry of the range of text within the text layout as
+     * an array of {@code PathElement}s, for for one of the three possible types:


typo for for

jperedadnr · 2025-02-04T18:41:50Z

modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextUtils.java

+     * <li>{@link #TYPE_STRIKETHROUGH} - strike-through shape
+     * <li>{@link #TYPE_TEXT} - text selection shape
+     * <li>{@link #TYPE_UNDERLINE} - underline shape


missing TextLayout before each type #TYPE_...

jperedadnr · 2025-02-04T18:47:14Z

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java

+    /** Callback to be called for each rectangular shape */
+    @FunctionalInterface
+    public static interface GeometryCallback {
+        public void addRectangle(float left, float top, float right, float bottom);


I understand that you use float here because all the calculations in PrismTextLayout::getRange use floats (from TextRun).

However, the calculations down the line to generate the Rectangle2D mix floats and doubles without any casting (TextUtils::getRange with implicit casts from double to float, PrismLayoutInfo::getGeometry with float and double sums).

Do you think we could use doubles here instead?

the use of float in this interface is correct, but the code in TextUtils::getRange is bad, and i should feel bad. good catch!

jperedadnr · 2025-02-04T23:09:51Z

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismLayoutInfo.java

+    }
+
+    private TextLine line(int ix) {
+        return layout.getLines()[ix];


bound check? In any case, this private method is not used.

jperedadnr · 2025-02-05T11:42:25Z

modules/javafx.graphics/src/main/java/javafx/scene/text/TextLineInfo.java

+ *
+ * @since 25
+ */
+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).

jperedadnr · 2025-02-05T12:04:14Z

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java

+     * @param includeLineSpacing determines whether the result includes the line spacing
+     * @return the immutable list of {@code Rectangle2D} objects
+     */
+    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.

jperedadnr · 2025-02-05T12:53:36Z

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java

+     * @param includeLineSpacing determines whether the result includes the line spacing
+     * @return the immutable list of {@code Rectangle2D} objects
+     */
+    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.

jperedadnr · 2025-02-05T13:07:50Z

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismLayoutInfo.java

+    @Override
+    public Rectangle2D getBounds(boolean includeLineSpacing) {
+        BaseBounds b = layout.getBounds();
+        Insets m = insets();


See my comments below about adding the insets to the layout information. Maybe we could also add another boolean includeInsets for this?

Good question! I don't think this is needed: the call returns the layout bounds (in the owning node coordinate system).

The insets only change the position of the layout within the owning node, unlike lineSpacing which actually changes the shape by adding or removing the last line spacing.

I think this method is ok as is.

andy-goryachev-oracle · 2025-02-05T21:25:45Z

thank you for thoughtful comments, @jperedadnr !

bridgekeeper · 2025-03-06T01:58:39Z

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

andy-goryachev-oracle · 2025-03-06T15:37:49Z

please review

mstr2 · 2025-03-08T08:22:57Z

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismCaretInfo.java

+    @Override
+    public String toString() {
+        StringBuilder sb = new StringBuilder();
+        sb.append("PrismCaretInfo{parts=[");


I think you shouldn't leak implementation details via the toString() method (the name of the implementing class, as well as parts=, which is not a term that appears in the API). Maybe just drop PrismCaretInfo{parts= entirely.

makes sense.

On a related note, we don't seem to have a consistent standard for toString() output (probably no one does!), so maybe it's worth polling the devs about it.

Standardized log output might be useful when one has to go through masses of old/transient logs, or when scraping the logs for analytics. A JSON output might be useful since it offers a cheap structured form, although it suffers from two issues: verbosity and difficulty to encode binary data and object types easily.

Any ideas?

mstr2 · 2025-03-08T08:23:41Z

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java

+     * @param leading whether the caret is biased on the leading edge of the character
+     * @return the caret geometry
+     */
+    public float[] getCaretGeometry(int offset, boolean leading);


Does this method throw any exceptions? If so, please specify.

This method does not throw any exceptions, so none are specified.

mstr2 · 2025-03-08T08:24:08Z

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java

+     * @param type the type of the geometry
+     * @param client the callback to invoke for each rectangular shape
+     */
+    public void getRange(int start, int end, int type, GeometryCallback client);


Does this method throw any exceptions?

mstr2 · 2025-03-08T08:29:19Z

modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java

+ *
+ * @since 25
+ */
+public abstract class LayoutInfo {


Any exceptions thrown by methods of this class?

only public abstract TextLineInfo getTextLine(int index, boolean includeLineSpacing);

(added)

mstr2 · 2025-03-10T17:04:22Z

modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java

@@ -56,6 +56,8 @@ protected CaretInfo() {
     *
     * @param index the line index
     * @return the bounds of the caret segment
+     * @throws IndexOutOfBoundsException if the index is outside of the range if the index is out of range


I think this sentence repeats the same thing twice, maybe check again. You can probably just say if the index is out of range {@code (...)}.

daylight saving time strikes!
thanks for noticing.

draft api

285db28

openjdk bot added the csr Need approved CSR to integrate pull request label Oct 8, 2024

bounds

43ac0e1

andy-goryachev-oracle marked this pull request as ready for review October 8, 2024 18:16

openjdk bot added the rfr Ready for review label Oct 8, 2024

prrace reviewed Oct 9, 2024

View reviewed changes

andy-goryachev-oracle added 2 commits October 9, 2024 12:13

review comments

21dbe6c

javadoc

04c4abb

prrace reviewed Oct 9, 2024

View reviewed changes

andy-goryachev-oracle added 2 commits October 9, 2024 13:41

clarify

6869c76

convert to wrapper

2009a7e

andy-goryachev-oracle marked this pull request as draft October 11, 2024 17:15

openjdk bot removed the rfr Ready for review label Oct 11, 2024

andy-goryachev-oracle added 2 commits October 11, 2024 10:33

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

6b7b079

text line info

c2e26d5

andy-goryachev-oracle marked this pull request as ready for review October 11, 2024 22:29

openjdk bot added the rfr Ready for review label Oct 11, 2024

kevinrushforth requested review from kevinrushforth and prrace November 26, 2024 17:58

andy-goryachev-oracle added 2 commits December 4, 2024 15:15

Merge branch 'master' into ag.text.layout.api

1dd2b0c

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

2a3a754

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

c1c46ab

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Jan 16, 2025

Merge branch 'master' into ag.text.layout.api

7d656df

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Jan 16, 2025

25 25

0ab5890

jperedadnr reviewed Feb 5, 2025

View reviewed changes

andy-goryachev-oracle added 2 commits February 5, 2025 12:19

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

d81f65d

review comments

d688d74

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

0f94efd

mstr2 reviewed Mar 8, 2025

View reviewed changes

andy-goryachev-oracle added 3 commits March 10, 2025 08:50

Merge remote-tracking branch 'origin/master' into ag.text.layout.api

3260b8e

review comments

0ad6f66

tests

9d499c1

mstr2 reviewed Mar 10, 2025

View reviewed changes

twice

b7033cf

+               *
+               * @since 24
+               */
+              public interface LayoutInfo {

8341670: [Text,TextFlow] Public API for Text Layout Info #1596

Are you sure you want to change the base?

8341670: [Text,TextFlow] Public API for Text Layout Info #1596

Conversation

andy-goryachev-oracle commented Oct 8, 2024 • edited by openjdk bot Loading

WARNING

Testing

See Also

Progress

Issues

Reviewing

bridgekeeper bot commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

openjdk bot commented Oct 8, 2024

mlbridge bot commented Oct 8, 2024 • edited Loading

Webrevs

prrace Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prrace Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Oct 10, 2024

andy-goryachev-oracle commented Oct 10, 2024

Jugen commented Oct 11, 2024

Jugen commented Oct 11, 2024

andy-goryachev-oracle commented Oct 11, 2024

andy-goryachev-oracle commented Oct 11, 2024

andy-goryachev-oracle commented Oct 11, 2024

openjdk bot commented Oct 11, 2024

bridgekeeper bot commented Nov 22, 2024

andy-goryachev-oracle commented Nov 22, 2024

bridgekeeper bot commented Dec 20, 2024

openjdk bot commented Jan 16, 2025

bridgekeeper bot commented Jan 17, 2025

andy-goryachev-oracle commented Jan 17, 2025

johanvos commented Feb 4, 2025

jperedadnr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Feb 5, 2025

bridgekeeper bot commented Mar 6, 2025

andy-goryachev-oracle commented Mar 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andy-goryachev-oracle commented Oct 8, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Oct 8, 2024 •

edited

Loading

prrace Oct 9, 2024 •

edited

Loading

prrace Oct 9, 2024 •

edited

Loading