The point you raise is one reason why I wouldn't advocate doing this in
other places, e.g., for scene graph updates, which do need to run
synchronously.
I note that the Animation docs currently state that these operations are
asynchronous (and yes, I know you were proposing the change this). So
while it might be surprising to some apps, it is within the current
spec, and I'm not sure it is any more surprising than the exception we
now throw in JavaFX 22.
If we could safely do what you suggest, and just wrap the part that
touches the AbstractPrimaryTimer, that might be the best option. I don't
know if that is feasible, though, especially going from a running state
to a stopped or paused state.
-- Kevin
On 1/24/2024 11:04 AM, Michael Strauß wrote:
I am not in favor of option 2 if the implementation was simply "wrap
the implementation in runLater", as this would be a surprising change.
Consider the following code:
var transition = new FadeTransition();
transition.play();
// Will always print "RUNNING" currently, but might print "STOPPED"
// if we go with the proposed change:
System.out.println(transition.getStatus());
Regardless of the race condition in AbstractPrimaryTimer, this code
always seemed to be working quite fine (albeit superficially), because
the play/stop/etc. methods change that status of the animation as one
would expect.
You are proposing to change that, such that calling these methods will
not predictably change the status of the animation. Instead, these
methods now act more like "requests" that may be fulfilled at some
later point in time, rather than statements of fact.
This is not a change that I think we should be doing on an ad-hoc
basis, since the same considerations potentially apply for many
methods in many places.
If we were to allow calling play/stop/etc. on a background thread, I
would be in favor of keeping the semantics that these methods
instantly and predictably affect the status of the animation. Only the
internal operation of adding the animation to AbstractPrimaryTimer
should be posted to the FX thread. If that is not possible, this
suggests to me that we should choose option 1. Introducing new and
surprising semantics to an existing API is not the way to go.
On Wed, Jan 24, 2024 at 7:27 PM Nir Lisker <nlis...@gmail.com> wrote:
These are the options I see as reasonable:
1. Document that these methods *must* be run on the FX thread and throw an
exception otherwise. This leaves it to the responsibility of the user. However,
this will cause the backwards compatibility problems that Jugen brought up. As
a side note, we do this in other methods already, but I always questioned why
let the developer do something illegal - if there's only one execution path,
force it.
2. Document that these methods *are* run on the FX thread (the user doesn't
need to do anything) and change the implementation to call runLater(...)
internally unless they are already on the FX thread. This will be backwards
compatible for the most part (see option 3). The downside might be some
surprise when these methods behave differently.
3. Document that it's *recommended* that these methods be run on the FX thread
and let the user be responsible for the threading. We can explain that
manipulating nodes that are attached to an active scenegraph should be avoided.
I prefer option 2 over 1 regardless of the backwards compatibility issue even, but would
like to know if I'm missing something here because in theory this change could be done to
any "must run on the FX thread" method and I question why the user had the
option to get an exception.
Option 3 is risky and I wager a guess that it will be used wrongly more often
than not. It does allow some (what I would call) valid niche uses. I never did
it.
