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

[Andy Goryachev]

On Wed, 17 Jan 2024 10:54:46 GMT, Karthik P K <kpk at openjdk.org> wrote:

>> In the `getHitInfo()` method of PrismTextLayout, RTL node orientation conditions were not considered, hence hit test values such as character index and insertion index values were incorrect.
>> 
>> Added checks for RTL orientation of nodes and  fixed the issue in `getHitInfo()` to calculate correct hit test values.
>> 
>> Added system tests to validate the changes.
>
> Karthik P K has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Fix multiline text insertion index calculation issue

Noticed a problem - on Windows 11 and on macOS 14.2.1
hit into shows different values for Text and TextFlow.  This issue can be seen with pretty much every text, even "aaa\nbbb\ncccc".  In this screenshot, the line corresponds to the mouse position:

![Screenshot 2024-01-17 at 11 42 03](https://github.com/openjdk/jfx/assets/107069028/36680d90-520f-4899-aa08-ba93d524da8f)

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. True for TextFlow and False for Text node.

technically, it's "true" and "false", e.g.
`{@code true}`

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 430:

> 428:         int insertionIndex = -1;
> 429:         int relIndex = 0;
> 430:         int LTRIndex = 0;

minor: it's a bit confusing to have a local var start with an uppercase letter.  maybe 'ltrIndex' ?

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 453:

> 451:                 if (isMirrored) {
> 452:                     int runIndex = -1;
> 453:                     for (int i = runs.length - 1; i >=0; i--) {

please add a space after `i >= `

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 510:

> 508:                         if (run.getTextSpan() != null && run.getTextSpan().getText().equals(text)) {
> 509:                             if ((x > run.getWidth() && !isMultiRunText) || textWidthPrevLine > 0) {
> 510:                                 BaseBounds textBounds = new BoxBounds();

here (and on line 544) we allocate `textBounds` inside the for loop.
Maybe we should allocate one before line 494 and use that instance in both for loops to avoid gc pressure?
The downside is that in some cases it may not be used, but I feel it'll better since alloc is cheap.

modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java line 521:

> 519:                             }
> 520:                             for (int j = runs.length - 1; j >= 0; j--) {
> 521:                                 if (runs[j].getTextSpan().getText().equals(text) && runs[j].getStart() != curRunStart) {

minor optimization: do the int comparison first, equals second

modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1062:

> 1060:             } else {
> 1061:                 double ptX = localToParent(x, y).getX();
> 1062:                 double ptY = localToParent(x, y).getY();

localToParent(x, y) computed twice, could we avoid that?

tests/system/src/test/java/test/robot/javafx/scene/RTLTextFlowCharacterIndexTest.java line 274:

> 272:         while (x > X_LEADING_OFFSET) {
> 273:             moveMouseOverTextFlow(x, Y_OFFSET);
> 274:            if (isLeading) {

indentation?

-------------

PR Comment: https://git.openjdk.org/jfx/pull/1323#issuecomment-1896553605
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456279189
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456284396
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456341154
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456297979
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456299608
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456305703
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1456326090