Reduce chance for leak in MediaPlayerData

MediaPlayerData#getMediaPlayerIndex had a chance to leak
data if called with two existing keys. This refactor reduces
the responsibility of #getMediaPlayerIndex, and adds a new method,

Fixes: 191150828
Test: atest SystemUITests
Change-Id: I989ac5e7d169d43c9ef6aa09a44603323543b305

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

@@ -204,7 +204,7 @@ class MediaCarouselController @Inject constructor(
                 isSsReactivated: Boolean
             ) {
                 if (addOrUpdatePlayer(key, oldKey, data)) {
-                    MediaPlayerData.getMediaPlayer(key, null)?.let {
+                    MediaPlayerData.getMediaPlayer(key)?.let {
                         logSmartspaceCardReported(759, // SMARTSPACE_CARD_RECEIVED
                                 it.mInstanceId,
                                 /* isRecommendationCard */ false,
@@ -241,7 +241,7 @@ class MediaCarouselController @Inject constructor(
                 if (DEBUG) Log.d(TAG, "Loading Smartspace media update")
                 if (data.isActive) {
                     addSmartspaceMediaRecommendations(key, data, shouldPrioritize)
-                    MediaPlayerData.getMediaPlayer(key, null)?.let {
+                    MediaPlayerData.getMediaPlayer(key)?.let {
                         logSmartspaceCardReported(759, // SMARTSPACE_CARD_RECEIVED
                                 it.mInstanceId,
                                 /* isRecommendationCard */ true,
@@ -344,7 +344,8 @@ class MediaCarouselController @Inject constructor(
     // Returns true if new player is added
     private fun addOrUpdatePlayer(key: String, oldKey: String?, data: MediaData): Boolean {
         val dataCopy = data.copy(backgroundColor = bgColor)
-        val existingPlayer = MediaPlayerData.getMediaPlayer(key, oldKey)
+        MediaPlayerData.moveIfExists(oldKey, key)
+        val existingPlayer = MediaPlayerData.getMediaPlayer(key)
         val curVisibleMediaKey = MediaPlayerData.playerKeys()
             .elementAtOrNull(mediaCarouselScrollHandler.visibleMediaIndex)
         if (existingPlayer == null) {
@@ -386,7 +387,7 @@ class MediaCarouselController @Inject constructor(
         shouldPrioritize: Boolean
     ) {
         if (DEBUG) Log.d(TAG, "Updating smartspace target in carousel")
-        if (MediaPlayerData.getMediaPlayer(key, null) != null) {
+        if (MediaPlayerData.getMediaPlayer(key) != null) {
             Log.w(TAG, "Skip adding smartspace target in carousel")
             return
         }
@@ -795,13 +796,18 @@ internal object MediaPlayerData {
         smartspaceMediaData = data
     }
 
-    fun getMediaPlayer(key: String, oldKey: String?): MediaControlPanel? {
-        // If the key was changed, update entry
-        oldKey?.let {
-            if (it != key) {
-                mediaData.remove(it)?.let { sortKey -> mediaData.put(key, sortKey) }
-            }
+    fun moveIfExists(oldKey: String?, newKey: String) {
+        if (oldKey == null || oldKey == newKey) {
+            return
         }
+
+        mediaData.remove(oldKey)?.let {
+            removeMediaPlayer(newKey)
+            mediaData.put(newKey, it)
+        }
+    }
+
+    fun getMediaPlayer(key: String): MediaControlPanel? {
         return mediaData.get(key)?.let { mediaPlayers.get(it) }
     }
 

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

@@ -22,14 +22,24 @@ import com.android.systemui.SysuiTestCase
 import com.google.common.truth.Truth.assertThat
 import org.junit.Before
 import org.junit.Ignore
+import org.junit.Rule
 import org.junit.Test
 import org.junit.runner.RunWith
+import org.mockito.Mock
 import org.mockito.Mockito.mock
+import org.mockito.junit.MockitoJUnit
 
 @SmallTest
 @RunWith(AndroidTestingRunner::class)
 public class MediaPlayerDataTest : SysuiTestCase() {
 
+    @Mock
+    private lateinit var playerIsPlaying: MediaControlPanel
+
+    @JvmField
+    @Rule
+    val mockito = MockitoJUnit.rule()
+
     companion object {
         val LOCAL = true
         val RESUMPTION = true
@@ -44,7 +54,6 @@ public class MediaPlayerDataTest : SysuiTestCase() {
 
     @Test
     fun addPlayingThenRemote() {
-        val playerIsPlaying = mock(MediaControlPanel::class.java)
         val dataIsPlaying = createMediaData("app1", PLAYING, LOCAL, !RESUMPTION)
 
         val playerIsRemote = mock(MediaControlPanel::class.java)
@@ -83,7 +92,6 @@ public class MediaPlayerDataTest : SysuiTestCase() {
 
     @Test
     fun fullOrderTest() {
-        val playerIsPlaying = mock(MediaControlPanel::class.java)
         val dataIsPlaying = createMediaData("app1", PLAYING, LOCAL, !RESUMPTION)
 
         val playerIsPlayingAndRemote = mock(MediaControlPanel::class.java)
@@ -115,6 +123,26 @@ public class MediaPlayerDataTest : SysuiTestCase() {
             playerUndetermined).inOrder()
     }
 
+    @Test
+    fun testMoveMediaKeysAround() {
+        val keyA = "a"
+        val keyB = "b"
+
+        val data = createMediaData("app1", PLAYING, LOCAL, !RESUMPTION)
+
+        MediaPlayerData.addMediaPlayer(keyA, data, playerIsPlaying)
+        MediaPlayerData.addMediaPlayer(keyB, data, playerIsPlaying)
+
+        assertThat(MediaPlayerData.players()).hasSize(2)
+
+        MediaPlayerData.moveIfExists(keyA, keyB)
+
+        assertThat(MediaPlayerData.players()).hasSize(1)
+
+        assertThat(MediaPlayerData.getMediaPlayer(keyA)).isNull()
+        assertThat(MediaPlayerData.getMediaPlayer(keyB)).isNotNull()
+    }
+
     private fun createMediaData(
         app: String,
         isPlaying: Boolean?,