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

trafficstars

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 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.

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.

48282b1067e37092916a9f7f0edafa66a43cfb5a: 8359257: Create accessibility protocol for TabGroup component
859a308043b16382246a317e1eb9e1cb153604a7: 8358770: incubator.richtext pom missing dependency on incubator.input
3922d38fd6b2cec8d7fa9a81f279c2add6dbeeb1: 8359445: GHA: Update gradle wrapper-validation action to v4
... and 2 more: https://git.openjdk.org/jfx/compare/11f31146a6f70881945799dadad63bca56ed8a80...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

Oct 08 '24 16:10 openjdk[bot]

@andy-goryachev-oracle The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

Oct 08 '24 16:10 openjdk[bot]

@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@andy-goryachev-oracle please create a CSR request for issue JDK-8341670 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

Oct 08 '24 16:10 openjdk[bot]

Webrevs

Oct 08 '24 18:10 mlbridge[bot]

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

Oct 10 '24 17:10 andy-goryachev-oracle

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?

Oct 10 '24 19:10 andy-goryachev-oracle

@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.

Oct 11 '24 13:10 Jugen

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.

Oct 11 '24 14:10 Jugen

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.

Oct 11 '24 17:10 andy-goryachev-oracle

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.

Oct 11 '24 17:10 andy-goryachev-oracle

/issue add JDK-8341672

Oct 11 '24 22:10 andy-goryachev-oracle

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

Oct 11 '24 22:10 openjdk[bot]

/issue add JDK-8341671

Oct 14 '24 21:10 andy-goryachev-oracle

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

Oct 14 '24 21:10 openjdk[bot]

@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!

Nov 22 '24 21:11 bridgekeeper[bot]

@prrace please take a look

Nov 22 '24 21:11 andy-goryachev-oracle

@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!

Dec 20 '24 22:12 bridgekeeper[bot]

@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

Jan 16 '25 16:01 openjdk[bot]

@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!

Jan 17 '25 23:01 bridgekeeper[bot]

keep-alive

Jan 17 '25 23:01 andy-goryachev-oracle

@jperedadnr Can you review this?

Feb 04 '25 18:02 johanvos

thank you for thoughtful comments, @jperedadnr !

Feb 05 '25 21:02 andy-goryachev-oracle

@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!

Mar 06 '25 01:03 bridgekeeper[bot]

please review

Mar 06 '25 15:03 andy-goryachev-oracle

Can this class be sealed?

Theoretically, yes, but what problem would that solve?

I can understand when sealing would make the platform more secure, but in this case I see no value whatsoever. Could you give me an example where sealing the LayoutInfo class might help?

edit: I can see the value of sealing in some security context, but I really dislike how this exposes the internals via permits.

Apr 07 '25 18:04 andy-goryachev-oracle

Can this class be sealed?

Theoretically, yes, but what problem would that solve?

I can understand when sealing would make the platform more secure, but in this case I see no value whatsoever. Could you give me an example where sealing the LayoutInfo class might help?

I think a class should always be final or sealed by default, no further justification required (it's one of the defaults the Java language arguably got wrong). Justification should be given for a class to be extensible, as that will invariably add to the incompatibility budget required for future changes. Designing a class for extensibility is also usually very different from designing a sealed class, and there are many examples in JavaFX where careless extensibility leads to problems down the line.

edit: I can see the value of sealing in some security context, but I really dislike how this exposes the internals via permits.

How so? You can't do anything with non-exported permitted subclasses, so it's not leaking anything in that sense. Javadoc also doesn't show you non-exported permitted classes.

Apr 07 '25 18:04 mstr2

I think a class should always be final or sealed by default

contr-argument: testing. having extendable classes makes it easier to mock, although in the case of a platform your argument carries greater weight.

I can make it sealed.

Apr 07 '25 19:04 andy-goryachev-oracle

I'll update the CSR once the javadoc settles.

May 02 '25 17:05 andy-goryachev-oracle

/issue remove JDK-8341671

May 21 '25 22:05 andy-goryachev-oracle

jfx
jfx copied to clipboard

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

WARNING

See Also

Progress

Issues

Reviewing

Webrev

Webrevs

jfx jfx copied to clipboard

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

WARNING

See Also

Progress

Issues

Reviewing

Webrev

Webrevs

jfx
jfx copied to clipboard