From 2a835643699badefb91d323f4586e916d22060e8 Mon Sep 17 00:00:00 2001 From: Yining Liu Date: Mon, 3 Oct 2022 22:07:15 +0000 Subject: [PATCH] Fix Clear all Notifications Animation to avoid QS Panel Flicking Avoid AmbientState.mScrollY from being set to 0(happens multiple times) during the clear all animation so that QS Panel would not flicking to show up when mScrollY > 0 before clear all. Bug: 214214514 Test: `atest AmbientStateTest NotificationStackScrollLayoutTest NotificationPanelViewControllerTest` and manual: receive multiple notifications on phone so that there's scroll to show the clear all button. Click clear all, NotificationStackScrollLayout should not scroll to 0 suddenly before shade's closing. Change-Id: Ie9a0a8aa51278077999873e08cd0c7a0b50f91fb --- .../NotificationPanelViewController.java | 13 +++-- .../notification/stack/AmbientState.java | 20 ++++++++ .../stack/NotificationStackScrollLayout.java | 14 +++-- .../NotificationPanelViewControllerTest.java | 29 +++++++++++ .../notification/stack/AmbientStateTest.kt | 16 ++++++ .../NotificationStackScrollLayoutTest.java | 51 +++++++++++++++++++ 6 files changed, 136 insertions(+), 7 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/shade/NotificationPanelViewController.java b/packages/SystemUI/src/com/android/systemui/shade/NotificationPanelViewController.java index e3318127af931..46243c0111c5f 100644 --- a/packages/SystemUI/src/com/android/systemui/shade/NotificationPanelViewController.java +++ b/packages/SystemUI/src/com/android/systemui/shade/NotificationPanelViewController.java @@ -2090,7 +2090,8 @@ public final class NotificationPanelViewController { animator.start(); } - private void onFlingEnd(boolean cancelled) { + @VisibleForTesting + void onFlingEnd(boolean cancelled) { mIsFlinging = false; // No overshoot when the animation ends setOverExpansionInternal(0, false /* isFromGesture */); @@ -3825,12 +3826,14 @@ public final class NotificationPanelViewController { } } - private void setIsClosing(boolean isClosing) { + @VisibleForTesting + void setIsClosing(boolean isClosing) { boolean wasClosing = isClosing(); mClosing = isClosing; if (wasClosing != isClosing) { mPanelEventsEmitter.notifyPanelCollapsingChanged(isClosing); } + mAmbientState.setIsClosing(isClosing); } private void updateDozingVisibilities(boolean animate) { @@ -4621,14 +4624,16 @@ public final class NotificationPanelViewController { Log.v(TAG, (mViewName != null ? (mViewName + ": ") : "") + String.format(fmt, args)); } - private void notifyExpandingStarted() { + @VisibleForTesting + void notifyExpandingStarted() { if (!mExpanding) { mExpanding = true; onExpandingStarted(); } } - private void notifyExpandingFinished() { + @VisibleForTesting + void notifyExpandingFinished() { endClosing(); if (mExpanding) { mExpanding = false; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/AmbientState.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/AmbientState.java index 2719dd88b7be0..b2628e40e77e2 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/AmbientState.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/AmbientState.java @@ -142,6 +142,11 @@ public class AmbientState implements Dumpable { */ private boolean mIsFlingRequiredAfterLockScreenSwipeUp = false; + /** + * Whether the shade is currently closing. + */ + private boolean mIsClosing; + @VisibleForTesting public boolean isFlingRequiredAfterLockScreenSwipeUp() { return mIsFlingRequiredAfterLockScreenSwipeUp; @@ -717,6 +722,20 @@ public class AmbientState implements Dumpable { && mStatusBarKeyguardViewManager.isBouncerInTransit(); } + /** + * @param isClosing Whether the shade is currently closing. + */ + public void setIsClosing(boolean isClosing) { + mIsClosing = isClosing; + } + + /** + * @return Whether the shade is currently closing. + */ + public boolean isClosing() { + return mIsClosing; + } + @Override public void dump(PrintWriter pw, String[] args) { pw.println("mTopPadding=" + mTopPadding); @@ -761,5 +780,6 @@ public class AmbientState implements Dumpable { + mIsFlingRequiredAfterLockScreenSwipeUp); pw.println("mZDistanceBetweenElements=" + mZDistanceBetweenElements); pw.println("mBaseZHeight=" + mBaseZHeight); + pw.println("mIsClosing=" + mIsClosing); } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java index 55c577f1ea39e..2272411b43144 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayout.java @@ -255,7 +255,6 @@ public class NotificationStackScrollLayout extends ViewGroup implements Dumpable private boolean mClearAllInProgress; private FooterClearAllListener mFooterClearAllListener; private boolean mFlingAfterUpEvent; - /** * Was the scroller scrolled to the top when the down motion was observed? */ @@ -4020,8 +4019,9 @@ public class NotificationStackScrollLayout extends ViewGroup implements Dumpable setOwnScrollY(0); } + @VisibleForTesting @ShadeViewRefactor(RefactorComponent.COORDINATOR) - private void setIsExpanded(boolean isExpanded) { + void setIsExpanded(boolean isExpanded) { boolean changed = isExpanded != mIsExpanded; mIsExpanded = isExpanded; mStackScrollAlgorithm.setIsExpanded(isExpanded); @@ -4842,13 +4842,21 @@ public class NotificationStackScrollLayout extends ViewGroup implements Dumpable } } + @VisibleForTesting @ShadeViewRefactor(RefactorComponent.COORDINATOR) - private void setOwnScrollY(int ownScrollY) { + void setOwnScrollY(int ownScrollY) { setOwnScrollY(ownScrollY, false /* animateScrollChangeListener */); } @ShadeViewRefactor(RefactorComponent.COORDINATOR) private void setOwnScrollY(int ownScrollY, boolean animateStackYChangeListener) { + // Avoid Flicking during clear all + // when the shade finishes closing, onExpansionStopped will call + // resetScrollPosition to setOwnScrollY to 0 + if (mAmbientState.isClosing()) { + return; + } + if (ownScrollY != mOwnScrollY) { // We still want to call the normal scrolled changed for accessibility reasons onScrollChanged(mScrollX, ownScrollY, mScrollX, mOwnScrollY); diff --git a/packages/SystemUI/tests/src/com/android/systemui/shade/NotificationPanelViewControllerTest.java b/packages/SystemUI/tests/src/com/android/systemui/shade/NotificationPanelViewControllerTest.java index 37be3439fdc8b..853f81f20e926 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/shade/NotificationPanelViewControllerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/shade/NotificationPanelViewControllerTest.java @@ -38,6 +38,7 @@ import static org.mockito.Mockito.atLeast; import static org.mockito.Mockito.atLeastOnce; import static org.mockito.Mockito.clearInvocations; import static org.mockito.Mockito.doAnswer; +import static org.mockito.Mockito.inOrder; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -173,6 +174,7 @@ import org.junit.Before; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; +import org.mockito.InOrder; import org.mockito.Mock; import org.mockito.MockitoAnnotations; import org.mockito.stubbing.Answer; @@ -1506,6 +1508,33 @@ public class NotificationPanelViewControllerTest extends SysuiTestCase { ); } + + /** + * When shade is flinging to close and this fling is not intercepted, + * {@link AmbientState#setIsClosing(boolean)} should be called before + * {@link NotificationStackScrollLayoutController#onExpansionStopped()} + * to ensure scrollY can be correctly set to be 0 + */ + @Test + public void onShadeFlingClosingEnd_mAmbientStateSetClose_thenOnExpansionStopped() { + // Given: Shade is expanded + mNotificationPanelViewController.notifyExpandingFinished(); + mNotificationPanelViewController.setIsClosing(false); + + // When: Shade flings to close not canceled + mNotificationPanelViewController.notifyExpandingStarted(); + mNotificationPanelViewController.setIsClosing(true); + mNotificationPanelViewController.onFlingEnd(false); + + // Then: AmbientState's mIsClosing should be set to false + // before mNotificationStackScrollLayoutController.onExpansionStopped() is called + // to ensure NotificationStackScrollLayout.resetScrollPosition() -> resetScrollPosition + // -> setOwnScrollY(0) can set scrollY to 0 when shade is closed + InOrder inOrder = inOrder(mAmbientState, mNotificationStackScrollLayoutController); + inOrder.verify(mAmbientState).setIsClosing(false); + inOrder.verify(mNotificationStackScrollLayoutController).onExpansionStopped(); + } + private static MotionEvent createMotionEvent(int x, int y, int action) { return MotionEvent.obtain( /* downTime= */ 0, /* eventTime= */ 0, action, x, y, /* metaState= */ 0); diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/AmbientStateTest.kt b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/AmbientStateTest.kt index 11798a7a4f964..87f4c323b7ccb 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/AmbientStateTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/AmbientStateTest.kt @@ -361,6 +361,22 @@ class AmbientStateTest : SysuiTestCase() { assertThat(sut.isOnKeyguard).isFalse() } // endregion + + // region mIsClosing + @Test + fun isClosing_whenShadeClosing_shouldReturnTrue() { + sut.setIsClosing(true) + + assertThat(sut.isClosing).isTrue() + } + + @Test + fun isClosing_whenShadeFinishClosing_shouldReturnFalse() { + sut.setIsClosing(false) + + assertThat(sut.isClosing).isFalse() + } + // endregion } // region Arrange helper methods. diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutTest.java index 43530365360b4..35c8b61b63837 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/stack/NotificationStackScrollLayoutTest.java @@ -728,6 +728,57 @@ public class NotificationStackScrollLayoutTest extends SysuiTestCase { verify(mNotificationStackSizeCalculator).computeHeight(any(), anyInt(), anyFloat()); } + @Test + public void testSetOwnScrollY_shadeNotClosing_scrollYChanges() { + // Given: shade is not closing, scrollY is 0 + mAmbientState.setScrollY(0); + assertEquals(0, mAmbientState.getScrollY()); + mAmbientState.setIsClosing(false); + + // When: call NotificationStackScrollLayout.setOwnScrollY to set scrollY to 1 + mStackScroller.setOwnScrollY(1); + + // Then: scrollY should be set to 1 + assertEquals(1, mAmbientState.getScrollY()); + + // Reset scrollY back to 0 to avoid interfering with other tests + mStackScroller.setOwnScrollY(0); + assertEquals(0, mAmbientState.getScrollY()); + } + + @Test + public void testSetOwnScrollY_shadeClosing_scrollYDoesNotChange() { + // Given: shade is closing, scrollY is 0 + mAmbientState.setScrollY(0); + assertEquals(0, mAmbientState.getScrollY()); + mAmbientState.setIsClosing(true); + + // When: call NotificationStackScrollLayout.setOwnScrollY to set scrollY to 1 + mStackScroller.setOwnScrollY(1); + + // Then: scrollY should not change, it should still be 0 + assertEquals(0, mAmbientState.getScrollY()); + + // Reset scrollY and mAmbientState.mIsClosing to avoid interfering with other tests + mAmbientState.setIsClosing(false); + mStackScroller.setOwnScrollY(0); + assertEquals(0, mAmbientState.getScrollY()); + } + + @Test + public void onShadeFlingClosingEnd_scrollYShouldBeSetToZero() { + // Given: mAmbientState.mIsClosing is set to be true + // mIsExpanded is set to be false + mAmbientState.setIsClosing(true); + mStackScroller.setIsExpanded(false); + + // When: onExpansionStopped is called + mStackScroller.onExpansionStopped(); + + // Then: mAmbientState.scrollY should be set to be 0 + assertEquals(mAmbientState.getScrollY(), 0); + } + private void setBarStateForTest(int state) { // Can't inject this through the listener or we end up on the actual implementation // rather than the mock because the spy just coppied the anonymous inner /shruggie.