From 11a332b7c4f899b2bdab19f5710a876dda23acb9 Mon Sep 17 00:00:00 2001 From: Matt Pietal Date: Mon, 31 Aug 2020 09:07:48 -0400 Subject: [PATCH] Media - Leave playing media in QS Previously, with resumption off, when dismissing a playing media player from QQS would also remove the player from QS, leaving the user with no notification to control the media. Only remove players from QS when the media is paused. Fixes: 160425592 Test: atest MediaDataFilterTest Test: manual, switch resumption setting off, play media, and dismiss from QQS Change-Id: Ia7c9a8d9b100506f2c12ef2d14e4a952e151bd05 --- .../systemui/media/MediaCarouselController.kt | 37 ++++++++----- .../com/android/systemui/media/MediaData.kt | 12 ++++- .../android/systemui/media/MediaDataFilter.kt | 8 +-- .../systemui/media/MediaDataManager.kt | 15 +++++- .../systemui/media/MediaResumeListener.kt | 2 - .../media/MediaDataCombineLatestTest.java | 2 +- .../systemui/media/MediaPlayerDataTest.kt | 54 ++++++++++--------- 7 files changed, 80 insertions(+), 50 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt index e5a9ac10389fa..f150381f40700 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt @@ -42,7 +42,7 @@ class MediaCarouselController @Inject constructor( private val mediaHostStatesManager: MediaHostStatesManager, private val activityStarter: ActivityStarter, @Main executor: DelayableExecutor, - mediaManager: MediaDataManager, + private val mediaManager: MediaDataManager, configurationController: ConfigurationController, falsingManager: FalsingManager ) { @@ -109,6 +109,7 @@ class MediaCarouselController @Inject constructor( private val pageIndicator: PageIndicator private val visualStabilityCallback: VisualStabilityManager.Callback private var needsReordering: Boolean = false + private var keysNeedRemoval = mutableSetOf() private var isRtl: Boolean = false set(value) { if (value != field) { @@ -161,6 +162,10 @@ class MediaCarouselController @Inject constructor( needsReordering = false reorderAllPlayers() } + + keysNeedRemoval.forEach { removePlayer(it) } + keysNeedRemoval.clear() + // Let's reset our scroll position mediaCarouselScrollHandler.scrollToStart() } @@ -168,13 +173,19 @@ class MediaCarouselController @Inject constructor( true /* persistent */) mediaManager.addListener(object : MediaDataManager.Listener { override fun onMediaDataLoaded(key: String, oldKey: String?, data: MediaData) { - if (!data.active && !Utils.useMediaResumption(context)) { - // This view is inactive, let's remove this! This happens e.g when dismissing / - // timing out a view. We still have the data around because resumption could - // be on, but we should save the resources and release this. - onMediaDataRemoved(key) + addOrUpdatePlayer(key, oldKey, data) + val canRemove = data.isPlaying?.let { !it } ?: data.isClearable + if (canRemove && !Utils.useMediaResumption(context)) { + // This view isn't playing, let's remove this! This happens e.g when + // dismissing/timing out a view. We still have the data around because + // resumption could be on, but we should save the resources and release this. + if (visualStabilityManager.isReorderingAllowed) { + onMediaDataRemoved(key) + } else { + keysNeedRemoval.add(key) + } } else { - addOrUpdatePlayer(key, oldKey, data) + keysNeedRemoval.remove(key) } } @@ -236,12 +247,12 @@ class MediaCarouselController @Inject constructor( var newPlayer = mediaControlPanelFactory.get() newPlayer.attach(PlayerViewHolder.create(LayoutInflater.from(context), mediaContent)) newPlayer.mediaViewController.sizeChangedListener = this::updateCarouselDimensions - MediaPlayerData.addMediaPlayer(key, data, newPlayer) val lp = LinearLayout.LayoutParams(ViewGroup.LayoutParams.MATCH_PARENT, ViewGroup.LayoutParams.WRAP_CONTENT) newPlayer.view?.player?.setLayoutParams(lp) newPlayer.bind(data) newPlayer.setListening(currentlyExpanded) + MediaPlayerData.addMediaPlayer(key, data, newPlayer) updatePlayerToState(newPlayer, noAnimation = true) reorderAllPlayers() } else { @@ -271,6 +282,9 @@ class MediaCarouselController @Inject constructor( removed.onDestroy() mediaCarouselScrollHandler.onPlayersChanged() updatePageIndicator() + + // Inform the media manager of a potentially late dismissal + mediaManager.dismissMediaData(key, 0L) } } @@ -478,12 +492,11 @@ class MediaCarouselController @Inject constructor( internal object MediaPlayerData { private data class MediaSortKey( val data: MediaData, - val updateTime: Long = 0, - val isPlaying: Boolean = false + val updateTime: Long = 0 ) private val comparator = - compareByDescending { it.isPlaying } + compareByDescending { it.data.isPlaying } .thenByDescending { it.data.isLocalSession } .thenByDescending { !it.data.resumption } .thenByDescending { it.updateTime } @@ -493,7 +506,7 @@ internal object MediaPlayerData { fun addMediaPlayer(key: String, data: MediaData, player: MediaControlPanel) { removeMediaPlayer(key) - val sortKey = MediaSortKey(data, System.currentTimeMillis(), player.isPlaying()) + val sortKey = MediaSortKey(data, System.currentTimeMillis()) mediaData.put(key, sortKey) mediaPlayers.put(sortKey, player) } diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaData.kt b/packages/SystemUI/src/com/android/systemui/media/MediaData.kt index d6a02687c905a..40a879abde341 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaData.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaData.kt @@ -94,7 +94,17 @@ data class MediaData( * Notification key for cancelling a media player after a timeout (when not using resumption.) */ val notificationKey: String? = null, - var hasCheckedForResume: Boolean = false + var hasCheckedForResume: Boolean = false, + + /** + * If apps do not report PlaybackState, set as null to imply 'undetermined' + */ + val isPlaying: Boolean? = null, + + /** + * Set from the notification and used as fallback when PlaybackState cannot be determined + */ + val isClearable: Boolean = true ) /** State of a media action. */ diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaDataFilter.kt b/packages/SystemUI/src/com/android/systemui/media/MediaDataFilter.kt index 0664a41f841db..1f580a953d099 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaDataFilter.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaDataFilter.kt @@ -136,14 +136,8 @@ class MediaDataFilter @Inject constructor( /** * Are there any media entries we should display? - * If resumption is enabled, this will include inactive players - * If resumption is disabled, we only want to show active players */ - fun hasAnyMedia() = if (mediaResumeListener.isResumptionEnabled()) { - userEntries.isNotEmpty() - } else { - hasActiveMedia() - } + fun hasAnyMedia() = userEntries.isNotEmpty() /** * Add a listener for filtered [MediaData] changes diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt index b2ad19b5f42f8..cb6b22c2321f4 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt @@ -47,6 +47,7 @@ import com.android.systemui.dagger.qualifiers.Background import com.android.systemui.dagger.qualifiers.Main import com.android.systemui.dump.DumpManager import com.android.systemui.plugins.ActivityStarter +import com.android.systemui.statusbar.NotificationMediaManager.isPlayingState import com.android.systemui.statusbar.notification.MediaNotificationProcessor import com.android.systemui.statusbar.notification.row.HybridGroupManager import com.android.systemui.util.Assert @@ -350,6 +351,16 @@ class MediaDataManager( } fun dismissMediaData(key: String, delay: Long) { + backgroundExecutor.execute { + mediaEntries[key]?.let { mediaData -> + if (mediaData.isLocalSession) { + mediaData.token?.let { + val mediaController = mediaControllerFactory.create(it) + mediaController.transportControls.stop() + } + } + } + } foregroundExecutor.executeDelayed({ removeEntry(key) }, delay) } @@ -500,6 +511,7 @@ class MediaDataManager( val isLocalSession = mediaController.playbackInfo?.playbackType == MediaController.PlaybackInfo.PLAYBACK_TYPE_LOCAL ?: true + val isPlaying = mediaController.playbackState?.let { isPlayingState(it.state) } ?: null foregroundExecutor.execute { val resumeAction: Runnable? = mediaEntries[key]?.resumeAction @@ -509,7 +521,8 @@ class MediaDataManager( smallIconDrawable, artist, song, artWorkIcon, actionIcons, actionsToShowCollapsed, sbn.packageName, token, notif.contentIntent, null, active, resumeAction = resumeAction, isLocalSession = isLocalSession, - notificationKey = key, hasCheckedForResume = hasCheckedForResume)) + notificationKey = key, hasCheckedForResume = hasCheckedForResume, + isPlaying = isPlaying, isClearable = sbn.isClearable())) } } diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt b/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt index c00b5e92f93d8..5b59214afdc94 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt @@ -125,8 +125,6 @@ class MediaResumeListener @Inject constructor( }, Settings.Secure.MEDIA_CONTROLS_RESUME_BLOCKED) } - fun isResumptionEnabled() = useMediaResumption - private fun loadSavedComponents() { // Make sure list is empty (if we switched users) resumeComponents.clear() diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataCombineLatestTest.java b/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataCombineLatestTest.java index 89538ac8bc9f6..609b8474d1346 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataCombineLatestTest.java +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataCombineLatestTest.java @@ -74,7 +74,7 @@ public class MediaDataCombineLatestTest extends SysuiTestCase { mMediaData = new MediaData(USER_ID, true, BG_COLOR, APP, null, ARTIST, TITLE, null, new ArrayList<>(), new ArrayList<>(), PACKAGE, null, null, null, true, null, true, - false, KEY, false); + false, KEY, false, false, false); mDeviceData = new MediaDeviceData(true, null, DEVICE_NAME); } diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaPlayerDataTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaPlayerDataTest.kt index da1ec9869d87c..ef8d322ca2ece 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaPlayerDataTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaPlayerDataTest.kt @@ -25,7 +25,6 @@ import org.junit.Ignore import org.junit.Test import org.junit.runner.RunWith import org.mockito.Mockito.mock -import org.mockito.Mockito.`when` as whenever @SmallTest @RunWith(AndroidTestingRunner::class) @@ -34,6 +33,8 @@ public class MediaPlayerDataTest : SysuiTestCase() { companion object { val LOCAL = true val RESUMPTION = true + val PLAYING = true + val UNDETERMINED = null } @Before @@ -44,15 +45,13 @@ public class MediaPlayerDataTest : SysuiTestCase() { @Test fun addPlayingThenRemote() { val playerIsPlaying = mock(MediaControlPanel::class.java) - whenever(playerIsPlaying.isPlaying).thenReturn(true) - val dataIsPlaying = createMediaData("app1", LOCAL, !RESUMPTION) + val dataIsPlaying = createMediaData("app1", PLAYING, LOCAL, !RESUMPTION) val playerIsRemote = mock(MediaControlPanel::class.java) - whenever(playerIsRemote.isPlaying).thenReturn(false) - val dataIsRemote = createMediaData("app2", !LOCAL, !RESUMPTION) + val dataIsRemote = createMediaData("app2", PLAYING, !LOCAL, !RESUMPTION) - MediaPlayerData.addMediaPlayer("1", dataIsPlaying, playerIsPlaying) MediaPlayerData.addMediaPlayer("2", dataIsRemote, playerIsRemote) + MediaPlayerData.addMediaPlayer("1", dataIsPlaying, playerIsPlaying) val players = MediaPlayerData.players() assertThat(players).hasSize(2) @@ -63,18 +62,16 @@ public class MediaPlayerDataTest : SysuiTestCase() { @Ignore("Flaky") fun switchPlayersPlaying() { val playerIsPlaying1 = mock(MediaControlPanel::class.java) - whenever(playerIsPlaying1.isPlaying).thenReturn(true) - val dataIsPlaying1 = createMediaData("app1", LOCAL, !RESUMPTION) + var dataIsPlaying1 = createMediaData("app1", PLAYING, LOCAL, !RESUMPTION) val playerIsPlaying2 = mock(MediaControlPanel::class.java) - whenever(playerIsPlaying2.isPlaying).thenReturn(false) - val dataIsPlaying2 = createMediaData("app2", LOCAL, !RESUMPTION) + var dataIsPlaying2 = createMediaData("app2", !PLAYING, LOCAL, !RESUMPTION) MediaPlayerData.addMediaPlayer("1", dataIsPlaying1, playerIsPlaying1) MediaPlayerData.addMediaPlayer("2", dataIsPlaying2, playerIsPlaying2) - whenever(playerIsPlaying1.isPlaying).thenReturn(false) - whenever(playerIsPlaying2.isPlaying).thenReturn(true) + dataIsPlaying1 = createMediaData("app1", !PLAYING, LOCAL, !RESUMPTION) + dataIsPlaying2 = createMediaData("app2", PLAYING, LOCAL, !RESUMPTION) MediaPlayerData.addMediaPlayer("1", dataIsPlaying1, playerIsPlaying1) MediaPlayerData.addMediaPlayer("2", dataIsPlaying2, playerIsPlaying2) @@ -87,38 +84,43 @@ public class MediaPlayerDataTest : SysuiTestCase() { @Test fun fullOrderTest() { val playerIsPlaying = mock(MediaControlPanel::class.java) - whenever(playerIsPlaying.isPlaying).thenReturn(true) - val dataIsPlaying = createMediaData("app1", LOCAL, !RESUMPTION) + val dataIsPlaying = createMediaData("app1", PLAYING, LOCAL, !RESUMPTION) val playerIsPlayingAndRemote = mock(MediaControlPanel::class.java) - whenever(playerIsPlayingAndRemote.isPlaying).thenReturn(true) - val dataIsPlayingAndRemote = createMediaData("app2", !LOCAL, !RESUMPTION) + val dataIsPlayingAndRemote = createMediaData("app2", PLAYING, !LOCAL, !RESUMPTION) val playerIsStoppedAndLocal = mock(MediaControlPanel::class.java) - whenever(playerIsStoppedAndLocal.isPlaying).thenReturn(false) - val dataIsStoppedAndLocal = createMediaData("app3", LOCAL, !RESUMPTION) + val dataIsStoppedAndLocal = createMediaData("app3", !PLAYING, LOCAL, !RESUMPTION) val playerIsStoppedAndRemote = mock(MediaControlPanel::class.java) - whenever(playerIsStoppedAndLocal.isPlaying).thenReturn(false) - val dataIsStoppedAndRemote = createMediaData("app4", !LOCAL, !RESUMPTION) + val dataIsStoppedAndRemote = createMediaData("app4", !PLAYING, !LOCAL, !RESUMPTION) val playerCanResume = mock(MediaControlPanel::class.java) - whenever(playerCanResume.isPlaying).thenReturn(false) - val dataCanResume = createMediaData("app5", LOCAL, RESUMPTION) + val dataCanResume = createMediaData("app5", !PLAYING, LOCAL, RESUMPTION) + + val playerUndetermined = mock(MediaControlPanel::class.java) + val dataUndetermined = createMediaData("app6", UNDETERMINED, LOCAL, RESUMPTION) MediaPlayerData.addMediaPlayer("3", dataIsStoppedAndLocal, playerIsStoppedAndLocal) MediaPlayerData.addMediaPlayer("5", dataIsStoppedAndRemote, playerIsStoppedAndRemote) MediaPlayerData.addMediaPlayer("4", dataCanResume, playerCanResume) MediaPlayerData.addMediaPlayer("1", dataIsPlaying, playerIsPlaying) MediaPlayerData.addMediaPlayer("2", dataIsPlayingAndRemote, playerIsPlayingAndRemote) + MediaPlayerData.addMediaPlayer("6", dataUndetermined, playerUndetermined) val players = MediaPlayerData.players() - assertThat(players).hasSize(5) + assertThat(players).hasSize(6) assertThat(players).containsExactly(playerIsPlaying, playerIsPlayingAndRemote, - playerIsStoppedAndLocal, playerCanResume, playerIsStoppedAndRemote).inOrder() + playerIsStoppedAndLocal, playerCanResume, playerIsStoppedAndRemote, + playerUndetermined).inOrder() } - private fun createMediaData(app: String, isLocalSession: Boolean, resumption: Boolean) = + private fun createMediaData( + app: String, + isPlaying: Boolean?, + isLocalSession: Boolean, + resumption: Boolean + ) = MediaData(0, false, 0, app, null, null, null, null, emptyList(), emptyList(), "", - null, null, null, true, null, isLocalSession, resumption, null, false) + null, null, null, true, null, isLocalSession, resumption, null, false, isPlaying) }