From cc0a34b5cb458a7821acfdaab33cf5ca0f376baa Mon Sep 17 00:00:00 2001 From: Ibrahim Yilmaz Date: Tue, 21 Feb 2023 17:32:58 +0000 Subject: [PATCH] Use immutable shouldBeAnimated during Unfold Animation Context: We decide to run animations by using shouldBeAnimated lambda function. It represents mutable state which is combination of the StatusBar and some external business logic. It shows that shouldBeAnimated value may change for some reasons after the animation is fired. This may cause some visual issues like wrong translationX values in the view(animation started but not completed properly). Solution: We make shouldBeAnimated immutable variable instead of lambda func in ViewToTranslate and use it during the animation. We already map `ViewIdToTranslate` to `ViewToTranslate` on each animation start. It also verifies that we invalidate the preserved value on each animation start. Test: atest UnfoldConstantTranslateAnimatorTest Bug: 264454435 Change-Id: I75f84a668ab3395193901cff569ca56e55456316 --- .../UnfoldConstantTranslateAnimator.kt | 33 +++++++++-------- .../statusbar/phone/CentralSurfacesImpl.java | 6 ++-- .../UnfoldConstantTranslateAnimatorTest.kt | 35 ++++++++++++++----- 3 files changed, 49 insertions(+), 25 deletions(-) diff --git a/packages/SystemUI/shared/src/com/android/systemui/shared/animation/UnfoldConstantTranslateAnimator.kt b/packages/SystemUI/shared/src/com/android/systemui/shared/animation/UnfoldConstantTranslateAnimator.kt index 12e0b9ab835a2..c5979cc50c3c4 100644 --- a/packages/SystemUI/shared/src/com/android/systemui/shared/animation/UnfoldConstantTranslateAnimator.kt +++ b/packages/SystemUI/shared/src/com/android/systemui/shared/animation/UnfoldConstantTranslateAnimator.kt @@ -65,35 +65,40 @@ class UnfoldConstantTranslateAnimator( } else { 1 } - viewsToTranslate.forEach { (view, direction, shouldBeAnimated) -> - if (shouldBeAnimated()) { - view.get()?.translationX = xTrans * direction.multiplier * rtlMultiplier - } + viewsToTranslate.forEach { (view, direction) -> + view.get()?.translationX = xTrans * direction.multiplier * rtlMultiplier } } /** Finds in [parent] all views specified by [ids] and register them for the animation. */ private fun registerViewsForAnimation(parent: ViewGroup, ids: Set) { viewsToTranslate = - ids.mapNotNull { (id, dir, pred) -> - parent.findViewById(id)?.let { view -> - ViewToTranslate(WeakReference(view), dir, pred) + ids.asSequence() + .filter { it.shouldBeAnimated() } + .mapNotNull { + parent.findViewById(it.viewId)?.let { view -> + ViewToTranslate(WeakReference(view), it.direction) + } } - } + .toList() } - /** Represents a view to animate. [rootView] should contain a view with [viewId] inside. */ + /** + * Represents a view to animate. [rootView] should contain a view with [viewId] inside. + * [shouldBeAnimated] is only evaluated when the viewsToTranslate is registered in + * [registerViewsForAnimation]. + */ data class ViewIdToTranslate( val viewId: Int, val direction: Direction, val shouldBeAnimated: () -> Boolean = { true } ) - private data class ViewToTranslate( - val view: WeakReference, - val direction: Direction, - val shouldBeAnimated: () -> Boolean - ) + /** + * Represents a view whose animation process is in-progress. It should be immutable because the + * started animation should be completed. + */ + private data class ViewToTranslate(val view: WeakReference, val direction: Direction) /** Direction of the animation. */ enum class Direction(val multiplier: Float) { diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesImpl.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesImpl.java index 6b4a4d1389ffd..be791f9e2242d 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesImpl.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/CentralSurfacesImpl.java @@ -2211,10 +2211,10 @@ public class CentralSurfacesImpl implements CoreStartable, CentralSurfaces { pw.println("Current Status Bar state:"); pw.println(" mExpandedVisible=" + mShadeController.isExpandedVisible()); pw.println(" mDisplayMetrics=" + mDisplayMetrics); - pw.println(" mStackScroller: " + CentralSurfaces.viewInfo(mStackScroller)); - pw.println(" mStackScroller: " + CentralSurfaces.viewInfo(mStackScroller) - + " scroll " + mStackScroller.getScrollX() + pw.print(" mStackScroller: " + CentralSurfaces.viewInfo(mStackScroller)); + pw.print(" scroll " + mStackScroller.getScrollX() + "," + mStackScroller.getScrollY()); + pw.println(" translationX " + mStackScroller.getTranslationX()); } pw.print(" mInteractingWindows="); pw.println(mInteractingWindows); diff --git a/packages/SystemUI/tests/src/com/android/systemui/shared/animation/UnfoldConstantTranslateAnimatorTest.kt b/packages/SystemUI/tests/src/com/android/systemui/shared/animation/UnfoldConstantTranslateAnimatorTest.kt index 7a74b1229c5c8..56fc0c74dafa8 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/shared/animation/UnfoldConstantTranslateAnimatorTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/shared/animation/UnfoldConstantTranslateAnimatorTest.kt @@ -36,21 +36,26 @@ class UnfoldConstantTranslateAnimatorTest : SysuiTestCase() { private val progressProvider = TestUnfoldTransitionProvider() - @Mock private lateinit var parent: ViewGroup + @Mock + private lateinit var parent: ViewGroup + + @Mock + private lateinit var shouldBeAnimated: () -> Boolean private lateinit var animator: UnfoldConstantTranslateAnimator - private val viewsIdToRegister = - setOf( - ViewIdToTranslate(START_VIEW_ID, Direction.START), - ViewIdToTranslate(END_VIEW_ID, Direction.END)) + private val viewsIdToRegister + get() = + setOf( + ViewIdToTranslate(START_VIEW_ID, Direction.START, shouldBeAnimated), + ViewIdToTranslate(END_VIEW_ID, Direction.END, shouldBeAnimated) + ) @Before fun setup() { MockitoAnnotations.initMocks(this) - - animator = - UnfoldConstantTranslateAnimator(viewsIdToRegister, progressProvider) + whenever(shouldBeAnimated.invoke()).thenReturn(true) + animator = UnfoldConstantTranslateAnimator(viewsIdToRegister, progressProvider) animator.init(parent, MAX_TRANSLATION) } @@ -96,6 +101,20 @@ class UnfoldConstantTranslateAnimatorTest : SysuiTestCase() { moveAndValidate(listOf(leftView to START, rightView to END), View.LAYOUT_DIRECTION_LTR) } + @Test + fun onTransition_completeStartedTranslation() { + // GIVEN + val leftView = View(context) + whenever(parent.findViewById(START_VIEW_ID)).thenReturn(leftView) + // To start animation, shouldBeAnimated should return true. + // There is a possibility for shouldBeAnimated to return false during the animation. + whenever(shouldBeAnimated.invoke()).thenReturn(true).thenReturn(false) + + // shouldBeAnimated state may change during the animation. + // However, started animation should be completed. + moveAndValidate(listOf(leftView to START), View.LAYOUT_DIRECTION_LTR) + } + private fun moveAndValidate(list: List>, layoutDirection: Int) { // Compare values as ints because -0f != 0f