On Thu, 24 Oct 2024 22:45:28 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 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 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](https://bugs.openjdk.org/browse/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. >> >> >> ## See Also >> >> https://github.com/FXMisc/RichTextFX/pull/1246 > > Andy Goryachev has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 14 additional > commits since the last revision: > > - Merge remote-tracking branch 'origin/master' into ag.text.layout.api > - Merge remote-tracking branch 'origin/master' into ag.text.layout.api > - remove line spacing > - tests > - whitespace > - caret info > - text line info > - Merge remote-tracking branch 'origin/master' into ag.text.layout.api > - convert to wrapper > - clarify > - ... and 4 more: https://git.openjdk.org/jfx/compare/a528a56c...eb990081 Finished the first pass through the API, with a few more questions. modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java line 42: > 40: > 41: /** > 42: * Returns the number of lines representing the caret. Since this is in the text class, the term "lines" is ambiguous. You mean geometric lines, not text lines, right? modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java line 44: > 42: * Returns the number of lines representing the caret. > 43: * > 44: * @return the number of parts representing the caret I wouldn't mix the terms "lines" and "parts". Pick one of them and use that. modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java line 52: > 50: * <p> > 51: * The geometry is encoded in an array of [x, ymin, ymax] values which > 52: * represent a line drawn from (x, ymin) to (x, ymax). This seems overly restrictive, unless I don't understand what this does. How would this work for caret that is "caret" shaped -- `^` -- or shaped line an "I" with serifs? Presuming that specifying it with a series of geometric lines is sufficient, should it be defined as `[xmin, xmax, ymin, ymax]` instead? modules/javafx.graphics/src/main/java/javafx/scene/text/CaretInfo.java line 57: > 55: * @return the array of [x, ymin, ymax] values > 56: */ > 57: public abstract double[] getLineAt(int index); Have you considered returning a record for this? If you did that, it might be easier for apps to consume. It would also make it easy for you to add a convenient `List<CaretInfo.Line> getLines()` method. modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java line 54: > 52: * <li>{@code minY} is the ascent of the first line (negative) > 53: * <li>{@code width} the width of the widest line > 54: * <li>{@code height} the sum of all lines height Is this correct ? What about spacing between lines, or is that included in the line height of each line? modules/javafx.graphics/src/main/java/javafx/scene/text/LayoutInfo.java line 94: > 92: > 93: /** > 94: * Returns the geometry of the strike-through shape, as an array of > {@code Rectangle2D} objects, Same comment here (and below) as above: this is a list not an array; it should ideally be immutable. ------------- PR Review: https://git.openjdk.org/jfx/pull/1596#pullrequestreview-2395417389 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816764790 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816765714 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816768652 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816758792 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816790045 PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1816793085
