On Sat, 27 Jan 2024 17:26:45 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 documentation on AnimationTimer methods
Changes look fine. I have two minor comments.
modules/javafx.graphics/src/main/java/com/sun/javafx/util/Utils.java line 1008:
> 1006: * @param runnable a {@code Runnable} encapsulating the code
> 1007: */
> 1008: public static void runOnFxThread(Runnable runnable) {
I understand that this is a private package, and not public API, but do we need
a null check of `runnable`? (Not needed for the current uses added from this
PR, but possibly for future uses?)
modules/javafx.graphics/src/main/java/javafx/animation/Animation.java line 903:
> 901: }
> 902:
> 903: private void playFromOnFxThread(String cuePoint) {
I take that adding these private `xxOnFxThread` methods is convenient so they
can be passed easily to the runnable in `Utils::runOnFxThread`, or accessed
from other subclasses.
However, I'm not sure about the method naming `OnFxThread`, as it might imply
that what it does is already done on the FX thread, therefore not needing to be
wrapped up by `runOnFxThread`.
Does `runXX` (i.e. `runPlayFrom`) make more sense? In any case, I'd be fine
with the current proposal as is.
-------------
PR Review: https://git.openjdk.org/jfx/pull/1352#pullrequestreview-1847205003
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1468586394
PR Review Comment: https://git.openjdk.org/jfx/pull/1352#discussion_r1468590831
