On Fri, 12 Jan 2024 16:14:20 GMT, Andy Goryachev <ango...@openjdk.org> wrote:
>> 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 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?
The first code branch is used to calculate hit info of Text node which are not
embedded in TextFlow and hit info requested on TextFlow, where as the second
code branch is used to calculate hit info of Text node embedded in TextFlow.
Updated the comments.
> 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?
Yes, it handles the LTR text present in RTL mode. Rephrased the comment.
> 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;
> }
Added comment and used bit logic in the condition.
Do you think we should create a method in TextRun? I believe it is out of scope
of this PR as it will be used in other functions as well.
> 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?
Refactored the function. Changed `runIndex` to `ix`.
I could remove some repetition of `runs[runIndex]`. Had to keep some instances
since previous or next runs are used in some cases.
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1453234155
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1453235252
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1453237200
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1453239916
