On Mon, 7 Apr 2025 19:37:27 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> Please refer to >> >> https://github.com/andy-goryachev-oracle/Test/blob/main/doc/Text/LayoutInfo.md >> >> 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 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](https://bugs.openjdk.org/browse/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](https://bugs.openjdk.org/browse/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. >> >> >> ## See Also >> >> https://github.com/FXMisc/RichTextFX/pull/1246 > > Andy Goryachev has updated the pull request incrementally with one additional > commit since the last revision: > > sealed I did a first pass review of the public API -- I didn't look at the implementation yet. modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java line 36: > 34: * as well as geometry of other shapes derived from the layout > (selection, underline, etc.). > 35: * <p> > 36: * The information obtained via this object may change after the next > layout cycle, which may come as a result Since this is a view of the layout info for the current layout pass, do you think something like the following would be clearer: "The information obtained via this object is stable until the next layout cycle. It may change after the next layout cycle, which ..." modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java line 57: > 55: * @return the layout bounds > 56: */ > 57: public abstract Rectangle2D getBounds(boolean includeLineSpacing); Should this return a `Bounds` object? modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java line 81: > 79: * @return the {@code TextLineInfo} object > 80: * @throws IndexOutOfBoundsException if the index is out of range > 81: * {@code (index < 0 || index > getTextLineCount())} Shouldn't that be ">= getTextLineCount()" ? modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 866: > 864: /** > 865: * The shape of the selection in coordinates > 866: * relative to the font base line. Why did you remove the word "local"? The term "local coordinates" has meaning that is still valid here. modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1119: > 1117: > 1118: /** > 1119: * Returns the shape for the strike-through in coordinates "local coordinates" modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 2100: > 2098: * While there is no general guarantee that successive invocations > of this method return the same instance, > 2099: * it is safe to either cache this object or call this method each > time, since the information obtained from > 2100: * this lightweight object remains valid until the next layout > cycle. I'm trying to parse this to get a better idea of the model. If this really is a "view" into the current layout information for the text node, then the information returned by the getters in the TextLayout will change over time, right? This is true whether you call the TextLayoutInfo once on a given text node and reuse it for the life of that text node or whether you call `Text::getLayoutInfo` every time you want to use it. I guess that's what you are trying to say. If so, is there a need to mention that there is no guarantee that it will return the same instance? modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 2104: > 2102: * The information obtained after the next layout cycle might be > different as a result > 2103: * of actions such as resizing of the container, or modification of > certain properties. > 2104: * For example updating the text or the font might change the > layout, but a change of color would not. This is a little ambiguous. It's the information that is obtained by the Layout info that is obtained by this`getLayout` method that might change, not the object returned by this method. So I recommend either deleting this paragraph -- the LayoutInfo object describes the information it returns -- or else clarifying that this applies to the information obtained _from the LayoutInfo ojbect_. ------------- PR Review: https://git.openjdk.org/jfx/pull/1596#pullrequestreview-2810058831 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r2070594753 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r2070640628 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r2070644149 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r2070424622 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r2070429860 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r2070589595 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r2070654997
