On Fri, 17 Jan 2020 22:48:11 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>>> 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. > So I think the right thing to do is fix the test. Since the animation plays from the end for an arbitrary duration (at least I think it's arbitrary, compared to a pulse), I changed the assert to check that the property value is in the playing range of the animation and not equals to some specific value. ------------- PR: https://git.openjdk.java.net/jfx/pull/82
