On Thu, 18 May 2023 09:27:18 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:
>
> 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
