On Mon, 8 May 2023 12:22:17 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:
>
> Fix insertion index calculation issue with emojis. Add additional test cases
minor changes requested
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 456:
> 454:
> 455: insertionIndex = charIndex;
> 456: String txt = new String(getText());
possible NPE: looks like getText() might return null, in which case this line
will throw an NPE.
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 458:
> 456: String txt = new String(getText());
> 457: if (!leading) {
> 458: if (txt != null) {
This check should be done earlier for getText()
modules/javafx.graphics/src/main/java/com/sun/javafx/text/PrismTextLayout.java
line 461:
> 459: int next;
> 460: BreakIterator charIterator =
> BreakIterator.getCharacterInstance();
> 461: synchronized(charIterator) {
question: why do we need to synchronize on the instance here?
BreakIterator.get*Instance() creates a new instance each time, so
synchronization is probably unnecessary.
-------------
Changes requested by angorya (Committer).
PR Review: https://git.openjdk.org/jfx/pull/1091#pullrequestreview-1417028467
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1187585950
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1187586532
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1187595409
