8236753: Animations do not play backwards after being stopped

[nlisker]

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, which also mentions a bug relating to pausing and playing backwards, so be mindful of it when testing.

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

JDK-8236753: Animations do not play backwards after being stopped

Approvers

• Kevin Rushforth (kcr - Reviewer)
• Ambarish Rapte (arapte - Reviewer)

[bridgekeeper]

👋 Welcome back nlisker! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request (refresh this page to view it).

[mlbridge]

Webrevs

• 01: Full (18098da)
• 00: Full (3332a97)

[kevinrushforth]

I'll review this next week. This seems a fine candidate for openjfx14, so it (along with a couple other pending reviews that can be for 14) will be a good test of targeting a PR to the stabilization branch.

I also request @arapte to review.

[kevinrushforth]

I should add that it will depend on whether there are any regressions. One thing we do need to be careful of is introducing regressions during rampdown.

[arapte]

The fix looks good to me.
After this change the steps needed for playing an Animation backwards will change.
Earlier this was documented with Animation.play() API as below.

---

To play an Animation backwards from the end:

animation.setRate(negative rate);
animation.jumpTo(overall duration of animation);
animation.play();

---

After this PR call to jumpTo() won't be needed here.
So this PR may need a document change for Animation.play().
Also to note that the behavior of above mentioned three calls remains same with the changes in this change.

[mlbridge]

Mailing list message from Scott Palmer on openjfx-dev:

On Jan 14, 2020, at 11:50 AM, Ambarish Rapte <arapte at openjdk.java.net> wrote:

On Fri, 10 Jan 2020 00:39:53 GMT, Kevin Rushforth <kcr at openjdk.org> wrote:

I'll review this next week. This seems a fine candidate for openjfx14, so it (along with a couple other pending reviews that can be for 14) will be a good test of targeting a PR to the stabilization branch.

I also request @arapte to review.

I should add that it will depend on whether there are any regressions. One thing we do need to be careful of is introducing regressions during rampdown.

The fix looks good to me.
After this change the steps needed for playing an `Animation` backwards will change.
Earlier this was documented with `Animation.play()` API as below.
-------------
To play an `Animation` backwards from the end:
animation.setRate(negative rate);
animation.jumpTo(overall duration of animation);
animation.play();
-------------

After this PR call to `jumpTo()` won't be needed here.
So this PR may need a document change for `Animation.play()`.
Also to note that the behavior of above mentioned three calls remains same with the changes in this change.

If the jumpTo isn?t required, then this isn?t this a change in behaviour? I?m wonder in particular if this has an effect on cycleCount.

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?

Scott

[nlisker]

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.

[nlisker]

So this PR may need a document change for Animation.play()

Yes, and the docs need clarification in other places anyway. The parent issue 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.

[kevinrushforth]

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 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 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
4. 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.

[nlisker]

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.

[kevinrushforth]

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.

[openjdk]

@nlisker This change now passes all automated pre-integration checks. When the change also fulfills all project specific requirements, type /integrate in a new comment to proceed. After integration, the commit message will be:

8236753: Animations do not play backwards after being stopped

Reviewed-by: kcr, arapte

• If you would like to add a summary, use the /summary command.
• To credit additional contributors, use the /contributor command.
• To add additional solved issues, use the /solves command.

Since the source branch of this PR was last updated there have been 18 commits pushed to the jfx14 branch. Since there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to do this manually, please merge jfx14 into your branch first.

➡️ To integrate this PR with the above commit message, type /integrate in a new comment.

[arapte]

The changes look good to me.

[nlisker]

/integrate

[openjdk]

@nlisker The following commits have been pushed to jfx14 since your change was applied:

• 16cea41: Merge
• f907026: 8236808: javafx_iio can not be used in static environment
• 63520a0: Merge
• 8a5344c: Merge
• 2e0d01c: Merge
• a96704e: Merge
• 71fa9af: Merge
• a3711e2: Merge
• 2d5d7e0: Merge
• 962bdd1: 8232214: Improved internal validations
• 2d2b824: 8232128: Better formatting for numbers
• 81f7738: 8232121: Better numbering system
• 8e55294: Merge
• 1325f11: Merge
• f759dd9: Merge
• fed185a: Merge
• 592d745: Merge
• a0b4b14: 8227473: Improve gstreamer media support

Your commit was automatically rebased without conflicts.

Pushed as commit 9ae37f1.

[mlbridge]

Mailing list message from Nir Lisker on openjfx-dev:

Changeset: 9ae37f1
Author: Nir Lisker <nlisker at openjdk.org>
Date: 2020-01-22 20:58:12 +0000
URL: https://git.openjdk.java.net/jfx/commit/9ae37f1f

8236753: Animations do not play backwards after being stopped

Reviewed-by: kcr, arapte

! modules/javafx.graphics/src/main/java/javafx/animation/Animation.java
! modules/javafx.graphics/src/test/java/test/javafx/animation/AnimationTest.java
! modules/javafx.graphics/src/test/java/test/javafx/animation/SequentialTransitionPlayTest.java

[nlisker]

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
4. 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.

Filed as JDK-8237748.

[mlbridge]

Mailing list message from Nir Lisker on openjfx-dev:

Changeset: 9ae37f1
Author: Nir Lisker <nlisker at openjdk.org>
Date: 2020-01-22 20:58:12 +0000
URL: https://git.openjdk.java.net/jfx/commit/9ae37f1f

8236753: Animations do not play backwards after being stopped

Reviewed-by: kcr, arapte

! modules/javafx.graphics/src/main/java/javafx/animation/Animation.java
! modules/javafx.graphics/src/test/java/test/javafx/animation/AnimationTest.java
! modules/javafx.graphics/src/test/java/test/javafx/animation/SequentialTransitionPlayTest.java

[nlisker]

@kevinrushforth Any idea why I got another integration message from mlbridgebot 2 days later?

[kevinrushforth]

I just now did a sync from jfx14 --> master but I don't know why that would have triggered the bot to update the PR. I expected the push notification sent to openjfx-changes (which was sent correctly), but I didn't expect this.

@rwestberg any ideas?