From ab6b0f6e10faf5133d5ed0cb08c57eda1602cbd3 Mon Sep 17 00:00:00 2001 From: jbibik Date: Tue, 22 Apr 2025 07:29:27 -0700 Subject: [PATCH] [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 | 5 +++++ .../ui/compose/state/NextButtonState.kt | 10 ++++++--- .../ui/compose/state/PlayPauseButtonState.kt | 5 ++++- .../ui/compose/state/PlaybackSpeedState.kt | 12 ++++++++--- .../ui/compose/state/PreviousButtonState.kt | 11 +++++++--- .../ui/compose/state/RepeatButtonState.kt | 15 +++++++------ .../ui/compose/state/ShuffleButtonState.kt | 12 ++++++++--- .../ui/compose/state/NextButtonStateTest.kt | 18 ++++++++++++++++ .../compose/state/PlayPauseButtonStateTest.kt | 21 +++++++++++++++++++ .../compose/state/PlaybackSpeedStateTest.kt | 18 ++++++++++++++++ .../compose/state/PreviousButtonStateTest.kt | 18 ++++++++++++++++ .../ui/compose/state/RepeatButtonStateTest.kt | 18 ++++++++++++++++ .../compose/state/ShuffleButtonStateTest.kt | 18 ++++++++++++++++ 13 files changed, 162 insertions(+), 19 deletions(-) diff --git a/RELEASENOTES.md b/RELEASENOTES.md index f9bc9b1182..79f505b6de 100644 --- a/RELEASENOTES.md +++ b/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 diff --git a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/NextButtonState.kt b/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/NextButtonState.kt index 257dd34113..19764b4a70 100644 --- a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/NextButtonState.kt +++ b/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) } diff --git a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PlayPauseButtonState.kt b/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PlayPauseButtonState.kt index b1c1232380..78ca13a0d3 100644 --- a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PlayPauseButtonState.kt +++ b/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) } } + } } diff --git a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PlaybackSpeedState.kt b/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PlaybackSpeedState.kt index b6bdaaa40e..ff3811ec6b 100644 --- a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PlaybackSpeedState.kt +++ b/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) } diff --git a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PreviousButtonState.kt b/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PreviousButtonState.kt index f32411f6aa..cca04c7b75 100644 --- a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/PreviousButtonState.kt +++ b/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) } diff --git a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/RepeatButtonState.kt b/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/RepeatButtonState.kt index 1fb382d812..7e83e037fc 100644 --- a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/RepeatButtonState.kt +++ b/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,13 +94,17 @@ class RepeatButtonState( ) ) { repeatModeState = repeatMode - isEnabled = isCommandAvailable(Player.COMMAND_SET_REPEAT_MODE) + isEnabled = isRepeatModeEnabled(this) } } + } private fun getNextRepeatModeInSequence(): @Player.RepeatMode Int { val currRepeatModeIndex = toggleModeSequence.indexOf(player.repeatMode) // -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() } diff --git a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/ShuffleButtonState.kt b/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/ShuffleButtonState.kt index 305e83a9e5..ecb9e444e1 100644 --- a/libraries/ui_compose/src/main/java/androidx/media3/ui/compose/state/ShuffleButtonState.kt +++ b/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) } diff --git a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/NextButtonStateTest.kt b/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/NextButtonStateTest.kt index d313c8e0c8..466b3e5ebf 100644 --- a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/NextButtonStateTest.kt +++ b/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() + } } diff --git a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PlayPauseButtonStateTest.kt b/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PlayPauseButtonStateTest.kt index cdeb0de4d3..8085085460 100644 --- a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PlayPauseButtonStateTest.kt +++ b/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() + } } diff --git a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PlaybackSpeedStateTest.kt b/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PlaybackSpeedStateTest.kt index 1ff409adf2..4993083bd3 100644 --- a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PlaybackSpeedStateTest.kt +++ b/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) + } } diff --git a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PreviousButtonStateTest.kt b/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PreviousButtonStateTest.kt index b84bc042ef..43a88f8784 100644 --- a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/PreviousButtonStateTest.kt +++ b/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() + } } diff --git a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/RepeatButtonStateTest.kt b/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/RepeatButtonStateTest.kt index c5b90f2e6c..7b67531c37 100644 --- a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/RepeatButtonStateTest.kt +++ b/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) + } } diff --git a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/ShuffleButtonStateTest.kt b/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/ShuffleButtonStateTest.kt index 6ca2978b3e..3d3fcbd860 100644 --- a/libraries/ui_compose/src/test/java/androidx/media3/ui/compose/state/ShuffleButtonStateTest.kt +++ b/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() + } }