This is the ballpark of what I meant with "the downside might be some surprise when these methods behave differently".
The example you give will only show a potential change if 'play' is not called from the FX thread. In such a case, what's the chance that this is an undeliberate misuse vs. an informed use? This brings me again to: what is the use case for running an animation from a background thread? In your simple example, listening on the Status property would work. Also, while runLater makes no guarantees, I've never seen a non-negligible delay in its execution, so how surprising is it really going to be? If you want to be able to run an animation from a background thread with no race conditions, why not opt for option 3? And option 1 is also new and surprising because now code that worked (or pretended to work) throws, which ruins properly written code (with respect to multithreading), or exposes a bug. On Wed, Jan 24, 2024 at 9:04 PM Michael Strauß <michaelstr...@gmail.com> 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. >
