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

[Kevin Rushforth]

On Fri, 19 May 2023 16:24:44 GMT, Andy Goryachev <angorya at openjdk.org> wrote:

>> Adding `Toolkit::firePulse` is not making the tests stable. In fact I'm getting new failure after adding this in `testTextFlowInsertionIndexUsingMultipleEmojis()` test.
>> 
>> TextFlowSurrogatePairInsertionIndexTest > testTextFlowInsertionIndexUsingMultipleEmojis FAILED
>>     java.lang.NullPointerException: Cannot read the array length because "runs" is null
>>         at javafx.graphics at 21-internal/javafx.scene.text.Text.getSpanBounds(Text.java:359)
>>         at javafx.graphics at 21-internal/javafx.scene.text.Text.doComputeGeomBounds(Text.java:1159)
>>         at javafx.graphics at 21-internal/javafx.scene.text.Text$1.doComputeGeomBounds(Text.java:149)
>>         at javafx.graphics at 21-internal/com.sun.javafx.scene.shape.TextHelper.computeGeomBoundsImpl(TextHelper.java:90)
>>         at javafx.graphics at 21-internal/com.sun.javafx.scene.NodeHelper.computeGeomBounds(NodeHelper.java:117)
>>         at javafx.graphics at 21-internal/javafx.scene.Node.updateGeomBounds(Node.java:3813)
>>         at javafx.graphics at 21-internal/javafx.scene.Node.getGeomBounds(Node.java:3775)
>>         at javafx.graphics at 21-internal/javafx.scene.Node.getLocalBounds(Node.java:3723)
>>         at javafx.graphics at 21-internal/javafx.scene.Node.updateTxBounds(Node.java:3877)
>>         at javafx.graphics at 21-internal/javafx.scene.Node.getTransformedBounds(Node.java:3669)
>>         at javafx.graphics at 21-internal/javafx.scene.Node.updateBounds(Node.java:777)
>>         at javafx.graphics at 21-internal/javafx.scene.Parent.updateBounds(Parent.java:1834)
>>         at javafx.graphics at 21-internal/javafx.scene.Scene$ScenePulseListener.pulse(Scene.java:2615)
>>         at javafx.graphics at 21-internal/com.sun.javafx.tk.Toolkit.lambda$runPulse$2(Toolkit.java:401)
>>         at java.base/java.security.AccessController.doPrivileged(AccessController.java:399)
>>         at javafx.graphics at 21-internal/com.sun.javafx.tk.Toolkit.runPulse(Toolkit.java:400)
>>         at javafx.graphics at 21-internal/com.sun.javafx.tk.Toolkit.firePulse(Toolkit.java:430)
>>         at test.robot.javafx.scene.TextFlowSurrogatePairInsertionIndexTest.testTextFlowInsertionIndexUsingMultipleEmojis(TextFlowSurrogatePairInsertionIndexTest.java:233)
>> 
>> Could increasing the delay time is the only solution now for the test failure seen previously and then we follow up with the approach suggested by @kevinrushforth ?
>> 
>> @andy-goryachev-oracle  could you please help me in understanding how `Util.runAndWait(() -> { });` help in this case? 
>> Also I'm using the...
>
>> could you please help me in understanding how `Util.runAndWait(() -> { });` help in this case
> 
> After discussing the issue with @kevinrushforth it probably won't.
> 
> The idea was to replace Thread.sleep() with something that can work reliably even when the process takes a long time (i.e. cold boot).  Simply increasing the sleep time to 1000 ms may not be sufficient, I think.
> 
> What we need is an equivalent of java.awt.Robot.waitForIdle() which we don't have in FX.  The problem here is that all the processing involving CSS, layout may take several pulses, and it certainly not guaranteed to be over when the next event is processed in `Util.runAndWait()`.
> 
> We could still use Toolkit.firePulse(), but this apparently is a hack, and it alters the normal control flow - that is something we are trying to avoid.
> 
> Another variant is to add something like that to Util and use that in place of Thread.sleep().  This method will trigger and wait for an arbitrary number of pulses (currently 10, but we can pick any reasonable number):
> 
> 
>     /**
>      * Triggers and waits for 10 pulses to complete in the specified scene.
>      */
>     public static void waitForIdle(Scene scene) {
>         int count = 10;
>         CountDownLatch latch = new CountDownLatch(count);
>         Runnable pulseListener = () -> {
>             latch.countDown();
>             Platform.requestNextPulse();
>         };
> 
>         runAndWait(() -> {
>             scene.addPostLayoutPulseListener(pulseListener);
>         });
> 
>         try {
>             Platform.requestNextPulse();
>             waitForLatch(latch, 30, "waitForIdle timeout");
>         } finally {
>             runAndWait(() -> {
>                 scene.removePostLayoutPulseListener(pulseListener);
>             });
>         }
>     }
> 
> 
> I am not sure why we have Scene.addPulseListener() and not a static equivalent of Robot.waitForIdle(), but here is some earlier work on the pulse listener:
> 
> https://bugs.openjdk.org/browse/JDK-8097917

@andy-goryachev-oracle Do you think this needs a second reviewer, or are you satisfied that a single review is sufficient?

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

PR Comment: https://git.openjdk.org/jfx/pull/1091#issuecomment-1557471464