On Thu, 16 Jan 2020 01:09:56 GMT, Nir Lisker <nlis...@openjdk.org> wrote:
>>> If cycleCount is set to 2 does the animation play the same number of times >>> with or without the jumpTo both before and after this change? >> >> Before the change: >> * With `jumpTo`: plays backwards 2 times. >> * Without `jumpTo`: doesn't play no mater how many times you call `play()`. >> Compare with the case where `cycleCount` is 1, in which case the first call >> does nothing (moves the playing head to the start) and the second one plays >> backwards. >> >> After the change: >> * With `jumpTo`: plays backwards 2 times. >> * Without `jumpTo`: plays backwards 2 times. >> >>> If the jumpTo isn't required, then this isn't this a change in behaviour? >> >> Current code with `jumpTo` will behave the same. The question (which I >> presented initially) is whether this behavior is considered a bug or not >> when it comes to the initial state of an animation. It is certainly a bug >> when `stop()` is called, so I assume it is similar for the initial state. > >> So this PR may need a document change for `Animation.play()` > > Yes, and the docs need clarification in other places anyway. The [parent > issue](https://bugs.openjdk.java.net/browse/JDK-8210238) from which this bug > was isolated talks about these. Not sure at what point I will go over it > since `Animation` is going to get some more changes in the (hopefully) near > future. Based on my testing, and thinking through all of the implications of the fix, I think the proposed fix is correct. I modified the Timeline.java test program attached to [JDK-8210238](https://bugs.openjdk.java.net/browse/JDK-8210238) to add controls for auto-reverse and cycle count (1, 2, or INDEFINITE), and attached it as TimelineTest2.java. I tried several combinations, and the change in behavior looks correct in all cases to me. As you point out in the description, the failing unit test, [SequentialTransitionPlayTest.testCycleReverse](https://github.com/openjdk/jfx/blob/16cea4111b25a00f812b2f6ba8a54adbcffc1c86/modules/javafx.graphics/src/test/java/test/javafx/animation/SequentialTransitionPlayTest.java#L632) is coded to test for the existing behavior when you first call play with `rate = -1`. I think it likely that this is merely because that's how it worked rather than because there was deliberate thought given to whether it was the right behavior. So I think the right thing to do is fix the test. You also mentioned that jumping, even to the start or end, sets `lastPlayFinished` to `false`. I believe this is correct behavior and should not be changed. As part of my testing, I ran into what may be a bug, but since the behavior is the same with or without your patch, I think it would be best to file a new bug and deal with it separately (if, in fact, it is a bug). Here are the steps I used: 1. Run TimelineTest2 2. Select cycle=2; Select AutoReverse 3. Play animation: it correctly goes forward from start to end and then backwards from end to start 3. Set rate=-1 5. Play animation: BUG? It still goes forward from start to end and then backwards from end to start Subsequent calls to play do what I would expect, in that it goes backwards from end to start and then forward from start to end. The same thing happens when switching back to rate=1 : the first time it continues the direction it was prior to changing the rate. ------------- PR: https://git.openjdk.java.net/jfx/pull/82
