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

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

Fix icon update race condition 

When the icon is set in QSIconViewImpl there are two different paths
that could be followed:

* If the `State.state` changes (and then the tint of the icon should
  change), we animate the color of the existing icon, and only set the
  new icon when the animation is finished.
* If the `State.state` didn't change, we immediately set the new icon.

Therefore, if two calls to change the state come in short succession, it
could happen that the icon for the first call is set after the one for
the second call (because it was set at the end of an animation).

If that is the case, with this fix, we ignore the schedule icon change
at the end of the animation if another icon has been set in the
meantime.

Test: atest QSIconViewImplTest_311121830
Test: manual, turn APM on and off multiple times with QS open
Fixes: 311121830
Fixes: 293524288
Flag: None

Change-Id: Id150f36d10c374f30e72d658bd833e4eae7677b4
parent 2791d50e

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

+14 −3
Original line number Diff line number Diff line
@@ -33,11 +33,13 @@ import android.view.View; 
import android.widget.ImageView;

import android.widget.ImageView.ScaleType;



import androidx.annotation.VisibleForTesting;



import com.android.settingslib.Utils;

import com.android.systemui.res.R;

import com.android.systemui.plugins.qs.QSIconView;

import com.android.systemui.plugins.qs.QSTile;

import com.android.systemui.plugins.qs.QSTile.State;

import com.android.systemui.res.R;



import java.util.Objects;
@@ -52,7 +54,10 @@ public class QSIconViewImpl extends QSIconView { 
    private boolean mDisabledByPolicy = false;

    private int mTint;

    @Nullable

    private QSTile.Icon mLastIcon;

    @VisibleForTesting

    QSTile.Icon mLastIcon;



    private boolean mIconChangeScheduled;



    private ValueAnimator mColorAnimator = new ValueAnimator();
@@ -112,6 +117,7 @@ public class QSIconViewImpl extends QSIconView { 
    }



    protected void updateIcon(ImageView iv, State state, boolean allowAnimations) {

        mIconChangeScheduled = false;

        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);
@@ -167,7 +173,12 @@ public class QSIconViewImpl extends QSIconView { 
            mState = state.state;

            mDisabledByPolicy = state.disabledByPolicy;

            if (mTint != 0 && allowAnimations && shouldAnimate(iv)) {

                animateGrayScale(mTint, color, iv, () -> updateIcon(iv, state, allowAnimations));

                mIconChangeScheduled = true;

                animateGrayScale(mTint, color, iv, () -> {

                    if (mIconChangeScheduled) {

                        updateIcon(iv, state, allowAnimations);

                    }

                });

            } else {

                setTint(iv, color);

                updateIcon(iv, state, allowAnimations);

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

0 → 100644
+94 −0
Original line number Diff line number Diff line 
/*

 * Copyright (C) 2024 The Android Open Source Project

 *

 * Licensed under the Apache License, Version 2.0 (the "License");

 * you may not use this file except in compliance with the License.

 * You may obtain a copy of the License at

 *

 *      http://www.apache.org/licenses/LICENSE-2.0

 *

 * Unless required by applicable law or agreed to in writing, software

 * distributed under the License is distributed on an "AS IS" BASIS,

 * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.

 * See the License for the specific language governing permissions and

 * limitations under the License.

 */



package com.android.systemui.qs.tileimpl



import android.animation.AnimatorTestRule

import android.content.Context

import android.service.quicksettings.Tile

import android.testing.AndroidTestingRunner

import android.testing.UiThreadTest

import android.view.ContextThemeWrapper

import android.view.View

import android.widget.ImageView

import androidx.test.filters.SmallTest

import com.android.systemui.SysuiTestCase

import com.android.systemui.plugins.qs.QSTile

import com.android.systemui.res.R

import com.android.systemui.statusbar.connectivity.WifiIcons

import com.google.common.truth.Truth.assertThat

import kotlin.test.Test

import org.junit.Rule

import org.junit.runner.RunWith



/** Test for regression b/311121830 */

@RunWith(AndroidTestingRunner::class)

@UiThreadTest

@SmallTest

class QSIconViewImplTest_311121830 : SysuiTestCase() {



    @get:Rule val animatorRule = AnimatorTestRule()



    @Test

    fun alwaysLastIcon() {

        // 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_INACTIVE

                icon = QSTileImpl.ResourceIcon.get(R.drawable.ic_qs_no_internet_available)

            }

        val firstState =

            QSTile.State().apply {

                state = Tile.STATE_ACTIVE

                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[4])

            }



        // Start with the initial state

        iconView.setIcon(initialState, /* allowAnimations= */ false)



        // Set the first state to animate, and advance time to half the time of the animation

        iconView.setIcon(firstState, /* allowAnimations= */ true)

        animatorRule.advanceTimeBy(QSIconViewImpl.QS_ANIM_LENGTH / 2)



        // Set the second state to animate (it shouldn't, because `State.state` is the same) and

        // advance time to 2 animations length

        iconView.setIcon(secondState, /* allowAnimations= */ true)

        animatorRule.advanceTimeBy(QSIconViewImpl.QS_ANIM_LENGTH * 2)



        assertThat(iconView.mLastIcon).isEqualTo(secondState.icon)

    }



    private class AnimateQSIconViewImpl(context: Context) : QSIconViewImpl(context) {

        override fun createIcon(): View {

            return object : ImageView(context) {

                override fun isShown(): Boolean {

                    return true

                }

            }

        }

    }

}