From 0942153f1f617c0ccfb7d5335c2159d4924ae135 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Fabi=C3=A1n=20Kozynski?= Date: Thu, 1 Feb 2024 15:52:13 -0500 Subject: [PATCH] Fix a newer race condition in QSIconViewImpl If multiple changes of state are pushed into QSIconViewImpl, they will all schedule an icon change, and cancel the last animator, prompting the previous icon to be displayed. However, because setting an icon would prevent other scheduled icons to be applied, only the first one was applied. With this fix, we assign a transaction ID (long) to each icon that is scheduled to be applied. We only apply it at the end (or cancel) of the animation if no other icon has been applied or scheduled since. Test: atest QSIconViewImplTest_311121830 Fixes: 323125376 Flag: NONE Change-Id: I8fd5eef54b3bee034f8066bc5ce79117c6260008 --- .../systemui/qs/tileimpl/QSIconViewImpl.java | 17 +++++-- .../tileimpl/QSIconViewImplTest_311121830.kt | 51 ++++++++++++++++++- 2 files changed, 63 insertions(+), 5 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/qs/tileimpl/QSIconViewImpl.java b/packages/SystemUI/src/com/android/systemui/qs/tileimpl/QSIconViewImpl.java index 53f287b81be9..720120b630d5 100644 --- a/packages/SystemUI/src/com/android/systemui/qs/tileimpl/QSIconViewImpl.java +++ b/packages/SystemUI/src/com/android/systemui/qs/tileimpl/QSIconViewImpl.java @@ -47,6 +47,8 @@ public class QSIconViewImpl extends QSIconView { public static final long QS_ANIM_LENGTH = 350; + private static final long ICON_APPLIED_TRANSACTION_ID = -1; + protected final View mIcon; protected int mIconSizePx; private boolean mAnimationEnabled = true; @@ -57,7 +59,8 @@ public class QSIconViewImpl extends QSIconView { @VisibleForTesting QSTile.Icon mLastIcon; - private boolean mIconChangeScheduled; + private long mScheduledIconChangeTransactionId = ICON_APPLIED_TRANSACTION_ID; + private long mHighestScheduledIconChangeTransactionId = ICON_APPLIED_TRANSACTION_ID; private ValueAnimator mColorAnimator = new ValueAnimator(); @@ -117,7 +120,7 @@ public void setIcon(State state, boolean allowAnimations) { } protected void updateIcon(ImageView iv, State state, boolean allowAnimations) { - mIconChangeScheduled = false; + mScheduledIconChangeTransactionId = ICON_APPLIED_TRANSACTION_ID; final QSTile.Icon icon = state.iconSupplier != null ? state.iconSupplier.get() : state.icon; if (!Objects.equals(icon, iv.getTag(R.id.qs_icon_tag))) { boolean shouldAnimate = allowAnimations && shouldAnimate(iv); @@ -173,9 +176,10 @@ protected void setIcon(ImageView iv, QSTile.State state, boolean allowAnimations mState = state.state; mDisabledByPolicy = state.disabledByPolicy; if (mTint != 0 && allowAnimations && shouldAnimate(iv)) { - mIconChangeScheduled = true; + final long iconTransactionId = getNextIconTransactionId(); + mScheduledIconChangeTransactionId = iconTransactionId; animateGrayScale(mTint, color, iv, () -> { - if (mIconChangeScheduled) { + if (mScheduledIconChangeTransactionId == iconTransactionId) { updateIcon(iv, state, allowAnimations); } }); @@ -237,6 +241,11 @@ protected final void layout(View child, int left, int top) { child.layout(left, top, left + child.getMeasuredWidth(), top + child.getMeasuredHeight()); } + private long getNextIconTransactionId() { + mHighestScheduledIconChangeTransactionId++; + return mHighestScheduledIconChangeTransactionId; + } + /** * Color to tint the tile icon based on state */ diff --git a/packages/SystemUI/tests/src/com/android/systemui/qs/tileimpl/QSIconViewImplTest_311121830.kt b/packages/SystemUI/tests/src/com/android/systemui/qs/tileimpl/QSIconViewImplTest_311121830.kt index e8aa8f0bdc5d..bbae0c90170a 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/qs/tileimpl/QSIconViewImplTest_311121830.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/qs/tileimpl/QSIconViewImplTest_311121830.kt @@ -34,7 +34,7 @@ import kotlin.test.Test import org.junit.Rule import org.junit.runner.RunWith -/** Test for regression b/311121830 */ +/** Test for regression b/311121830 and b/323125376 */ @RunWith(AndroidTestingRunner::class) @UiThreadTest @SmallTest @@ -82,6 +82,55 @@ class QSIconViewImplTest_311121830 : SysuiTestCase() { assertThat(iconView.mLastIcon).isEqualTo(secondState.icon) } + @Test + fun alwaysLastIcon_twoStateChanges() { + // Need to inflate with the correct theme so the colors can be retrieved and the animations + // are run + val iconView = + AnimateQSIconViewImpl( + ContextThemeWrapper(context, R.style.Theme_SystemUI_QuickSettings) + ) + + val initialState = + QSTile.State().apply { + state = Tile.STATE_ACTIVE + icon = QSTileImpl.ResourceIcon.get(WifiIcons.WIFI_FULL_ICONS[4]) + } + val firstState = + QSTile.State().apply { + state = Tile.STATE_INACTIVE + icon = QSTileImpl.ResourceIcon.get(WifiIcons.WIFI_NO_INTERNET_ICONS[4]) + } + val secondState = + QSTile.State().apply { + state = Tile.STATE_ACTIVE + icon = QSTileImpl.ResourceIcon.get(WifiIcons.WIFI_FULL_ICONS[3]) + } + val thirdState = + QSTile.State().apply { + state = Tile.STATE_INACTIVE + icon = QSTileImpl.ResourceIcon.get(WifiIcons.WIFI_NO_NETWORK) + } + + // Start with the initial state + iconView.setIcon(initialState, /* allowAnimations= */ false) + + // Set the first state to animate, and advance time to one third of the animation + iconView.setIcon(firstState, /* allowAnimations= */ true) + animatorRule.advanceTimeBy(QSIconViewImpl.QS_ANIM_LENGTH / 3) + + // Set the second state to animate and advance time by another third of animations length + iconView.setIcon(secondState, /* allowAnimations= */ true) + animatorRule.advanceTimeBy(QSIconViewImpl.QS_ANIM_LENGTH / 3) + + // Set the third state to animate and advance time by two times the animation length + // to guarantee that all animations are done + iconView.setIcon(thirdState, /* allowAnimations= */ true) + animatorRule.advanceTimeBy(QSIconViewImpl.QS_ANIM_LENGTH * 2) + + assertThat(iconView.mLastIcon).isEqualTo(thirdState.icon) + } + private class AnimateQSIconViewImpl(context: Context) : QSIconViewImpl(context) { override fun createIcon(): View { return object : ImageView(context) {