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

Close the media preview screen ASAP with sending queue enabled #4089

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from
Open
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 @@ -15,9 +15,10 @@ import io.element.android.libraries.di.RoomScope
import io.element.android.libraries.di.SessionScope
import io.element.android.libraries.di.SingleIn
import io.element.android.libraries.matrix.api.room.MatrixRoom
import io.element.android.libraries.matrix.impl.di.SessionMatrixModule

@SingleIn(RoomScope::class)
@MergeSubcomponent(RoomScope::class)
@MergeSubcomponent(RoomScope::class, modules = [SessionMatrixModule::class])
interface RoomComponent : NodeFactoriesBindings {
@MergeSubcomponent.Builder
interface Builder {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ 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.di.annotations.SessionCoroutineScope
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
Expand All @@ -48,6 +49,7 @@ class AttachmentsPreviewPresenter @AssistedInject constructor(
private val permalinkBuilder: PermalinkBuilder,
private val temporaryUriDeleter: TemporaryUriDeleter,
private val featureFlagService: FeatureFlagService,
@SessionCoroutineScope private val sessionCoroutineScope: CoroutineScope,
) : Presenter<AttachmentsPreviewState> {
@AssistedFactory
interface Factory {
Expand Down Expand Up @@ -92,13 +94,22 @@ class AttachmentsPreviewPresenter @AssistedInject constructor(
// Pre-processing is done, send the attachment
val caption = markdownTextEditorState.getMessageMarkdown(permalinkBuilder)
.takeIf { it.isNotEmpty() }
ongoingSendAttachmentJob.value = coroutineScope.launch {

// Send it using the session coroutine scope so it doesn't matter if this screen or the chat one is closed
val sendOnBackground = featureFlagService.isFeatureEnabled(FeatureFlags.MediaUploadOnSendQueue)
val sendMediaScope = if (sendOnBackground) sessionCoroutineScope else coroutineScope
ongoingSendAttachmentJob.value = sendMediaScope.launch {
sendPreProcessedMedia(
mediaUploadInfo = mediaUploadInfo.data,
caption = caption,
sendActionState = sendActionState,
)
}

// If we're supposed to send the media as a background job, we can dismiss this screen already
if (sendOnBackground) {
onDoneListener()
}
}
is AsyncData.Failure -> {
// Pre-processing has failed, show the error
Expand Down Expand Up @@ -240,7 +251,6 @@ class AttachmentsPreviewPresenter @AssistedInject constructor(
cleanUp(mediaUploadInfo)
// Reset the sendActionState to ensure that dialog is closed before the screen
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 @@ -49,6 +49,7 @@ import io.element.android.tests.testutils.test
import io.mockk.mockk
import kotlinx.coroutines.CompletableDeferred
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.advanceUntilIdle
import kotlinx.coroutines.test.runTest
import org.junit.Rule
Expand Down Expand Up @@ -400,15 +401,15 @@ class AttachmentsPreviewPresenterTest {
}

@Test
fun `present - send media failure scenario`() = runTest {
fun `present - send media failure scenario without media queue`() = runTest {
val failure = MediaPreProcessor.Failure(null)
val sendFileResult = lambdaRecorder<File, FileInfo, String?, String?, ProgressCallback?, Result<FakeMediaUploadHandler>> { _, _, _, _, _ ->
Result.failure(failure)
}
val room = FakeMatrixRoom(
sendFileResult = sendFileResult,
)
val presenter = createAttachmentsPreviewPresenter(room = room)
val presenter = createAttachmentsPreviewPresenter(room = room, mediaUploadOnSendQueueEnabled = false)
moleculeFlow(RecompositionMode.Immediate) {
presenter.present()
}.test {
Expand All @@ -428,8 +429,41 @@ class AttachmentsPreviewPresenterTest {
}

@Test
fun `present - dismissing the progress dialog stops media upload`() = runTest {
val presenter = createAttachmentsPreviewPresenter()
fun `present - send media failure scenario with media queue`() = runTest {
val failure = MediaPreProcessor.Failure(null)
val sendFileResult = lambdaRecorder<File, FileInfo, String?, String?, ProgressCallback?, Result<FakeMediaUploadHandler>> { _, _, _, _, _ ->
Result.failure(failure)
}
val onDoneListenerResult = lambdaRecorder<Unit> {}
val room = FakeMatrixRoom(
sendFileResult = sendFileResult,
)
val presenter = createAttachmentsPreviewPresenter(room = room, mediaUploadOnSendQueueEnabled = true, onDoneListener = onDoneListenerResult)
moleculeFlow(RecompositionMode.Immediate) {
presenter.present()
}.test {
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)

// Check that the onDoneListener is called so the screen would be dismissed
onDoneListenerResult.assertions().isCalledOnce()

val failureState = awaitItem()
assertThat(failureState.sendActionState).isEqualTo(SendActionState.Failure(failure))
sendFileResult.assertions().isCalledOnce()
failureState.eventSink(AttachmentsPreviewEvents.ClearSendState)
val clearedState = awaitItem()
assertThat(clearedState.sendActionState).isEqualTo(SendActionState.Idle)
}
}

@Test
fun `present - dismissing the progress dialog stops media upload without media queue`() = runTest {
val presenter = createAttachmentsPreviewPresenter(mediaUploadOnSendQueueEnabled = false)
moleculeFlow(RecompositionMode.Immediate) {
presenter.present()
}.test {
Expand All @@ -444,7 +478,28 @@ class AttachmentsPreviewPresenterTest {
}
}

private fun createAttachmentsPreviewPresenter(
@Test
fun `present - dismissing the progress dialog stops media upload with media queue`() = runTest {
val onDoneListenerResult = lambdaRecorder<Unit> {}
val presenter = createAttachmentsPreviewPresenter(mediaUploadOnSendQueueEnabled = true, onDoneListener = onDoneListenerResult)
moleculeFlow(RecompositionMode.Immediate) {
presenter.present()
}.test {
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)

// Check that the onDoneListener is called so the screen would be dismissed
onDoneListenerResult.assertions().isCalledOnce()
}
}

private fun TestScope.createAttachmentsPreviewPresenter(
localMedia: LocalMedia = aLocalMedia(
uri = mockMediaUrl,
),
Expand All @@ -469,7 +524,8 @@ class AttachmentsPreviewPresenterTest {
FeatureFlags.MediaCaptionCreation.key to allowCaption,
FeatureFlags.MediaCaptionWarning.key to showCaptionCompatibilityWarning,
),
)
),
sessionCoroutineScope = this,
)
}
}
Loading