RFR: 8236753: Animations do not play backwards after being stopped

[Kevin Rushforth]

On Thu, 16 Jan 2020 01:09:56 GMT, Nir Lisker <nlisker at 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