On Tue, 21 Jan 2020 19:57:56 GMT, Nir Lisker <nlis...@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
