From b631f9cf56cf24e4d3943a47c085f0af880e9bff Mon Sep 17 00:00:00 2001 From: George Mount Date: Thu, 1 Jun 2023 08:39:37 -0700 Subject: [PATCH] Don't call property getters during initialization. Fixes: 274967902 A robustness was added to AnimatorSet initialization where property values could be queried while initializing an AnimatorSet so that it could handle an edge case where AnimatorSets had cross-talk in their child animators (two sets playing together have childr animators that animate the same value). However, some clients will crash when a property value is requested during the initialization, so that feature was rolled back. Test: new test, existing animation CTS and APCT Change-Id: Iba6e41d57fab80fb30beda8438c3c202477e1166 --- core/java/android/animation/Animator.java | 4 +- core/java/android/animation/AnimatorSet.java | 92 +++++++------------ .../java/android/animation/ValueAnimator.java | 28 +++--- .../animation/AnimatorSetActivityTest.java | 37 ++++++++ 4 files changed, 84 insertions(+), 77 deletions(-) diff --git a/core/java/android/animation/Animator.java b/core/java/android/animation/Animator.java index 12026aa3f72a6..4cad58521c093 100644 --- a/core/java/android/animation/Animator.java +++ b/core/java/android/animation/Animator.java @@ -568,13 +568,13 @@ public abstract class Animator implements Cloneable { * repetition. lastPlayTime is similar and is used to calculate how many repeats have been * done between the two times. */ - void animateValuesInRange(long currentPlayTime, long lastPlayTime, boolean notify) {} + void animateValuesInRange(long currentPlayTime, long lastPlayTime) {} /** * Internal use only. This animates any animation that has ended since lastPlayTime. * If an animation hasn't been finished, no change will be made. */ - void animateSkipToEnds(long currentPlayTime, long lastPlayTime, boolean notify) {} + void animateSkipToEnds(long currentPlayTime, long lastPlayTime) {} /** * Internal use only. Adds all start times (after delay) to and end times to times. diff --git a/core/java/android/animation/AnimatorSet.java b/core/java/android/animation/AnimatorSet.java index 60659dc12342c..70c3d7ae3f82a 100644 --- a/core/java/android/animation/AnimatorSet.java +++ b/core/java/android/animation/AnimatorSet.java @@ -825,8 +825,7 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim private void animateBasedOnPlayTime( long currentPlayTime, long lastPlayTime, - boolean inReverse, - boolean notify + boolean inReverse ) { if (currentPlayTime < 0 || lastPlayTime < -1) { throw new UnsupportedOperationException("Error: Play time should never be negative."); @@ -857,8 +856,8 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim while (index < endIndex) { long playTime = startEndTimes[index]; if (lastPlayTime != playTime) { - animateSkipToEnds(playTime, lastPlayTime, notify); - animateValuesInRange(playTime, lastPlayTime, notify); + animateSkipToEnds(playTime, lastPlayTime); + animateValuesInRange(playTime, lastPlayTime); lastPlayTime = playTime; } index++; @@ -868,15 +867,15 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim index--; long playTime = startEndTimes[index]; if (lastPlayTime != playTime) { - animateSkipToEnds(playTime, lastPlayTime, notify); - animateValuesInRange(playTime, lastPlayTime, notify); + animateSkipToEnds(playTime, lastPlayTime); + animateValuesInRange(playTime, lastPlayTime); lastPlayTime = playTime; } } } if (currentPlayTime != lastPlayTime) { - animateSkipToEnds(currentPlayTime, lastPlayTime, notify); - animateValuesInRange(currentPlayTime, lastPlayTime, notify); + animateSkipToEnds(currentPlayTime, lastPlayTime); + animateValuesInRange(currentPlayTime, lastPlayTime); } } @@ -896,13 +895,11 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim } @Override - void animateSkipToEnds(long currentPlayTime, long lastPlayTime, boolean notify) { + void animateSkipToEnds(long currentPlayTime, long lastPlayTime) { initAnimation(); if (lastPlayTime > currentPlayTime) { - if (notify) { - notifyStartListeners(true); - } + notifyStartListeners(true); for (int i = mEvents.size() - 1; i >= 0; i--) { AnimationEvent event = mEvents.get(i); Node node = event.mNode; @@ -916,31 +913,25 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim if (currentPlayTime <= start && start < lastPlayTime) { animator.animateSkipToEnds( 0, - lastPlayTime - node.mStartTime, - notify + lastPlayTime - node.mStartTime ); - if (notify) { - mPlayingSet.remove(node); - } + mPlayingSet.remove(node); } else if (start <= currentPlayTime && currentPlayTime <= end) { animator.animateSkipToEnds( currentPlayTime - node.mStartTime, - lastPlayTime - node.mStartTime, - notify + lastPlayTime - node.mStartTime ); - if (notify && !mPlayingSet.contains(node)) { + if (!mPlayingSet.contains(node)) { mPlayingSet.add(node); } } } } - if (currentPlayTime <= 0 && notify) { + if (currentPlayTime <= 0) { notifyEndListeners(true); } } else { - if (notify) { - notifyStartListeners(false); - } + notifyStartListeners(false); int eventsSize = mEvents.size(); for (int i = 0; i < eventsSize; i++) { AnimationEvent event = mEvents.get(i); @@ -955,45 +946,39 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim if (lastPlayTime < end && end <= currentPlayTime) { animator.animateSkipToEnds( end - node.mStartTime, - lastPlayTime - node.mStartTime, - notify + lastPlayTime - node.mStartTime ); - if (notify) { - mPlayingSet.remove(node); - } + mPlayingSet.remove(node); } else if (start <= currentPlayTime && currentPlayTime <= end) { animator.animateSkipToEnds( currentPlayTime - node.mStartTime, - lastPlayTime - node.mStartTime, - notify + lastPlayTime - node.mStartTime ); - if (notify && !mPlayingSet.contains(node)) { + if (!mPlayingSet.contains(node)) { mPlayingSet.add(node); } } } } - if (currentPlayTime >= getTotalDuration() && notify) { + if (currentPlayTime >= getTotalDuration()) { notifyEndListeners(false); } } } @Override - void animateValuesInRange(long currentPlayTime, long lastPlayTime, boolean notify) { + void animateValuesInRange(long currentPlayTime, long lastPlayTime) { initAnimation(); - if (notify) { - if (lastPlayTime < 0 || (lastPlayTime == 0 && currentPlayTime > 0)) { - notifyStartListeners(false); - } else { - long duration = getTotalDuration(); - if (duration >= 0 - && (lastPlayTime > duration || (lastPlayTime == duration - && currentPlayTime < duration)) - ) { - notifyStartListeners(true); - } + if (lastPlayTime < 0 || (lastPlayTime == 0 && currentPlayTime > 0)) { + notifyStartListeners(false); + } else { + long duration = getTotalDuration(); + if (duration >= 0 + && (lastPlayTime > duration || (lastPlayTime == duration + && currentPlayTime < duration)) + ) { + notifyStartListeners(true); } } @@ -1014,8 +999,7 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim ) { animator.animateValuesInRange( currentPlayTime - node.mStartTime, - Math.max(-1, lastPlayTime - node.mStartTime), - notify + Math.max(-1, lastPlayTime - node.mStartTime) ); } } @@ -1111,7 +1095,7 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim } } mSeekState.setPlayTime(playTime, mReversing); - animateBasedOnPlayTime(playTime, lastPlayTime, mReversing, true); + animateBasedOnPlayTime(playTime, lastPlayTime, mReversing); } /** @@ -1144,16 +1128,7 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim private void initChildren() { if (!isInitialized()) { mChildrenInitialized = true; - - // We have to initialize all the start values so that they are based on the previous - // values. - long[] times = ensureChildStartAndEndTimes(); - - long previousTime = -1; - for (long time : times) { - animateBasedOnPlayTime(time, previousTime, false, false); - previousTime = time; - } + skipToEndValue(false); } } @@ -1489,6 +1464,7 @@ public final class AnimatorSet extends Animator implements AnimationHandler.Anim anim.mPauseTime = -1; anim.mSeekState = new SeekState(); anim.mSelfPulse = true; + anim.mStartListenersCalled = false; anim.mPlayingSet = new ArrayList(); anim.mNodeMap = new ArrayMap(); anim.mNodes = new ArrayList(nodeCount); diff --git a/core/java/android/animation/ValueAnimator.java b/core/java/android/animation/ValueAnimator.java index ead238f75ba47..5de7f387b2068 100644 --- a/core/java/android/animation/ValueAnimator.java +++ b/core/java/android/animation/ValueAnimator.java @@ -1417,21 +1417,19 @@ public class ValueAnimator extends Animator implements AnimationHandler.Animatio * will be called. */ @Override - void animateValuesInRange(long currentPlayTime, long lastPlayTime, boolean notify) { + void animateValuesInRange(long currentPlayTime, long lastPlayTime) { if (currentPlayTime < 0 || lastPlayTime < -1) { throw new UnsupportedOperationException("Error: Play time should never be negative."); } initAnimation(); long duration = getTotalDuration(); - if (notify) { - if (lastPlayTime < 0 || (lastPlayTime == 0 && currentPlayTime > 0)) { - notifyStartListeners(false); - } else if (lastPlayTime > duration - || (lastPlayTime == duration && currentPlayTime < duration) - ) { - notifyStartListeners(true); - } + if (lastPlayTime < 0 || (lastPlayTime == 0 && currentPlayTime > 0)) { + notifyStartListeners(false); + } else if (lastPlayTime > duration + || (lastPlayTime == duration && currentPlayTime < duration) + ) { + notifyStartListeners(true); } if (duration >= 0) { lastPlayTime = Math.min(duration, lastPlayTime); @@ -1448,7 +1446,7 @@ public class ValueAnimator extends Animator implements AnimationHandler.Animatio iteration = Math.min(iteration, mRepeatCount); lastIteration = Math.min(lastIteration, mRepeatCount); - if (notify && iteration != lastIteration) { + if (iteration != lastIteration) { notifyListeners(AnimatorCaller.ON_REPEAT, false); } } @@ -1464,7 +1462,7 @@ public class ValueAnimator extends Animator implements AnimationHandler.Animatio } @Override - void animateSkipToEnds(long currentPlayTime, long lastPlayTime, boolean notify) { + void animateSkipToEnds(long currentPlayTime, long lastPlayTime) { boolean inReverse = currentPlayTime < lastPlayTime; boolean doSkip; if (currentPlayTime <= 0 && lastPlayTime > 0) { @@ -1474,13 +1472,9 @@ public class ValueAnimator extends Animator implements AnimationHandler.Animatio doSkip = duration >= 0 && currentPlayTime >= duration && lastPlayTime < duration; } if (doSkip) { - if (notify) { - notifyStartListeners(inReverse); - } + notifyStartListeners(inReverse); skipToEndValue(inReverse); - if (notify) { - notifyEndListeners(inReverse); - } + notifyEndListeners(inReverse); } } diff --git a/core/tests/coretests/src/android/animation/AnimatorSetActivityTest.java b/core/tests/coretests/src/android/animation/AnimatorSetActivityTest.java index a7538701807a3..0525443ecf82b 100644 --- a/core/tests/coretests/src/android/animation/AnimatorSetActivityTest.java +++ b/core/tests/coretests/src/android/animation/AnimatorSetActivityTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertTrue; +import android.util.Property; import android.view.View; import androidx.test.annotation.UiThreadTest; @@ -576,6 +577,42 @@ public class AnimatorSetActivityTest { }); } + @Test + public void testInitializeWithoutReadingValues() throws Throwable { + // Some consumers crash while reading values before the animator starts + Property property = new Property<>(Integer.class, "firstValue") { + @Override + public Integer get(int[] target) { + throw new IllegalStateException("Shouldn't be called"); + } + + @Override + public void set(int[] target, Integer value) { + target[0] = value; + } + }; + + int[] target1 = new int[1]; + int[] target2 = new int[1]; + int[] target3 = new int[1]; + ObjectAnimator animator1 = ObjectAnimator.ofInt(target1, property, 0, 100); + ObjectAnimator animator2 = ObjectAnimator.ofInt(target2, property, 0, 100); + ObjectAnimator animator3 = ObjectAnimator.ofInt(target3, property, 0, 100); + AnimatorSet set = new AnimatorSet(); + set.playSequentially(animator1, animator2, animator3); + + mActivityRule.runOnUiThread(() -> { + set.setCurrentPlayTime(900); + assertEquals(100, target1[0]); + assertEquals(100, target2[0]); + assertEquals(100, target3[0]); + set.setCurrentPlayTime(0); + assertEquals(0, target1[0]); + assertEquals(0, target2[0]); + assertEquals(0, target3[0]); + }); + } + /** * Check that the animator list contains exactly the given animators and nothing else. */