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() + } }