diff --git a/features/location/impl/src/main/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenter.kt b/features/location/impl/src/main/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenter.kt index ccdf598e93..05af96fe5e 100644 --- a/features/location/impl/src/main/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenter.kt +++ b/features/location/impl/src/main/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenter.kt @@ -217,8 +217,8 @@ class ShareLocationPresenter( val replyMode = messageComposerContext.composerMode as? MessageComposerMode.Reply val inReplyToEventId = replyMode?.eventId val geoUri = event.location.toGeoUri() - getTimeline().flatMap { - it.sendLocation( + withTimeline { timeline -> + timeline.sendLocation( body = generateBody(geoUri), geoUri = geoUri, description = null, @@ -237,10 +237,22 @@ class ShareLocationPresenter( ) } - private suspend fun getTimeline(): Result { + /** + * Invokes [block] with the [Timeline] matching [timelineMode]. + * + * For a thread, a dedicated timeline is created and closed once [block] returns, since such a + * timeline owns SDK resources. For the other modes, the live timeline is used, and is not + * closed here since it is owned by its room. + */ + private suspend fun withTimeline(block: suspend (Timeline) -> Result): Result { return when (timelineMode) { - is Timeline.Mode.Thread -> room.createTimeline(CreateTimelineParams.Threaded(timelineMode.threadRootId)) - else -> Result.success(room.liveTimeline) + is Timeline.Mode.Thread -> { + room.createTimeline(CreateTimelineParams.Threaded(timelineMode.threadRootId)) + .flatMap { threadedTimeline -> + threadedTimeline.use { block(it) } + } + } + else -> block(room.liveTimeline) } } } diff --git a/features/location/impl/src/test/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenterTest.kt b/features/location/impl/src/test/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenterTest.kt index 7500d29616..129ea1e4cd 100644 --- a/features/location/impl/src/test/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenterTest.kt +++ b/features/location/impl/src/test/kotlin/io/element/android/features/location/impl/share/ShareLocationPresenterTest.kt @@ -708,6 +708,45 @@ class ShareLocationPresenterTest { } } + @Test + fun `ShareStaticLocation in a thread closes the created timeline`() = runTest { + val threadedTimeline = FakeTimeline(mode = Timeline.Mode.Thread(A_THREAD_ID)).apply { + sendLocationLambda = { _, _, _, _, _, _ -> Result.success(Unit) } + } + val liveTimeline = FakeTimeline() + val joinedRoom = FakeJoinedRoom( + liveTimeline = liveTimeline, + createTimelineResult = { Result.success(threadedTimeline) }, + ) + val shareLocationPresenter = createShareLocationPresenter( + joinedRoom = joinedRoom, + timelineMode = Timeline.Mode.Thread(A_THREAD_ID), + ) + fakePermissionsPresenter.givenState( + aPermissionsState( + permissions = PermissionsState.Permissions.AllGranted, + shouldShowRationale = false, + ) + ) + + shareLocationPresenter.test { + val initialState = awaitFirstItem() + + initialState.eventSink( + ShareLocationEvent.ShareStaticLocation( + location = Location(lat = 3.0, lon = 4.0, accuracy = 5.0f), + isPinned = false, + ) + ) + + advanceUntilIdle() + + assertThat(threadedTimeline.closeCounter).isEqualTo(1) + assertThat(liveTimeline.closeCounter).isEqualTo(0) + cancelAndIgnoreRemainingEvents() + } + } + private suspend fun ReceiveTurbine.awaitFirstItem(): T { skipItems(1) return awaitItem() diff --git a/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/DefaultMediaSender.kt b/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/DefaultMediaSender.kt index 37f77d11f8..36c3ba99fb 100644 --- a/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/DefaultMediaSender.kt +++ b/libraries/mediaupload/impl/src/main/kotlin/io/element/android/libraries/mediaupload/impl/DefaultMediaSender.kt @@ -100,9 +100,9 @@ class DefaultMediaSender( inReplyToEventId: EventId?, ): Result { val mediaLogId = mediaId(mediaUploadInfo.file) - return getTimeline().flatMap { - Timber.d("Started sending media $mediaLogId using timeline: ${it.mode}") - it.sendMedia( + return withTimeline { timeline -> + Timber.d("Started sending media $mediaLogId using timeline: ${timeline.mode}") + timeline.sendMedia( uploadInfo = mediaUploadInfo, caption = caption, formattedCaption = formattedCaption, @@ -128,12 +128,14 @@ class DefaultMediaSender( mediaOptimizationConfig = mediaOptimizationConfig, ) .flatMapCatching { info -> - getTimeline().getOrThrow().sendMedia( - uploadInfo = info, - caption = caption, - formattedCaption = formattedCaption, - inReplyToEventId = inReplyToEventId, - ) + withTimeline { timeline -> + timeline.sendMedia( + uploadInfo = info, + caption = caption, + formattedCaption = formattedCaption, + inReplyToEventId = inReplyToEventId, + ) + } } .handleSendResult(mediaId(uri)) } @@ -158,12 +160,14 @@ class DefaultMediaSender( audioInfo = audioInfo, waveform = waveForm, ) - getTimeline().getOrThrow().sendMedia( - uploadInfo = newInfo, - caption = null, - formattedCaption = null, - inReplyToEventId = inReplyToEventId, - ) + withTimeline { timeline -> + timeline.sendMedia( + uploadInfo = newInfo, + caption = null, + formattedCaption = null, + inReplyToEventId = inReplyToEventId, + ) + } } .handleSendResult(mediaId(uri)) } @@ -176,7 +180,7 @@ class DefaultMediaSender( ): Result { val galleryLogId = "gallery[${mediaUploadInfos.size} items]" Timber.d("Sending $galleryLogId") - return getTimeline().flatMap { timeline -> + return withTimeline { timeline -> val galleryItems = mediaUploadInfos.map { it.toGalleryItemInfo() } timeline.sendGallery( items = galleryItems, @@ -184,10 +188,10 @@ class DefaultMediaSender( formattedCaption = formattedCaption, inReplyToEventId = inReplyToEventId, ) + .flatMapCatching { uploadHandler -> + uploadHandler.await() + } } - .flatMapCatching { uploadHandler -> - uploadHandler.await() - } .handleSendResult(galleryLogId) } @@ -269,12 +273,23 @@ class DefaultMediaSender( } } - private suspend fun getTimeline(): Result { + /** + * Invokes [block] with the [Timeline] matching [timelineMode]. + * + * For a thread, a dedicated timeline is created and closed once [block] returns, since such a + * timeline owns SDK resources. [block] must then perform the whole sending, including waiting + * for the media upload to complete. For the other modes, the live timeline is used, and is not + * closed here since it is owned by its room. + */ + private suspend fun withTimeline(block: suspend (Timeline) -> Result): Result { return when (timelineMode) { is Timeline.Mode.Thread -> { room.createTimeline(CreateTimelineParams.Threaded(threadRootEventId = timelineMode.threadRootId)) + .flatMap { threadedTimeline -> + threadedTimeline.use { block(it) } + } } - else -> Result.success(room.liveTimeline) + else -> block(room.liveTimeline) } } diff --git a/libraries/mediaupload/impl/src/test/kotlin/io/element/android/libraries/mediaupload/impl/DefaultMediaSenderTest.kt b/libraries/mediaupload/impl/src/test/kotlin/io/element/android/libraries/mediaupload/impl/DefaultMediaSenderTest.kt index cfa3585629..97a9b95c51 100644 --- a/libraries/mediaupload/impl/src/test/kotlin/io/element/android/libraries/mediaupload/impl/DefaultMediaSenderTest.kt +++ b/libraries/mediaupload/impl/src/test/kotlin/io/element/android/libraries/mediaupload/impl/DefaultMediaSenderTest.kt @@ -16,6 +16,7 @@ import io.element.android.libraries.matrix.api.media.FileInfo import io.element.android.libraries.matrix.api.media.ImageInfo import io.element.android.libraries.matrix.api.room.JoinedRoom import io.element.android.libraries.matrix.api.timeline.Timeline +import io.element.android.libraries.matrix.test.A_THREAD_ID import io.element.android.libraries.matrix.test.media.FakeMediaUploadHandler import io.element.android.libraries.matrix.test.room.FakeJoinedRoom import io.element.android.libraries.matrix.test.timeline.FakeTimeline @@ -156,14 +157,52 @@ class DefaultMediaSenderTest : RobolectricTest() { sendFileResult.assertions().isCalledOnce() } + @Test + fun `given a thread, the created timeline is closed once the media is sent`() = runTest { + val threadedTimeline = FakeTimeline(mode = Timeline.Mode.Thread(A_THREAD_ID)).apply { + sendFileLambda = { _, _, _, _, _ -> Result.success(FakeMediaUploadHandler()) } + } + val liveTimeline = FakeTimeline() + val room = FakeJoinedRoom( + liveTimeline = liveTimeline, + createTimelineResult = { Result.success(threadedTimeline) }, + ) + val sender = createDefaultMediaSender( + room = room, + timelineMode = Timeline.Mode.Thread(A_THREAD_ID), + ) + + val uri = Uri.parse("content://image.jpg") + val result = sender.sendMedia(uri = uri, mimeType = MimeTypes.Jpeg, mediaOptimizationConfig = mediaOptimizationConfig) + + assertThat(result.isSuccess).isTrue() + assertThat(threadedTimeline.closeCounter).isEqualTo(1) + assertThat(liveTimeline.closeCounter).isEqualTo(0) + } + + @Test + fun `given a live timeline, the timeline is not closed once the media is sent`() = runTest { + val liveTimeline = FakeTimeline().apply { + sendFileLambda = { _, _, _, _, _ -> Result.success(FakeMediaUploadHandler()) } + } + val sender = createDefaultMediaSender(room = FakeJoinedRoom(liveTimeline = liveTimeline)) + + val uri = Uri.parse("content://image.jpg") + val result = sender.sendMedia(uri = uri, mimeType = MimeTypes.Jpeg, mediaOptimizationConfig = mediaOptimizationConfig) + + assertThat(result.isSuccess).isTrue() + assertThat(liveTimeline.closeCounter).isEqualTo(0) + } + private fun createDefaultMediaSender( preProcessor: MediaPreProcessor = FakeMediaPreProcessor(), room: JoinedRoom = FakeJoinedRoom(), + timelineMode: Timeline.Mode = Timeline.Mode.Live, mediaOptimizationConfigProvider: MediaOptimizationConfigProvider = MediaOptimizationConfigProvider { mediaOptimizationConfig }, ) = DefaultMediaSender( preProcessor = preProcessor, room = room, - timelineMode = Timeline.Mode.Live, + timelineMode = timelineMode, mediaOptimizationConfigProvider = mediaOptimizationConfigProvider, ) }