On Thu, 16 Jan 2025 19:34:06 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: > > 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. modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java line 83: > 81: @FunctionalInterface > 82: public static interface GeometryCallback { > 83: 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? modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java line 256: > 254: * @return the caret geometry > 255: */ > 256: public float[] getCaretInf(int offset, boolean leading); Is this a typo or is there a reason for naming this method `getCaretInf` instead of `getCaretInfo`? modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java line 268: > 266: * @param start the start offset > 267: * @param end the end offset > 268: * @param the type of the geometry missing `type`after `@param` modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismCaretInfo.java line 48: > 46: @Override > 47: public Rectangle2D getSegmentAt(int index) { > 48: return parts[index]; do we need a bound check here? modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismLayoutInfo.java line 57: > 55: public Rectangle2D getBounds(boolean includeLineSpacing) { > 56: BaseBounds b = layout.getBounds(); > 57: 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? modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismLayoutInfo.java line 127: > 125: > 126: private TextLine line(int ix) { > 127: return layout.getLines()[ix]; bound check? In any case, this private method is not used. modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextUtils.java line 45: > 43: /** > 44: * Queries the range geometry of the range of text within the text > layout as > 45: * an array of {@code PathElement}s, for for one of the three > possible types: typo `for for` modules/javafx.graphics/src/main/java/com/sun/javafx/text/TextUtils.java line 49: > 47: * <li>{@link #TYPE_STRIKETHROUGH} - strike-through shape > 48: * <li>{@link #TYPE_TEXT} - text selection shape > 49: * <li>{@link #TYPE_UNDERLINE} - underline shape missing `TextLayout` before each type `#TYPE_...` 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. 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. 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). ------------- PR Review: https://git.openjdk.org/jfx/pull/1596#pullrequestreview-2593730681 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1941724308 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1941708045 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1941712304 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1941998555 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1942916469 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1942017852 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1941714592 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1941717570 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1942752033 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1942895045 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1942718447
