From 62aab87c0924736cb29c3845ac94b1f8df64dcfd Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Thu, 7 Nov 2024 09:20:48 +0100 Subject: [PATCH] Delete the temporary file only when the user explicitly cancel the upload. --- .../preview/AttachmentsPreviewEvents.kt | 1 + .../preview/AttachmentsPreviewNode.kt | 10 ++++- .../preview/AttachmentsPreviewPresenter.kt | 38 +++++++++------- .../preview/AttachmentsPreviewState.kt | 1 - .../preview/AttachmentsPreviewView.kt | 19 ++++---- .../attachments/preview/OnDoneListener.kt | 12 +++++ .../AttachmentsPreviewPresenterTest.kt | 45 ++++++++++++++++--- 7 files changed, 88 insertions(+), 38 deletions(-) create mode 100644 features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/OnDoneListener.kt diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewEvents.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewEvents.kt index 28cc95eaf0..742e78e8e0 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewEvents.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewEvents.kt @@ -12,5 +12,6 @@ import androidx.compose.runtime.Immutable @Immutable sealed interface AttachmentsPreviewEvents { data object SendAttachment : AttachmentsPreviewEvents + data object Cancel : AttachmentsPreviewEvents data object ClearSendState : AttachmentsPreviewEvents } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt index a435821197..2417d8346e 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt @@ -31,7 +31,14 @@ class AttachmentsPreviewNode @AssistedInject constructor( private val inputs: Inputs = inputs() - private val presenter = presenterFactory.create(inputs.attachment) + private val onDoneListener = OnDoneListener { + navigateUp() + } + + private val presenter = presenterFactory.create( + attachment = inputs.attachment, + onDoneListener = onDoneListener, + ) @Composable override fun View(modifier: Modifier) { @@ -39,7 +46,6 @@ class AttachmentsPreviewNode @AssistedInject constructor( val state = presenter.present() AttachmentsPreviewView( state = state, - onDismiss = this::navigateUp, modifier = modifier ) } 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 a3ba509a15..bc3e121070 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,7 +8,6 @@ package io.element.android.features.messages.impl.attachments.preview import androidx.compose.runtime.Composable -import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.MutableState import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf @@ -36,13 +35,17 @@ import kotlin.coroutines.coroutineContext 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, ) : Presenter { @AssistedFactory interface Factory { - fun create(attachment: Attachment): AttachmentsPreviewPresenter + fun create( + attachment: Attachment, + onDoneListener: OnDoneListener, + ): AttachmentsPreviewPresenter } @Composable @@ -60,20 +63,6 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( val ongoingSendAttachmentJob = remember { mutableStateOf(null) } - DisposableEffect(Unit) { - onDispose { - // Delete the temporary file when the composable is disposed, in case it was not sent - if (sendActionState.value == SendActionState.Idle) { - // Attachment has not been sent, maybe delete it - when (attachment) { - is Attachment.Media -> { - temporaryUriDeleter.delete(attachment.localMedia.uri) - } - } - } - } - } - fun handleEvents(attachmentsPreviewEvents: AttachmentsPreviewEvents) { when (attachmentsPreviewEvents) { is AttachmentsPreviewEvents.SendAttachment -> { @@ -85,6 +74,9 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( sendActionState = sendActionState, ) } + AttachmentsPreviewEvents.Cancel -> { + coroutineScope.cancel(attachment) + } AttachmentsPreviewEvents.ClearSendState -> { ongoingSendAttachmentJob.value?.let { it.cancel() @@ -119,6 +111,18 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( } } + private fun CoroutineScope.cancel( + attachment: Attachment, + ) = launch { + // Delete the temporary file + when (attachment) { + is Attachment.Media -> { + temporaryUriDeleter.delete(attachment.localMedia.uri) + } + } + onDoneListener() + } + private suspend fun sendMedia( mediaAttachment: Attachment.Media, caption: String?, @@ -141,7 +145,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor( ).getOrThrow() }.fold( onSuccess = { - sendActionState.value = SendActionState.Done + onDoneListener() }, onFailure = { error -> Timber.e(error, "Failed to send attachment") 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 72ea0a2098..5ffe9364ff 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 @@ -36,5 +36,4 @@ sealed interface SendActionState { } data class Failure(val error: Throwable) : SendActionState - data object Done : SendActionState } 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 2906f26b3c..1380add329 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 @@ -7,6 +7,7 @@ package io.element.android.features.messages.impl.attachments.preview +import androidx.activity.compose.BackHandler import androidx.compose.foundation.background import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.IntrinsicSize @@ -17,9 +18,6 @@ import androidx.compose.foundation.layout.imePadding import androidx.compose.foundation.layout.navigationBarsPadding import androidx.compose.material3.ExperimentalMaterial3Api import androidx.compose.runtime.Composable -import androidx.compose.runtime.LaunchedEffect -import androidx.compose.runtime.getValue -import androidx.compose.runtime.rememberUpdatedState import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource @@ -50,22 +48,22 @@ import me.saket.telephoto.zoomable.rememberZoomableState @Composable fun AttachmentsPreviewView( state: AttachmentsPreviewState, - onDismiss: () -> Unit, modifier: Modifier = Modifier, ) { fun postSendAttachment() { state.eventSink(AttachmentsPreviewEvents.SendAttachment) } + fun postCancel() { + state.eventSink(AttachmentsPreviewEvents.Cancel) + } + fun postClearSendState() { state.eventSink(AttachmentsPreviewEvents.ClearSendState) } - if (state.sendActionState is SendActionState.Done) { - val latestOnDismiss by rememberUpdatedState(onDismiss) - LaunchedEffect(state.sendActionState) { - latestOnDismiss() - } + BackHandler(enabled = state.sendActionState !is SendActionState.Sending) { + postCancel() } Scaffold( @@ -75,7 +73,7 @@ fun AttachmentsPreviewView( navigationIcon = { BackButton( imageVector = CompoundIcons.Close(), - onClick = onDismiss, + onClick = ::postCancel, ) }, title = {}, @@ -202,6 +200,5 @@ private fun AttachmentsPreviewBottomActions( internal fun AttachmentsPreviewViewPreview(@PreviewParameter(AttachmentsPreviewStateProvider::class) state: AttachmentsPreviewState) = ElementPreviewDark { AttachmentsPreviewView( state = state, - onDismiss = {}, ) } diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/OnDoneListener.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/OnDoneListener.kt new file mode 100644 index 0000000000..2e53ab2c50 --- /dev/null +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/OnDoneListener.kt @@ -0,0 +1,12 @@ +/* + * Copyright 2024 New Vector Ltd. + * + * SPDX-License-Identifier: AGPL-3.0-only + * Please see LICENSE in the repository root for full details. + */ + +package io.element.android.features.messages.impl.attachments.preview + +fun interface OnDoneListener { + operator fun invoke() +} 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 85acefb5fe..ac753f6cdb 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 @@ -16,6 +16,7 @@ import app.cash.turbine.test import com.google.common.truth.Truth.assertThat import io.element.android.features.messages.impl.attachments.preview.AttachmentsPreviewEvents import io.element.android.features.messages.impl.attachments.preview.AttachmentsPreviewPresenter +import io.element.android.features.messages.impl.attachments.preview.OnDoneListener 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 @@ -42,6 +43,7 @@ import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value import io.mockk.mockk import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test @@ -69,7 +71,11 @@ class AttachmentsPreviewPresenterTest { ), sendFileResult = sendFileResult, ) - val presenter = createAttachmentsPreviewPresenter(room = room) + val onDoneListener = lambdaRecorder { } + val presenter = createAttachmentsPreviewPresenter( + room = room, + onDoneListener = { onDoneListener() }, + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { @@ -80,9 +86,28 @@ 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)) - val successState = awaitItem() - assertThat(successState.sendActionState).isEqualTo(SendActionState.Done) + advanceUntilIdle() sendFileResult.assertions().isCalledOnce() + onDoneListener.assertions().isCalledOnce() + } + } + + @Test + fun `present - cancel scenario`() = runTest { + val onDoneListener = lambdaRecorder { } + val deleteCallback = lambdaRecorder {} + val presenter = createAttachmentsPreviewPresenter( + temporaryUriDeleter = FakeTemporaryUriDeleter(deleteCallback), + onDoneListener = { onDoneListener() }, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitItem() + assertThat(initialState.sendActionState).isEqualTo(SendActionState.Idle) + initialState.eventSink(AttachmentsPreviewEvents.Cancel) + deleteCallback.assertions().isCalledOnce() + onDoneListener.assertions().isCalledOnce() } } @@ -98,9 +123,11 @@ class AttachmentsPreviewPresenterTest { val room = FakeMatrixRoom( sendImageResult = sendImageResult, ) + val onDoneListener = lambdaRecorder { } val presenter = createAttachmentsPreviewPresenter( room = room, mediaPreProcessor = mediaPreProcessor, + onDoneListener = { onDoneListener() }, ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() @@ -110,8 +137,7 @@ class AttachmentsPreviewPresenterTest { initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) - val successState = awaitItem() - assertThat(successState.sendActionState).isEqualTo(SendActionState.Done) + advanceUntilIdle() sendImageResult.assertions().isCalledOnce().with( any(), any(), @@ -120,6 +146,7 @@ class AttachmentsPreviewPresenterTest { any(), any(), ) + onDoneListener.assertions().isCalledOnce() } } @@ -135,9 +162,11 @@ class AttachmentsPreviewPresenterTest { val room = FakeMatrixRoom( sendVideoResult = sendVideoResult, ) + val onDoneListener = lambdaRecorder { } val presenter = createAttachmentsPreviewPresenter( room = room, mediaPreProcessor = mediaPreProcessor, + onDoneListener = { onDoneListener() }, ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() @@ -147,8 +176,7 @@ class AttachmentsPreviewPresenterTest { initialState.textEditorState.setMarkdown(A_CAPTION) initialState.eventSink(AttachmentsPreviewEvents.SendAttachment) assertThat(awaitItem().sendActionState).isEqualTo(SendActionState.Sending.Processing) - val successState = awaitItem() - assertThat(successState.sendActionState).isEqualTo(SendActionState.Done) + advanceUntilIdle() sendVideoResult.assertions().isCalledOnce().with( any(), any(), @@ -157,6 +185,7 @@ class AttachmentsPreviewPresenterTest { any(), any(), ) + onDoneListener.assertions().isCalledOnce() } } @@ -210,9 +239,11 @@ class AttachmentsPreviewPresenterTest { permalinkBuilder: PermalinkBuilder = FakePermalinkBuilder(), mediaPreProcessor: MediaPreProcessor = FakeMediaPreProcessor(), temporaryUriDeleter: TemporaryUriDeleter = FakeTemporaryUriDeleter(), + onDoneListener: OnDoneListener = OnDoneListener {}, ): AttachmentsPreviewPresenter { return AttachmentsPreviewPresenter( attachment = aMediaAttachment(localMedia), + onDoneListener = onDoneListener, mediaSender = MediaSender(mediaPreProcessor, room, InMemorySessionPreferencesStore()), permalinkBuilder = permalinkBuilder, temporaryUriDeleter = temporaryUriDeleter,