Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 0942153f authored by Fabián Kozynski's avatar Fabián Kozynski
Browse files

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
parent 3459cd0e

packages/SystemUI/src/com/android/systemui/qs/tileimpl/QSIconViewImpl.java

+13 −4
Original line number Diff line number Diff line
@@ -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 class QSIconViewImpl extends QSIconView { 
    }



    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 @@ public class QSIconViewImpl extends QSIconView { 
            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 @@ public class QSIconViewImpl extends QSIconView { 
        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

     */

packages/SystemUI/tests/src/com/android/systemui/qs/tileimpl/QSIconViewImplTest_311121830.kt

+50 −1
Original line number Diff line number Diff line
@@ -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) {