RFR: 8324658: Allow animation play/start/stop/pause methods to be called on any thread [v9]

[Kevin Rushforth]

On Mon, 29 Jan 2024 03:28:51 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> Added a utility method to run code on the FX thread if it's not already, and changed the animation methods to use it.
>
> Nir Lisker has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Update tests

I left a few more inline comments, mostly on the test, but I also noted some missing `parent != null` checks.

modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 900:

> 898:      */
> 899:     public void playFrom(String cuePoint) {
> 900:         Utils.runOnFxThread(() -> playFromOnFxThread(cuePoint));

I think you need to add the check for `parent != null`.

modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 936:

> 934:      */
> 935:     public void playFrom(Duration time) {
> 936:         Utils.runOnFxThread(() -> playFromOnFxThread(time));

I think you need to add the check for `parent != null`.

modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 964:

> 962:      */
> 963:     public void playFromStart() {
> 964:         Utils.runOnFxThread(this::playFromStartOnFxThread);

I think you need to add the check for `parent != null`.

tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java line 22:

> 20:         anim.setCycleCount(Animation.INDEFINITE);
> 21: 
> 22:         while (true) {

Maybe add a comment indicating that the runnable will run until stopped by the test harness?

tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTest.java line 30:

> 28:         }
> 29:     }
> 30: }

Minor: add newline at end of file.

tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java line 24:

> 22:     public static void main(String[] args) {
> 23:         Application.launch(args);
> 24:     }

The main method is not needed.

tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java line 65:

> 63:         }
> 64: 
> 65:         waiter.await(GRACE_PERIOD, TimeUnit.SECONDS);

I don't think this will do what you want. This will return almost immediately, before any of the animation has run (as soon as the runnable that sets the uncaught exception handler finished). What I think you need is something more like this:


        Platform.runLater(() -> registerExceptionHandler());
        assertTrue(waiter.await(GRACE_PERIOD, TimeUnit.SECONDS));

        // start the 10 threads...

        Util.sleep(GRACE_PERIOD);


The await ensures that the uncaught exception handler has been registered before starting the threads. The sleep then runs the threads for a fixed amount of time.

tests/system/src/test/java/test/com/sun/javafx/animation/SynchronisityTest.java line 89:

> 87:         } catch (InterruptedException e) {
> 88:         }
> 89:     }

You can use the existing `Util.sleep` method, which already does this.

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

PR Review: https://git.openjdk.org/jfx/pull/1352#pullrequestreview-1848586078
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469551343
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469550259
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469551543
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469544786
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469560856
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469564886
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469567412
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1469546961