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

[Andy Goryachev]

On Thu, 11 Jan 2024 10:15:01 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:
> 
>   Code review changes

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

> 472:                         if (x < run.getWidth()) break;
> 473:                         if (i + 1 < runs.length) {
> 474:                             if (runs[i + 1].isLinebreak()) break;

could we surround break; with { }'s here and on lines 461, 472, 474 please?

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

> 478:                 }
> 479:             } else {
> 480:                 // To calculate hit info of Text embedded in TextFlow

the comments on lines 480 and 451 are a bit confusing: both refer to Text.  could you please clarify?

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

> 490:                     for (TextRun r: runs) {
> 491:                         if (r.getStart() != curRunStart && r.getTextSpan().getText().equals(text)) {
> 492:                             isMultiRunText = true;

minor: we could reduce the scope of `isMultiRunText` by moving its declaration from line 427 to line 490

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

> 522:                             break;
> 523:                         }
> 524:                         // For only English Text embedded in TextFlow in RTL orientation

this comment seems misleading (by English you mean LTR, right?)
would it be possible to re-phrase, explaining why?  
does it mean the RTL text has been handled by the previous code block and now we are dealing with LTR?

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

> 560:                     charIndex += textWidthPrevLine;
> 561:                     charIndex += relIndex;
> 562:                     if (run.getLevel() % 2 != 0) {

I wish there was an explanation of the meaning of `level`
And since there are several places where it checks for it being odd, I wish there was a method in TextRun with a descriptive name rather than this computation (and bit logic might be faster):


public boolean isLevelOdd() { // or whatever the meaning is
  return (level & 0x01) != 0; 
}

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

> 1038: 
> 1039:     private int findRunIndex(double x, double y, GlyphList[] runs) {
> 1040:         int runIndex = 0;

some general comment for this method:
1. there are many places where runs[runIndex] is repeated within the same code block, I wonder if it would make sense to create a local variable.  It might be tricky because the runIndex changes mid-flight.
2. perhaps `runIndex` could be shortened to `ix` to make the lines shorter?

tests/system/src/test/java/test/robot/javafx/scene/RTLTextCharacterIndexTest.java line 111:

> 109:     static Random random;
> 110:     static Robot robot;
> 111:     static Text textOne;

maybe rename to 'text' since there is only one instance?  or `control`?

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450655360
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450660028
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450676811
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450680153
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450687339
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450691638
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1450693272