Fix issues with delayed transitions

Previously, there were two distinct problems with how delayed
transitions were being run:
- there would be a delay between the transition being put into
a preDrawListener (to be kicked off when that listener fired) and
being removed from the pending list. This allowed another delayed
transition to be run in parallel, which would cause conflicting/
clobbering issues with transition values on the same objects.
- there would be an extra frame delay in some cases due to how/when the
delayed transition would be started. Specfically, we would postOnAnimation()
to call a method that would then add the onPreDraw listener. This two-step
forwarding caused issues noted above.

The fix is to simply add the transition to the preDrawListener immediately, removing
the two-step problem, and also ensuring that the transition is only removed
from the pending list when it is actually started, which prevents other transitions
from starting in the meantime.

Also, added more debug logging to help chase future issues with transitions.

Change-Id: Ie2ff8e73d29f342512842e9641bd8d605e74544c

diff --git a/core/java/android/view/transition/Fade.java b/core/java/android/view/transition/Fade.java
index c2aa90b..4fd60c1 100644 (file)
--- a/core/java/android/view/transition/Fade.java
+++ b/core/java/android/view/transition/Fade.java
@@ -32,6 +32,8 @@ import android.view.ViewGroup;
  */
 public class Fade extends Visibility {
 
+    private static boolean DBG = Transition.DBG && false;
+
     private static final String LOG_TAG = "Fade";
     private static final String PROPNAME_SCREEN_X = "android:fade:screenX";
     private static final String PROPNAME_SCREEN_Y = "android:fade:screenY";
@@ -121,7 +123,7 @@ public class Fade extends Visibility {
         View view;
         View startView = (startValues != null) ? startValues.view : null;
         View endView = (endValues != null) ? endValues.view : null;
-        if (Transition.DBG) {
+        if (DBG) {
             Log.d(LOG_TAG, "Fade.predisappear: startView, startVis, endView, endVis = " +
                         startView + ", " + startVisibility + ", " + endView + ", " + endVisibility);
         }

diff --git a/core/java/android/view/transition/Transition.java b/core/java/android/view/transition/Transition.java
index 6d5e61a..f99ddc0 100644 (file)
--- a/core/java/android/view/transition/Transition.java
+++ b/core/java/android/view/transition/Transition.java
@@ -20,6 +20,7 @@ import android.animation.Animator;
 import android.animation.AnimatorListenerAdapter;
 import android.animation.TimeInterpolator;
 import android.util.ArrayMap;
+import android.util.Log;
 import android.util.LongSparseArray;
 import android.util.Pair;
 import android.util.SparseArray;
@@ -84,11 +85,14 @@ public abstract class Transition implements Cloneable {
     int mNumInstances = 0;
 
 
-    /**
-     * The set of listeners to be sent transition lifecycle events.
-     */
+
+    // The set of listeners to be sent transition lifecycle events.
     ArrayList<TransitionListener> mListeners = null;
 
+    // The set of animators collected from calls to play(), to be run in runAnimations()
+    ArrayMap<Pair<TransitionValues, TransitionValues>, Animator> mAnimatorMap =
+            new ArrayMap<Pair<TransitionValues, TransitionValues>, Animator>();
+
     /**
      * Constructs a Transition object with no target objects. A transition with
      * no targets defaults to running on all target objects in the scene hierarchy
@@ -203,6 +207,9 @@ public abstract class Transition implements Cloneable {
      */
     protected void play(ViewGroup sceneRoot, TransitionValuesMaps startValues,
             TransitionValuesMaps endValues) {
+        if (DBG) {
+            Log.d(LOG_TAG, "play() for " + this);
+        }
         mPlayStartValuesList.clear();
         mPlayEndValuesList.clear();
         ArrayMap<View, TransitionValues> endCopy =
@@ -312,20 +319,45 @@ public abstract class Transition implements Cloneable {
         for (int i = 0; i < startValuesList.size(); ++i) {
             TransitionValues start = startValuesList.get(i);
             TransitionValues end = endValuesList.get(i);
-            // TODO: what to do about targetIds and itemIds?
-            Animator animator = play(sceneRoot, start, end);
-            if (animator != null) {
-                mAnimatorMap.put(new Pair(start, end), animator);
-                // Note: we've already done the check against targetIDs in these lists
-                mPlayStartValuesList.add(start);
-                mPlayEndValuesList.add(end);
+            // Only bother trying to animate with values that differ between start/end
+            if (start != null || end != null) {
+                if (start == null || !start.equals(end)) {
+                    if (DBG) {
+                        View view = (end != null) ? end.view : start.view;
+                        Log.d(LOG_TAG, "  differing start/end values for view " +
+                                view);
+                        if (start == null || end == null) {
+                            if (start == null) {
+                                Log.d(LOG_TAG, "    " + ((start == null) ?
+                                        "start null, end non-null" : "start non-null, end null"));
+                            }
+                        } else {
+                            for (String key : start.values.keySet()) {
+                                Object startValue = start.values.get(key);
+                                Object endValue = end.values.get(key);
+                                if (startValue != endValue && !startValue.equals(endValue)) {
+                                    Log.d(LOG_TAG, "    " + key + ": start(" + startValue +
+                                            "), end(" + endValue +")");
+                                }
+                            }
+                        }
+                    }
+                    // TODO: what to do about targetIds and itemIds?
+                    Animator animator = play(sceneRoot, start, end);
+                    if (animator != null) {
+                        mAnimatorMap.put(new Pair(start, end), animator);
+                        // Note: we've already done the check against targetIDs in these lists
+                        mPlayStartValuesList.add(start);
+                        mPlayEndValuesList.add(end);
+                    }
+                } else if (DBG) {
+                    View view = (end != null) ? end.view : start.view;
+                    Log.d(LOG_TAG, "  No change for view " + view);
+                }
             }
         }
     }
 
-    ArrayMap<Pair<TransitionValues, TransitionValues>, Animator> mAnimatorMap =
-            new ArrayMap<Pair<TransitionValues, TransitionValues>, Animator>();
-
     /**
      * Internal utility method for checking whether a given view/id
      * is valid for this transition, where "valid" means that either
@@ -364,14 +396,20 @@ public abstract class Transition implements Cloneable {
      * @hide
      */
     protected void runAnimations() {
-
+        if (DBG && mPlayStartValuesList.size() > 0) {
+            Log.d(LOG_TAG, "runAnimations (" + mPlayStartValuesList.size() + ") on " + this);
+        }
         startTransition();
         // Now walk the list of TransitionValues, calling play for each pair
         for (int i = 0; i < mPlayStartValuesList.size(); ++i) {
             TransitionValues start = mPlayStartValuesList.get(i);
             TransitionValues end = mPlayEndValuesList.get(i);
+            Animator anim = mAnimatorMap.get(new Pair(start, end));
+            if (DBG) {
+                Log.d(LOG_TAG, "  anim: " + anim);
+            }
             startTransition();
-            runAnimator(mAnimatorMap.get(new Pair(start, end)));
+            runAnimator(anim);
         }
         mPlayStartValuesList.clear();
         mPlayEndValuesList.clear();
@@ -871,27 +909,35 @@ public abstract class Transition implements Cloneable {
     String toString(String indent) {
         String result = indent + getClass().getSimpleName() + "@" +
                 Integer.toHexString(hashCode()) + ": ";
-        result += "dur(" + mDuration + ") ";
-        result += "dly(" + mStartDelay + ") ";
-        result += "interp(" + mInterpolator + ") ";
-        result += "tgts(";
-        if (mTargetIds != null) {
-            for (int i = 0; i < mTargetIds.length; ++i) {
-                if (i > 0) {
-                    result += ", ";
+        if (mDuration != -1) {
+            result += "dur(" + mDuration + ") ";
+        }
+        if (mStartDelay != -1) {
+            result += "dly(" + mStartDelay + ") ";
+        }
+        if (mInterpolator != null) {
+            result += "interp(" + mInterpolator + ") ";
+        }
+        if (mTargetIds != null || mTargets != null) {
+            result += "tgts(";
+            if (mTargetIds != null) {
+                for (int i = 0; i < mTargetIds.length; ++i) {
+                    if (i > 0) {
+                        result += ", ";
+                    }
+                    result += mTargetIds[i];
                 }
-                result += mTargetIds[i];
             }
-        }
-        if (mTargets != null) {
-            for (int i = 0; i < mTargets.length; ++i) {
-                if (i > 0) {
-                    result += ", ";
+            if (mTargets != null) {
+                for (int i = 0; i < mTargets.length; ++i) {
+                    if (i > 0) {
+                        result += ", ";
+                    }
+                    result += mTargets[i];
                 }
-                result += mTargets[i];
             }
+            result += ")";
         }
-        result += ")";
         return result;
     }
 

diff --git a/core/java/android/view/transition/TransitionManager.java b/core/java/android/view/transition/TransitionManager.java
index b200a6d..7a3d9e2 100644 (file)
--- a/core/java/android/view/transition/TransitionManager.java
+++ b/core/java/android/view/transition/TransitionManager.java
@@ -17,6 +17,7 @@
 package android.view.transition;
 
 import android.util.ArrayMap;
+import android.util.Log;
 import android.view.ViewGroup;
 import android.view.ViewTreeObserver;
 
@@ -36,6 +37,8 @@ import java.util.ArrayList;
 public class TransitionManager {
     // TODO: how to handle enter/exit?
 
+    private static String LOG_TAG = "TransitionManager";
+
     private static final Transition sDefaultTransition = new AutoTransition();
     private Transition mDefaultTransition = new AutoTransition();
 
@@ -164,6 +167,7 @@ public class TransitionManager {
             observer.addOnPreDrawListener(new ViewTreeObserver.OnPreDrawListener() {
                 public boolean onPreDraw() {
                     sceneRoot.getViewTreeObserver().removeOnPreDrawListener(this);
+                    sPendingTransitions.remove(sceneRoot);
                     // Add to running list, handle end to remove it
                     sRunningTransitions.put(sceneRoot, transition);
                     transition.addListener(new Transition.TransitionListenerAdapter() {
@@ -316,8 +320,11 @@ public class TransitionManager {
      * value of null causes the TransitionManager to use the default transition.
      */
     public static void beginDelayedTransition(final ViewGroup sceneRoot, Transition transition) {
-
-        if (!sPendingTransitions.contains(sceneRoot)) {
+        if (!sPendingTransitions.contains(sceneRoot) && sceneRoot.hasLayout()) {
+            if (Transition.DBG) {
+                Log.d(LOG_TAG, "beginDelayedTransition: root, transition = " +
+                        sceneRoot + ", " + transition);
+            }
             sPendingTransitions.add(sceneRoot);
             if (transition == null) {
                 transition = sDefaultTransition;
@@ -325,13 +332,7 @@ public class TransitionManager {
             final Transition finalTransition = transition.clone();
             sceneChangeSetup(sceneRoot, transition);
             sceneRoot.setCurrentScene(null);
-            sceneRoot.postOnAnimation(new Runnable() {
-                @Override
-                public void run() {
-                    sPendingTransitions.remove(sceneRoot);
-                    sceneChangeRunTransition(sceneRoot, finalTransition);
-                }
-            });
+            sceneChangeRunTransition(sceneRoot, finalTransition);
         }
     }
 }

diff --git a/core/java/android/view/transition/TransitionValues.java b/core/java/android/view/transition/TransitionValues.java
index f361666..6e5d3d3 100644 (file)
--- a/core/java/android/view/transition/TransitionValues.java
+++ b/core/java/android/view/transition/TransitionValues.java
@@ -53,6 +53,23 @@ public class TransitionValues {
     public final Map<String, Object> values = new ArrayMap<String, Object>();
 
     @Override
+    public boolean equals(Object other) {
+        if (other instanceof TransitionValues) {
+            if (view == ((TransitionValues) other).view) {
+                if (values.equals(((TransitionValues) other).values)) {
+                    return true;
+                }
+            }
+        }
+        return false;
+    }
+
+    @Override
+    public int hashCode() {
+        return 31*view.hashCode() + values.hashCode();
+    }
+
+    @Override
     public String toString() {
         String returnValue = "TransitionValues@" + Integer.toHexString(hashCode()) + ":\n";
         returnValue += "    view = " + view + "\n";