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

[Andy Goryachev]

On Wed, 28 Feb 2024 12:18:08 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:
> 
>   Simplified approach

Tested with the MonkeyTester, using different justification (left, right, center, justify) and node orientation, for both Text and TextFlow.  Multiple Text instances, rich text, inline nodes, bidi, various line breaking scenarios.

I could not find any issues!  The new code also looks much cleaner and much, much easier to understand.  I left a few non-essential suggestions inline.

Overall, thank you @karthikpandelu and @hjohn.    Good job guys!

P.S. Tested on on macOS 14.3.1.  The results on Windows/Linux should be identical, but if we can get some testing done on these two platforms that would be nice.

modules/javafx.graphics/src/main/java/com/sun/javafx/scene/text/TextLayout.java line 212:

> 210:      *             and non-null in the case of {@link javafx.scene.text.Text}
> 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.

please remove these three parameters (text, textRunStart, curRunStart)

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

> 438:             TextRun run = null;
> 439:             x -= bounds.getMinX();
> 440:             //TODO binary search

we can remove this TODO because the new approach is incompatible with the binary search

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

> 441:             for (int i = 0; i < runs.length; i++) {
> 442:                 run = runs[i];
> 443:                 if (x < run.getWidth()) break;

I would strongly recommend placing `break`s on separate lines for convenience during debugging.

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

> 443:                 if (x < run.getWidth()) break;
> 444:                 if (i + 1 < runs.length) {
> 445:                     if (runs[i + 1].isLinebreak()) break;

same here

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

> 708:         while (index < lineCount) {
> 709:             bottom += lines[index].getBounds().getHeight() + spacing;
> 710:             if (index + 1 == lineCount) bottom -= lines[index].getLeading();

please keep one statement per line, if possible

if (index + 1 == lineCount) {
  bottom -= lines[index].getLeading();
}

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

> 709:             bottom += lines[index].getBounds().getHeight() + spacing;
> 710:             if (index + 1 == lineCount) bottom -= lines[index].getLeading();
> 711:             if (bottom > y) break;

same here

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

> 1034:         double py = y;
> 1035: 
> 1036:         if(isSpan()) {

please insert a space between `if` and `(`

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

PR Review: https://git.openjdk.org/jfx/pull/1323#pullrequestreview-1907661966
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506809039
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506822747
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506822242
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506822301
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506825343
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506825604
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1506827456