[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8159048: Null PulseReciever in AbstractMasterTimer
From:       Andy Goryachev <angorya () openjdk ! org>
Date:       2023-07-31 17:15:50
Message-ID: 7xMZmkxN1DhZHC8aLJCq_Ev4SCN0TOSHqTcsXSWepi0=.7c971504-d13a-43fc-b4c0-6f4b8147422e () github ! com
[Download RAW message or body]

On Mon, 3 Jul 2023 15:49:31 GMT, Jose Pereda <jpereda@openjdk.org> wrote:

> This PR adds a check to the Animation and AnimationTimer public methods to verify \
> that these are called from the JavaFX Application thread. If the call is done from \
> any other thread, an IllegalStateException will be thrown. 
> This will prevent users from getting unexpected errors (typically NPE, like the one \
> posted in the JBS issue), and will fail fast with a clear exception and reason for \
> it. 
> The javadoc of the Animation and AnimationTimer classes and public methods has been \
> updated accordingly. 
> Tests for both classes have been included, failing (as in no exceptions were thrown \
> when calling from a background thread) before this patch, and passing (as in ISE \
> was thrown).

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

> 988:      */
> 989:     public void play() {
> 990:         Toolkit.getToolkit().checkFxUserThread();

minor: perhaps this should first check for non-null parent, then for fx thread (here \
and below)

tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTimerTest.java line \
62:

> 60:             startupLatch.countDown();
> 61:         }
> 62: 

minor: extra newline?

tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTimerTest.java line \
82:

> 80:                 @Override public void handle(long l) {
> 81:                     frameCounter.countDown();
> 82:                     if (frameCounter.getCount() == 0L) {

thank you for 0L!

tests/system/src/test/java/test/com/sun/javafx/animation/AnimationTimerTest.java line \
112:

> 110:         Platform.runLater(timer::start);
> 111:     }
> 112: 

minor: extra newline

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

PR Review Comment: https://git.openjdk.org/jfx/pull/1167#discussion_r1279641091
PR Review Comment: https://git.openjdk.org/jfx/pull/1167#discussion_r1279642701
PR Review Comment: https://git.openjdk.org/jfx/pull/1167#discussion_r1279642982
PR Review Comment: https://git.openjdk.org/jfx/pull/1167#discussion_r1279642236


[prev in list] [next in list] [prev in thread] [next in thread]