Merge "[Chipbar] Force the views' alphas to 1 if the animate in fails." into tm-qpr-dev am: b68a869107 am: 07e560cd76

Original change: https://googleplex-android-review.googlesource.com/c/platform/frameworks/base/+/21098625 Change-Id: Ief446ebad42b7d6fe0708188056ff4f609f2ef61 Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
2023-01-31 17:11:59 +00:00
parent b4ee4ef146 07e560cd76
commit e3499afea9
5 changed files with 180 additions and 29 deletions
--- a/packages/SystemUI/src/com/android/systemui/temporarydisplay/TemporaryViewLogger.kt
+++ b/packages/SystemUI/src/com/android/systemui/temporarydisplay/TemporaryViewLogger.kt
@@ -80,6 +80,26 @@ open class TemporaryViewLogger<T : TemporaryViewInfo>(
        )
    }

+    /** Logs that there was a failure to animate the view in. */
+    fun logAnimateInFailure() {
+        buffer.log(
+            tag,
+            LogLevel.WARNING,
+            {},
+            { "View's appearance animation failed. Forcing view display manually." },
+        )
+    }
+
+    /** Logs that there was a failure to animate the view out. */
+    fun logAnimateOutFailure() {
+        buffer.log(
+            tag,
+            LogLevel.WARNING,
+            {},
+            { "View's disappearance animation failed." },
+        )
+    }
+
    fun logViewHidden(info: T) {
        buffer.log(
            tag,
--- a/packages/SystemUI/src/com/android/systemui/temporarydisplay/chipbar/ChipbarAnimator.kt
+++ b/packages/SystemUI/src/com/android/systemui/temporarydisplay/chipbar/ChipbarAnimator.kt
@@ -0,0 +1,83 @@
+/*
+ * Copyright (C) 2023 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.temporarydisplay.chipbar
+
+import android.view.View
+import android.view.ViewGroup
+import com.android.systemui.animation.Interpolators
+import com.android.systemui.animation.ViewHierarchyAnimator
+import com.android.systemui.dagger.SysUISingleton
+import com.android.systemui.util.children
+import javax.inject.Inject
+
+/**
+ * A class controlling chipbar animations. Typically delegates to [ViewHierarchyAnimator].
+ *
+ * Used so that animations can be mocked out in tests.
+ */
+@SysUISingleton
+open class ChipbarAnimator @Inject constructor() {
+    /**
+     * Animates [innerView] and its children into view.
+     *
+     * @return true if the animation was successfully started and false if the animation can't be
+     * run for any reason.
+     *
+     * See [ViewHierarchyAnimator.animateAddition].
+     */
+    open fun animateViewIn(innerView: ViewGroup, onAnimationEnd: Runnable): Boolean {
+        return ViewHierarchyAnimator.animateAddition(
+            innerView,
+            ViewHierarchyAnimator.Hotspot.TOP,
+            Interpolators.EMPHASIZED_DECELERATE,
+            duration = ANIMATION_IN_DURATION,
+            includeMargins = true,
+            includeFadeIn = true,
+            onAnimationEnd = onAnimationEnd,
+        )
+    }
+
+    /**
+     * Animates [innerView] and its children out of view.
+     *
+     * @return true if the animation was successfully started and false if the animation can't be
+     * run for any reason.
+     *
+     * See [ViewHierarchyAnimator.animateRemoval].
+     */
+    open fun animateViewOut(innerView: ViewGroup, onAnimationEnd: Runnable): Boolean {
+        return ViewHierarchyAnimator.animateRemoval(
+            innerView,
+            ViewHierarchyAnimator.Hotspot.TOP,
+            Interpolators.EMPHASIZED_ACCELERATE,
+            ANIMATION_OUT_DURATION,
+            includeMargins = true,
+            onAnimationEnd,
+        )
+    }
+
+    /** Force shows this view and all child views. Should be used in case [animateViewIn] fails. */
+    fun forceDisplayView(innerView: View) {
+        innerView.alpha = 1f
+        if (innerView is ViewGroup) {
+            innerView.children.forEach { forceDisplayView(it) }
+        }
+    }
+}
+
+private const val ANIMATION_IN_DURATION = 500L
+private const val ANIMATION_OUT_DURATION = 250L
--- a/packages/SystemUI/src/com/android/systemui/temporarydisplay/chipbar/ChipbarCoordinator.kt
+++ b/packages/SystemUI/src/com/android/systemui/temporarydisplay/chipbar/ChipbarCoordinator.kt
@@ -32,8 +32,6 @@ import androidx.annotation.IdRes
 import com.android.internal.widget.CachingIconView
 import com.android.systemui.Gefingerpoken
 import com.android.systemui.R
-import com.android.systemui.animation.Interpolators
-import com.android.systemui.animation.ViewHierarchyAnimator
 import com.android.systemui.classifier.FalsingCollector
 import com.android.systemui.common.shared.model.ContentDescription.Companion.loadContentDescription
 import com.android.systemui.common.shared.model.Text.Companion.loadText
@@ -78,6 +76,7 @@ constructor(
    configurationController: ConfigurationController,
    dumpManager: DumpManager,
    powerManager: PowerManager,
+    private val chipbarAnimator: ChipbarAnimator,
    private val falsingManager: FalsingManager,
    private val falsingCollector: FalsingCollector,
    private val swipeChipbarAwayGestureHandler: SwipeChipbarAwayGestureHandler?,
@@ -206,23 +205,17 @@ constructor(
    }

    override fun animateViewIn(view: ViewGroup) {
+        // We can only request focus once the animation finishes.
        val onAnimationEnd = Runnable {
            maybeGetAccessibilityFocus(view.getTag(INFO_TAG) as ChipbarInfo?, view)
        }
-        val added =
-            ViewHierarchyAnimator.animateAddition(
-                view.getInnerView(),
-                ViewHierarchyAnimator.Hotspot.TOP,
-                Interpolators.EMPHASIZED_DECELERATE,
-                duration = ANIMATION_IN_DURATION,
-                includeMargins = true,
-                includeFadeIn = true,
-                // We can only request focus once the animation finishes.
-                onAnimationEnd = onAnimationEnd,
-            )
-        // If the view doesn't get animated, the [onAnimationEnd] runnable won't get run. So, just
-        // run it immediately.
-        if (!added) {
+        val animatedIn = chipbarAnimator.animateViewIn(view.getInnerView(), onAnimationEnd)
+
+        // If the view doesn't get animated, the [onAnimationEnd] runnable won't get run and the
+        // views would remain un-displayed. So, just force-set/run those items immediately.
+        if (!animatedIn) {
+            logger.logAnimateInFailure()
+            chipbarAnimator.forceDisplayView(view.getInnerView())
            onAnimationEnd.run()
        }
    }
@@ -230,18 +223,11 @@ constructor(
    override fun animateViewOut(view: ViewGroup, removalReason: String?, onAnimationEnd: Runnable) {
        val innerView = view.getInnerView()
        innerView.accessibilityLiveRegion = ACCESSIBILITY_LIVE_REGION_NONE
-        val removed =
-            ViewHierarchyAnimator.animateRemoval(
-                innerView,
-                ViewHierarchyAnimator.Hotspot.TOP,
-                Interpolators.EMPHASIZED_ACCELERATE,
-                ANIMATION_OUT_DURATION,
-                includeMargins = true,
-                onAnimationEnd,
-            )
+        val removed = chipbarAnimator.animateViewOut(innerView, onAnimationEnd)
        // If the view doesn't get animated, the [onAnimationEnd] runnable won't get run. So, just
        // run it immediately.
        if (!removed) {
+            logger.logAnimateOutFailure()
            onAnimationEnd.run()
        }

@@ -299,8 +285,6 @@ constructor(
    }
 }

-private const val ANIMATION_IN_DURATION = 500L
-private const val ANIMATION_OUT_DURATION = 250L
@IdRes private val INFO_TAG = R.id.tag_chipbar_info
 private const val SWIPE_UP_GESTURE_REASON = "SWIPE_UP_GESTURE_DETECTED"
 private const val TAG = "ChipbarCoordinator"
--- a/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinatorTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/media/taptotransfer/sender/MediaTttSenderCoordinatorTest.kt
@@ -45,6 +45,7 @@ import com.android.systemui.statusbar.CommandQueue
 import com.android.systemui.statusbar.VibratorHelper
 import com.android.systemui.statusbar.policy.ConfigurationController
 import com.android.systemui.temporarydisplay.TemporaryViewDisplayController
+import com.android.systemui.temporarydisplay.chipbar.ChipbarAnimator
 import com.android.systemui.temporarydisplay.chipbar.ChipbarCoordinator
 import com.android.systemui.temporarydisplay.chipbar.ChipbarLogger
 import com.android.systemui.temporarydisplay.chipbar.SwipeChipbarAwayGestureHandler
@@ -145,6 +146,7 @@ class MediaTttSenderCoordinatorTest : SysuiTestCase() {
                configurationController,
                dumpManager,
                powerManager,
+                ChipbarAnimator(),
                falsingManager,
                falsingCollector,
                swipeHandler,
--- a/packages/SystemUI/tests/src/com/android/systemui/temporarydisplay/chipbar/ChipbarCoordinatorTest.kt
+++ b/packages/SystemUI/tests/src/com/android/systemui/temporarydisplay/chipbar/ChipbarCoordinatorTest.kt
@@ -79,6 +79,7 @@ class ChipbarCoordinatorTest : SysuiTestCase() {
    @Mock private lateinit var viewUtil: ViewUtil
    @Mock private lateinit var vibratorHelper: VibratorHelper
    @Mock private lateinit var swipeGestureHandler: SwipeChipbarAwayGestureHandler
+    private lateinit var chipbarAnimator: TestChipbarAnimator
    private lateinit var fakeWakeLockBuilder: WakeLockFake.Builder
    private lateinit var fakeWakeLock: WakeLockFake
    private lateinit var fakeClock: FakeSystemClock
@@ -98,6 +99,7 @@ class ChipbarCoordinatorTest : SysuiTestCase() {
        fakeWakeLockBuilder.setWakeLock(fakeWakeLock)

        uiEventLoggerFake = UiEventLoggerFake()
+        chipbarAnimator = TestChipbarAnimator()

        underTest =
            ChipbarCoordinator(
@@ -109,6 +111,7 @@ class ChipbarCoordinatorTest : SysuiTestCase() {
                configurationController,
                dumpManager,
                powerManager,
+                chipbarAnimator,
                falsingManager,
                falsingCollector,
                swipeGestureHandler,
@@ -371,6 +374,26 @@ class ChipbarCoordinatorTest : SysuiTestCase() {
        verify(vibratorHelper).vibrate(VibrationEffect.get(VibrationEffect.EFFECT_DOUBLE_CLICK))
    }

+    /** Regression test for b/266119467. */
+    @Test
+    fun displayView_animationFailure_viewsStillBecomeVisible() {
+        chipbarAnimator.allowAnimation = false
+
+        underTest.displayView(
+            createChipbarInfo(
+                Icon.Resource(R.id.check_box, null),
+                Text.Loaded("text"),
+                endItem = ChipbarEndItem.Loading,
+            )
+        )
+
+        val view = getChipbarView()
+        assertThat(view.getInnerView().alpha).isEqualTo(1f)
+        assertThat(view.getStartIconView().alpha).isEqualTo(1f)
+        assertThat(view.getLoadingIcon().alpha).isEqualTo(1f)
+        assertThat(view.getChipTextView().alpha).isEqualTo(1f)
+    }
+
    @Test
    fun updateView_viewUpdated() {
        // First, display a view
@@ -453,6 +476,25 @@ class ChipbarCoordinatorTest : SysuiTestCase() {
        verify(windowManager).removeView(chipbarView)
    }

+    /** Regression test for b/266209420. */
+    @Test
+    fun removeView_animationFailure_viewStillRemoved() {
+        chipbarAnimator.allowAnimation = false
+
+        underTest.displayView(
+            createChipbarInfo(
+                Icon.Resource(R.drawable.ic_cake, contentDescription = null),
+                Text.Loaded("title text"),
+                endItem = ChipbarEndItem.Error,
+            ),
+        )
+        val chipbarView = getChipbarView()
+
+        underTest.removeView(DEVICE_ID, "test reason")
+
+        verify(windowManager).removeView(chipbarView)
+    }
+
    @Test
    fun swipeToDismiss_false_neverListensForGesture() {
        underTest.displayView(
@@ -560,8 +602,9 @@ class ChipbarCoordinatorTest : SysuiTestCase() {

    private fun ViewGroup.getStartIconView() = this.requireViewById<ImageView>(R.id.start_icon)

-    private fun ViewGroup.getChipText(): String =
-        (this.requireViewById<TextView>(R.id.text)).text as String
+    private fun ViewGroup.getChipTextView() = this.requireViewById<TextView>(R.id.text)
+
+    private fun ViewGroup.getChipText(): String = this.getChipTextView().text as String

    private fun ViewGroup.getLoadingIcon(): View = this.requireViewById(R.id.loading)

@@ -574,6 +617,25 @@ class ChipbarCoordinatorTest : SysuiTestCase() {
        verify(windowManager).addView(viewCaptor.capture(), any())
        return viewCaptor.value as ViewGroup
    }
+
+    /** Test class that lets us disallow animations. */
+    inner class TestChipbarAnimator : ChipbarAnimator() {
+        var allowAnimation: Boolean = true
+
+        override fun animateViewIn(innerView: ViewGroup, onAnimationEnd: Runnable): Boolean {
+            if (!allowAnimation) {
+                return false
+            }
+            return super.animateViewIn(innerView, onAnimationEnd)
+        }
+
+        override fun animateViewOut(innerView: ViewGroup, onAnimationEnd: Runnable): Boolean {
+            if (!allowAnimation) {
+                return false
+            }
+            return super.animateViewOut(innerView, onAnimationEnd)
+        }
+    }
 }

 private const val TIMEOUT = 10000