Fix cancellation of AnimatorSet when child animation has delay

Previously, AnimatorSet incorrectly checked whether child animations were
'running' to figure out what to cancel. If a child animation was started, but
sitting in a startDelay phase, it was not 'running', so the right cancel/end
events would not propagate.

The fix is to add a new isStarted() API to Animator, which returns true when
the animator has started (but not yet ended), regardless of whether the animator
has a startDelay or not. It's basically a superset of the existing isRunning()
method, which only returns true when an animator has actually started setting values.

Change-Id: I126814cb6637b58295b6d18d9b155235671f99be

6 files changed, 161 insertions, 52 deletions

diff --git a/api/current.txt b/api/current.txt
index 5a599ce..73fe3b2 100644
--- a/api/current.txt
+++ b/api/current.txt
@@ -2179,6 +2179,7 @@ package android.animation {
     method public java.util.ArrayList<android.animation.Animator.AnimatorListener> getListeners();
     method public abstract long getStartDelay();
     method public abstract boolean isRunning();
+    method public boolean isStarted();
     method public void removeAllListeners();
     method public void removeListener(android.animation.Animator.AnimatorListener);
     method public abstract android.animation.Animator setDuration(long);
diff --git a/core/java/android/animation/Animator.java b/core/java/android/animation/Animator.java
index 57e0583..e01fa1a 100644
--- a/core/java/android/animation/Animator.java
+++ b/core/java/android/animation/Animator.java
@@ -111,12 +111,29 @@ public abstract class Animator implements Cloneable {
     public abstract void setInterpolator(TimeInterpolator value);
 
     /**
-     * Returns whether this Animator is currently running (having been started and not yet ended).
+     * Returns whether this Animator is currently running (having been started and gone past any
+     * initial startDelay period and not yet ended).
+     *
      * @return Whether the Animator is running.
      */
     public abstract boolean isRunning();
 
     /**
+     * Returns whether this Animator has been started and not yet ended. This state is a superset
+     * of the state of {@link #isRunning()}, because an Animator with a nonzero
+     * {@link #getStartDelay() startDelay} will return true for {@link #isStarted()} during the
+     * delay phase, whereas {@link #isRunning()} will return true only after the delay phase
+     * is complete.
+     *
+     * @return Whether the Animator has been started and not yet ended.
+     */
+    public boolean isStarted() {
+        // Default method returns value for isRunning(). Subclasses should override to return a
+        // real value.
+        return isRunning();
+    }
+
+    /**
      * Adds a listener to the set of listeners that are sent events through the life of an
      * animation, such as start, repeat, and end.
      *
diff --git a/core/java/android/animation/AnimatorSet.java b/core/java/android/animation/AnimatorSet.java
index ce3dd13..0b68dd8 100644
--- a/core/java/android/animation/AnimatorSet.java
+++ b/core/java/android/animation/AnimatorSet.java
@@ -95,6 +95,12 @@ public final class AnimatorSet extends Animator {
      */
     boolean mTerminated = false;
 
+    /**
+     * Indicates whether an AnimatorSet has been start()'d, whether or
+     * not there is a nonzero startDelay.
+     */
+    private boolean mStarted = false;
+
     // The amount of time in ms to delay starting the animation after start() is called
     private long mStartDelay = 0;
 
@@ -267,14 +273,14 @@ public final class AnimatorSet extends Animator {
     /**
      * {@inheritDoc}
      *
-     * <p>Note that canceling a <code>AnimatorSet</code> also cancels all of the animations that it is
-     * responsible for.</p>
+     * <p>Note that canceling a <code>AnimatorSet</code> also cancels all of the animations that it
+     * is responsible for.</p>
      */
     @SuppressWarnings("unchecked")
     @Override
     public void cancel() {
         mTerminated = true;
-        if (isRunning()) {
+        if (isStarted()) {
             ArrayList<AnimatorListener> tmpListeners = null;
             if (mListeners != null) {
                 tmpListeners = (ArrayList<AnimatorListener>) mListeners.clone();
@@ -296,6 +302,7 @@ public final class AnimatorSet extends Animator {
                     listener.onAnimationEnd(this);
                 }
             }
+            mStarted = false;
         }
     }
 
@@ -308,7 +315,7 @@ public final class AnimatorSet extends Animator {
     @Override
     public void end() {
         mTerminated = true;
-        if (isRunning()) {
+        if (isStarted()) {
             if (mSortedNodes.size() != mNodes.size()) {
                 // hasn't been started yet - sort the nodes now, then end them
                 sortNodes();
@@ -334,12 +341,13 @@ public final class AnimatorSet extends Animator {
                     listener.onAnimationEnd(this);
                 }
             }
+            mStarted = false;
         }
     }
 
     /**
-     * Returns true if any of the child animations of this AnimatorSet have been started and have not
-     * yet ended.
+     * Returns true if any of the child animations of this AnimatorSet have been started and have
+     * not yet ended.
      * @return Whether this AnimatorSet has been started and has not yet ended.
      */
     @Override
@@ -349,8 +357,12 @@ public final class AnimatorSet extends Animator {
                 return true;
             }
         }
-        // Also return true if we're currently running the startDelay animator
-        return (mDelayAnim != null && mDelayAnim.isRunning());
+        return false;
+    }
+
+    @Override
+    public boolean isStarted() {
+        return mStarted;
     }
 
     /**
@@ -435,6 +447,7 @@ public final class AnimatorSet extends Animator {
     @Override
     public void start() {
         mTerminated = false;
+        mStarted = true;
 
         // First, sort the nodes (if necessary). This will ensure that sortedNodes
         // contains the animation nodes in the correct order.
@@ -514,9 +527,17 @@ public final class AnimatorSet extends Animator {
             int numListeners = tmpListeners.size();
             for (int i = 0; i < numListeners; ++i) {
                 tmpListeners.get(i).onAnimationStart(this);
-                if (mNodes.size() == 0) {
-                    // Handle unusual case where empty AnimatorSet is started - should send out
-                    // end event immediately since the event will not be sent out at all otherwise
+            }
+        }
+        if (mNodes.size() == 0 && mStartDelay == 0) {
+            // Handle unusual case where empty AnimatorSet is started - should send out
+            // end event immediately since the event will not be sent out at all otherwise
+            mStarted = false;
+            if (mListeners != null) {
+                ArrayList<AnimatorListener> tmpListeners =
+                        (ArrayList<AnimatorListener>) mListeners.clone();
+                int numListeners = tmpListeners.size();
+                for (int i = 0; i < numListeners; ++i) {
                     tmpListeners.get(i).onAnimationEnd(this);
                 }
             }
@@ -536,6 +557,7 @@ public final class AnimatorSet extends Animator {
          */
         anim.mNeedsSort = true;
         anim.mTerminated = false;
+        anim.mStarted = false;
         anim.mPlayingSet = new ArrayList<Animator>();
         anim.mNodeMap = new HashMap<Animator, Node>();
         anim.mNodes = new ArrayList<Node>();
@@ -732,6 +754,7 @@ public final class AnimatorSet extends Animator {
                             tmpListeners.get(i).onAnimationEnd(mAnimatorSet);
                         }
                     }
+                    mAnimatorSet.mStarted = false;
                 }
             }
         }
@@ -936,9 +959,9 @@ public final class AnimatorSet extends Animator {
      * The <code>Builder</code> object is a utility class to facilitate adding animations to a
      * <code>AnimatorSet</code> along with the relationships between the various animations. The
      * intention of the <code>Builder</code> methods, along with the {@link
-     * AnimatorSet#play(Animator) play()} method of <code>AnimatorSet</code> is to make it possible to
-     * express the dependency relationships of animations in a natural way. Developers can also use
-     * the {@link AnimatorSet#playTogether(Animator[]) playTogether()} and {@link
+     * AnimatorSet#play(Animator) play()} method of <code>AnimatorSet</code> is to make it possible
+     * to express the dependency relationships of animations in a natural way. Developers can also
+     * use the {@link AnimatorSet#playTogether(Animator[]) playTogether()} and {@link
      * AnimatorSet#playSequentially(Animator[]) playSequentially()} methods if these suit the need,
      * but it might be easier in some situations to express the AnimatorSet of animations in pairs.
      * <p/>
diff --git a/core/java/android/animation/ValueAnimator.java b/core/java/android/animation/ValueAnimator.java
index c22306a..5df8bdc 100755
--- a/core/java/android/animation/ValueAnimator.java
+++ b/core/java/android/animation/ValueAnimator.java
@@ -193,6 +193,12 @@ public class ValueAnimator extends Animator {
      * Note that delayed animations are different: they are not started until their first
      * animation frame, which occurs after their delay elapses.
      */
+    private boolean mRunning = false;
+
+    /**
+     * Additional playing state to indicate whether an animator has been start()'d, whether or
+     * not there is a nonzero startDelay.
+     */
     private boolean mStarted = false;
 
     /**
@@ -628,7 +634,7 @@ public class ValueAnimator extends Animator {
                         for (int i = 0; i < numReadyAnims; ++i) {
                             ValueAnimator anim = readyAnims.get(i);
                             anim.startAnimation();
-                            anim.mStarted = true;
+                            anim.mRunning = true;
                             delayedAnims.remove(anim);
                         }
                         readyAnims.clear();
@@ -913,13 +919,14 @@ public class ValueAnimator extends Animator {
         mPlayingBackwards = playBackwards;
         mCurrentIteration = 0;
         mPlayingState = STOPPED;
+        mStarted = true;
         mStartedDelay = false;
         sPendingAnimations.get().add(this);
         if (mStartDelay == 0) {
             // This sets the initial value of the animation, prior to actually starting it running
             setCurrentPlayTime(getCurrentPlayTime());
             mPlayingState = STOPPED;
-            mStarted = true;
+            mRunning = true;
 
             if (mListeners != null) {
                 ArrayList<AnimatorListener> tmpListeners =
@@ -950,7 +957,7 @@ public class ValueAnimator extends Animator {
         if (mPlayingState != STOPPED || sPendingAnimations.get().contains(this) ||
                 sDelayedAnims.get().contains(this)) {
             // Only notify listeners if the animator has actually started
-            if (mStarted && mListeners != null) {
+            if (mRunning && mListeners != null) {
                 ArrayList<AnimatorListener> tmpListeners =
                         (ArrayList<AnimatorListener>) mListeners.clone();
                 for (AnimatorListener listener : tmpListeners) {
@@ -982,7 +989,12 @@ public class ValueAnimator extends Animator {
 
     @Override
     public boolean isRunning() {
-        return (mPlayingState == RUNNING || mStarted);
+        return (mPlayingState == RUNNING || mRunning);
+    }
+
+    @Override
+    public boolean isStarted() {
+        return mStarted;
     }
 
     /**
@@ -1013,7 +1025,7 @@ public class ValueAnimator extends Animator {
         sPendingAnimations.get().remove(this);
         sDelayedAnims.get().remove(this);
         mPlayingState = STOPPED;
-        if (mStarted && mListeners != null) {
+        if (mRunning && mListeners != null) {
             ArrayList<AnimatorListener> tmpListeners =
                     (ArrayList<AnimatorListener>) mListeners.clone();
             int numListeners = tmpListeners.size();
@@ -1021,6 +1033,7 @@ public class ValueAnimator extends Animator {
                 tmpListeners.get(i).onAnimationEnd(this);
             }
         }
+        mRunning = false;
         mStarted = false;
     }
 
diff --git a/core/tests/coretests/src/android/animation/AnimatorSetEventsTest.java b/core/tests/coretests/src/android/animation/AnimatorSetEventsTest.java
index 65f2b8e..d415e4e 100644
--- a/core/tests/coretests/src/android/animation/AnimatorSetEventsTest.java
+++ b/core/tests/coretests/src/android/animation/AnimatorSetEventsTest.java
@@ -15,25 +15,74 @@
 */
 package android.animation;
 
+import android.os.Handler;
+import android.test.suitebuilder.annotation.MediumTest;
+import android.test.suitebuilder.annotation.SmallTest;
 import android.widget.Button;
 import com.android.frameworks.coretests.R;
 
+import java.util.concurrent.TimeUnit;
+
 /**
  * Listener tests for AnimatorSet.
  */
 public class AnimatorSetEventsTest extends EventsTest {
 
+    Button button;
+    ObjectAnimator xAnim = ObjectAnimator.ofFloat(this, "translationX", 0, 100);
+    ObjectAnimator yAnim = ObjectAnimator.ofFloat(this, "translationY", 0, 100);
+
     @Override
     public void setUp() throws Exception {
-        final BasicAnimatorActivity activity = getActivity();
-        Button button = (Button) activity.findViewById(R.id.animatingButton);
-
-        ObjectAnimator xAnim = ObjectAnimator.ofFloat(button, "translationX", 0, 100);
-        ObjectAnimator yAnim = ObjectAnimator.ofFloat(button, "translationX", 0, 100);
+        button = (Button) getActivity().findViewById(R.id.animatingButton);
         mAnimator = new AnimatorSet();
         ((AnimatorSet)mAnimator).playSequentially(xAnim, yAnim);
 
         super.setUp();
     }
 
+    @Override
+    protected long getTimeout() {
+        return (xAnim.getDuration() + yAnim.getDuration()) +
+                (xAnim.getStartDelay() + yAnim.getStartDelay()) +
+                ANIM_DELAY + FUTURE_RELEASE_DELAY;
+    }
+
+    /**
+     * Tests that an AnimatorSet can be correctly canceled during the delay of one of
+     * its children
+     */
+    @MediumTest
+    public void testPlayingCancelDuringChildDelay() throws Exception {
+        yAnim.setStartDelay(500);
+        final AnimatorSet animSet = new AnimatorSet();
+        animSet.playSequentially(xAnim, yAnim);
+        mFutureListener = new FutureReleaseListener(mFuture);
+        getActivity().runOnUiThread(new Runnable() {
+            @Override
+            public void run() {
+                try {
+                    Handler handler = new Handler();
+                    animSet.addListener(mFutureListener);
+                    mRunning = true;
+                    animSet.start();
+                    handler.postDelayed(new Canceler(animSet, mFuture), ANIM_DURATION + 250);
+                } catch (junit.framework.AssertionFailedError e) {
+                    mFuture.setException(new RuntimeException(e));
+                }
+            }
+        });
+        mFuture.get(getTimeout(), TimeUnit.MILLISECONDS);
+    }
+
+    public void setTranslationX(float value) {
+        button.setTranslationX(value);
+    }
+
+
+    public void setTranslationY(float value) {
+        button.setTranslationY(value);
+    }
+
+
 }
diff --git a/core/tests/coretests/src/android/animation/EventsTest.java b/core/tests/coretests/src/android/animation/EventsTest.java
index 6ea2845..701a3f0 100644
--- a/core/tests/coretests/src/android/animation/EventsTest.java
+++ b/core/tests/coretests/src/android/animation/EventsTest.java
@@ -38,18 +38,17 @@ import java.util.concurrent.TimeUnit;
 public abstract class EventsTest
         extends ActivityInstrumentationTestCase2<BasicAnimatorActivity> {
 
-    private static final int ANIM_DURATION = 400;
-    private static final int ANIM_DELAY = 100;
-    private static final int ANIM_MID_DURATION = ANIM_DURATION / 2;
-    private static final int ANIM_MID_DELAY = ANIM_DELAY / 2;
-    private static final int FUTURE_RELEASE_DELAY = 50;
-    private static final int TIMEOUT = ANIM_DURATION + ANIM_DELAY + FUTURE_RELEASE_DELAY;
+    protected static final int ANIM_DURATION = 400;
+    protected static final int ANIM_DELAY = 100;
+    protected static final int ANIM_MID_DURATION = ANIM_DURATION / 2;
+    protected static final int ANIM_MID_DELAY = ANIM_DELAY / 2;
+    protected static final int FUTURE_RELEASE_DELAY = 50;
 
     private boolean mStarted;  // tracks whether we've received the onAnimationStart() callback
-    private boolean mRunning;  // tracks whether we've started the animator
+    protected boolean mRunning;  // tracks whether we've started the animator
     private boolean mCanceled; // trackes whether we've canceled the animator
-    private Animator.AnimatorListener mFutureListener; // mechanism for delaying the end of the test
-    private FutureWaiter mFuture; // Mechanism for waiting for the UI test to complete
+    protected Animator.AnimatorListener mFutureListener; // mechanism for delaying the end of the test
+    protected FutureWaiter mFuture; // Mechanism for waiting for the UI test to complete
     private Animator.AnimatorListener mListener; // Listener that handles/tests the events
 
     protected Animator mAnimator; // The animator used in the tests. Must be set in subclass
@@ -59,7 +58,7 @@ public abstract class EventsTest
      * Cancels the given animator. Used to delay cancellation until some later time (after the
      * animator has started playing).
      */
-    static class Canceler implements Runnable {
+    protected static class Canceler implements Runnable {
         Animator mAnim;
         FutureWaiter mFuture;
         public Canceler(Animator anim, FutureWaiter future) {
@@ -77,6 +76,13 @@ public abstract class EventsTest
     };
 
     /**
+     * Timeout length, based on when the animation should reasonably be complete.
+     */
+    protected long getTimeout() {
+        return ANIM_DURATION + ANIM_DELAY + FUTURE_RELEASE_DELAY;
+    }
+
+    /**
      * Ends the given animator. Used to delay ending until some later time (after the
      * animator has started playing).
      */
@@ -102,7 +108,7 @@ public abstract class EventsTest
      * it releases it after some further delay, to give the test time to do other things right
      * after an animation ends.
      */
-    static class FutureReleaseListener extends AnimatorListenerAdapter {
+    protected static class FutureReleaseListener extends AnimatorListenerAdapter {
         FutureWaiter mFuture;
 
         public FutureReleaseListener(FutureWaiter future) {
@@ -232,7 +238,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT, TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(), TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -255,7 +261,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT, TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(), TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -278,7 +284,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT, TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(), TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -301,7 +307,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT, TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(), TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -324,7 +330,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT, TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(), TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -347,7 +353,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT, TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(), TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -371,7 +377,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT,  TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(),  TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -395,7 +401,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT,  TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(),  TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -412,7 +418,7 @@ public abstract class EventsTest
                     // would have finished. This tests to make sure that we're not calling
                     // the listeners with cancel/end callbacks since they won't be called
                     // with the start event.
-                    mFutureListener = new FutureReleaseListener(mFuture, TIMEOUT);
+                    mFutureListener = new FutureReleaseListener(mFuture, getTimeout());
                     Handler handler = new Handler();
                     mRunning = true;
                     mAnimator.start();
@@ -422,7 +428,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT + 100,  TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout() + 100,  TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -439,7 +445,7 @@ public abstract class EventsTest
                     // would have finished. This tests to make sure that we're not calling
                     // the listeners with cancel/end callbacks since they won't be called
                     // with the start event.
-                    mFutureListener = new FutureReleaseListener(mFuture, TIMEOUT);
+                    mFutureListener = new FutureReleaseListener(mFuture, getTimeout());
                     Handler handler = new Handler();
                     mRunning = true;
                     mAnimator.start();
@@ -449,7 +455,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT + 100,  TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout() + 100,  TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -473,7 +479,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT,  TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(),  TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -497,7 +503,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT,  TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(),  TimeUnit.MILLISECONDS);
     }
 
     /**
@@ -521,7 +527,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT,  TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(),  TimeUnit.MILLISECONDS);
      }
 
     /**
@@ -545,7 +551,7 @@ public abstract class EventsTest
                 }
             }
         });
-        mFuture.get(TIMEOUT,  TimeUnit.MILLISECONDS);
+        mFuture.get(getTimeout(),  TimeUnit.MILLISECONDS);
      }
 
 }