On Mon, 29 Jan 2024 03:28:51 GMT, Nir Lisker <nlis...@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
