[Rev 01] RFR: 8236753: Animations do not play backwards after being stopped

[Kevin Rushforth]

On Tue, 21 Jan 2020 19:57:56 GMT, Nir Lisker <nlisker at openjdk.org> wrote:

>> The private field `lastPlayFinished` is responsible for 2 cases where an animation in `STOPPED` status does not play after `play()` is called if the rate is negative:
>> 
>> 1. When the animation is created, it is `STOPPED` and `lastPlayFinished` is `false`. Setting a negative rate and calling `play()` will not jump to the end of the animation (in order to play it backwards) because the `if (lastPlayedFinished)` check is `false`. Creating the animation with `lastPlayFinished = true` fixes this. However, `SequentialTransitionPlayTest#testCycleReverse`'s initial state test implies that the original behavior is correct. *That test currently fails with this change.* Either the fix is reverted or the test is corrected.
>> 2. When the animation is stopped (if it was not `STOPPED` already), `jumpTo(Duration.ZERO)` sets `lastPlayFinished` to `false`, which causes the same issue above with `play()`. Setting `lastPlayFinished = true` at the end `stop()` fixes this issue.
>> 
>> A test was added for case 2 to check that the playing head indeed jumps to the end of the animation. Without this fix, it stays at the start.
>> 
>> I'm still somewhat confused as to what constitutes a "last play finished". Any `jumpTo` resets `lastPlayFinished` to `false`, even if the jump is to the start/end of the animation. In this case, stopping an animation, jumping to its start/end, setting the rate to negative/positive, and playing, will do nothing as the end condition is reached immediately. This is what the behavior that was fixed for cases 1 and 2, but maybe this is also incorrect behavior for jumping to start/end.
>> 
>> A test app is included in the "parent" [bug](https://bugs.openjdk.java.net/browse/JDK-8210238), which also mentions a bug relating to **pausing** and playing backwards, so be mindful of it when testing.
> 
> The pull request has been updated with 1 additional commit.

Approved.

The fix looks fine to me, and now passes all unit tests. The change you made to the test is what I would expect (I had done a similar thing locally to get them to pass when I was testing).

Regarding the question that came up earlier in the review about modifying the docs to remove the call to `jumpTo` from the example, I do not recommend changing the docs as part of this fix. The existing example code is still correct. More importantly, the code snippet in question is part of the docs for `Animation::play`, and isn't just talking about playing when stopped. If you play after a paused animation and want to play backwards from the end, then the `jumpTo` is still needed.

Additionally, there are other questionable aspects of the Animation docs. For example, consider the following:

> When rate > 0 (forward play), if an Animation is already positioned at the end, the first cycle will not be played

This is misleading in that it is only true after an explicit call to "jumpTo(end)". If you play a forward animation to completion, then it isn't. My point for bring it up is that I don't really want to make changes to the docs without considering the larger implications.

Please wait for @arapte to finish reviewing.

-------------

Marked as reviewed by kcr (Lead).

PR: https://git.openjdk.java.net/jfx/pull/82