Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-25 Thread Kevin Rushforth
If you mean the changes proposed in Draft PR https://github.com/openjdk/jfx/pull/1349 those are too-intrusive for where we are in JavaFX 22. I see no chance that we will get agreement on the approach and be able to finish the code review and CSR in the next 4 business days. If we later get ge

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-25 Thread Michael Strauß
Hi Kevin, have you considered my proposal, which is basically the same approach: it uses runLater internally to dispatch the interaction with AbstractPrimaryTimer (instead of trying to make AbstractPrimaryTimer work under concurrent access). But from the point of view of the caller, the API works

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-25 Thread Michael Strauß
Hi John, the threading restrictions are now removed. What I mean by those seemingly contradictory stratements is the following: This change doesn't make Animation inherently thread-safe, so if you configure the animation on thread A, you can't just call play() on thread B and expect it to work. Wh

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-25 Thread Kevin Rushforth
And that's why the "be careful" option is not a satisfying solution. Let's proceed with the option to change the implementation of play/pause/stop/start to do the work in a runLater, and change the docs accordingly. It's the only feasible option for JavaFX 22 (other than "do nothing"), and I a

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-25 Thread Nir Lisker
> > Option 3 is basically document it as "be careful" and remove the thread > restriction recently introduced, am I correct? > Yes :) IMHO that can simply can't work at all, because this is what > (theoretically) can happen: > 1. Non-FX thread starts animation > - Fields are manipulated in A

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-25 Thread John Hendrikx
Hi Michael, I'm not quite sure I see the point of this change.  The PR did not remove the threading restrictions on play/stop. I'm also confused by the seemingly contradictory statements: - this proposal does NOT allow Animation.play/stop/etc. to be "called on any thread" - It merely removes

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-25 Thread John Hendrikx
On 24/01/2024 22:06, Nir Lisker wrote: This is the ballpark of what I meant with "the downside might be some surprise when these methods behave differently". That can be documented, and basically already is (because that is what the "asynchronous" is alluding to, the fact that after calling "pl

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Michael Strauß
Note: this proposal does NOT allow Animation.play/stop/etc. to be "called on any thread" as mentioned in JDK-8324658 [0]. It merely removes the requirement that these methods must be called on the FX thread, but this doesn't make the class inherently thread-safe. That is an important distinction t

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Michael Strauß
Here's another option, which I have implemented as a proof of concept [0]: The play/stop/etc. methods are currently specified to be "asynchronous". This language should be removed, such that the methods will be implied to execute synchronously from the point of view of the caller (like every metho

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Nir Lisker
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 inf

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Kevin Rushforth
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). S

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Michael Strauß
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

Re: [External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Kevin Rushforth
Thanks! I just filed https://bugs.openjdk.org/browse/JDK-8324658 It should mostly revert JDK-8159048 (although the sentence added to the class docs about animations running on the JavaFX app thread is still valid), and some of the unit tests might still be valid. -- Kevin On 1/24/2024 10:50

Re: [External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Nir Lisker
If there's an agreement, I can do it tomorrow. It will effectively revert JDK-8159048 and supersede JDK-8324219. On Wed, Jan 24, 2024 at 8:42 PM Kevin Rushforth wrote: > I also now favor option 2 and was in the process of writing something up > recommending that we do that. Phil and I (and a cou

Re: [External] : Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Kevin Rushforth
I also now favor option 2 and was in the process of writing something up recommending that we do that. Phil and I (and a couple others) had an offline discussion and think this is the way to go. Given the short amount of time to get this into 22, I will file the JBS issue in the next hour or s

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Nir Lisker
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 th

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Kevin Rushforth
I'd like to hear from the others on this. I don't see any fundamental problem with having the play/pause/stop methods wrap their implementation in a runLater (if not on the FX Application thread already), and documenting that it does so, if we can get general agreement. -- Kevin On 1/24/2024

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Jurgen Doll
Hi Kevin If I may make one more final appeal then to an alternative solution please. Could we then instead of throwing an Exception rather invoke runLater if needed inside play, stop, and resume. Putting the onus on the developer is fine if it is the developer that is invoking the call, bu

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Kevin Rushforth
Thank you to Jurgen for raising the question and to Nir, John, and Michael for evaluating it. I conclude that there is insufficient motivation to revert the change in behavior implemented by JDK-8159048 to allow calling the play/pause/stop methods of Animation on a background thread. Doing so

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread John Hendrikx
Hi Jurgen, See my answers inline. On 24/01/2024 10:12, Jurgen Doll wrote: Hi John Thank you for the hypothetical receivers array scenario, I think it explains the problem exactly and is why replacing the array with CopyOnWriteArrayList removes the NPE. Your perspective then is that Abstrac

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Jurgen Doll
Hi John Thank you for the hypothetical receivers array scenario, I think it explains the problem exactly and is why replacing the array with CopyOnWriteArrayList removes the NPE. Your perspective then is that AbstractPrimaryTimer is designed for single threaded use only. If that is indeed

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-24 Thread Nir Lisker
After playing around with the code sample, I think that this is not the right way to use the animation. The reason is that there is no point in starting the animation before the control is attached to the scenegraph, or even made visible. A small refactoring where, e.g., the controller class expose

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-23 Thread Michael Strauß
Hi Jurgen, even assuming that it was as easy as you say it is, there are more problems with this proposal. 1. If non-FX thread access to some methods is allowed, this will become a permanent addition to the API and a complication we have to deal with. There are proposals to refactor the animation

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-23 Thread John Hendrikx
On 23/01/2024 16:56, Jurgen Doll wrote: Hi John and others, I don't think we are entirely on the same page, so here's the objective. The Goal: To determine if the FX animation thread and a SINGLE other thread can access Animation in a safe manner wrt play, stop, and resume. The number of thre

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-23 Thread Jurgen Doll
Hi John and others, I don't think we are entirely on the same page, so here's the objective. The Goal: To determine if the FX animation thread and a SINGLE other thread can access Animation in a safe manner wrt play, stop, and resume. Non Goal: Multi-threaded access of Animation play, stop,

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-22 Thread John Hendrikx
On 22/01/2024 17:46, Jurgen Doll wrote: I've been delving into the usage of `aborted` and `inTimePulse` as mentioned by John and gleaned the following: 1. stop makes a best effort to abort the 'animation' if it is in the process of execution. 2. `aborted` and `inTimePulse` are reset with eve

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-22 Thread Kevin Rushforth
I would not support your proposed 4th option. It's basically a partial fix for the solution John mentioned as his third option. Of the three John mentions, I favor either 1 or 2. I don't see a compelling reason to guarantee thread-safety for Animation objects (option 3), in which case the ques

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-22 Thread Jurgen Doll
I've been delving into the usage of `aborted` and `inTimePulse` as mentioned by John and gleaned the following: 1. stop makes a best effort to abort the 'animation' if it is in the process of execution. 2. `aborted` and `inTimePulse` are reset with every pulse. As to the options that John m

Re: HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-22 Thread John Hendrikx
This seems like a reasonable use case, and perhaps this was the original intent of the "asynchronous call" documentation. The problem though is that the play/stop code does not seem to take into account being called from a different thread (there are several synchronization issues when I delve

HEADS-UP: Threading restriction for Animation play, pause, stop now enforced USE CASE

2024-01-22 Thread Jurgen Doll
Here's an example as requested by Nir: public class FxTimeLineTest extends Application { private BorderPane bp = new BorderPane( new Label("Loading") ); public static void main( String[] args ) { launch( FxTimeLineTest.class, args ); } @Override public void