On Wed, 14 Feb 2024 04:27:42 GMT, John Hendrikx <jhendr...@openjdk.org> wrote:
>> Karthik P K has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Review comments
>
> modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java
> line 213:
>
>> 211: * @param textRunStart Start position of first Text run where hit
>> info is requested.
>> 212: * @param curRunStart Start position of text run where hit info is
>> requested.
>> 213: * @param forTextFlow Indicates if the hit info is requested for
>> TextFlow or Text node. {@code true} for TextFlow and {@code false} for Text
>> node.
>
> I have the impression that we're overcomplicating things here. There is a
> flag (`forTextFlow`) for Text/TextFlow, and a String (`text`) for
> Text/TextFlow, and there are already `setContent` methods for Text/TextFlow.
>
> I don't see a need for any of these changes to `getHitInfo` at all.
>
> If the content was set with `setContent(TextSpan[] spans)`, then we know it
> is a TextFlow (actually we don't care, we have spans which is the difference
> we're interested in). This fact can be checked at any time with:
>
> boolean isThisForTextFlow = this.spans != null;
>
> See how the `setContent` methods work; they either set `spans` or they don't.
> The rest of the code is already full of alternate paths with `if (spans ==
> null) { /* do Text stuff */ } else { /* do TextFlow stuff */ }`
>
> The `text` parameter is also already known, because this is explicitely set
> for `Text` nodes because they use `setContent(String text, Object font)`.
> However, before using it (specifically for `Text`), make sure that check
> `spans == null` as it is filled for `TextFlow` as well at a later point.
>
> So, I think:
> - the `text` parameter should never have been added (it wasn't until recently)
> - `forTextFlow` flag is unnecessary, just check `spans != null`.
The `getHitInfo` calculates the hit test information for the Text nodes
embedded inside a TextFlow as well. Because of this `boolean isThisForTextFlow
= this.spans != null;` is not enough.
When hit test is requested on a Text embedded in TextFlow, if we use
`getText()`, it gives the entire content rather than giving the Text node on
which hit test is requested. Since we only have x, y coordinates, I found
sending text as parameter is necessary.
We are sending the run start value and start value of the first run in the
line, but this also did not prove to be enough when we have cases such as
multiline text, emojis, RTL text embedded in LTR text, repeated texts etc. So I
have kept the `text` parameter as it is and removed `forTextFlow` parameter.
This is now calculated using following condition
`boolean forTextFlow = spans != null && text == null;`
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1494300074
