On Fri, 19 May 2023 16:24:44 GMT, Andy Goryachev <ango...@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@21-internal/javafx.scene.text.Text.getSpanBounds(Text.java:359)
>> at
>> javafx.graphics@21-internal/javafx.scene.text.Text.doComputeGeomBounds(Text.java:1159)
>> at
>> javafx.graphics@21-internal/javafx.scene.text.Text$1.doComputeGeomBounds(Text.java:149)
>> at
>> javafx.graphics@21-internal/com.sun.javafx.scene.shape.TextHelper.computeGeomBoundsImpl(TextHelper.java:90)
>> at
>> javafx.graphics@21-internal/com.sun.javafx.scene.NodeHelper.computeGeomBounds(NodeHelper.java:117)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.updateGeomBounds(Node.java:3813)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.getGeomBounds(Node.java:3775)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.getLocalBounds(Node.java:3723)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.updateTxBounds(Node.java:3877)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.getTransformedBounds(Node.java:3669)
>> at
>> javafx.graphics@21-internal/javafx.scene.Node.updateBounds(Node.java:777)
>> at
>> javafx.graphics@21-internal/javafx.scene.Parent.updateBounds(Parent.java:1834)
>> at
>> javafx.graphics@21-internal/javafx.scene.Scene$ScenePulseListener.pulse(Scene.java:2615)
>> at
>> javafx.graphics@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@21-internal/com.sun.javafx.tk.Toolkit.runPulse(Toolkit.java:400)
>> at
>> javafx.graphics@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
