From c1507fb24e77092d589de0b260786ebad5a338ee Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 25 Nov 2024 14:37:37 +0100 Subject: [PATCH 1/9] Pre-process media during the attachment preview --- .../preview/AttachmentsPreviewPresenter.kt | 133 +++++++++++++++--- .../preview/AttachmentsPreviewState.kt | 1 + .../preview/AttachmentsPreviewView.kt | 15 +- .../AttachmentsPreviewPresenterTest.kt | 125 +++++++++++++++- .../libraries/mediaupload/api/MediaSender.kt | 29 ++++ .../mediaupload/api/MediaUploadInfo.kt | 9 ++ .../mediaupload/test/FakeMediaPreProcessor.kt | 6 +- 7 files changed, 290 insertions(+), 28 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt index bc3e121070..c447e2ce5b 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt @@ -8,6 +8,7 @@ package io.element.android.features.messages.impl.attachments.preview import androidx.compose.runtime.Composable +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.MutableState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf @@ -19,26 +20,39 @@ import dagger.assisted.AssistedFactory import dagger.assisted.AssistedInject import io.element.android.features.messages.impl.attachments.Attachment import io.element.android.libraries.androidutils.file.TemporaryUriDeleter +import io.element.android.libraries.androidutils.file.safeDelete +import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.architecture.Presenter +import io.element.android.libraries.featureflag.api.FeatureFlagService +import io.element.android.libraries.featureflag.api.FeatureFlags import io.element.android.libraries.matrix.api.core.ProgressCallback import io.element.android.libraries.matrix.api.permalink.PermalinkBuilder import io.element.android.libraries.mediaupload.api.MediaSender +import io.element.android.libraries.mediaupload.api.MediaUploadInfo +import io.element.android.libraries.mediaupload.api.allFiles import io.element.android.libraries.textcomposer.model.TextEditorState import io.element.android.libraries.textcomposer.model.rememberMarkdownTextEditorState import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.filter +import kotlinx.coroutines.flow.flatMapConcat import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import timber.log.Timber import kotlin.coroutines.coroutineContext +@OptIn(ExperimentalCoroutinesApi::class) class AttachmentsPreviewPresenter @AssistedInject constructor( @Assisted private val attachment: Attachment, @Assisted private val onDoneListener: OnDoneListener, private val mediaSender: MediaSender, private val permalinkBuilder: PermalinkBuilder, private val temporaryUriDeleter: TemporaryUriDeleter, + private val featureFlagsService: FeatureFlagService, ) : Presenter { @AssistedFactory interface Factory { @@ -63,19 +77,61 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( val ongoingSendAttachmentJob = remember { mutableStateOf(null) } + val userSentAttachment = remember { + MutableStateFlow(false) + } + + val mediaUploadInfoStateFlow = remember { MutableStateFlow>(AsyncData.Uninitialized) } + var prePropressingJob: Job? = null + LaunchedEffect(Unit) { + prePropressingJob = preProcessAttachment( + attachment, + mediaUploadInfoStateFlow, + ) + } + + LaunchedEffect(Unit) { + userSentAttachment.filter { it } + .flatMapConcat { + mediaUploadInfoStateFlow.filter { it.isReady() } + } + .distinctUntilChanged() + .collect { mediaUploadInfo -> + if (mediaUploadInfo is AsyncData.Success) { + val caption = markdownTextEditorState.getMessageMarkdown(permalinkBuilder) + .takeIf { it.isNotEmpty() } + ongoingSendAttachmentJob.value = coroutineScope.launch { + sendPreProcessedMedia( + mediaUploadInfo = mediaUploadInfo.data, + caption = caption, + sendActionState = sendActionState, + ) + } + } else if (mediaUploadInfo is AsyncData.Failure) { + sendActionState.value = SendActionState.Failure(mediaUploadInfo.error) + } + // else: cannot happen since we filtered with isReady() + } + } + fun handleEvents(attachmentsPreviewEvents: AttachmentsPreviewEvents) { when (attachmentsPreviewEvents) { - is AttachmentsPreviewEvents.SendAttachment -> { - val caption = markdownTextEditorState.getMessageMarkdown(permalinkBuilder) - .takeIf { it.isNotEmpty() } - ongoingSendAttachmentJob.value = coroutineScope.sendAttachment( - attachment = attachment, - caption = caption, - sendActionState = sendActionState, - ) + is AttachmentsPreviewEvents.SendAttachment -> coroutineScope.launch { + val useSendQueue = featureFlagsService.isFeatureEnabled(FeatureFlags.MediaUploadOnSendQueue) + userSentAttachment.value = true + val instantSending = mediaUploadInfoStateFlow.value.isReady() && useSendQueue + sendActionState.value = if (instantSending) { + SendActionState.Sending.InstantSending + } else { + SendActionState.Sending.Processing + } } AttachmentsPreviewEvents.Cancel -> { - coroutineScope.cancel(attachment) + coroutineScope.cancel( + attachment, + prePropressingJob, + mediaUploadInfoStateFlow.value, + ) } AttachmentsPreviewEvents.ClearSendState -> { ongoingSendAttachmentJob.value?.let { @@ -95,56 +151,91 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( ) } - private fun CoroutineScope.sendAttachment( + private fun CoroutineScope.preProcessAttachment( attachment: Attachment, - caption: String?, - sendActionState: MutableState, + mediaUploadInfoState: MutableStateFlow>, ) = launch { when (attachment) { is Attachment.Media -> { - sendMedia( + preProcessMedia( mediaAttachment = attachment, - caption = caption, - sendActionState = sendActionState, + mediaUploadInfoState = mediaUploadInfoState, ) } } } + private suspend fun preProcessMedia( + mediaAttachment: Attachment.Media, + mediaUploadInfoState: MutableStateFlow>, + ) { + mediaUploadInfoState.emit(AsyncData.Loading()) + mediaSender.preProcessMedia( + uri = mediaAttachment.localMedia.uri, + mimeType = mediaAttachment.localMedia.info.mimeType, + ).fold( + onSuccess = { mediaUploadInfo -> + mediaUploadInfoState.emit(AsyncData.Success(mediaUploadInfo)) + }, + onFailure = { + Timber.e(it, "Failed to pre-process media") + if (it is CancellationException) { + throw it + } else { + mediaUploadInfoState.emit(AsyncData.Failure(it)) + } + } + ) + } + private fun CoroutineScope.cancel( attachment: Attachment, + preProcessingJob: Job?, + mediaUploadInfo: AsyncData, ) = launch { // Delete the temporary file when (attachment) { is Attachment.Media -> { temporaryUriDeleter.delete(attachment.localMedia.uri) + preProcessingJob?.cancel() + mediaUploadInfo.dataOrNull()?.let { data -> + cleanUp(data) + } } } onDoneListener() } - private suspend fun sendMedia( - mediaAttachment: Attachment.Media, + private fun cleanUp( + mediaUploadInfo: MediaUploadInfo, + ) { + mediaUploadInfo.allFiles().forEach { file -> + file.safeDelete() + } + } + + private suspend fun sendPreProcessedMedia( + mediaUploadInfo: MediaUploadInfo, caption: String?, sendActionState: MutableState, ) = runCatching { val context = coroutineContext val progressCallback = object : ProgressCallback { override fun onProgress(current: Long, total: Long) { + // Note will not happen if useSendQueue is true if (context.isActive) { sendActionState.value = SendActionState.Sending.Uploading(current.toFloat() / total.toFloat()) } } } - sendActionState.value = SendActionState.Sending.Processing - mediaSender.sendMedia( - uri = mediaAttachment.localMedia.uri, - mimeType = mediaAttachment.localMedia.info.mimeType, + mediaSender.sendPreProcessedMedia( + mediaUploadInfo = mediaUploadInfo, caption = caption, progressCallback = progressCallback ).getOrThrow() }.fold( onSuccess = { + cleanUp(mediaUploadInfo) onDoneListener() }, onFailure = { error -> diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt index 5fed2acfb2..5d5f058375 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt @@ -27,6 +27,7 @@ sealed interface SendActionState { @Immutable sealed interface Sending : SendActionState { + data object InstantSending : Sending data object Processing : Sending data class Uploading(val progress: Float) : Sending } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt index 1380add329..2145092282 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt @@ -99,12 +99,17 @@ private fun AttachmentSendStateView( onRetryClick: () -> Unit ) { when (sendActionState) { - is SendActionState.Sending -> { + is SendActionState.Sending.Processing -> { ProgressDialog( - type = when (sendActionState) { - is SendActionState.Sending.Uploading -> ProgressDialogType.Determinate(sendActionState.progress) - SendActionState.Sending.Processing -> ProgressDialogType.Indeterminate - }, + type = ProgressDialogType.Indeterminate, + text = stringResource(id = CommonStrings.common_sending), + showCancelButton = true, + onDismissRequest = onDismissClick, + ) + } + is SendActionState.Sending.Uploading -> { + ProgressDialog( + type = ProgressDialogType.Determinate(sendActionState.progress), text = stringResource(id = CommonStrings.common_sending), showCancelButton = true, onDismissRequest = onDismissClick, diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt index 4f35c592f1..b1c2fbfcd6 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt @@ -20,6 +20,8 @@ import io.element.android.features.messages.impl.attachments.preview.OnDoneListe import io.element.android.features.messages.impl.attachments.preview.SendActionState import io.element.android.features.messages.impl.fixtures.aMediaAttachment import io.element.android.libraries.androidutils.file.TemporaryUriDeleter +import io.element.android.libraries.featureflag.api.FeatureFlags +import io.element.android.libraries.featureflag.test.FakeFeatureFlagService import io.element.android.libraries.matrix.api.core.ProgressCallback import io.element.android.libraries.matrix.api.media.AudioInfo import io.element.android.libraries.matrix.api.media.FileInfo @@ -40,10 +42,12 @@ import io.element.android.libraries.preferences.test.InMemorySessionPreferencesS import io.element.android.tests.testutils.WarmUpRule import io.element.android.tests.testutils.fake.FakeTemporaryUriDeleter import io.element.android.tests.testutils.lambda.any +import io.element.android.tests.testutils.lambda.lambdaError import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value import io.element.android.tests.testutils.test import io.mockk.mockk +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest @@ -94,6 +98,121 @@ class AttachmentsPreviewPresenterTest { } } + @Test + fun `present - send media after pre-processing success scenario`() = runTest { + val sendFileResult = lambdaRecorder> { _, _, _, _, _ -> + Result.success(FakeMediaUploadHandler()) + } + val room = FakeMatrixRoom( + sendFileResult = sendFileResult, + ) + val onDoneListener = lambdaRecorder { } + val processLatch = CompletableDeferred() + val presenter = createAttachmentsPreviewPresenter( + room = room, + mediaPreProcessor = FakeMediaPreProcessor( + processLatch = processLatch, + ), + onDoneListener = { onDoneListener() }, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem() + assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) + // Pre-processing finishes + processLatch.complete(Unit) + advanceUntilIdle() + initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.InstantSending) + advanceUntilIdle() + sendFileResult.assertions().isCalledOnce() + onDoneListener.assertions().isCalledOnce() + } + } + + @Test + fun `present - send media before pre-processing success scenario`() = runTest { + val sendFileResult = lambdaRecorder> { _, _, _, _, _ -> + Result.success(FakeMediaUploadHandler()) + } + val room = FakeMatrixRoom( + sendFileResult = sendFileResult, + ) + val onDoneListener = lambdaRecorder { } + val processLatch = CompletableDeferred() + val presenter = createAttachmentsPreviewPresenter( + room = room, + mediaPreProcessor = FakeMediaPreProcessor( + processLatch = processLatch, + ), + onDoneListener = { onDoneListener() }, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem() + assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) + initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) + // Pre-processing finishes + processLatch.complete(Unit) + advanceUntilIdle() + sendFileResult.assertions().isCalledOnce() + onDoneListener.assertions().isCalledOnce() + } + } + + @Test + fun `present - send media with pre-processing failure after user sends media`() = runTest { + val room = FakeMatrixRoom() + val onDoneListener = lambdaRecorder { } + val processLatch = CompletableDeferred() + val presenter = createAttachmentsPreviewPresenter( + room = room, + mediaPreProcessor = FakeMediaPreProcessor().apply { + givenResult(Result.failure(Exception())) + }, + onDoneListener = { onDoneListener() }, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem() + assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) + initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) + // Pre-processing finishes + processLatch.complete(Unit) + assertThat(awaitItem().sendActionState).isInstanceOf(SendActionState.Failure::class.java) + } + } + + @Test + fun `present - send media with pre-processing failure before user sends media`() = runTest { + val room = FakeMatrixRoom() + val onDoneListener = lambdaRecorder { } + val processLatch = CompletableDeferred() + val presenter = createAttachmentsPreviewPresenter( + room = room, + mediaPreProcessor = FakeMediaPreProcessor().apply { + givenResult(Result.failure(Exception())) + }, + onDoneListener = { onDoneListener() }, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem() + assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) + // Pre-processing finishes + processLatch.complete(Unit) + advanceUntilIdle() + initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isInstanceOf(SendActionState.Failure::class.java) + } + } + @Test fun `present - cancel scenario`() = runTest { val onDoneListener = lambdaRecorder { } @@ -277,7 +396,8 @@ class AttachmentsPreviewPresenterTest { permalinkBuilder: PermalinkBuilder = FakePermalinkBuilder(), mediaPreProcessor: MediaPreProcessor = FakeMediaPreProcessor(), temporaryUriDeleter: TemporaryUriDeleter = FakeTemporaryUriDeleter(), - onDoneListener: OnDoneListener = OnDoneListener {}, + onDoneListener: OnDoneListener = OnDoneListener { lambdaError() }, + mediaUploadOnSendQueueEnabled: Boolean = true, ): AttachmentsPreviewPresenter { return AttachmentsPreviewPresenter( attachment = aMediaAttachment(localMedia), @@ -285,6 +405,9 @@ class AttachmentsPreviewPresenterTest { mediaSender = MediaSender(mediaPreProcessor, room, InMemorySessionPreferencesStore()), permalinkBuilder = permalinkBuilder, temporaryUriDeleter = temporaryUriDeleter, + featureFlagsService = FakeFeatureFlagService( + initialState = mapOf(FeatureFlags.MediaUploadOnSendQueue.key to mediaUploadOnSendQueueEnabled), + ) ) } } diff --git a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt index 77e6d021d5..567315f9db 100644 --- a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt +++ b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt @@ -27,6 +27,35 @@ class MediaSender @Inject constructor( private val ongoingUploadJobs = ConcurrentHashMap() val hasOngoingMediaUploads get() = ongoingUploadJobs.isNotEmpty() + suspend fun preProcessMedia( + uri: Uri, + mimeType: String, + ): Result { + val compressIfPossible = sessionPreferencesStore.doesCompressMedia().first() + return preProcessor + .process( + uri = uri, + mimeType = mimeType, + deleteOriginal = false, + compressIfPossible = compressIfPossible, + ) + } + + suspend fun sendPreProcessedMedia( + mediaUploadInfo: MediaUploadInfo, + caption: String? = null, + formattedCaption: String? = null, + progressCallback: ProgressCallback? = null + ): Result { + return room.sendMedia( + uploadInfo = mediaUploadInfo, + progressCallback = progressCallback, + caption = caption, + formattedCaption = formattedCaption + ) + .handleSendResult() + } + suspend fun sendMedia( uri: Uri, mimeType: String, diff --git a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaUploadInfo.kt b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaUploadInfo.kt index 279acd1719..f9786b9b4f 100644 --- a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaUploadInfo.kt +++ b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaUploadInfo.kt @@ -22,3 +22,12 @@ sealed interface MediaUploadInfo { data class VoiceMessage(override val file: File, val audioInfo: AudioInfo, val waveform: List) : MediaUploadInfo data class AnyFile(override val file: File, val fileInfo: FileInfo) : MediaUploadInfo } + +fun MediaUploadInfo.allFiles(): List { + return listOfNotNull( + file, + (this@allFiles as? MediaUploadInfo.Image)?.thumbnailFile, + (this@allFiles as? MediaUploadInfo.Video)?.thumbnailFile, + ) +} + diff --git a/libraries/mediaupload/test/src/main/kotlin/io/element/android/libraries/mediaupload/test/FakeMediaPreProcessor.kt b/libraries/mediaupload/test/src/main/kotlin/io/element/android/libraries/mediaupload/test/FakeMediaPreProcessor.kt index efb3b77ed8..3ec4efcfaa 100644 --- a/libraries/mediaupload/test/src/main/kotlin/io/element/android/libraries/mediaupload/test/FakeMediaPreProcessor.kt +++ b/libraries/mediaupload/test/src/main/kotlin/io/element/android/libraries/mediaupload/test/FakeMediaPreProcessor.kt @@ -16,10 +16,13 @@ import io.element.android.libraries.matrix.api.media.VideoInfo import io.element.android.libraries.mediaupload.api.MediaPreProcessor import io.element.android.libraries.mediaupload.api.MediaUploadInfo import io.element.android.tests.testutils.simulateLongTask +import kotlinx.coroutines.CompletableDeferred import java.io.File import kotlin.time.Duration.Companion.seconds -class FakeMediaPreProcessor : MediaPreProcessor { +class FakeMediaPreProcessor( + private val processLatch: CompletableDeferred? = null, +) : MediaPreProcessor { var processCallCount = 0 private set @@ -41,6 +44,7 @@ class FakeMediaPreProcessor : MediaPreProcessor { deleteOriginal: Boolean, compressIfPossible: Boolean ): Result = simulateLongTask { + processLatch?.await() processCallCount++ result } From 9a1f36409d6f05f7a4eeb41182394d30ce23dba8 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 25 Nov 2024 17:18:07 +0100 Subject: [PATCH 2/9] Close the progress dialog when the treatment is over (avoid UI glitch) --- .../preview/AttachmentsPreviewPresenter.kt | 6 ++++++ .../attachments/preview/AttachmentsPreviewState.kt | 1 + .../attachments/AttachmentsPreviewPresenterTest.kt | 13 +++++++------ 3 files changed, 14 insertions(+), 6 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt index c447e2ce5b..a4482514b7 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt @@ -131,6 +131,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( attachment, prePropressingJob, mediaUploadInfoStateFlow.value, + sendActionState, ) } AttachmentsPreviewEvents.ClearSendState -> { @@ -192,6 +193,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( attachment: Attachment, preProcessingJob: Job?, mediaUploadInfo: AsyncData, + sendActionState: MutableState, ) = launch { // Delete the temporary file when (attachment) { @@ -203,6 +205,8 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( } } } + // Reset the sendActionState to ensure that dialog is closed before the screen + sendActionState.value = SendActionState.Done onDoneListener() } @@ -236,6 +240,8 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( }.fold( onSuccess = { cleanUp(mediaUploadInfo) + // Reset the sendActionState to ensure that dialog is closed before the screen + sendActionState.value = SendActionState.Done onDoneListener() }, onFailure = { error -> diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt index 5d5f058375..0e9724c460 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt @@ -33,4 +33,5 @@ sealed interface SendActionState { } data class Failure(val error: Throwable) : SendActionState + data object Done : SendActionState } diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt index b1c2fbfcd6..204a5b5b76 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt @@ -92,7 +92,7 @@ class AttachmentsPreviewPresenterTest { assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Uploading(0f)) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Uploading(0.5f)) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Uploading(1f)) - advanceUntilIdle() + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendFileResult.assertions().isCalledOnce() onDoneListener.assertions().isCalledOnce() } @@ -125,7 +125,7 @@ class AttachmentsPreviewPresenterTest { advanceUntilIdle() initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.InstantSending) - advanceUntilIdle() + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendFileResult.assertions().isCalledOnce() onDoneListener.assertions().isCalledOnce() } @@ -157,7 +157,7 @@ class AttachmentsPreviewPresenterTest { assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) // Pre-processing finishes processLatch.complete(Unit) - advanceUntilIdle() + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendFileResult.assertions().isCalledOnce() onDoneListener.assertions().isCalledOnce() } @@ -227,6 +227,7 @@ class AttachmentsPreviewPresenterTest { val initialState = awaitItem() assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.eventSink(AttachmentsPreviewEvents.Cancel) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) deleteCallback.assertions().isCalledOnce() onDoneListener.assertions().isCalledOnce() } @@ -258,7 +259,7 @@ class AttachmentsPreviewPresenterTest { initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) - advanceUntilIdle() + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendImageResult.assertions().isCalledOnce().with( any(), any(), @@ -297,7 +298,7 @@ class AttachmentsPreviewPresenterTest { initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) - advanceUntilIdle() + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendVideoResult.assertions().isCalledOnce().with( any(), any(), @@ -334,7 +335,7 @@ class AttachmentsPreviewPresenterTest { initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) - advanceUntilIdle() + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendAudioResult.assertions().isCalledOnce().with( any(), any(), From b9caede2d3620e696a337c6c5bb342430890b0d4 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 26 Nov 2024 09:17:39 +0100 Subject: [PATCH 3/9] Remove blank line --- .../element/android/libraries/mediaupload/api/MediaUploadInfo.kt | 1 - 1 file changed, 1 deletion(-) diff --git a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaUploadInfo.kt b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaUploadInfo.kt index f9786b9b4f..dfc36a40f5 100644 --- a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaUploadInfo.kt +++ b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaUploadInfo.kt @@ -30,4 +30,3 @@ fun MediaUploadInfo.allFiles(): List { (this@allFiles as? MediaUploadInfo.Video)?.thumbnailFile, ) } - From 008a554ca68c0b30e7d2ad48e31d750c5267dae4 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 26 Nov 2024 09:18:11 +0100 Subject: [PATCH 4/9] Remove default parameter values. This improve code coverage since some default value was never used. --- .../impl/attachments/preview/AttachmentsPreviewPresenter.kt | 1 + .../android/libraries/mediaupload/api/MediaSender.kt | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt index a4482514b7..f87ea3fb24 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt @@ -235,6 +235,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( mediaSender.sendPreProcessedMedia( mediaUploadInfo = mediaUploadInfo, caption = caption, + formattedCaption = null, progressCallback = progressCallback ).getOrThrow() }.fold( diff --git a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt index 567315f9db..b9618d86dd 100644 --- a/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt +++ b/libraries/mediaupload/api/src/main/kotlin/io/element/android/libraries/mediaupload/api/MediaSender.kt @@ -43,9 +43,9 @@ class MediaSender @Inject constructor( suspend fun sendPreProcessedMedia( mediaUploadInfo: MediaUploadInfo, - caption: String? = null, - formattedCaption: String? = null, - progressCallback: ProgressCallback? = null + caption: String?, + formattedCaption: String?, + progressCallback: ProgressCallback?, ): Result { return room.sendMedia( uploadInfo = mediaUploadInfo, From 1087ad6a1652b6c24b2fbfe45dc9f44caf3ac69d Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 26 Nov 2024 09:19:26 +0100 Subject: [PATCH 5/9] Add preview for the Processing state. --- .../impl/attachments/preview/AttachmentsPreviewStateProvider.kt | 1 + 1 file changed, 1 insertion(+) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewStateProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewStateProvider.kt index 78f3ffc81a..1c55245e46 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewStateProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewStateProvider.kt @@ -26,6 +26,7 @@ open class AttachmentsPreviewStateProvider : PreviewParameterProvider Date: Tue, 26 Nov 2024 08:34:11 +0000 Subject: [PATCH 6/9] Update screenshots --- ...messages.impl.attachments.preview_AttachmentsView_4_en.png | 4 ++-- ...messages.impl.attachments.preview_AttachmentsView_5_en.png | 4 ++-- ...messages.impl.attachments.preview_AttachmentsView_6_en.png | 3 +++ 3 files changed, 7 insertions(+), 4 deletions(-) create mode 100644 tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_6_en.png diff --git a/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_4_en.png b/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_4_en.png index e799726c52..8510d977be 100644 --- a/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_4_en.png +++ b/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_4_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:361ff23fd2ff99aecfdffd10b45e9235d86183d4856cb2a3e99f85b9e04c2d59 -size 51376 +oid sha256:3a92cb782d97553f364fab3df3fe159557a26aad8b7e5c176b40616c2f05abdd +size 51410 diff --git a/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_5_en.png b/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_5_en.png index df965394aa..e799726c52 100644 --- a/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_5_en.png +++ b/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_5_en.png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:efbfed755b29293009f45fca33f58863b612772de9a1d55593c979dbb04ff6f2 -size 88981 +oid sha256:361ff23fd2ff99aecfdffd10b45e9235d86183d4856cb2a3e99f85b9e04c2d59 +size 51376 diff --git a/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_6_en.png b/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_6_en.png new file mode 100644 index 0000000000..df965394aa --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/features.messages.impl.attachments.preview_AttachmentsView_6_en.png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:efbfed755b29293009f45fca33f58863b612772de9a1d55593c979dbb04ff6f2 +size 88981 From e01ebb40ac1268b226506e9cc2499ab7100648d5 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 26 Nov 2024 12:06:24 +0100 Subject: [PATCH 7/9] Avoid using MutableStateFlow, just MutableState in presenter. --- .../preview/AttachmentsPreviewPresenter.kt | 63 ++++++++----------- .../AttachmentsPreviewPresenterTest.kt | 25 +++++++- 2 files changed, 48 insertions(+), 40 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt index f87ea3fb24..1ca48848a2 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt @@ -34,18 +34,12 @@ import io.element.android.libraries.textcomposer.model.TextEditorState import io.element.android.libraries.textcomposer.model.rememberMarkdownTextEditorState import kotlinx.coroutines.CancellationException import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.Job -import kotlinx.coroutines.flow.MutableStateFlow -import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.filter -import kotlinx.coroutines.flow.flatMapConcat import kotlinx.coroutines.isActive import kotlinx.coroutines.launch import timber.log.Timber import kotlin.coroutines.coroutineContext -@OptIn(ExperimentalCoroutinesApi::class) class AttachmentsPreviewPresenter @AssistedInject constructor( @Assisted private val attachment: Attachment, @Assisted private val onDoneListener: OnDoneListener, @@ -77,41 +71,34 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( val ongoingSendAttachmentJob = remember { mutableStateOf(null) } - val userSentAttachment = remember { - MutableStateFlow(false) - } + val userSentAttachment = remember { mutableStateOf(false) } - val mediaUploadInfoStateFlow = remember { MutableStateFlow>(AsyncData.Uninitialized) } + val mediaUploadInfoState = remember { mutableStateOf>(AsyncData.Uninitialized) } var prePropressingJob: Job? = null LaunchedEffect(Unit) { prePropressingJob = preProcessAttachment( attachment, - mediaUploadInfoStateFlow, + mediaUploadInfoState, ) } - LaunchedEffect(Unit) { - userSentAttachment.filter { it } - .flatMapConcat { - mediaUploadInfoStateFlow.filter { it.isReady() } - } - .distinctUntilChanged() - .collect { mediaUploadInfo -> - if (mediaUploadInfo is AsyncData.Success) { - val caption = markdownTextEditorState.getMessageMarkdown(permalinkBuilder) - .takeIf { it.isNotEmpty() } - ongoingSendAttachmentJob.value = coroutineScope.launch { - sendPreProcessedMedia( - mediaUploadInfo = mediaUploadInfo.data, - caption = caption, - sendActionState = sendActionState, - ) - } - } else if (mediaUploadInfo is AsyncData.Failure) { - sendActionState.value = SendActionState.Failure(mediaUploadInfo.error) + LaunchedEffect(userSentAttachment.value, mediaUploadInfoState.value) { + val mediaUploadInfo = mediaUploadInfoState.value + if (userSentAttachment.value && mediaUploadInfo.isReady()) + if (mediaUploadInfo is AsyncData.Success) { + val caption = markdownTextEditorState.getMessageMarkdown(permalinkBuilder) + .takeIf { it.isNotEmpty() } + ongoingSendAttachmentJob.value = coroutineScope.launch { + sendPreProcessedMedia( + mediaUploadInfo = mediaUploadInfo.data, + caption = caption, + sendActionState = sendActionState, + ) } - // else: cannot happen since we filtered with isReady() + } else if (mediaUploadInfo is AsyncData.Failure) { + sendActionState.value = SendActionState.Failure(mediaUploadInfo.error) } + // else: cannot happen since we filtered with isReady() } fun handleEvents(attachmentsPreviewEvents: AttachmentsPreviewEvents) { @@ -119,7 +106,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( is AttachmentsPreviewEvents.SendAttachment -> coroutineScope.launch { val useSendQueue = featureFlagsService.isFeatureEnabled(FeatureFlags.MediaUploadOnSendQueue) userSentAttachment.value = true - val instantSending = mediaUploadInfoStateFlow.value.isReady() && useSendQueue + val instantSending = mediaUploadInfoState.value.isReady() && useSendQueue sendActionState.value = if (instantSending) { SendActionState.Sending.InstantSending } else { @@ -130,7 +117,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( coroutineScope.cancel( attachment, prePropressingJob, - mediaUploadInfoStateFlow.value, + mediaUploadInfoState.value, sendActionState, ) } @@ -154,7 +141,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( private fun CoroutineScope.preProcessAttachment( attachment: Attachment, - mediaUploadInfoState: MutableStateFlow>, + mediaUploadInfoState: MutableState>, ) = launch { when (attachment) { is Attachment.Media -> { @@ -168,22 +155,22 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( private suspend fun preProcessMedia( mediaAttachment: Attachment.Media, - mediaUploadInfoState: MutableStateFlow>, + mediaUploadInfoState: MutableState>, ) { - mediaUploadInfoState.emit(AsyncData.Loading()) + mediaUploadInfoState.value = AsyncData.Loading() mediaSender.preProcessMedia( uri = mediaAttachment.localMedia.uri, mimeType = mediaAttachment.localMedia.info.mimeType, ).fold( onSuccess = { mediaUploadInfo -> - mediaUploadInfoState.emit(AsyncData.Success(mediaUploadInfo)) + mediaUploadInfoState.value = AsyncData.Success(mediaUploadInfo) }, onFailure = { Timber.e(it, "Failed to pre-process media") if (it is CancellationException) { throw it } else { - mediaUploadInfoState.emit(AsyncData.Failure(it)) + mediaUploadInfoState.value = AsyncData.Failure(it) } } ) diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt index 204a5b5b76..48e5c208b6 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt @@ -88,6 +88,8 @@ class AttachmentsPreviewPresenterTest { val initialState = awaitItem() assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Uploading(0f)) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Uploading(0.5f)) @@ -124,6 +126,8 @@ class AttachmentsPreviewPresenterTest { processLatch.complete(Unit) advanceUntilIdle() initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.InstantSending) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendFileResult.assertions().isCalledOnce() @@ -154,9 +158,11 @@ class AttachmentsPreviewPresenterTest { val initialState = awaitItem() assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) // Pre-processing finishes processLatch.complete(Unit) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendFileResult.assertions().isCalledOnce() onDoneListener.assertions().isCalledOnce() @@ -181,6 +187,8 @@ class AttachmentsPreviewPresenterTest { val initialState = awaitItem() assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) // Pre-processing finishes processLatch.complete(Unit) @@ -209,6 +217,9 @@ class AttachmentsPreviewPresenterTest { processLatch.complete(Unit) advanceUntilIdle() initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.InstantSending) assertThat(awaitItem().sendActionState).isInstanceOf(SendActionState.Failure::class.java) } } @@ -227,6 +238,7 @@ class AttachmentsPreviewPresenterTest { val initialState = awaitItem() assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.eventSink(AttachmentsPreviewEvents.Cancel) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) deleteCallback.assertions().isCalledOnce() onDoneListener.assertions().isCalledOnce() @@ -258,6 +270,8 @@ class AttachmentsPreviewPresenterTest { assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendImageResult.assertions().isCalledOnce().with( @@ -297,6 +311,8 @@ class AttachmentsPreviewPresenterTest { assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendVideoResult.assertions().isCalledOnce().with( @@ -334,6 +350,8 @@ class AttachmentsPreviewPresenterTest { assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Done) sendAudioResult.assertions().isCalledOnce().with( @@ -363,8 +381,9 @@ class AttachmentsPreviewPresenterTest { val initialState = awaitItem() assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) - val loadingState = awaitItem() - assertThat(loadingState.sendActionState).isEqualTo(SendActionState.Sending.Processing) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) val failureState = awaitItem() assertThat(failureState.sendActionState).isEqualTo(SendActionState.Failure(failure)) sendFileResult.assertions().isCalledOnce() @@ -383,6 +402,8 @@ class AttachmentsPreviewPresenterTest { val initialState = awaitItem() assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) + assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) initialState.eventSink(AttachmentsPreviewEvents.ClearSendState) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Idle) From fe2d75700f75f4a69490ab7d68e8871d7cc74c30 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 26 Nov 2024 15:07:22 +0100 Subject: [PATCH 8/9] No need to keep the Job instance. --- .../impl/attachments/preview/AttachmentsPreviewPresenter.kt | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt index 1ca48848a2..4ac8eecc5c 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt @@ -74,9 +74,8 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( val userSentAttachment = remember { mutableStateOf(false) } val mediaUploadInfoState = remember { mutableStateOf>(AsyncData.Uninitialized) } - var prePropressingJob: Job? = null LaunchedEffect(Unit) { - prePropressingJob = preProcessAttachment( + preProcessAttachment( attachment, mediaUploadInfoState, ) @@ -116,7 +115,6 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( AttachmentsPreviewEvents.Cancel -> { coroutineScope.cancel( attachment, - prePropressingJob, mediaUploadInfoState.value, sendActionState, ) @@ -178,7 +176,6 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( private fun CoroutineScope.cancel( attachment: Attachment, - preProcessingJob: Job?, mediaUploadInfo: AsyncData, sendActionState: MutableState, ) = launch { @@ -186,7 +183,6 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( when (attachment) { is Attachment.Media -> { temporaryUriDeleter.delete(attachment.localMedia.uri) - preProcessingJob?.cancel() mediaUploadInfo.dataOrNull()?.let { data -> cleanUp(data) } From 75f3556d61eff2d9627636d2a9b98c4864b93936 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 26 Nov 2024 16:08:19 +0100 Subject: [PATCH 9/9] Rework and comment the code. --- .../preview/AttachmentsPreviewPresenter.kt | 37 ++++++++++++------- 1 file changed, 23 insertions(+), 14 deletions(-) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt index 4ac8eecc5c..69d28d8f20 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt @@ -82,22 +82,31 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( } LaunchedEffect(userSentAttachment.value, mediaUploadInfoState.value) { - val mediaUploadInfo = mediaUploadInfoState.value - if (userSentAttachment.value && mediaUploadInfo.isReady()) - if (mediaUploadInfo is AsyncData.Success) { - val caption = markdownTextEditorState.getMessageMarkdown(permalinkBuilder) - .takeIf { it.isNotEmpty() } - ongoingSendAttachmentJob.value = coroutineScope.launch { - sendPreProcessedMedia( - mediaUploadInfo = mediaUploadInfo.data, - caption = caption, - sendActionState = sendActionState, - ) + if (userSentAttachment.value) { + // User confirmed sending the attachment + when (val mediaUploadInfo = mediaUploadInfoState.value) { + is AsyncData.Success -> { + // Pre-processing is done, send the attachment + val caption = markdownTextEditorState.getMessageMarkdown(permalinkBuilder) + .takeIf { it.isNotEmpty() } + ongoingSendAttachmentJob.value = coroutineScope.launch { + sendPreProcessedMedia( + mediaUploadInfo = mediaUploadInfo.data, + caption = caption, + sendActionState = sendActionState, + ) + } + } + is AsyncData.Failure -> { + // Pre-processing has failed, show the error + sendActionState.value = SendActionState.Failure(mediaUploadInfo.error) + } + AsyncData.Uninitialized, + is AsyncData.Loading -> { + // Pre-processing is still in progress, do nothing } - } else if (mediaUploadInfo is AsyncData.Failure) { - sendActionState.value = SendActionState.Failure(mediaUploadInfo.error) } - // else: cannot happen since we filtered with isReady() + } } fun handleEvents(attachmentsPreviewEvents: AttachmentsPreviewEvents) {