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