From c43524f3869cc0d36974fce61986017093f2ecd2 Mon Sep 17 00:00:00 2001 From: Chet Haase Date: Tue, 16 Jul 2013 14:40:11 -0700 Subject: [PATCH] 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 --- core/java/android/view/transition/Fade.java | 4 +- core/java/android/view/transition/Transition.java | 108 +++++++++++++++------ .../android/view/transition/TransitionManager.java | 19 ++-- .../android/view/transition/TransitionValues.java | 17 ++++ 4 files changed, 107 insertions(+), 41 deletions(-) diff --git a/core/java/android/view/transition/Fade.java b/core/java/android/view/transition/Fade.java index c2aa90beba76..4fd60c14f47e 100644 --- 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 6d5e61ab8209..f99ddc0f018d 100644 --- 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 mListeners = null; + // The set of animators collected from calls to play(), to be run in runAnimations() + ArrayMap, Animator> mAnimatorMap = + new ArrayMap, 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 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, Animator> mAnimatorMap = - new ArrayMap, 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 b200a6d5f117..7a3d9e2d15a8 100644 --- 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 f36166655df6..6e5d3d332d43 100644 --- 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 values = new ArrayMap(); @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"; -- 2.11.0