From f6d7fd8717a59737eef2e6dd836e75810b62c21e Mon Sep 17 00:00:00 2001 From: Beth Thibodeau Date: Mon, 19 Sep 2022 21:45:05 -0500 Subject: [PATCH] Update media hosts after recommendations expire Previously updateViewVisibility was only being called if the recommendation card was removed from the carousel immediately, and never if it timed out or was removed due to carousel visibility change. This adds calls to that function when the card is actually removed in those cases, so that MediaHosts have the correct visibility Also adds more debug logging Fixes: 245566782 Test: manual - verify shade looks normal after recommendations expire (steps in bug); verify recommendations do not disappear while visible Test: atest Change-Id: Ied7d504ca9fba85c66e5d72e16cac1347420b3e5 --- .../systemui/media/MediaCarouselController.kt | 24 +++++++++++--- .../media/MediaCarouselControllerLogger.kt | 31 +++++++++++++++++++ .../systemui/media/MediaDataManager.kt | 1 + .../systemui/media/MediaHierarchyManager.kt | 5 +++ .../com/android/systemui/media/MediaHost.kt | 6 +++- .../systemui/media/MediaViewController.kt | 2 +- .../media/MediaCarouselControllerTest.kt | 28 +++++++++++++++++ 7 files changed, 91 insertions(+), 6 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt index cc77ed15b5f0f..e25f5dabe5a9c 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt @@ -202,6 +202,7 @@ class MediaCarouselController @Inject constructor( * It will be called when the container is out of view. */ lateinit var updateUserVisibility: () -> Unit + lateinit var updateHostVisibility: () -> Unit private val isReorderingAllowed: Boolean get() = visualStabilityProvider.isReorderingAllowed @@ -225,7 +226,13 @@ class MediaCarouselController @Inject constructor( reorderAllPlayers(previousVisiblePlayerKey = null) } - keysNeedRemoval.forEach { removePlayer(it) } + keysNeedRemoval.forEach { + removePlayer(it) + } + if (keysNeedRemoval.size > 0) { + // Carousel visibility may need to be updated after late removals + updateHostVisibility() + } keysNeedRemoval.clear() // Update user visibility so that no extra impression will be logged when @@ -247,6 +254,7 @@ class MediaCarouselController @Inject constructor( receivedSmartspaceCardLatency: Int, isSsReactivated: Boolean ) { + debugLogger.logMediaLoaded(key) if (addOrUpdatePlayer(key, oldKey, data, isSsReactivated)) { // Log card received if a new resumable media card is added MediaPlayerData.getMediaPlayer(key)?.let { @@ -315,7 +323,7 @@ class MediaCarouselController @Inject constructor( data: SmartspaceMediaData, shouldPrioritize: Boolean ) { - if (DEBUG) Log.d(TAG, "Loading Smartspace media update") + debugLogger.logRecommendationLoaded(key) // Log the case where the hidden media carousel with the existed inactive resume // media is shown by the Smartspace signal. if (data.isActive) { @@ -370,13 +378,21 @@ class MediaCarouselController @Inject constructor( } override fun onMediaDataRemoved(key: String) { + debugLogger.logMediaRemoved(key) removePlayer(key) } override fun onSmartspaceMediaDataRemoved(key: String, immediately: Boolean) { - if (DEBUG) Log.d(TAG, "My Smartspace media removal request is received") + debugLogger.logRecommendationRemoved(key, immediately) if (immediately || isReorderingAllowed) { - onMediaDataRemoved(key) + removePlayer(key) + if (!immediately) { + // Although it wasn't requested, we were able to process the removal + // immediately since reordering is allowed. So, notify hosts to update + if (this@MediaCarouselController::updateHostVisibility.isInitialized) { + updateHostVisibility() + } + } } else { keysNeedRemoval.add(key) } diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselControllerLogger.kt b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselControllerLogger.kt index 04ebd5a71137a..b1018f9544c00 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselControllerLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselControllerLogger.kt @@ -40,6 +40,37 @@ class MediaCarouselControllerLogger @Inject constructor( "Removing control panel for $str1 from map without calling #onDestroy" } ) + + fun logMediaLoaded(key: String) = buffer.log( + TAG, + LogLevel.DEBUG, + { str1 = key }, + { "add player $str1" } + ) + + fun logMediaRemoved(key: String) = buffer.log( + TAG, + LogLevel.DEBUG, + { str1 = key }, + { "removing player $str1" } + ) + + fun logRecommendationLoaded(key: String) = buffer.log( + TAG, + LogLevel.DEBUG, + { str1 = key }, + { "add recommendation $str1" } + ) + + fun logRecommendationRemoved(key: String, immediately: Boolean) = buffer.log( + TAG, + LogLevel.DEBUG, + { + str1 = key + bool1 = immediately + }, + { "removing recommendation $str1, immediate=$bool1" } + ) } private const val TAG = "MediaCarouselCtlrLog" diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt index fb37446c1cec1..5096a797a32b1 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt @@ -1322,6 +1322,7 @@ class MediaDataManager( println("externalListeners: ${mediaDataFilter.listeners}") println("mediaEntries: $mediaEntries") println("useMediaResumption: $useMediaResumption") + println("allowMediaRecommendations: $allowMediaRecommendations") } } } diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaHierarchyManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaHierarchyManager.kt index 6baf6e137ab4d..e0b6d1f9de7b9 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaHierarchyManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaHierarchyManager.kt @@ -546,6 +546,11 @@ class MediaHierarchyManager @Inject constructor( mediaCarouselController.updateUserVisibility = { mediaCarouselController.mediaCarouselScrollHandler.visibleToUser = isVisibleToUser() } + mediaCarouselController.updateHostVisibility = { + mediaHosts.forEach { + it?.updateViewVisibility() + } + } panelEventsEvents.registerListener(object : NotifPanelEvents.Listener { override fun onExpandImmediateChanged(isExpandImmediateEnabled: Boolean) { diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaHost.kt b/packages/SystemUI/src/com/android/systemui/media/MediaHost.kt index 8bfb8aae5c91e..bffb0fdec7071 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaHost.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaHost.kt @@ -167,7 +167,11 @@ class MediaHost constructor( } } - private fun updateViewVisibility() { + /** + * Updates this host's state based on the current media data's status, and invokes listeners if + * the visibility has changed + */ + fun updateViewVisibility() { state.visible = if (showsOnlyActiveMedia) { mediaDataManager.hasActiveMediaOrRecommendation() } else { diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaViewController.kt b/packages/SystemUI/src/com/android/systemui/media/MediaViewController.kt index 27cd1ce2b440b..ac59175d46469 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaViewController.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaViewController.kt @@ -353,7 +353,7 @@ class MediaViewController @Inject constructor( type: TYPE ) = traceSection("MediaViewController#attach") { updateMediaViewControllerType(type) - logger.logMediaLocation("attach", currentStartLocation, currentEndLocation) + logger.logMediaLocation("attach $type", currentStartLocation, currentEndLocation) this.transitionLayout = transitionLayout layoutController.attach(transitionLayout) if (currentEndLocation == -1) { diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt index 5a50a9f644876..5dd1cfcb76e49 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt @@ -26,6 +26,7 @@ import com.android.systemui.dagger.qualifiers.Main import com.android.systemui.dump.DumpManager import com.android.systemui.plugins.ActivityStarter import com.android.systemui.plugins.FalsingManager +import com.android.systemui.statusbar.notification.collection.provider.OnReorderingAllowedListener import com.android.systemui.statusbar.notification.collection.provider.VisualStabilityProvider import com.android.systemui.statusbar.policy.ConfigurationController import com.android.systemui.util.animation.TransitionLayout @@ -78,6 +79,7 @@ class MediaCarouselControllerTest : SysuiTestCase() { @Mock lateinit var mediaViewController: MediaViewController @Mock lateinit var smartspaceMediaData: SmartspaceMediaData @Captor lateinit var listener: ArgumentCaptor + @Captor lateinit var visualStabilityCallback: ArgumentCaptor private val clock = FakeSystemClock() private lateinit var mediaCarouselController: MediaCarouselController @@ -102,6 +104,8 @@ class MediaCarouselControllerTest : SysuiTestCase() { debugLogger ) verify(mediaDataManager).addListener(capture(listener)) + verify(visualStabilityProvider) + .addPersistentReorderingAllowedListener(capture(visualStabilityCallback)) whenever(mediaControlPanelFactory.get()).thenReturn(mediaPlayer) whenever(mediaPlayer.mediaViewController).thenReturn(mediaViewController) whenever(mediaDataManager.smartspaceMediaData).thenReturn(smartspaceMediaData) @@ -374,4 +378,28 @@ class MediaCarouselControllerTest : SysuiTestCase() { playerIndex = MediaPlayerData.getMediaPlayerIndex("playing local") assertEquals(playerIndex, 0) } + + @Test + fun testRecommendationRemovedWhileNotVisible_updateHostVisibility() { + var result = false + mediaCarouselController.updateHostVisibility = { result = true } + + whenever(visualStabilityProvider.isReorderingAllowed).thenReturn(true) + listener.value.onSmartspaceMediaDataRemoved(SMARTSPACE_KEY, false) + + assertEquals(true, result) + } + + @Test + fun testRecommendationRemovedWhileVisible_thenReorders_updateHostVisibility() { + var result = false + mediaCarouselController.updateHostVisibility = { result = true } + + whenever(visualStabilityProvider.isReorderingAllowed).thenReturn(false) + listener.value.onSmartspaceMediaDataRemoved(SMARTSPACE_KEY, false) + assertEquals(false, result) + + visualStabilityCallback.value.onReorderingAllowed() + assertEquals(true, result) + } }