On Wed, 5 Feb 2025 13:08:07 GMT, Jose Pereda <jper...@openjdk.org> wrote:
>> 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.
thank you for thoughtful comments, @jperedadnr !
> 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.
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.
> 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.
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.
> 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).
it was intentional decision to use the light weight `Rectangle2D` class instead
of a `Shape` (which extends `Node`).
-------------
PR Comment: https://git.openjdk.org/jfx/pull/1596#issuecomment-2638059384
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1943674678
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1943677479
PR Review Comment: https://git.openjdk.org/jfx/pull/1596#discussion_r1943679700
