On Thu, 29 Feb 2024 05:38:05 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:
>
> Review comments
Please add a JUnit test. Example included.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1031:
> 1029: if (runs.length != 0) {
> 1030: textRunStart = findFirstRunStart(runs);
> 1031: }
Can you rewrite this to be:
int textRunStart = findFirstRunStart();
The `runs` parameter doesn't need to be passed (`findFirstRunStart` can access
it just as easily), and the length check can be moved inside that method as
well.
modules/javafx.graphics/src/main/java/javafx/scene/text/Text.java line 1049:
> 1047: for (GlyphList r: runs) {
> 1048: if (((TextRun) r).getStart() < start) {
> 1049: start = ((TextRun) r).getStart();
minor: you can extract a new local here instead of casting and calling
`getStart` twice.
-------------
Changes requested by jhendrikx (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1323#pullrequestreview-1908709396
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1507478198
PR Review Comment: https://git.openjdk.org/jfx/pull/1323#discussion_r1507480047
