On Mon, 15 May 2023 09:57:59 GMT, Karthik P K <k...@openjdk.org> wrote:
>> Since surrogate pairs are internally considered as 2 characters and text
>> field is null in `HitInfo` when `getInsertionIndex` is invoked from
>> `TextFlow`, wrong insertion index was returned.
>>
>> Updated code to calculate insertion index in `getHitInfo` method of
>> `PrismTextLayout` class when `hitTest` of trailing side of surrogate pair is
>> requested. Since text runs are processed in this method already, calculating
>> the insertion index in this method looks better than calculating in
>> `getInsertionIndex` of `HitInfo` method.
>> The latter approach also requires the text to be sent to `HitInfo` as
>> parameter from the `hitTest` method of `TextFlow`. If the number of `Text`
>> nodes in `TextFlow` are very large, processing all the `Text` nodes on each
>> `hitTest` method invocation might cause performance issue. Hence implemented
>> first approach.
>>
>> Added system test to validate the fix.
>
> Karthik P K has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Initialize insertion index in PrismTextLayout
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 479:
> 477: insertionIndex = charIndex;
> 478: if (!leading) {
> 479: insertionIndex += 1;
I would have initialized insertionIndex directly in all the blocks instead of
adding conditional logic, but this code does seem to produce the desired
result.
For clarify, would it be possible to avoid this additional conditional logic?
Also, do existing unit tests cover the newly modified paths?
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1194033275
