Merge "Deduplicate resumption controls when notif removed" into rvc-dev

packages/SystemUI/src/com/android/systemui/media/MediaCarouselController.kt

@@ -5,6 +5,7 @@ import android.content.Intent
 import android.content.res.Configuration
 import android.graphics.Color
 import android.provider.Settings.ACTION_MEDIA_CONTROLS_SETTINGS
+import android.util.Log
 import android.util.MathUtils
 import android.view.LayoutInflater
 import android.view.View
@@ -25,6 +26,7 @@ import javax.inject.Inject
 import javax.inject.Provider
 import javax.inject.Singleton
 
+private const val TAG = "MediaCarouselController"
 private val settingsIntent = Intent().setAction(ACTION_MEDIA_CONTROLS_SETTINGS)
 
 /**
@@ -236,7 +238,9 @@ class MediaCarouselController @Inject constructor(
         val oldData = mediaPlayers[oldKey]
         if (oldData != null) {
             val oldData = mediaPlayers.remove(oldKey)
-            mediaPlayers.put(key, oldData!!)
+            mediaPlayers.put(key, oldData!!)?.let {
+                Log.wtf(TAG, "new key $key already exists when migrating from $oldKey")
+            }
         }
         var existingPlayer = mediaPlayers[key]
         if (existingPlayer == null) {
@@ -271,6 +275,11 @@ class MediaCarouselController @Inject constructor(
         updatePageIndicator()
         mediaCarouselScrollHandler.onPlayersChanged()
         mediaCarousel.requiresRemeasuring = true
+        // Check postcondition: mediaContent should have the same number of children as there are
+        // elements in mediaPlayers.
+        if (mediaPlayers.size != mediaContent.childCount) {
+            Log.wtf(TAG, "Size of players list and number of views in carousel are out of sync")
+        }
     }
 
     private fun removePlayer(key: String) {

packages/SystemUI/src/com/android/systemui/media/MediaDataManager.kt

@@ -517,22 +517,36 @@ class MediaDataManager(
 
     fun onNotificationRemoved(key: String) {
         Assert.isMainThread()
-        if (useMediaResumption && mediaEntries.get(key)?.resumeAction != null) {
+        val removed = mediaEntries.remove(key)
+        if (useMediaResumption && removed?.resumeAction != null) {
             Log.d(TAG, "Not removing $key because resumable")
-            // Move to resume key aka package name
-            val data = mediaEntries.remove(key)!!
-            val resumeAction = getResumeMediaAction(data.resumeAction!!)
-            val updated = data.copy(token = null, actions = listOf(resumeAction),
+            // Move to resume key (aka package name) if that key doesn't already exist.
+            val resumeAction = getResumeMediaAction(removed.resumeAction!!)
+            val updated = removed.copy(token = null, actions = listOf(resumeAction),
                     actionsToShowInCompact = listOf(0), active = false, resumption = true)
-            mediaEntries.put(data.packageName, updated)
-            // Notify listeners of "new" controls
+            val pkg = removed?.packageName
+            val migrate = mediaEntries.put(pkg, updated) == null
+            // Notify listeners of "new" controls when migrating or removed and update when not
             val listenersCopy = listeners.toSet()
-            listenersCopy.forEach {
-                it.onMediaDataLoaded(data.packageName, key, updated)
+            if (migrate) {
+                listenersCopy.forEach {
+                    it.onMediaDataLoaded(pkg, key, updated)
+                }
+            } else {
+                // Since packageName is used for the key of the resumption controls, it is
+                // possible that another notification has already been reused for the resumption
+                // controls of this package. In this case, rather than renaming this player as
+                // packageName, just remove it and then send a update to the existing resumption
+                // controls.
+                listenersCopy.forEach {
+                    it.onMediaDataRemoved(key)
+                }
+                listenersCopy.forEach {
+                    it.onMediaDataLoaded(pkg, pkg, updated)
+                }
             }
             return
         }
-        val removed = mediaEntries.remove(key)
         if (removed != null) {
             val listenersCopy = listeners.toSet()
             listenersCopy.forEach {

packages/SystemUI/tests/src/com/android/systemui/media/MediaDataManagerTest.kt

@@ -31,6 +31,7 @@ import org.mockito.junit.MockitoJUnit
 import org.mockito.Mockito.`when` as whenever
 
 private const val KEY = "KEY"
+private const val KEY_2 = "KEY_2"
 private const val PACKAGE_NAME = "com.android.systemui"
 private const val APP_NAME = "SystemUI"
 private const val SESSION_ARTIST = "artist"
@@ -156,8 +157,43 @@ class MediaDataManagerTest : SysuiTestCase() {
         mediaDataManager.onMediaDataLoaded(KEY, null, data.copy(resumeAction = Runnable {}))
         // WHEN the notification is removed
         mediaDataManager.onNotificationRemoved(KEY)
-        // THEN the media data indicates that it is
+        // THEN the media data indicates that it is for resumption
         assertThat(listener.data!!.resumption).isTrue()
+        // AND the new key is the package name
+        assertThat(listener.key!!).isEqualTo(PACKAGE_NAME)
+        assertThat(listener.oldKey!!).isEqualTo(KEY)
+        assertThat(listener.removedKey).isNull()
+    }
+
+    @Test
+    fun testOnNotificationRemoved_twoWithResumption() {
+        // GIVEN that the manager has two notifications with resume actions
+        val listener = TestListener()
+        mediaDataManager.addListener(listener)
+        whenever(controller.metadata).thenReturn(metadataBuilder.build())
+        mediaDataManager.onNotificationAdded(KEY, mediaNotification)
+        mediaDataManager.onNotificationAdded(KEY_2, mediaNotification)
+        assertThat(backgroundExecutor.runAllReady()).isEqualTo(2)
+        assertThat(foregroundExecutor.runAllReady()).isEqualTo(2)
+        val data = listener.data!!
+        assertThat(data.resumption).isFalse()
+        val resumableData = data.copy(resumeAction = Runnable {})
+        mediaDataManager.onMediaDataLoaded(KEY, null, resumableData)
+        mediaDataManager.onMediaDataLoaded(KEY_2, null, resumableData)
+        // WHEN the first is removed
+        mediaDataManager.onNotificationRemoved(KEY)
+        // THEN the data is for resumption and the key is migrated to the package name
+        assertThat(listener.data!!.resumption).isTrue()
+        assertThat(listener.key!!).isEqualTo(PACKAGE_NAME)
+        assertThat(listener.oldKey!!).isEqualTo(KEY)
+        assertThat(listener.removedKey).isNull()
+        // WHEN the second is removed
+        mediaDataManager.onNotificationRemoved(KEY_2)
+        // THEN the data is for resumption and the second key is removed
+        assertThat(listener.data!!.resumption).isTrue()
+        assertThat(listener.key!!).isEqualTo(PACKAGE_NAME)
+        assertThat(listener.oldKey!!).isEqualTo(PACKAGE_NAME)
+        assertThat(listener.removedKey!!).isEqualTo(KEY_2)
     }
 
     @Test
@@ -190,6 +226,7 @@ class MediaDataManagerTest : SysuiTestCase() {
         var data: MediaData? = null
         var key: String? = null
         var oldKey: String? = null
+        var removedKey: String? = null
 
         override fun onMediaDataLoaded(key: String, oldKey: String?, data: MediaData) {
             this.key = key
@@ -198,9 +235,7 @@ class MediaDataManagerTest : SysuiTestCase() {
         }
 
         override fun onMediaDataRemoved(key: String) {
-            this.key = key
-            oldKey = null
-            data = null
+            removedKey = key
         }
     }
 }