Merge pull request #3819 from element-hq/feature/bma/fixDeleteOrigina…

…lFile Do not delete the original file if it's not a temporary file when sending it to a room.
element-hq · Nov 8, 2024 · a0fb244 · a0fb244
2 parents 64b1f30 + 8dfe530
commit a0fb244
Show file tree

Hide file tree

Showing 18 changed files with 424 additions and 118 deletions.
diff --git a/...otlin/io/element/android/features/createroom/impl/configureroom/ConfigureRoomPresenter.kt b/...otlin/io/element/android/features/createroom/impl/configureroom/ConfigureRoomPresenter.kt
@@ -175,7 +175,12 @@ class ConfigureRoomPresenter @Inject constructor(
     }
 
     private suspend fun uploadAvatar(avatarUri: Uri): String {
-        val preprocessed = mediaPreProcessor.process(avatarUri, MimeTypes.Jpeg, compressIfPossible = false).getOrThrow()
+        val preprocessed = mediaPreProcessor.process(
+            uri = avatarUri,
+            mimeType = MimeTypes.Jpeg,
+            deleteOriginal = false,
+            compressIfPossible = false,
+        ).getOrThrow()
         val byteArray = preprocessed.file.readBytes()
         return matrixClient.uploadMedia(MimeTypes.Jpeg, byteArray, null).getOrThrow()
     }

diff --git a/...io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewEvents.kt b/...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/...n/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt b/...n/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewNode.kt
@@ -31,15 +31,21 @@ 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) {
         ForcedDarkElementTheme {
             val state = presenter.present()
             AttachmentsPreviewView(
                 state = state,
-                onDismiss = this::navigateUp,
                 modifier = modifier
             )
         }

diff --git a/...element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt b/...element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt
@@ -18,6 +18,7 @@ import dagger.assisted.Assisted
 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.architecture.Presenter
 import io.element.android.libraries.matrix.api.core.ProgressCallback
 import io.element.android.libraries.matrix.api.permalink.PermalinkBuilder
@@ -34,12 +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<AttachmentsPreviewState> {
     @AssistedFactory
     interface Factory {
-        fun create(attachment: Attachment): AttachmentsPreviewPresenter
+        fun create(
+            attachment: Attachment,
+            onDoneListener: OnDoneListener,
+        ): AttachmentsPreviewPresenter
     }
 
     @Composable
@@ -68,6 +74,9 @@ class AttachmentsPreviewPresenter @AssistedInject constructor(
                         sendActionState = sendActionState,
                     )
                 }
+                AttachmentsPreviewEvents.Cancel -> {
+                    coroutineScope.cancel(attachment)
+                }
                 AttachmentsPreviewEvents.ClearSendState -> {
                     ongoingSendAttachmentJob.value?.let {
                         it.cancel()
@@ -102,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?,
@@ -124,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/.../io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewState.kt b/.../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/...n/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt b/...n/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/...in/kotlin/io/element/android/features/messages/impl/attachments/preview/OnDoneListener.kt b/...in/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/.../io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt b/.../io/element/android/features/messages/impl/attachments/AttachmentsPreviewPresenterTest.kt
@@ -16,8 +16,10 @@ 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
 import io.element.android.libraries.matrix.api.core.ProgressCallback
 import io.element.android.libraries.matrix.api.media.FileInfo
 import io.element.android.libraries.matrix.api.media.ImageInfo
@@ -35,11 +37,13 @@ import io.element.android.libraries.mediaviewer.api.local.LocalMedia
 import io.element.android.libraries.mediaviewer.test.viewer.aLocalMedia
 import io.element.android.libraries.preferences.test.InMemorySessionPreferencesStore
 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.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
@@ -67,7 +71,11 @@ class AttachmentsPreviewPresenterTest {
             ),
             sendFileResult = sendFileResult,
         )
-        val presenter = createAttachmentsPreviewPresenter(room = room)
+        val onDoneListener = lambdaRecorder<Unit> { }
+        val presenter = createAttachmentsPreviewPresenter(
+            room = room,
+            onDoneListener = { onDoneListener() },
+        )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
@@ -78,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<Unit> { }
+        val deleteCallback = lambdaRecorder<Uri?, Unit> {}
+        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()
         }
     }
 
@@ -96,9 +123,11 @@ class AttachmentsPreviewPresenterTest {
         val room = FakeMatrixRoom(
             sendImageResult = sendImageResult,
         )
+        val onDoneListener = lambdaRecorder<Unit> { }
         val presenter = createAttachmentsPreviewPresenter(
             room = room,
             mediaPreProcessor = mediaPreProcessor,
+            onDoneListener = { onDoneListener() },
         )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
@@ -108,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(),
@@ -118,6 +146,7 @@ class AttachmentsPreviewPresenterTest {
                 any(),
                 any(),
             )
+            onDoneListener.assertions().isCalledOnce()
         }
     }
 
@@ -133,9 +162,11 @@ class AttachmentsPreviewPresenterTest {
         val room = FakeMatrixRoom(
             sendVideoResult = sendVideoResult,
         )
+        val onDoneListener = lambdaRecorder<Unit> { }
         val presenter = createAttachmentsPreviewPresenter(
             room = room,
             mediaPreProcessor = mediaPreProcessor,
+            onDoneListener = { onDoneListener() },
         )
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
@@ -145,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(),
@@ -155,6 +185,7 @@ class AttachmentsPreviewPresenterTest {
                 any(),
                 any(),
             )
+            onDoneListener.assertions().isCalledOnce()
         }
     }
 
@@ -207,11 +238,15 @@ class AttachmentsPreviewPresenterTest {
         room: MatrixRoom = FakeMatrixRoom(),
         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,
         )
     }
 }
-Original file line number
+Diff line change
@@ Expand Up / @@ -36,5 +36,4 @@ sealed interface SendActionState { @@
         }
         data class Failure(val error: Throwable) : SendActionState
-        data object Done : SendActionState
     }