RFR: 8319844 : Text/TextFlow.hitTest() is incorrect in RTL orientation [v11]

[Karthik P K]

On Wed, 14 Feb 2024 04:27:42 GMT, John Hendrikx <jhendrikx at 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