From ecf1d11f7234886d2affb3764cf8fbd0e110bd6f Mon Sep 17 00:00:00 2001 From: Beth Thibodeau Date: Wed, 27 Apr 2022 22:56:50 -0500 Subject: [PATCH] Update when PlaybackState state or actions change Some apps are updating PlaybackState without also posting a new notification, which is looks buggy when using the new actions. This adds a check so that when the PlaybackState updates, if the actions (standard or custom) have changed, or the state has changed between playing or not, MediaDataManager will get notified and update the PlaybackState-based buttons. Fixes: 214335602 Fixes: 229392609 Test: manual - see bug Test: atest MediaTimeoutListenerTest MediaDataManagerTest Change-Id: If2eb02c9c969124ebf193a89ff37edd5b1e6ede1 --- .../systemui/media/MediaDataManager.kt | 38 ++-- .../systemui/media/MediaTimeoutListener.kt | 84 ++++++++- .../systemui/media/MediaTimeoutLogger.kt | 11 ++ .../systemui/media/MediaDataManagerTest.kt | 34 ++++ .../media/MediaTimeoutListenerTest.kt | 167 ++++++++++++++++++ 5 files changed, 315 insertions(+), 19 deletions(-) diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt index b2751cec5d9ed..426b45a315502 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt @@ -261,6 +261,8 @@ class MediaDataManager( // Set up links back into the pipeline for listeners that need to send events upstream. mediaTimeoutListener.timeoutCallback = { key: String, timedOut: Boolean -> setTimedOut(key, timedOut) } + mediaTimeoutListener.stateCallback = { key: String, state: PlaybackState -> + updateState(key, state) } mediaResumeListener.setManager(this) mediaDataFilter.mediaDataManager = this @@ -502,6 +504,21 @@ class MediaDataManager( } } + /** + * Called when the player's [PlaybackState] has been updated with new actions and/or state + */ + private fun updateState(key: String, state: PlaybackState) { + mediaEntries.get(key)?.let { + val actions = createActionsFromState(it.packageName, + mediaControllerFactory.create(it.token), UserHandle(it.userId)) + val data = it.copy( + semanticActions = actions, + isPlaying = isPlayingState(state.state)) + if (DEBUG) Log.d(TAG, "State updated outside of notification") + onMediaDataLoaded(key, key, data) + } + } + private fun removeEntry(key: String) { mediaEntries.remove(key)?.let { logger.logMediaRemoved(it.appUid, it.packageName, it.instanceId) @@ -673,11 +690,8 @@ class MediaDataManager( // Otherwise, use the notification actions var actionIcons: List = emptyList() var actionsToShowCollapsed: List = emptyList() - var semanticActions: MediaButton? = null - if (mediaFlags.areMediaSessionActionsEnabled(sbn.packageName, sbn.user) && - mediaController.playbackState != null) { - semanticActions = createActionsFromState(sbn.packageName, mediaController) - } else { + val semanticActions = createActionsFromState(sbn.packageName, mediaController, sbn.user) + if (semanticActions == null) { val actions = createActionsFromNotification(sbn) actionIcons = actions.first actionsToShowCollapsed = actions.second @@ -789,13 +803,17 @@ class MediaDataManager( * @return a Pair consisting of a list of media actions, and a list of ints representing which * of those actions should be shown in the compact player */ - private fun createActionsFromState(packageName: String, controller: MediaController): - MediaButton? { + private fun createActionsFromState( + packageName: String, + controller: MediaController, + user: UserHandle + ): MediaButton? { val state = controller.playbackState - if (state == null) { - return MediaButton() + if (state == null || !mediaFlags.areMediaSessionActionsEnabled(packageName, user)) { + return null } - // First, check for} standard actions + + // First, check for standard actions val playOrPause = if (isConnectingState(state.state)) { // Spinner needs to be animating to render anything. Start it here. val drawable = context.getDrawable( diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt b/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt index 8c6710a6fd687..fc8d38d59d59c 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutListener.kt @@ -55,6 +55,13 @@ class MediaTimeoutListener @Inject constructor( */ lateinit var timeoutCallback: (String, Boolean) -> Unit + /** + * Callback representing that a media object [PlaybackState] has changed. + * @param key Media control unique identifier + * @param state The new [PlaybackState] + */ + lateinit var stateCallback: (String, PlaybackState) -> Unit + override fun onMediaDataLoaded( key: String, oldKey: String?, @@ -85,17 +92,17 @@ class MediaTimeoutListener @Inject constructor( } reusedListener?.let { - val wasPlaying = it.playing ?: false + val wasPlaying = it.isPlaying() logger.logUpdateListener(key, wasPlaying) it.mediaData = data it.key = key mediaListeners[key] = it - if (wasPlaying != it.playing) { + if (wasPlaying != it.isPlaying()) { // If a player becomes active because of a migration, we'll need to broadcast // its state. Doing it now would lead to reentrant callbacks, so let's wait // until we're done. mainExecutor.execute { - if (mediaListeners[key]?.playing == true) { + if (mediaListeners[key]?.isPlaying() == true) { logger.logDelayedUpdate(key) timeoutCallback.invoke(key, false /* timedOut */) } @@ -121,7 +128,7 @@ class MediaTimeoutListener @Inject constructor( ) : MediaController.Callback() { var timedOut = false - var playing: Boolean? = null + var lastState: PlaybackState? = null var resumption: Boolean? = null var destroyed = false @@ -145,6 +152,9 @@ class MediaTimeoutListener @Inject constructor( private var mediaController: MediaController? = null private var cancellation: Runnable? = null + fun Int.isPlaying() = isPlayingState(this) + fun isPlaying() = lastState?.state?.isPlaying() ?: false + init { mediaData = data } @@ -175,16 +185,26 @@ class MediaTimeoutListener @Inject constructor( private fun processState(state: PlaybackState?, dispatchEvents: Boolean) { logger.logPlaybackState(key, state) - val isPlaying = state != null && isPlayingState(state.state) + val playingStateSame = (state?.state?.isPlaying() == isPlaying()) + val actionsSame = (lastState?.actions == state?.actions) && + areCustomActionListsEqual(lastState?.customActions, state?.customActions) val resumptionChanged = resumption != mediaData.resumption - if (playing == isPlaying && playing != null && !resumptionChanged) { + + lastState = state + + if ((!actionsSame || !playingStateSame) && state != null && dispatchEvents) { + logger.logStateCallback(key) + stateCallback.invoke(key, state) + } + + if (playingStateSame && !resumptionChanged) { return } - playing = isPlaying resumption = mediaData.resumption - if (!isPlaying) { - logger.logScheduleTimeout(key, isPlaying, resumption!!) + val playing = isPlaying() + if (!playing) { + logger.logScheduleTimeout(key, playing, resumption!!) if (cancellation != null && !resumptionChanged) { // if the media changed resume state, we'll need to adjust the timeout length logger.logCancelIgnored(key) @@ -220,4 +240,50 @@ class MediaTimeoutListener @Inject constructor( cancellation = null } } + + private fun areCustomActionListsEqual( + first: List?, + second: List? + ): Boolean { + // Same object, or both null + if (first === second) { + return true + } + + // Only one null, or different number of actions + if ((first == null || second == null) || (first.size != second.size)) { + return false + } + + // Compare individual actions + first.asSequence().zip(second.asSequence()).forEach { (firstAction, secondAction) -> + if (!areCustomActionsEqual(firstAction, secondAction)) { + return false + } + } + return true + } + + private fun areCustomActionsEqual( + firstAction: PlaybackState.CustomAction, + secondAction: PlaybackState.CustomAction + ): Boolean { + if (firstAction.action != secondAction.action || + firstAction.name != secondAction.name || + firstAction.icon != secondAction.icon) { + return false + } + + if ((firstAction.extras == null) != (secondAction.extras == null)) { + return false + } + if (firstAction.extras != null) { + firstAction.extras.keySet().forEach { key -> + if (firstAction.extras.get(key) != secondAction.extras.get(key)) { + return false + } + } + } + return true + } } diff --git a/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutLogger.kt b/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutLogger.kt index a86515990fcb2..d9c58c0d0d76f 100644 --- a/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutLogger.kt +++ b/packages/SystemUI/src/com/android/systemui/media/MediaTimeoutLogger.kt @@ -102,6 +102,17 @@ class MediaTimeoutLogger @Inject constructor( } ) + fun logStateCallback(key: String) = buffer.log( + TAG, + LogLevel.VERBOSE, + { + str1 = key + }, + { + "dispatching state update for $key" + } + ) + fun logScheduleTimeout(key: String, playing: Boolean, resumption: Boolean) = buffer.log( TAG, LogLevel.DEBUG, 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 0cbceb6700b43..333e148475df0 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt @@ -30,6 +30,7 @@ import com.android.systemui.statusbar.SbnBuilder import com.android.systemui.tuner.TunerService import com.android.systemui.util.concurrency.FakeExecutor import com.android.systemui.util.mockito.any +import com.android.systemui.util.mockito.argumentCaptor import com.android.systemui.util.mockito.capture import com.android.systemui.util.mockito.eq import com.android.systemui.util.time.FakeSystemClock @@ -47,6 +48,7 @@ import org.mockito.Mock import org.mockito.Mockito import org.mockito.Mockito.never import org.mockito.Mockito.reset +import org.mockito.Mockito.times import org.mockito.Mockito.verify import org.mockito.Mockito.verifyNoMoreInteractions import org.mockito.junit.MockitoJUnit @@ -938,6 +940,38 @@ class MediaDataManagerTest : SysuiTestCase() { eq(instanceId), eq(MediaData.PLAYBACK_CAST_REMOTE)) } + @Test + fun testPlaybackStateChange_keyExists_callsListener() { + // Notification has been added + addNotificationAndLoad() + val callbackCaptor = argumentCaptor<(String, PlaybackState) -> Unit>() + verify(mediaTimeoutListener).stateCallback = capture(callbackCaptor) + + // Callback gets an updated state + val state = PlaybackState.Builder() + .setState(PlaybackState.STATE_PLAYING, 0L, 1f) + .build() + callbackCaptor.value.invoke(KEY, state) + + // Listener is notified of updated state + verify(listener).onMediaDataLoaded(eq(KEY), eq(KEY), + capture(mediaDataCaptor), eq(true), eq(0), eq(false)) + assertThat(mediaDataCaptor.value.isPlaying).isTrue() + } + + @Test + fun testPlaybackStateChange_keyDoesNotExist_doesNothing() { + val state = PlaybackState.Builder().build() + val callbackCaptor = argumentCaptor<(String, PlaybackState) -> Unit>() + verify(mediaTimeoutListener).stateCallback = capture(callbackCaptor) + + // No media added with this key + + callbackCaptor.value.invoke(KEY, state) + verify(listener, never()).onMediaDataLoaded(eq(KEY), any(), any(), anyBoolean(), anyInt(), + anyBoolean()) + } + /** * Helper function to add a media notification and capture the resulting MediaData */ diff --git a/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt b/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt index 60cbb1754db61..91c0cc2ff8912 100644 --- a/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt +++ b/packages/SystemUI/tests/src/com/android/systemui/media/MediaTimeoutListenerTest.kt @@ -65,6 +65,7 @@ class MediaTimeoutListenerTest : SysuiTestCase() { @Mock private lateinit var logger: MediaTimeoutLogger private lateinit var executor: FakeExecutor @Mock private lateinit var timeoutCallback: (String, Boolean) -> Unit + @Mock private lateinit var stateCallback: (String, PlaybackState) -> Unit @Captor private lateinit var mediaCallbackCaptor: ArgumentCaptor @JvmField @Rule val mockito = MockitoJUnit.rule() private lateinit var metadataBuilder: MediaMetadata.Builder @@ -80,6 +81,7 @@ class MediaTimeoutListenerTest : SysuiTestCase() { executor = FakeExecutor(FakeSystemClock()) mediaTimeoutListener = MediaTimeoutListener(mediaControllerFactory, executor, logger) mediaTimeoutListener.timeoutCallback = timeoutCallback + mediaTimeoutListener.stateCallback = stateCallback // Create a media session and notification for testing. metadataBuilder = MediaMetadata.Builder().apply { @@ -368,4 +370,169 @@ class MediaTimeoutListenerTest : SysuiTestCase() { // THEN the timeout runnable is cancelled assertThat(executor.numPending()).isEqualTo(0) } + + @Test + fun testOnMediaDataLoaded_playbackActionsChanged_noCallback() { + // Load media data once + val pausedState = PlaybackState.Builder() + .setActions(PlaybackState.ACTION_PAUSE) + .build() + loadMediaDataWithPlaybackState(pausedState) + + // When media data is loaded again, with different actions + val playingState = PlaybackState.Builder() + .setActions(PlaybackState.ACTION_PLAY) + .build() + loadMediaDataWithPlaybackState(playingState) + + // Then the callback is not invoked + verify(stateCallback, never()).invoke(eq(KEY), any()) + } + + @Test + fun testOnPlaybackStateChanged_playbackActionsChanged_sendsCallback() { + // Load media data once + val pausedState = PlaybackState.Builder() + .setActions(PlaybackState.ACTION_PAUSE) + .build() + loadMediaDataWithPlaybackState(pausedState) + + // When the playback state changes, and has different actions + val playingState = PlaybackState.Builder() + .setActions(PlaybackState.ACTION_PLAY) + .build() + mediaCallbackCaptor.value.onPlaybackStateChanged(playingState) + + // Then the callback is invoked + verify(stateCallback).invoke(eq(KEY), eq(playingState!!)) + } + + @Test + fun testOnPlaybackStateChanged_differentCustomActions_sendsCallback() { + val customOne = PlaybackState.CustomAction.Builder( + "ACTION_1", + "custom action 1", + android.R.drawable.ic_media_ff) + .build() + val pausedState = PlaybackState.Builder() + .setActions(PlaybackState.ACTION_PAUSE) + .addCustomAction(customOne) + .build() + loadMediaDataWithPlaybackState(pausedState) + + // When the playback state actions change + val customTwo = PlaybackState.CustomAction.Builder( + "ACTION_2", + "custom action 2", + android.R.drawable.ic_media_rew) + .build() + val pausedStateTwoActions = PlaybackState.Builder() + .setActions(PlaybackState.ACTION_PAUSE) + .addCustomAction(customOne) + .addCustomAction(customTwo) + .build() + mediaCallbackCaptor.value.onPlaybackStateChanged(pausedStateTwoActions) + + // Then the callback is invoked + verify(stateCallback).invoke(eq(KEY), eq(pausedStateTwoActions!!)) + } + + @Test + fun testOnPlaybackStateChanged_sameActions_noCallback() { + val stateWithActions = PlaybackState.Builder() + .setActions(PlaybackState.ACTION_PLAY) + .build() + loadMediaDataWithPlaybackState(stateWithActions) + + // When the playback state updates with the same actions + mediaCallbackCaptor.value.onPlaybackStateChanged(stateWithActions) + + // Then the callback is not invoked again + verify(stateCallback, never()).invoke(eq(KEY), any()) + } + + @Test + fun testOnPlaybackStateChanged_sameCustomActions_noCallback() { + val actionName = "custom action" + val actionIcon = android.R.drawable.ic_media_ff + val customOne = PlaybackState.CustomAction.Builder(actionName, actionName, actionIcon) + .build() + val stateOne = PlaybackState.Builder() + .setActions(PlaybackState.ACTION_PAUSE) + .addCustomAction(customOne) + .build() + loadMediaDataWithPlaybackState(stateOne) + + // When the playback state is updated, but has the same actions + val customTwo = PlaybackState.CustomAction.Builder(actionName, actionName, actionIcon) + .build() + val stateTwo = PlaybackState.Builder() + .setActions(PlaybackState.ACTION_PAUSE) + .addCustomAction(customTwo) + .build() + mediaCallbackCaptor.value.onPlaybackStateChanged(stateTwo) + + // Then the callback is not invoked + verify(stateCallback, never()).invoke(eq(KEY), any()) + } + + @Test + fun testOnMediaDataLoaded_isPlayingChanged_noCallback() { + // Load media data in paused state + val pausedState = PlaybackState.Builder() + .setState(PlaybackState.STATE_PAUSED, 0L, 0f) + .build() + loadMediaDataWithPlaybackState(pausedState) + + // When media data is loaded again but playing + val playingState = PlaybackState.Builder() + .setState(PlaybackState.STATE_PLAYING, 0L, 1f) + .build() + loadMediaDataWithPlaybackState(playingState) + + // Then the callback is not invoked + verify(stateCallback, never()).invoke(eq(KEY), any()) + } + + @Test + fun testOnPlaybackStateChanged_isPlayingChanged_sendsCallback() { + // Load media data in paused state + val pausedState = PlaybackState.Builder() + .setState(PlaybackState.STATE_PAUSED, 0L, 0f) + .build() + loadMediaDataWithPlaybackState(pausedState) + + // When the playback state changes to playing + val playingState = PlaybackState.Builder() + .setState(PlaybackState.STATE_PLAYING, 0L, 1f) + .build() + mediaCallbackCaptor.value.onPlaybackStateChanged(playingState) + + // Then the callback is invoked + verify(stateCallback).invoke(eq(KEY), eq(playingState!!)) + } + + @Test + fun testOnPlaybackStateChanged_isPlayingSame_noCallback() { + // Load media data in paused state + val pausedState = PlaybackState.Builder() + .setState(PlaybackState.STATE_PAUSED, 0L, 0f) + .build() + loadMediaDataWithPlaybackState(pausedState) + + // When the playback state is updated, but still not playing + val playingState = PlaybackState.Builder() + .setState(PlaybackState.STATE_STOPPED, 0L, 0f) + .build() + mediaCallbackCaptor.value.onPlaybackStateChanged(playingState) + + // Then the callback is not invoked + verify(stateCallback, never()).invoke(eq(KEY), eq(playingState!!)) + } + + private fun loadMediaDataWithPlaybackState(state: PlaybackState) { + `when`(mediaController.playbackState).thenReturn(state) + mediaTimeoutListener.onMediaDataLoaded(KEY, null, mediaData) + verify(mediaController).registerCallback(capture(mediaCallbackCaptor)) + } }