[ui-compose] Eliminated race condition inside button state

A bug was introduced because it was unclear that button state creation and listener registration were not an atomic operation. This happens because the `LaunchedEffect` which starts the listen-to-Player-Events coroutine can be pre-empted with other side effects, including the ones that change something in the Player.

`remember*State` functions create some button state object and initialise it with the correct fresh values pulled out of the Player. The subscription to Player events with a registration of a Listener(via `*ButtonState.observe()`) is not immediate. It *returns* immediately, but is instead scheduled to happen *later*, although within the same Handler message. Other LaunchedEffects could have been scheduled earlier and could take place between the button state creation and listener subscription.

This is not a problem if no changes to the player happen, but if we miss the *relevant* player events, we might end up with a UI that is out of sync with reality (e.g. button enabled/disabled when it should be flipped, icons toggled the wrong way). The way to fix this is to pull the latest values out of the Player on demand upon starting to listen to Player events.

PiperOrigin-RevId: 750183793

RELEASENOTES.md

@@ -54,6 +54,11 @@
         connected to a legacy `MediaBrowserServiceCompat` produced a
         `NullPointerException`.
 *   UI:
+    *   Fix a Compose bug which resulted in a gap between setting the initial
+        button states and observing the change in state (e.g. icon shapes or
+        being enabled). Any changes made to the Player outside of the
+        observation period are now picked up
+        ([#2313](https://github.com/androidx/media/issues/2313)).
 *   Downloads:
     *   Add partial download support for progressive streams. Apps can prepare a
         progressive stream with `DownloadHelper`, and request a

libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/NextButtonState.kt

@@ -49,7 +49,7 @@ fun rememberNextButtonState(player: Player): NextButtonState {
  */
 @UnstableApi
 class NextButtonState(private val player: Player) {
-  var isEnabled by mutableStateOf(player.isCommandAvailable(Player.COMMAND_SEEK_TO_NEXT))
+  var isEnabled by mutableStateOf(isNextEnabled(player))
     private set
 
   fun onClick() {
@@ -61,10 +61,14 @@ class NextButtonState(private val player: Player) {
    * [Player.EVENT_AVAILABLE_COMMANDS_CHANGED] in order to determine whether the button should be
    * enabled, i.e. respond to user input.
    */
-  suspend fun observe(): Nothing =
+  suspend fun observe(): Nothing {
+    isEnabled = isNextEnabled(player)
     player.listen { events ->
       if (events.contains(Player.EVENT_AVAILABLE_COMMANDS_CHANGED)) {
-        isEnabled = isCommandAvailable(Player.COMMAND_SEEK_TO_NEXT)
+        isEnabled = isNextEnabled(this)
       }
     }
+  }
+
+  private fun isNextEnabled(player: Player) = player.isCommandAvailable(Player.COMMAND_SEEK_TO_NEXT)
 }

libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PlayPauseButtonState.kt

@@ -81,7 +81,9 @@ class PlayPauseButtonState(private val player: Player) {
    * * [Player.EVENT_AVAILABLE_COMMANDS_CHANGED] in order to determine whether the button should be
    *   enabled, i.e. respond to user input.
    */
-  suspend fun observe(): Nothing =
+  suspend fun observe(): Nothing {
+    showPlay = shouldShowPlayButton(player)
+    isEnabled = shouldEnablePlayPauseButton(player)
     player.listen { events ->
       if (
         events.containsAny(
@@ -94,4 +96,5 @@ class PlayPauseButtonState(private val player: Player) {
         isEnabled = shouldEnablePlayPauseButton(this)
       }
     }
+  }
 }

libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PlaybackSpeedState.kt

@@ -53,7 +53,7 @@ fun rememberPlaybackSpeedState(player: Player): PlaybackSpeedState {
  */
 @UnstableApi
 class PlaybackSpeedState(private val player: Player) {
-  var isEnabled by mutableStateOf(player.isCommandAvailable(Player.COMMAND_SET_SPEED_AND_PITCH))
+  var isEnabled by mutableStateOf(arePlaybackParametersEnabled(player))
     private set
 
   var playbackSpeed by mutableFloatStateOf(player.playbackParameters.speed)
@@ -70,7 +70,9 @@ class PlaybackSpeedState(private val player: Player) {
    * * [Player.EVENT_AVAILABLE_COMMANDS_CHANGED] in order to determine whether the UI element
    *   responsible for setting the playback speed should be enabled, i.e. respond to user input.
    */
-  suspend fun observe(): Nothing =
+  suspend fun observe(): Nothing {
+    playbackSpeed = player.playbackParameters.speed
+    isEnabled = arePlaybackParametersEnabled(player)
     player.listen { events ->
       if (
         events.containsAny(
@@ -79,7 +81,11 @@ class PlaybackSpeedState(private val player: Player) {
         )
       ) {
         playbackSpeed = playbackParameters.speed
-        isEnabled = isCommandAvailable(Player.COMMAND_SET_SPEED_AND_PITCH)
+        isEnabled = arePlaybackParametersEnabled(this)
       }
     }
+  }
+
+  private fun arePlaybackParametersEnabled(player: Player) =
+    player.isCommandAvailable(Player.COMMAND_SET_SPEED_AND_PITCH)
 }

libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PreviousButtonState.kt

@@ -49,7 +49,7 @@ fun rememberPreviousButtonState(player: Player): PreviousButtonState {
  */
 @UnstableApi
 class PreviousButtonState(private val player: Player) {
-  var isEnabled by mutableStateOf(player.isCommandAvailable(Player.COMMAND_SEEK_TO_PREVIOUS))
+  var isEnabled by mutableStateOf(isPreviousEnabled(player))
     private set
 
   fun onClick() {
@@ -61,10 +61,15 @@ class PreviousButtonState(private val player: Player) {
    * [Player.EVENT_AVAILABLE_COMMANDS_CHANGED] in order to determine whether the button should be
    * enabled, i.e. respond to user input.
    */
-  suspend fun observe(): Nothing =
+  suspend fun observe(): Nothing {
+    isEnabled = isPreviousEnabled(player)
     player.listen { events ->
       if (events.contains(Player.EVENT_AVAILABLE_COMMANDS_CHANGED)) {
-        isEnabled = isCommandAvailable(Player.COMMAND_SEEK_TO_PREVIOUS)
+        isEnabled = isPreviousEnabled(this)
       }
     }
+  }
+
+  private fun isPreviousEnabled(player: Player) =
+    player.isCommandAvailable(Player.COMMAND_SEEK_TO_PREVIOUS)
 }

libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/RepeatButtonState.kt

@@ -63,10 +63,7 @@ class RepeatButtonState(
   private val toggleModeSequence: List<@Player.RepeatMode Int> =
     listOf(Player.REPEAT_MODE_OFF, Player.REPEAT_MODE_ONE, Player.REPEAT_MODE_ALL),
 ) {
-  var isEnabled by
-    mutableStateOf(
-      player.isCommandAvailable(Player.COMMAND_SET_REPEAT_MODE) && toggleModeSequence.isNotEmpty()
-    )
+  var isEnabled by mutableStateOf(isRepeatModeEnabled(player))
     private set
 
   var repeatModeState by mutableIntStateOf(player.repeatMode)
@@ -86,7 +83,9 @@ class RepeatButtonState(
    * * [Player.EVENT_AVAILABLE_COMMANDS_CHANGED] in order to determine whether the button should be
    *   enabled, i.e. respond to user input.
    */
-  suspend fun observe(): Nothing =
+  suspend fun observe(): Nothing {
+    repeatModeState = player.repeatMode
+    isEnabled = isRepeatModeEnabled(player)
     player.listen { events ->
       if (
         events.containsAny(
@@ -95,7 +94,8 @@ class RepeatButtonState(
         )
       ) {
         repeatModeState = repeatMode
-        isEnabled = isCommandAvailable(Player.COMMAND_SET_REPEAT_MODE)
+        isEnabled = isRepeatModeEnabled(this)
+      }
     }
   }
 
@@ -104,4 +104,7 @@ class RepeatButtonState(
     // -1 (i.e. not found) and the last element both loop back to 0
     return toggleModeSequence[(currRepeatModeIndex + 1) % toggleModeSequence.size]
   }
+
+  private fun isRepeatModeEnabled(player: Player) =
+    player.isCommandAvailable(Player.COMMAND_SET_REPEAT_MODE) && toggleModeSequence.isNotEmpty()
 }

libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/ShuffleButtonState.kt

@@ -48,7 +48,7 @@ fun rememberShuffleButtonState(player: Player): ShuffleButtonState {
  */
 @UnstableApi
 class ShuffleButtonState(private val player: Player) {
-  var isEnabled by mutableStateOf(player.isCommandAvailable(Player.COMMAND_SET_SHUFFLE_MODE))
+  var isEnabled by mutableStateOf(isShuffleEnabled(player))
     private set
 
   var shuffleOn by mutableStateOf(player.shuffleModeEnabled)
@@ -65,7 +65,9 @@ class ShuffleButtonState(private val player: Player) {
    * * [Player.EVENT_AVAILABLE_COMMANDS_CHANGED] in order to determine whether the button should be
    *   enabled, i.e. respond to user input.
    */
-  suspend fun observe(): Nothing =
+  suspend fun observe(): Nothing {
+    shuffleOn = player.shuffleModeEnabled
+    isEnabled = isShuffleEnabled(player)
     player.listen { events ->
       if (
         events.containsAny(
@@ -74,7 +76,11 @@ class ShuffleButtonState(private val player: Player) {
         )
       ) {
         shuffleOn = shuffleModeEnabled
-        isEnabled = isCommandAvailable(Player.COMMAND_SET_SHUFFLE_MODE)
+        isEnabled = isShuffleEnabled(player)
       }
     }
+  }
+
+  private fun isShuffleEnabled(player: Player) =
+    player.isCommandAvailable(Player.COMMAND_SET_SHUFFLE_MODE)
 }

libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/NextButtonStateTest.kt

@@ -16,6 +16,7 @@
 
 package androidx.media3.ui.compose.state
 
+import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.ui.test.junit4.createComposeRule
 import androidx.media3.common.Player
 import androidx.media3.ui.compose.utils.TestPlayer
@@ -96,4 +97,21 @@ class NextButtonStateTest {
 
     assertThat(player.currentMediaItemIndex).isEqualTo(1)
   }
+
+  @Test
+  fun playerReachesLastItemWithDisabledNextButtonBeforeEventListenerRegisters_observeGetsTheLatestValues_uiIconInSync() {
+    val player = TestPlayer()
+
+    lateinit var state: NextButtonState
+    composeTestRule.setContent {
+      // Schedule LaunchedEffect to update player state before NextButtonState is created.
+      // This update could end up being executed *before* NextButtonState schedules the start of
+      // event listening and we don't want to lose it.
+      LaunchedEffect(player) { player.seekToNext() }
+      state = rememberNextButtonState(player = player)
+    }
+
+    // UI syncs up with the fact that we reached the last media item and NextButton is now disabled
+    assertThat(state.isEnabled).isFalse()
+  }
 }

libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PlayPauseButtonStateTest.kt

@@ -16,6 +16,7 @@
 
 package androidx.media3.ui.compose.state
 
+import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.ui.test.junit4.createComposeRule
 import androidx.media3.common.Player
 import androidx.media3.ui.compose.utils.TestPlayer
@@ -128,4 +129,24 @@ class PlayPauseButtonStateTest {
     assertThat(player.playWhenReady).isTrue()
     assertThat(player.playbackState).isEqualTo(Player.STATE_BUFFERING)
   }
+
+  @Test
+  fun playerIsScheduledToPlayBeforeEventListenerRegisters_observeGetsTheLatestValues_uiIconInSync() {
+    val player = TestPlayer()
+    player.playbackState = Player.STATE_BUFFERING
+    player.pause()
+
+    lateinit var state: PlayPauseButtonState
+    composeTestRule.setContent {
+      // Schedule LaunchedEffect to update player state before PlayPauseButtonState is created.
+      // This update could end up being executed *before* PlayPauseButtonState schedules the start
+      // of event listening and we don't want to lose it.
+      LaunchedEffect(player) { player.play() }
+      state = rememberPlayPauseButtonState(player = player)
+    }
+
+    // UI catches up with the fact that player.play() happened because observe() started by getting
+    // the most recent values
+    assertThat(state.showPlay).isFalse()
+  }
 }

libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PlaybackSpeedStateTest.kt

@@ -16,6 +16,7 @@
 
 package androidx.media3.ui.compose.state
 
+import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.ui.test.junit4.createComposeRule
 import androidx.media3.common.Player
 import androidx.media3.ui.compose.utils.TestPlayer
@@ -91,4 +92,21 @@ class PlaybackSpeedStateTest {
 
     assertThat(player.playbackParameters.speed).isEqualTo(2.7f)
   }
+
+  @Test
+  fun playerIncreasesPlaybackSpeedBeforeEventListenerRegisters_observeGetsTheLatestValues_uiIconInSync() {
+    val player = TestPlayer()
+
+    lateinit var state: PlaybackSpeedState
+    composeTestRule.setContent {
+      // Schedule LaunchedEffect to update player state before PlaybackSpeedState is created.
+      // This update could end up being executed *before* PlaybackSpeedState schedules the start of
+      // event listening and we don't want to lose it.
+      LaunchedEffect(player) { player.setPlaybackSpeed(player.playbackParameters.speed + 1f) }
+      state = rememberPlaybackSpeedState(player = player)
+    }
+
+    // UI syncs up with the fact that we increased playback speed
+    assertThat(state.playbackSpeed).isEqualTo(2f)
+  }
 }

libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PreviousButtonStateTest.kt

@@ -16,6 +16,7 @@
 
 package androidx.media3.ui.compose.state
 
+import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.ui.test.junit4.createComposeRule
 import androidx.media3.common.Player
 import androidx.media3.ui.compose.utils.TestPlayer
@@ -79,4 +80,21 @@ class PreviousButtonStateTest {
 
     assertThat(player.currentMediaItemIndex).isEqualTo(0)
   }
+
+  @Test
+  fun playerChangesAvailableCommandsBeforeEventListenerRegisters_observeGetsTheLatestValues_uiIconInSync() {
+    val player = TestPlayer()
+
+    lateinit var state: PreviousButtonState
+    composeTestRule.setContent {
+      // Schedule LaunchedEffect to update player state before PreviousButtonState is created.
+      // This update could end up being executed *before* PreviousButtonState schedules the start of
+      // event listening and we don't want to lose it.
+      LaunchedEffect(player) { player.removeCommands(Player.COMMAND_SEEK_TO_PREVIOUS) }
+      state = rememberPreviousButtonState(player = player)
+    }
+
+    // UI syncs up with the fact that PreviousButton is now disabled
+    assertThat(state.isEnabled).isFalse()
+  }
 }

libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/RepeatButtonStateTest.kt

@@ -16,6 +16,7 @@
 
 package androidx.media3.ui.compose.state
 
+import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.ui.test.junit4.createComposeRule
 import androidx.media3.common.Player
 import androidx.media3.ui.compose.utils.TestPlayer
@@ -103,4 +104,21 @@ class RepeatButtonStateTest {
     assertThat(state.repeatModeState)
       .isEqualTo(Player.REPEAT_MODE_ALL) // UI state synchronises with Player, icon jumps 2 steps
   }
+
+  @Test
+  fun playerChangesRepeatModeCommandsBeforeEventListenerRegisters_observeGetsTheLatestValues_uiIconInSync() {
+    val player = TestPlayer()
+
+    lateinit var state: RepeatButtonState
+    composeTestRule.setContent {
+      // Schedule LaunchedEffect to update player state before RepeatButtonState is created.
+      // This update could end up being executed *before* RepeatButtonState schedules the start of
+      // event listening and we don't want to lose it.
+      LaunchedEffect(player) { player.repeatMode = (player.repeatMode + 1) % 3 }
+      state = rememberRepeatButtonState(player)
+    }
+
+    // UI syncs up with the fact that repeat mode moved from REPEAT_MODE_OFF to ONE
+    assertThat(state.repeatModeState).isEqualTo(Player.REPEAT_MODE_ONE)
+  }
 }

libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/ShuffleButtonStateTest.kt

@@ -16,6 +16,7 @@
 
 package androidx.media3.ui.compose.state
 
+import androidx.compose.runtime.LaunchedEffect
 import androidx.compose.ui.test.junit4.createComposeRule
 import androidx.media3.ui.compose.utils.TestPlayer
 import androidx.test.ext.junit.runners.AndroidJUnit4
@@ -93,4 +94,21 @@ class ShuffleButtonStateTest {
     assertThat(player.shuffleModeEnabled).isFalse()
     assertThat(state.shuffleOn).isFalse() // UI state synchronises with Player
   }
+
+  @Test
+  fun playerChangesShuffleModeCommandsBeforeEventListenerRegisters_observeGetsTheLatestValues_uiIconInSync() {
+    val player = TestPlayer()
+
+    lateinit var state: ShuffleButtonState
+    composeTestRule.setContent {
+      // Schedule LaunchedEffect to update player state before ShuffleButtonState is created.
+      // This update could end up being executed *before* ShuffleButtonState schedules the start of
+      // event listening and we don't want to lose it.
+      LaunchedEffect(player) { player.shuffleModeEnabled = !player.shuffleModeEnabled }
+      state = rememberShuffleButtonState(player)
+    }
+
+    // UI syncs up with the fact that shuffle mode got flipped to true
+    assertThat(state.shuffleOn).isTrue()
+  }
 }