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

[Andy Goryachev]

On Mon, 4 Mar 2024 11:01:09 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:
> 
>   Add unit test

a couple of very minor comments, I am ok with pushing this code as is.

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

> 106:             if (obj == null)
> 107:                 return false;
> 108:             if (getClass() != obj.getClass())

any reason why this code uses getClass() != obj.getClass() ?

perhaps a better choice might be the usual pattern

if(x == this) {
 return true;
} else if(x instanceof Hit h) {
 return charIndex == h.charIndex && ...
}
return false;

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

> 1042:     private int findFirstRunStart() {
> 1043:         int start = Integer.MAX_VALUE;
> 1044:         for (GlyphList r: getRuns()) {

the old code had a 0 check for getRuns.length, presumably to avoid the iterator creation.
the new code is probably fine, because most of the time we'll have the need for the iterator anyway.

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

Marked as reviewed by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1323#pullrequestreview-1914727360
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1511416507
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1511420549