On Thu, 11 Jan 2024 10:15:01 GMT, Karthik P K <k...@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
