Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Do not delete the original file if it's not a temporary file when sending it to a room. #3819

Merged
merged 6 commits into from
Nov 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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()
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -68,6 +74,9 @@ class AttachmentsPreviewPresenter @AssistedInject constructor(
sendActionState = sendActionState,
)
}
AttachmentsPreviewEvents.Cancel -> {
coroutineScope.cancel(attachment)
}
AttachmentsPreviewEvents.ClearSendState -> {
ongoingSendAttachmentJob.value?.let {
it.cancel()
Expand Down Expand Up @@ -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?,
Expand All @@ -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")
Expand Down
Original file line number Diff 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
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -17,9 +18,6 @@
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
Expand Down Expand Up @@ -50,22 +48,22 @@
@Composable
fun AttachmentsPreviewView(
state: AttachmentsPreviewState,
onDismiss: () -> Unit,
modifier: Modifier = Modifier,
) {
fun postSendAttachment() {
state.eventSink(AttachmentsPreviewEvents.SendAttachment)
}

fun postCancel() {
state.eventSink(AttachmentsPreviewEvents.Cancel)

Check warning on line 58 in features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt

View check run for this annotation

Codecov / codecov/patch

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt#L58

Added line #L58 was not covered by tests
}

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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this check?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The cancellation during the sending state is emitting a ClearSendState event, so I did not want to change this behavior.

postCancel()

Check warning on line 66 in features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt

View check run for this annotation

Codecov / codecov/patch

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt#L66

Added line #L66 was not covered by tests
}

Scaffold(
Expand All @@ -75,7 +73,7 @@
navigationIcon = {
BackButton(
imageVector = CompoundIcons.Close(),
onClick = onDismiss,
onClick = ::postCancel,
)
},
title = {},
Expand Down Expand Up @@ -202,6 +200,5 @@
internal fun AttachmentsPreviewViewPreview(@PreviewParameter(AttachmentsPreviewStateProvider::class) state: AttachmentsPreviewState) = ElementPreviewDark {
AttachmentsPreviewView(
state = state,
onDismiss = {},
)
}
Original file line number Diff line number Diff line change
@@ -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()
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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 {
Expand All @@ -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()
}
}

Expand All @@ -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()
Expand All @@ -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(),
Expand All @@ -118,6 +146,7 @@ class AttachmentsPreviewPresenterTest {
any(),
any(),
)
onDoneListener.assertions().isCalledOnce()
}
}

Expand All @@ -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()
Expand All @@ -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(),
Expand All @@ -155,6 +185,7 @@ class AttachmentsPreviewPresenterTest {
any(),
any(),
)
onDoneListener.assertions().isCalledOnce()
}
}

Expand Down Expand Up @@ -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,
)
}
}
Loading
Loading