From 4053be9269a5e33492e38a5aa59b13ef2f629be8 Mon Sep 17 00:00:00 2001 From: Beth Thibodeau Date: Thu, 22 Apr 2021 15:30:34 -0400 Subject: [PATCH] Only allow local sessions to be resumable It is possible for an app to be resumable locally, but also post notifications when playing remotely that are not resumable long-term. This change makes it so that only local sessions will be used for resume players, and remote sessions will be removed when the underlying notification is removed. Bug: 185681682 Test: manual - start playback on Spotify web, observe notification on phone, then close app on phone and observe no resume player Test: atest MediaDataManagerTest MediaResumeListenerTest Change-Id: I0a6ff2bab234a33298f84fd6d99b7d537d3d5e34 --- .../systemui/media/MediaDataManager.kt | 4 +-- .../systemui/media/MediaResumeListener.kt | 2 +- .../systemui/media/MediaDataManagerTest.kt | 25 +++++++++++++++++++ .../systemui/media/MediaResumeListenerTest.kt | 10 ++++++++ 4 files changed, 38 insertions(+), 3 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt index 574ce0dffee0f..f689eb3ed7878 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt @@ -586,7 +586,7 @@ class MediaDataManager( } val isLocalSession = mediaController.playbackInfo?.playbackType == - MediaController.PlaybackInfo.PLAYBACK_TYPE_LOCAL ?: true + MediaController.PlaybackInfo.PLAYBACK_TYPE_LOCAL val isPlaying = mediaController.playbackState?.let { isPlayingState(it.state) } ?: null foregroundExecutor.execute { @@ -724,7 +724,7 @@ class MediaDataManager( Assert.isMainThread() val removed = mediaEntries.remove(key) if (useMediaResumption && removed?.resumeAction != null && - !isBlockedFromResume(removed.packageName)) { + !isBlockedFromResume(removed.packageName) && removed?.isLocalSession == true) { Log.d(TAG, "Not removing $key because resumable") // Move to resume key (aka package name) if that key doesn't already exist. val resumeAction = getResumeMediaAction(removed.resumeAction!!) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt b/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt index 80d13715ca077..b0be57668d085 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaResumeListener.kt @@ -173,7 +173,7 @@ class MediaResumeListener @Inject constructor( mediaBrowser?.disconnect() // If we don't have a resume action, check if we haven't already if (data.resumeAction == null && !data.hasCheckedForResume && - !blockedApps.contains(data.packageName)) { + !blockedApps.contains(data.packageName) && data.isLocalSession) { // TODO also check for a media button receiver intended for restarting (b/154127084) Log.d(TAG, "Checking for service component for " + data.packageName) val pm = context.packageManager diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt index 96eb4b0969311..fc03e3af7beed 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt @@ -60,6 +60,7 @@ class MediaDataManagerTest : SysuiTestCase() { @JvmField @Rule val mockito = MockitoJUnit.rule() @Mock lateinit var mediaControllerFactory: MediaControllerFactory @Mock lateinit var controller: MediaController + @Mock lateinit var playbackInfo: MediaController.PlaybackInfo lateinit var session: MediaSession lateinit var metadataBuilder: MediaMetadata.Builder lateinit var backgroundExecutor: FakeExecutor @@ -118,6 +119,9 @@ class MediaDataManagerTest : SysuiTestCase() { putString(MediaMetadata.METADATA_KEY_TITLE, SESSION_TITLE) } whenever(mediaControllerFactory.create(eq(session.sessionToken))).thenReturn(controller) + whenever(controller.playbackInfo).thenReturn(playbackInfo) + whenever(playbackInfo.playbackType).thenReturn( + MediaController.PlaybackInfo.PLAYBACK_TYPE_LOCAL) // This is an ugly hack for now. The mediaSessionBasedFilter is one of the internal // listeners in the internal processing pipeline. It receives events, but ince it is a @@ -229,6 +233,27 @@ class MediaDataManagerTest : SysuiTestCase() { verify(listener).onMediaDataRemoved(eq(KEY_2)) } + @Test + fun testOnNotificationRemoved_withResumption_butNotLocal() { + // GIVEN that the manager has a notification with a resume action, but is not local + whenever(controller.metadata).thenReturn(metadataBuilder.build()) + whenever(playbackInfo.playbackType).thenReturn( + MediaController.PlaybackInfo.PLAYBACK_TYPE_REMOTE) + mediaDataManager.onNotificationAdded(KEY, mediaNotification) + assertThat(backgroundExecutor.runAllReady()).isEqualTo(1) + assertThat(foregroundExecutor.runAllReady()).isEqualTo(1) + verify(listener).onMediaDataLoaded(eq(KEY), eq(null), capture(mediaDataCaptor)) + val data = mediaDataCaptor.value + val dataRemoteWithResume = data.copy(resumeAction = Runnable {}, isLocalSession = false) + mediaDataManager.onMediaDataLoaded(KEY, null, dataRemoteWithResume) + + // WHEN the notification is removed + mediaDataManager.onNotificationRemoved(KEY) + + // THEN the media data is removed + verify(listener).onMediaDataRemoved(eq(KEY)) + } + @Test fun testAppBlockedFromResumption() { // GIVEN that the manager has a notification with a resume action diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaResumeListenerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaResumeListenerTest.kt index 59c2b176f12ee..96d1d94795f6e 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaResumeListenerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaResumeListenerTest.kt @@ -181,6 +181,16 @@ class MediaResumeListenerTest : SysuiTestCase() { verify(mediaDataManager).setResumeAction(KEY, null) } + @Test + fun testOnLoad_remotePlayback_doesNotCheck() { + // When media data is loaded that has not been checked yet, and is not local + val dataRemote = data.copy(isLocalSession = false) + resumeListener.onMediaDataLoaded(KEY, null, dataRemote) + + // Then we do not take action + verify(mediaDataManager, never()).setResumeAction(any(), any()) + } + @Test fun testOnLoad_checksForResume_hasService() { // Set up mocks to successfully find a MBS that returns valid media