From 96e4106410a92bcb5af55ff12508c0ab6f19db7a Mon Sep 17 00:00:00 2001 From: Marco Romano Date: Tue, 14 Nov 2023 16:23:51 +0100 Subject: [PATCH] Allow to seek a voice message before playing it (#1780) --- .../timeline/VoiceMessagePlayer.kt | 48 ++++++----- .../timeline/VoiceMessagePresenter.kt | 13 ++- .../timeline/DefaultVoiceMessagePlayerTest.kt | 33 ++++++++ .../timeline/VoiceMessagePresenterTest.kt | 82 ++++++++++++++++++- 4 files changed, 152 insertions(+), 24 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePlayer.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePlayer.kt index f96342a308..57607507c9 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePlayer.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePlayer.kt @@ -17,10 +17,10 @@ package io.element.android.features.messages.impl.voicemessages.timeline import com.squareup.anvil.annotations.ContributesBinding -import io.element.android.libraries.mediaplayer.api.MediaPlayer import io.element.android.libraries.di.RoomScope import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.media.MediaSource +import io.element.android.libraries.mediaplayer.api.MediaPlayer import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.map @@ -75,11 +75,14 @@ interface VoiceMessagePlayer { fun pause() /** - * Seek to a specific position. + * Seek to a specific position acquiring control of the + * underlying [MediaPlayer] if needed. + * + * Will suspend whilst the media file is being downloaded. * * @param positionMs The position in milliseconds. */ - fun seekTo(positionMs: Long) + suspend fun seekTo(positionMs: Long): Result data class State( /** @@ -145,22 +148,8 @@ class DefaultVoiceMessagePlayer( ) }.distinctUntilChanged() - override suspend fun play(): Result = if (inControl()) { + override suspend fun play(): Result = acquireControl { mediaPlayer.play() - Result.success(Unit) - } else { - if (eventId != null) { - repo.getMediaFile().mapCatching { mediaFile -> - mediaPlayer.setMedia( - uri = mediaFile.path, - mediaId = eventId.value, - mimeType = "audio/ogg", // Files in the voice cache have no extension so we need to set the mime type manually. - ) - mediaPlayer.play() - } - } else { - Result.failure(IllegalStateException("Cannot play a voice message with no eventId")) - } } override fun pause() { @@ -169,10 +158,8 @@ class DefaultVoiceMessagePlayer( } } - override fun seekTo(positionMs: Long) { - ifInControl { - mediaPlayer.seekTo(positionMs) - } + override suspend fun seekTo(positionMs: Long): Result = acquireControl { + mediaPlayer.seekTo(positionMs) } private fun String?.isMyTrack(): Boolean = if (eventId == null) false else this == eventId.value @@ -182,4 +169,21 @@ class DefaultVoiceMessagePlayer( } private fun inControl(): Boolean = mediaPlayer.state.value.mediaId.isMyTrack() + + private suspend inline fun acquireControl(onReady: (state: MediaPlayer.State) -> Unit): Result = if (inControl()) { + onReady(mediaPlayer.state.value) + Result.success(Unit) + } else { + if (eventId != null) { + repo.getMediaFile().mapCatching { mediaFile -> + mediaPlayer.setMedia( + uri = mediaFile.path, + mediaId = eventId.value, + mimeType = "audio/ogg" // Files in the voice cache have no extension so we need to set the mime type manually. + ).let(onReady) + } + } else { + Result.failure(IllegalStateException("Cannot acquireControl on a voice message with no eventId")) + } + } } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePresenter.kt index 5df804c8ac..d0f7f3b2c1 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/VoiceMessagePresenter.kt @@ -126,7 +126,18 @@ class VoiceMessagePresenter @AssistedInject constructor( } } is VoiceMessageEvents.Seek -> { - player.seekTo((event.percentage * content.duration.toMillis()).toLong()) + scope.launch { + play.runUpdatingState( + errorTransform = { + analyticsService.trackError( + VoiceMessageException.PlayMessageError("Error while trying to seek voice message", it) + ) + it + }, + ) { + player.seekTo((event.percentage * content.duration.toMillis()).toLong()) + } + } } } } diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/DefaultVoiceMessagePlayerTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/DefaultVoiceMessagePlayerTest.kt index 78c34cd60b..62f117fcc9 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/DefaultVoiceMessagePlayerTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/DefaultVoiceMessagePlayerTest.kt @@ -118,6 +118,39 @@ class DefaultVoiceMessagePlayerTest { } } + @Test + fun `download and seek to works`() = runTest { + val player = createDefaultVoiceMessagePlayer() + player.state.test { + skipItems(1) // skip initial state. + Truth.assertThat(player.seekTo(2000).isSuccess).isTrue() + player.seekTo(2000) + awaitItem().let { + Truth.assertThat(it.isPlaying).isEqualTo(false) + Truth.assertThat(it.isMyMedia).isEqualTo(true) + Truth.assertThat(it.currentPosition).isEqualTo(0) + } + awaitItem().let { + Truth.assertThat(it.isPlaying).isEqualTo(false) + Truth.assertThat(it.isMyMedia).isEqualTo(true) + Truth.assertThat(it.currentPosition).isEqualTo(2000) + } + } + } + + @Test + fun `download and seek to fails`() = runTest { + val player = createDefaultVoiceMessagePlayer( + voiceMessageMediaRepo = FakeVoiceMessageMediaRepo().apply { + shouldFail = true + }, + ) + player.state.test { + skipItems(1) // skip initial state. + Truth.assertThat(player.seekTo(2000).isFailure).isTrue() + } + } + @Test fun `seek to works`() = runTest { val player = createDefaultVoiceMessagePlayer() diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/VoiceMessagePresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/VoiceMessagePresenterTest.kt index 26f4232531..af8dbfc1d4 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/VoiceMessagePresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/voicemessages/timeline/VoiceMessagePresenterTest.kt @@ -114,7 +114,10 @@ class VoiceMessagePresenterTest { Truth.assertThat(it.time).isEqualTo("0:02") } analyticsService.trackedErrors.first().also { - Truth.assertThat(it).isInstanceOf(VoiceMessageException.PlayMessageError::class.java) + Truth.assertThat(it).apply { + isInstanceOf(VoiceMessageException.PlayMessageError::class.java) + hasMessageThat().isEqualTo("Error while trying to play voice message") + } } } } @@ -167,6 +170,83 @@ class VoiceMessagePresenterTest { } } + @Test + fun `seeking downloads and seeks`() = runTest { + val presenter = createVoiceMessagePresenter( + content = aTimelineItemVoiceContent(durationMs = 10_000), + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem().also { + Truth.assertThat(it.button).isEqualTo(VoiceMessageState.Button.Play) + Truth.assertThat(it.progress).isEqualTo(0f) + Truth.assertThat(it.time).isEqualTo("0:10") + } + + initialState.eventSink(VoiceMessageEvents.Seek(0.5f)) + + awaitItem().also { + Truth.assertThat(it.button).isEqualTo(VoiceMessageState.Button.Downloading) + Truth.assertThat(it.progress).isEqualTo(0f) + Truth.assertThat(it.time).isEqualTo("0:10") + } + awaitItem().also { + Truth.assertThat(it.button).isEqualTo(VoiceMessageState.Button.Downloading) + Truth.assertThat(it.progress).isEqualTo(0f) + Truth.assertThat(it.time).isEqualTo("0:00") + } + awaitItem().also { + Truth.assertThat(it.button).isEqualTo(VoiceMessageState.Button.Downloading) + Truth.assertThat(it.progress).isEqualTo(0.5f) + Truth.assertThat(it.time).isEqualTo("0:05") + } + awaitItem().also { + Truth.assertThat(it.button).isEqualTo(VoiceMessageState.Button.Play) + Truth.assertThat(it.progress).isEqualTo(0.5f) + Truth.assertThat(it.time).isEqualTo("0:05") + } + } + } + + @Test + fun `seeking downloads and fails`() = runTest { + val analyticsService = FakeAnalyticsService() + val presenter = createVoiceMessagePresenter( + voiceMessageMediaRepo = FakeVoiceMessageMediaRepo().apply { shouldFail = true }, + analyticsService = analyticsService, + content = aTimelineItemVoiceContent(durationMs = 10_000), + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem().also { + Truth.assertThat(it.button).isEqualTo(VoiceMessageState.Button.Play) + Truth.assertThat(it.progress).isEqualTo(0f) + Truth.assertThat(it.time).isEqualTo("0:10") + } + + initialState.eventSink(VoiceMessageEvents.Seek(0.5f)) + + awaitItem().also { + Truth.assertThat(it.button).isEqualTo(VoiceMessageState.Button.Downloading) + Truth.assertThat(it.progress).isEqualTo(0f) + Truth.assertThat(it.time).isEqualTo("0:10") + } + awaitItem().also { + Truth.assertThat(it.button).isEqualTo(VoiceMessageState.Button.Retry) + Truth.assertThat(it.progress).isEqualTo(0f) + Truth.assertThat(it.time).isEqualTo("0:10") + } + analyticsService.trackedErrors.first().also { + Truth.assertThat(it).apply { + isInstanceOf(VoiceMessageException.PlayMessageError::class.java) + hasMessageThat().isEqualTo("Error while trying to seek voice message") + } + } + } + } + @Test fun `seeking seeks`() = runTest { val presenter = createVoiceMessagePresenter(