RFR: 8304831: TextFlow.hitTest.insertionIndex incorrect with surrogate pairs [v6]

[Andy Goryachev]

On Thu, 18 May 2023 09:27:18 GMT, Karthik P K <kpk at 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:
> 
>   Add new test. Optimizations to make the tests robust

please try adding firePulse(), and also please check on Windows and Linux.
thanks!

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 153:

> 151:             textFlow.getChildren().clear();
> 152:             textFlow.getChildren().setAll(text);
> 153: 

let's add Toolkit.getToolkit().firePulse(); before textSetLatch.countDown();

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 166:

> 164:             textFlow.getChildren().clear();
> 165:             textFlow.getChildren().setAll(text);
> 166: 

let's add Toolkit.getToolkit().firePulse(); before textSetLatch.countDown();

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 179:

> 177:             textFlow.getChildren().clear();
> 178:             textFlow.getChildren().setAll(text);
> 179: 

let's add Toolkit.getToolkit().firePulse(); before textSetLatch.countDown();

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 196:

> 194:             textFlow.getChildren().clear();
> 195:             textFlow.getChildren().setAll(text, emoji);
> 196: 

let's add Toolkit.getToolkit().firePulse(); before textSetLatch.countDown();

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 210:

> 208:             textFlow.getChildren().clear();
> 209:             textFlow.getChildren().setAll(text);
> 210: 

let's add Toolkit.getToolkit().firePulse(); before textSetLatch.countDown();

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 219:

> 217:     public void testTextFlowInsertionIndexUsingTwoEmojis() throws Exception {
> 218:         addTwoEmojis();
> 219:         Thread.sleep(500);

we can remove Thread.sleep() now

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 233:

> 231:     public void testTextFlowInsertionIndexUsingMultipleEmojis() throws Exception {
> 232:         addMultipleEmojis();
> 233:         Thread.sleep(500);

we can remove Thread.sleep() now

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 251:

> 249:     public void testTextFlowInsertionIndexUsingTextAndEmojis() throws Exception {
> 250:         addTextAndEmojis();
> 251:         Thread.sleep(500);

we can remove Thread.sleep() now

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 271:

> 269:     public void testTextFlowInsertionIndexUsingEmbeddedTextNodes() throws Exception {
> 270:         addTwoTextNodes();
> 271:         Thread.sleep(500);

we can remove Thread.sleep() now

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 292:

> 290:     public void testTextFlowInsertionIndexWhenMouseMovedOutsideText() throws Exception {
> 291:         addTextAndEmojis();
> 292:         Thread.sleep(500);

we can remove Thread.sleep() now

tests/system/src/test/java/test/robot/javafx/scene/TextFlowSurrogatePairInsertionIndexTest.java line 309:

> 307:     public void testTextFlowInsertionIndexUsingWrappedText() throws Exception {
> 308:         addLongText();
> 309:         Thread.sleep(500);

we can remove Thread.sleep() now

-------------

Changes requested by angorya (Reviewer).

PR Review: https://git.openjdk.org/jfx/pull/1091#pullrequestreview-1433090838
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198063741
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198063856
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198064029
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198064162
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198064333
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198064998
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198065122
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198065253
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198065402
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198065526
PR Review Comment: https://git.openjdk.org/jfx/pull/1091#discussion_r1198065732