From 0260c7563bd82a32427ee4d7ef4dced4e90b8cbc Mon Sep 17 00:00:00 2001 From: Selim Cinek Date: Mon, 11 May 2020 16:03:52 -0700 Subject: [PATCH] Resetting the scroll position of the media players when allowed Previously the scroll position would never be reset anymore unless the user actively did so. We're now resetting the position whenever the visual stability manager tells us we're allowed to do so. Bug: 154137987 Test: atest SystemUITests Change-Id: Ifd5858f47827610a64a9a12cff777bd2440334a9 --- .../systemui/media/MediaViewManager.kt | 16 +++++++-- .../NotificationViewHierarchyManager.java | 9 +++-- .../notification/VisualStabilityManager.java | 35 +++++++++++++++---- .../stack/NotificationChildrenContainer.java | 3 +- .../statusbar/phone/HeadsUpManagerPhone.java | 3 +- .../VisualStabilityManagerTest.java | 26 ++++++++++---- 6 files changed, 70 insertions(+), 22 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaViewManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaViewManager.kt index 9c92b0a27c03e..d72c3691c34b6 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaViewManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaViewManager.kt @@ -42,10 +42,10 @@ class MediaViewManager @Inject constructor( val mediaCarousel: HorizontalScrollView private val mediaContent: ViewGroup private val mediaPlayers: MutableMap = mutableMapOf() - private val visualStabilityCallback = ::reorderAllPlayers + private val visualStabilityCallback : VisualStabilityManager.Callback private var activeMediaIndex: Int = 0 + private var needsReordering: Boolean = false private var scrollIntoCurrentMedia: Int = 0 - private var currentlyExpanded = true set(value) { if (field != value) { @@ -75,6 +75,16 @@ class MediaViewManager @Inject constructor( mediaCarousel = inflateMediaCarousel() mediaCarousel.setOnScrollChangeListener(scrollChangedListener) mediaContent = mediaCarousel.requireViewById(R.id.media_carousel) + visualStabilityCallback = VisualStabilityManager.Callback { + if (needsReordering) { + needsReordering = false + reorderAllPlayers() + } + // Let's reset our scroll position + mediaCarousel.scrollX = 0 + } + visualStabilityManager.addReorderingAllowedCallback(visualStabilityCallback, + true /* persistent */) mediaManager.addListener(object : MediaDataManager.Listener { override fun onMediaDataLoaded(key: String, data: MediaData) { updateView(key, data) @@ -169,7 +179,7 @@ class MediaViewManager @Inject constructor( mediaContent.removeView(existingPlayer.view?.player) mediaContent.addView(existingPlayer.view?.player, 0) } else { - visualStabilityManager.addReorderingAllowedCallback(visualStabilityCallback) + needsReordering = true } } existingPlayer.bind(data) diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java index e7f26183fa8a6..765f85aab3192 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/NotificationViewHierarchyManager.java @@ -195,13 +195,15 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle boolean wasChildInGroup = ent.isChildInGroup(); if (isChildInGroup && !wasChildInGroup) { isChildInGroup = wasChildInGroup; - mVisualStabilityManager.addGroupChangesAllowedCallback(mEntryManager); + mVisualStabilityManager.addGroupChangesAllowedCallback(mEntryManager, + false /* persistent */); } else if (!isChildInGroup && wasChildInGroup) { // We allow grouping changes if the group was collapsed if (mGroupManager.isLogicalGroupExpanded(ent.getSbn())) { isChildInGroup = wasChildInGroup; parent = ent.getRow().getNotificationParent().getEntry(); - mVisualStabilityManager.addGroupChangesAllowedCallback(mEntryManager); + mVisualStabilityManager.addGroupChangesAllowedCallback(mEntryManager, + false /* persistent */); } } } @@ -286,7 +288,8 @@ public class NotificationViewHierarchyManager implements DynamicPrivacyControlle if (mVisualStabilityManager.canReorderNotification(targetChild)) { mListContainer.changeViewPosition(targetChild, i); } else { - mVisualStabilityManager.addReorderingAllowedCallback(mEntryManager); + mVisualStabilityManager.addReorderingAllowedCallback(mEntryManager, + false /* persistent */); } } j++; diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/VisualStabilityManager.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/VisualStabilityManager.java index 7ac59954cb576..8341c02b6b63e 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/VisualStabilityManager.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/VisualStabilityManager.java @@ -43,7 +43,9 @@ public class VisualStabilityManager implements OnHeadsUpChangedListener, Dumpabl private static final long TEMPORARY_REORDERING_ALLOWED_DURATION = 1000; private final ArrayList mReorderingAllowedCallbacks = new ArrayList<>(); + private final ArraySet mPersistentReorderingCallbacks = new ArraySet<>(); private final ArrayList mGroupChangesAllowedCallbacks = new ArrayList<>(); + private final ArraySet mPersistentGroupCallbacks = new ArraySet<>(); private final Handler mHandler; private boolean mPanelExpanded; @@ -85,8 +87,15 @@ public class VisualStabilityManager implements OnHeadsUpChangedListener, Dumpabl /** * Add a callback to invoke when reordering is allowed again. + * + * @param callback the callback to add + * @param persistent {@code true} if this callback should this callback be persisted, otherwise + * it will be removed after a single invocation */ - public void addReorderingAllowedCallback(Callback callback) { + public void addReorderingAllowedCallback(Callback callback, boolean persistent) { + if (persistent) { + mPersistentReorderingCallbacks.add(callback); + } if (mReorderingAllowedCallbacks.contains(callback)) { return; } @@ -95,8 +104,15 @@ public class VisualStabilityManager implements OnHeadsUpChangedListener, Dumpabl /** * Add a callback to invoke when group changes are allowed again. + * + * @param callback the callback to add + * @param persistent {@code true} if this callback should this callback be persisted, otherwise + * it will be removed after a single invocation */ - public void addGroupChangesAllowedCallback(Callback callback) { + public void addGroupChangesAllowedCallback(Callback callback, boolean persistent) { + if (persistent) { + mPersistentGroupCallbacks.add(callback); + } if (mGroupChangesAllowedCallbacks.contains(callback)) { return; } @@ -136,21 +152,26 @@ public class VisualStabilityManager implements OnHeadsUpChangedListener, Dumpabl boolean changedToTrue = reorderingAllowed && !mReorderingAllowed; mReorderingAllowed = reorderingAllowed; if (changedToTrue) { - notifyChangeAllowed(mReorderingAllowedCallbacks); + notifyChangeAllowed(mReorderingAllowedCallbacks, mPersistentReorderingCallbacks); } boolean groupChangesAllowed = (!mScreenOn || !mPanelExpanded) && !mPulsing; changedToTrue = groupChangesAllowed && !mGroupChangedAllowed; mGroupChangedAllowed = groupChangesAllowed; if (changedToTrue) { - notifyChangeAllowed(mGroupChangesAllowedCallbacks); + notifyChangeAllowed(mGroupChangesAllowedCallbacks, mPersistentGroupCallbacks); } } - private void notifyChangeAllowed(ArrayList callbacks) { + private void notifyChangeAllowed(ArrayList callbacks, + ArraySet persistentCallbacks) { for (int i = 0; i < callbacks.size(); i++) { - callbacks.get(i).onChangeAllowed(); + Callback callback = callbacks.get(i); + callback.onChangeAllowed(); + if (!persistentCallbacks.contains(callback)) { + callbacks.remove(callback); + i--; + } } - callbacks.clear(); } /** diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationChildrenContainer.java b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationChildrenContainer.java index c9b1318feb13b..99691b710cc26 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationChildrenContainer.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/notification/stack/NotificationChildrenContainer.java @@ -463,7 +463,8 @@ public class NotificationChildrenContainer extends ViewGroup { mAttachedChildren.add(i, desiredChild); result = true; } else { - visualStabilityManager.addReorderingAllowedCallback(callback); + visualStabilityManager.addReorderingAllowedCallback(callback, + false /* persistent */); } } } diff --git a/packages/SystemUI/src/com/android/systemui/statusbar/phone/HeadsUpManagerPhone.java b/packages/SystemUI/src/com/android/systemui/statusbar/phone/HeadsUpManagerPhone.java index 6b0df95f54dd7..3dcf7ed674c7f 100644 --- a/packages/SystemUI/src/com/android/systemui/statusbar/phone/HeadsUpManagerPhone.java +++ b/packages/SystemUI/src/com/android/systemui/statusbar/phone/HeadsUpManagerPhone.java @@ -439,7 +439,8 @@ public class HeadsUpManagerPhone extends HeadsUpManager implements Dumpable, // time out anyway && !entry.showingPulsing()) { mEntriesToRemoveWhenReorderingAllowed.add(entry); - mVisualStabilityManager.addReorderingAllowedCallback(HeadsUpManagerPhone.this); + mVisualStabilityManager.addReorderingAllowedCallback(HeadsUpManagerPhone.this, + false /* persistent */); } else if (mTrackingHeadsUp) { mEntriesToRemoveAfterExpand.add(entry); } else if (mIsAutoHeadsUp && mStatusBarState == StatusBarState.KEYGUARD) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/VisualStabilityManagerTest.java b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/VisualStabilityManagerTest.java index 3d06c57cac37a..ea1b498801ec8 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/VisualStabilityManagerTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/statusbar/notification/VisualStabilityManagerTest.java @@ -21,6 +21,7 @@ import static org.junit.Assert.assertTrue; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; +import static org.mockito.Mockito.times; import static org.mockito.Mockito.verify; import static org.mockito.Mockito.when; @@ -108,7 +109,7 @@ public class VisualStabilityManagerTest extends SysuiTestCase { public void testCallBackCalledScreenOn() { mVisualStabilityManager.setPanelExpanded(true); mVisualStabilityManager.setScreenOn(true); - mVisualStabilityManager.addReorderingAllowedCallback(mCallback); + mVisualStabilityManager.addReorderingAllowedCallback(mCallback, false /* persistent */); mVisualStabilityManager.setScreenOn(false); verify(mCallback).onChangeAllowed(); } @@ -117,7 +118,7 @@ public class VisualStabilityManagerTest extends SysuiTestCase { public void testCallBackCalledPanelExpanded() { mVisualStabilityManager.setPanelExpanded(true); mVisualStabilityManager.setScreenOn(true); - mVisualStabilityManager.addReorderingAllowedCallback(mCallback); + mVisualStabilityManager.addReorderingAllowedCallback(mCallback, false /* persistent */); mVisualStabilityManager.setPanelExpanded(false); verify(mCallback).onChangeAllowed(); } @@ -126,13 +127,24 @@ public class VisualStabilityManagerTest extends SysuiTestCase { public void testCallBackExactlyOnce() { mVisualStabilityManager.setPanelExpanded(true); mVisualStabilityManager.setScreenOn(true); - mVisualStabilityManager.addReorderingAllowedCallback(mCallback); + mVisualStabilityManager.addReorderingAllowedCallback(mCallback, false /* persistent */); mVisualStabilityManager.setScreenOn(false); mVisualStabilityManager.setScreenOn(true); mVisualStabilityManager.setScreenOn(false); verify(mCallback).onChangeAllowed(); } + @Test + public void testCallBackCalledContinuouslyWhenRequested() { + mVisualStabilityManager.setPanelExpanded(true); + mVisualStabilityManager.setScreenOn(true); + mVisualStabilityManager.addReorderingAllowedCallback(mCallback, true /* persistent */); + mVisualStabilityManager.setScreenOn(false); + mVisualStabilityManager.setScreenOn(true); + mVisualStabilityManager.setScreenOn(false); + verify(mCallback, times(2)).onChangeAllowed(); + } + @Test public void testAddedCanReorder() { mVisualStabilityManager.setPanelExpanded(true); @@ -188,7 +200,7 @@ public class VisualStabilityManagerTest extends SysuiTestCase { @Test public void testCallBackCalled_Pulsing() { mVisualStabilityManager.setPulsing(true); - mVisualStabilityManager.addReorderingAllowedCallback(mCallback); + mVisualStabilityManager.addReorderingAllowedCallback(mCallback, false /* persistent */); mVisualStabilityManager.setPulsing(false); verify(mCallback).onChangeAllowed(); } @@ -198,7 +210,7 @@ public class VisualStabilityManagerTest extends SysuiTestCase { // GIVEN having the panel open (which would block reordering) mVisualStabilityManager.setScreenOn(true); mVisualStabilityManager.setPanelExpanded(true); - mVisualStabilityManager.addReorderingAllowedCallback(mCallback); + mVisualStabilityManager.addReorderingAllowedCallback(mCallback, false /* persistent */); // WHEN we temprarily allow reordering mVisualStabilityManager.temporarilyAllowReordering(); @@ -212,7 +224,7 @@ public class VisualStabilityManagerTest extends SysuiTestCase { public void testTemporarilyAllowReorderingDoesntOverridePulsing() { // GIVEN we are in a pulsing state mVisualStabilityManager.setPulsing(true); - mVisualStabilityManager.addReorderingAllowedCallback(mCallback); + mVisualStabilityManager.addReorderingAllowedCallback(mCallback, false /* persistent */); // WHEN we temprarily allow reordering mVisualStabilityManager.temporarilyAllowReordering(); @@ -227,7 +239,7 @@ public class VisualStabilityManagerTest extends SysuiTestCase { // GIVEN having the panel open (which would block reordering) mVisualStabilityManager.setScreenOn(true); mVisualStabilityManager.setPanelExpanded(true); - mVisualStabilityManager.addReorderingAllowedCallback(mCallback); + mVisualStabilityManager.addReorderingAllowedCallback(mCallback, false /* persistent */); // WHEN we temprarily allow reordering and then wait until the window expires mVisualStabilityManager.temporarilyAllowReordering();