From 49f68af1be448ef36d434dabb9e217e6a6643bb5 Mon Sep 17 00:00:00 2001 From: Michael Mikhail Date: Fri, 16 Sep 2022 13:45:50 +0000 Subject: Track order of visible media players Create a new map that track the order of visible media players before they are reordered as the media carousel is visible to user. Media player map that was being used before is not consistent with UI during the time the carousel is visible to user. So during this time we need to keep track of the order. This also makes sure the media carousel scroll to the right app media player when user selects one of the recommendations. Bug: 248340474 Test: atest MediaCarouselControllerTest Test: Checked order of UMO cards in carousel. Change-Id: I549535125b251b03740a387cc4a1cfae91f4f4f5 --- .../systemui/media/MediaCarouselController.kt | 116 +++++++++++++++------ .../android/systemui/media/MediaControlPanel.java | 2 +- .../systemui/media/MediaCarouselControllerTest.kt | 63 +++++++++-- 3 files changed, 140 insertions(+), 41 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt index 80bff83d03a0..5977ed06b0f1 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt @@ -133,7 +133,7 @@ class MediaCarouselController @Inject constructor( private val visualStabilityCallback: OnReorderingAllowedListener private var needsReordering: Boolean = false private var keysNeedRemoval = mutableSetOf() - var shouldScrollToActivePlayer: Boolean = false + var shouldScrollToKey: Boolean = false private var isRtl: Boolean = false set(value) { if (value != field) { @@ -436,7 +436,10 @@ class MediaCarouselController @Inject constructor( return mediaCarousel } - private fun reorderAllPlayers(previousVisiblePlayerKey: MediaPlayerData.MediaSortKey?) { + private fun reorderAllPlayers( + previousVisiblePlayerKey: MediaPlayerData.MediaSortKey?, + key: String? = null + ) { mediaContent.removeAllViews() for (mediaPlayer in MediaPlayerData.players()) { mediaPlayer.mediaViewHolder?.let { @@ -446,18 +449,18 @@ class MediaCarouselController @Inject constructor( } } mediaCarouselScrollHandler.onPlayersChanged() - + MediaPlayerData.updateVisibleMediaPlayers() // Automatically scroll to the active player if needed - if (shouldScrollToActivePlayer) { - shouldScrollToActivePlayer = false - val activeMediaIndex = MediaPlayerData.firstActiveMediaIndex() - if (activeMediaIndex != -1) { + if (shouldScrollToKey) { + shouldScrollToKey = false + val mediaIndex = key?.let { MediaPlayerData.getMediaPlayerIndex(it) } ?: -1 + if (mediaIndex != -1) { previousVisiblePlayerKey?.let { val previousVisibleIndex = MediaPlayerData.playerKeys() .indexOfFirst { key -> it == key } mediaCarouselScrollHandler - .scrollToPlayer(previousVisibleIndex, activeMediaIndex) - } ?: mediaCarouselScrollHandler.scrollToPlayer(destIndex = activeMediaIndex) + .scrollToPlayer(previousVisibleIndex, mediaIndex) + } ?: mediaCarouselScrollHandler.scrollToPlayer(destIndex = mediaIndex) } } } @@ -471,9 +474,8 @@ class MediaCarouselController @Inject constructor( ): Boolean = traceSection("MediaCarouselController#addOrUpdatePlayer") { MediaPlayerData.moveIfExists(oldKey, key) val existingPlayer = MediaPlayerData.getMediaPlayer(key) - val curVisibleMediaKey = MediaPlayerData.playerKeys() + val curVisibleMediaKey = MediaPlayerData.visiblePlayerKeys() .elementAtOrNull(mediaCarouselScrollHandler.visibleMediaIndex) - val isCurVisibleMediaPlaying = curVisibleMediaKey?.data?.isPlaying if (existingPlayer == null) { val newPlayer = mediaControlPanelFactory.get() newPlayer.attachPlayer(MediaViewHolder.create( @@ -488,8 +490,10 @@ class MediaCarouselController @Inject constructor( key, data, newPlayer, systemClock, isSsReactivated, debugLogger ) updatePlayerToState(newPlayer, noAnimation = true) - if (data.active) { - reorderAllPlayers(curVisibleMediaKey) + // Media data added from a recommendation card should starts playing. + if ((shouldScrollToKey && data.isPlaying == true) || + (!shouldScrollToKey && data.active)) { + reorderAllPlayers(curVisibleMediaKey, key) } else { needsReordering = true } @@ -498,14 +502,16 @@ class MediaCarouselController @Inject constructor( MediaPlayerData.addMediaPlayer( key, data, existingPlayer, systemClock, isSsReactivated, debugLogger ) - // Check the playing status of both current visible and new media players - // To make sure we scroll to the active playing media card. + val packageName = MediaPlayerData.smartspaceMediaData?.packageName ?: String() + // In case of recommendations hits. + // Check the playing status of media player and the package name. + // To make sure we scroll to the right app's media player. if (isReorderingAllowed || - shouldScrollToActivePlayer && + shouldScrollToKey && data.isPlaying == true && - isCurVisibleMediaPlaying == false + packageName == data.packageName ) { - reorderAllPlayers(curVisibleMediaKey) + reorderAllPlayers(curVisibleMediaKey, key) } else { needsReordering = true } @@ -534,7 +540,7 @@ class MediaCarouselController @Inject constructor( val existingSmartspaceMediaKey = MediaPlayerData.smartspaceMediaKey() existingSmartspaceMediaKey?.let { - val removedPlayer = MediaPlayerData.removeMediaPlayer(existingSmartspaceMediaKey) + val removedPlayer = MediaPlayerData.removeMediaPlayer(existingSmartspaceMediaKey, true) removedPlayer?.run { debugLogger.logPotentialMemoryLeak(existingSmartspaceMediaKey) } } @@ -546,7 +552,7 @@ class MediaCarouselController @Inject constructor( ViewGroup.LayoutParams.WRAP_CONTENT) newRecs.recommendationViewHolder?.recommendations?.setLayoutParams(lp) newRecs.bindRecommendation(data) - val curVisibleMediaKey = MediaPlayerData.playerKeys() + val curVisibleMediaKey = MediaPlayerData.visiblePlayerKeys() .elementAtOrNull(mediaCarouselScrollHandler.visibleMediaIndex) MediaPlayerData.addMediaRecommendation( key, data, newRecs, shouldPrioritize, systemClock, debugLogger @@ -572,7 +578,10 @@ class MediaCarouselController @Inject constructor( logger.logRecommendationRemoved(it.packageName, it.instanceId) } } - val removed = MediaPlayerData.removeMediaPlayer(key) + val removed = MediaPlayerData.removeMediaPlayer( + key, + dismissMediaData || dismissRecommendation + ) removed?.apply { mediaCarouselScrollHandler.onPrePlayerRemoved(removed) mediaContent.removeView(removed.mediaViewHolder?.player) @@ -835,18 +844,20 @@ class MediaCarouselController @Inject constructor( fun logSmartspaceImpression(qsExpanded: Boolean) { val visibleMediaIndex = mediaCarouselScrollHandler.visibleMediaIndex if (MediaPlayerData.players().size > visibleMediaIndex) { - val mediaControlPanel = MediaPlayerData.players().elementAt(visibleMediaIndex) + val mediaControlPanel = MediaPlayerData.getMediaControlPanel(visibleMediaIndex) val hasActiveMediaOrRecommendationCard = MediaPlayerData.hasActiveMediaOrRecommendationCard() if (!hasActiveMediaOrRecommendationCard && !qsExpanded) { // Skip logging if on LS or QQS, and there is no active media card return } - logSmartspaceCardReported(800, // SMARTSPACE_CARD_SEEN - mediaControlPanel.mSmartspaceId, - mediaControlPanel.mUid, - intArrayOf(mediaControlPanel.surfaceForSmartspaceLogging)) - mediaControlPanel.mIsImpressed = true + mediaControlPanel?.let { + logSmartspaceCardReported(800, // SMARTSPACE_CARD_SEEN + it.mSmartspaceId, + it.mUid, + intArrayOf(it.surfaceForSmartspaceLogging)) + it.mIsImpressed = true + } } } @@ -885,7 +896,7 @@ class MediaCarouselController @Inject constructor( return } - val mediaControlKey = MediaPlayerData.playerKeys().elementAt(rank) + val mediaControlKey = MediaPlayerData.visiblePlayerKeys().elementAt(rank) // Only log media resume card when Smartspace data is available if (!mediaControlKey.isSsMediaRec && !mediaManager.smartspaceMediaData.isActive && @@ -960,7 +971,8 @@ class MediaCarouselController @Inject constructor( pw.apply { println("keysNeedRemoval: $keysNeedRemoval") println("dataKeys: ${MediaPlayerData.dataKeys()}") - println("playerSortKeys: ${MediaPlayerData.playerKeys()}") + println("orderedPlayerSortKeys: ${MediaPlayerData.playerKeys()}") + println("visiblePlayerSortKeys: ${MediaPlayerData.visiblePlayerKeys()}") println("smartspaceMediaData: ${MediaPlayerData.smartspaceMediaData}") println("shouldPrioritizeSs: ${MediaPlayerData.shouldPrioritizeSs}") println("current size: $currentCarouselWidth x $currentCarouselHeight") @@ -1000,6 +1012,7 @@ internal object MediaPlayerData { data class MediaSortKey( val isSsMediaRec: Boolean, // Whether the item represents a Smartspace media recommendation. val data: MediaData, + val key: String, val updateTime: Long = 0, val isSsReactivated: Boolean = false ) @@ -1018,6 +1031,8 @@ internal object MediaPlayerData { private val mediaPlayers = TreeMap(comparator) private val mediaData: MutableMap = mutableMapOf() + // A map that tracks order of visible media players before they get reordered. + private val visibleMediaPlayers = LinkedHashMap() fun addMediaPlayer( key: String, @@ -1032,9 +1047,10 @@ internal object MediaPlayerData { debugLogger?.logPotentialMemoryLeak(key) } val sortKey = MediaSortKey(isSsMediaRec = false, - data, clock.currentTimeMillis(), isSsReactivated = isSsReactivated) + data, key, clock.currentTimeMillis(), isSsReactivated = isSsReactivated) mediaData.put(key, sortKey) mediaPlayers.put(sortKey, player) + visibleMediaPlayers.put(key, sortKey) } fun addMediaRecommendation( @@ -1050,10 +1066,16 @@ internal object MediaPlayerData { if (removedPlayer != null && removedPlayer != player) { debugLogger?.logPotentialMemoryLeak(key) } - val sortKey = MediaSortKey(isSsMediaRec = true, - EMPTY.copy(isPlaying = false), clock.currentTimeMillis(), isSsReactivated = true) + val sortKey = MediaSortKey( + isSsMediaRec = true, + EMPTY.copy(isPlaying = false), + key, + clock.currentTimeMillis(), + isSsReactivated = true + ) mediaData.put(key, sortKey) mediaPlayers.put(sortKey, player) + visibleMediaPlayers.put(key, sortKey) smartspaceMediaData = data } @@ -1067,12 +1089,18 @@ internal object MediaPlayerData { } mediaData.remove(oldKey)?.let { + // MediaPlayer should not be visible + // no need to set isDismissed flag. val removedPlayer = removeMediaPlayer(newKey) removedPlayer?.run { debugLogger?.logPotentialMemoryLeak(newKey) } mediaData.put(newKey, it) } } + fun getMediaControlPanel(visibleIndex: Int): MediaControlPanel? { + return mediaPlayers.get(visiblePlayerKeys().elementAt(visibleIndex)) + } + fun getMediaPlayer(key: String): MediaControlPanel? { return mediaData.get(key)?.let { mediaPlayers.get(it) } } @@ -1087,10 +1115,17 @@ internal object MediaPlayerData { return -1 } - fun removeMediaPlayer(key: String) = mediaData.remove(key)?.let { + /** + * Removes media player given the key. + * @param isDismissed determines whether the media player is removed from the carousel. + */ + fun removeMediaPlayer(key: String, isDismissed: Boolean = false) = mediaData.remove(key)?.let { if (it.isSsMediaRec) { smartspaceMediaData = null } + if (isDismissed) { + visibleMediaPlayers.remove(key) + } mediaPlayers.remove(it) } @@ -1102,6 +1137,8 @@ internal object MediaPlayerData { fun playerKeys() = mediaPlayers.keys + fun visiblePlayerKeys() = visibleMediaPlayers.values + /** Returns the index of the first non-timeout media. */ fun firstActiveMediaIndex(): Int { mediaPlayers.entries.forEachIndexed { index, e -> @@ -1126,6 +1163,7 @@ internal object MediaPlayerData { fun clear() { mediaData.clear() mediaPlayers.clear() + visibleMediaPlayers.clear() } /* Returns true if there is active media player card or recommendation card */ @@ -1140,4 +1178,16 @@ internal object MediaPlayerData { } fun isSsReactivated(key: String): Boolean = mediaData.get(key)?.isSsReactivated ?: false + + /** + * This method is called when media players are reordered. + * To make sure we have the new version of the order of + * media players visible to user. + */ + fun updateVisibleMediaPlayers() { + visibleMediaPlayers.clear() + playerKeys().forEach { + visibleMediaPlayers.put(it.key, it) + } + } } diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java index 759795f84963..fba51dda86e0 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java +++ b/packages/SystemUI/src/com/android/systemui/media/MediaControlPanel.java @@ -1441,7 +1441,7 @@ public class MediaControlPanel { } // Automatically scroll to the active player once the media is loaded. - mMediaCarouselController.setShouldScrollToActivePlayer(true); + mMediaCarouselController.setShouldScrollToKey(true); }); } 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 cc6874ba3ba7..7e0be6dd1815 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaCarouselControllerTest.kt @@ -76,7 +76,6 @@ class MediaCarouselControllerTest : SysuiTestCase() { @Mock lateinit var dumpManager: DumpManager @Mock lateinit var logger: MediaUiEventLogger @Mock lateinit var debugLogger: MediaCarouselControllerLogger - @Mock lateinit var mediaPlayer: MediaControlPanel @Mock lateinit var mediaViewController: MediaViewController @Mock lateinit var smartspaceMediaData: SmartspaceMediaData @Captor lateinit var listener: ArgumentCaptor @@ -107,8 +106,8 @@ class MediaCarouselControllerTest : SysuiTestCase() { verify(mediaDataManager).addListener(capture(listener)) verify(visualStabilityProvider) .addPersistentReorderingAllowedListener(capture(visualStabilityCallback)) - whenever(mediaControlPanelFactory.get()).thenReturn(mediaPlayer) - whenever(mediaPlayer.mediaViewController).thenReturn(mediaViewController) + whenever(mediaControlPanelFactory.get()).thenReturn(panel) + whenever(panel.mediaViewController).thenReturn(mediaViewController) whenever(mediaDataManager.smartspaceMediaData).thenReturn(smartspaceMediaData) MediaPlayerData.clear() } @@ -189,6 +188,10 @@ class MediaCarouselControllerTest : SysuiTestCase() { for ((index, key) in MediaPlayerData.playerKeys().withIndex()) { assertEquals(expected.get(index).first, key.data.notificationKey) } + + for ((index, key) in MediaPlayerData.visiblePlayerKeys().withIndex()) { + assertEquals(expected.get(index).first, key.data.notificationKey) + } } @Test @@ -203,6 +206,22 @@ class MediaCarouselControllerTest : SysuiTestCase() { assertTrue(MediaPlayerData.playerKeys().elementAt(2).isSsMediaRec) } + @Test + fun testOrderWithSmartspace_prioritized_updatingVisibleMediaPlayers() { + testPlayerOrdering() + + // If smartspace is prioritized + listener.value.onSmartspaceMediaDataLoaded( + SMARTSPACE_KEY, + EMPTY_SMARTSPACE_MEDIA_DATA.copy(isActive = true), + true + ) + + // Then it should be shown immediately after any actively playing controls + assertTrue(MediaPlayerData.playerKeys().elementAt(2).isSsMediaRec) + assertTrue(MediaPlayerData.visiblePlayerKeys().elementAt(2).isSsMediaRec) + } + @Test fun testOrderWithSmartspace_notPrioritized() { testPlayerOrdering() @@ -216,6 +235,31 @@ class MediaCarouselControllerTest : SysuiTestCase() { assertTrue(MediaPlayerData.playerKeys().elementAt(idx).isSsMediaRec) } + @Test + fun testPlayingExistingMediaPlayerFromCarousel_visibleMediaPlayersNotUpdated() { + testPlayerOrdering() + // playing paused player + listener.value.onMediaDataLoaded("paused local", + "paused local", + DATA.copy(active = true, isPlaying = true, + playbackLocation = MediaData.PLAYBACK_LOCAL, resumption = false)) + listener.value.onMediaDataLoaded("playing local", + "playing local", + DATA.copy(active = true, isPlaying = false, + playbackLocation = MediaData.PLAYBACK_LOCAL, resumption = true) + ) + + assertEquals( + MediaPlayerData.getMediaPlayerIndex("paused local"), + mediaCarouselController.mediaCarouselScrollHandler.visibleMediaIndex + ) + // paused player order should stays the same in visibleMediaPLayer map. + // paused player order should be first in mediaPlayer map. + assertEquals( + MediaPlayerData.visiblePlayerKeys().elementAt(3), + MediaPlayerData.playerKeys().elementAt(0) + ) + } @Test fun testSwipeDismiss_logged() { mediaCarouselController.mediaCarouselScrollHandler.dismissCallback.invoke() @@ -295,7 +339,7 @@ class MediaCarouselControllerTest : SysuiTestCase() { // adding a media recommendation card. listener.value.onSmartspaceMediaDataLoaded(SMARTSPACE_KEY, EMPTY_SMARTSPACE_MEDIA_DATA, false) - mediaCarouselController.shouldScrollToActivePlayer = true + mediaCarouselController.shouldScrollToKey = true // switching between media players. listener.value.onMediaDataLoaded("playing local", "playing local", @@ -315,8 +359,11 @@ class MediaCarouselControllerTest : SysuiTestCase() { @Test fun testMediaLoadedFromRecommendationCard_ScrollToActivePlayer() { - MediaPlayerData.addMediaRecommendation(SMARTSPACE_KEY, EMPTY_SMARTSPACE_MEDIA_DATA, panel, - false, clock) + listener.value.onSmartspaceMediaDataLoaded( + SMARTSPACE_KEY, + EMPTY_SMARTSPACE_MEDIA_DATA.copy(packageName = "PACKAGE_NAME", isActive = true), + false + ) listener.value.onMediaDataLoaded("playing local", null, DATA.copy(active = true, isPlaying = true, @@ -332,10 +379,12 @@ class MediaCarouselControllerTest : SysuiTestCase() { // Replaying the same media player one more time. // And check that the card stays in its position. + mediaCarouselController.shouldScrollToKey = true listener.value.onMediaDataLoaded("playing local", null, DATA.copy(active = true, isPlaying = true, - playbackLocation = MediaData.PLAYBACK_LOCAL, resumption = false) + playbackLocation = MediaData.PLAYBACK_LOCAL, resumption = false, + packageName = "PACKAGE_NAME") ) playerIndex = MediaPlayerData.getMediaPlayerIndex("playing local") assertEquals(playerIndex, 0) -- cgit v1.2.3-59-g8ed1b