On Tue, 9 May 2023 05:10:25 GMT, Karthik P K <[email protected]> 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:
>
> Address code review
Testing looks good so far.
There is one question whether it's possible to always initialize
HitInfo.insertionIndex and remove the secondary computation from
HitInfo.getInsertionIndex()
I am not clear under which conditions it is not possible to initialize, and
whether subsequent HitInfo.getInsertionIndex() would produce a meaningful
result.
Perhaps in a separate RFE.
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 425:
> 423: public Hit getHitInfo(float x, float y) {
> 424: int charIndex = -1;
> 425: int insertionIndex = -1;
[question]
Currently, there are a few scenarios when a negative insertionIndex is passed
down to HitInfo. This will trigger a similar (and probably incorrect)
computation of the insertion index in HitInfo, see for example
[JDK-8302511](https://bugs.openjdk.org/browse/JDK-8302511).
My question is - should we instead resolve the insertion index always?
-------------
PR Review: https://git.openjdk.org/jfx/pull/1091#pullrequestreview-1419212526
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1188972363
