diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/datasource/MediaGalleryDataSource.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/datasource/MediaGalleryDataSource.kt index f5418c76c9..7bdd4872a3 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/datasource/MediaGalleryDataSource.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/datasource/MediaGalleryDataSource.kt @@ -32,6 +32,7 @@ import timber.log.Timber import java.util.concurrent.atomic.AtomicBoolean interface MediaGalleryDataSource { + val isReady: Boolean fun start(coroutineScope: CoroutineScope) fun groupedMediaItemsFlow(): Flow> fun getLastData(): AsyncData @@ -49,7 +50,7 @@ class TimelineMediaGalleryDataSource( ) : MediaGalleryDataSource { private var timeline: Timeline? = null - private val groupedMediaItemsFlow = MutableSharedFlow>(replay = 1) + private val groupedMediaItemsFlow = MutableSharedFlow>(replay = 1, extraBufferCapacity = 10) override fun groupedMediaItemsFlow(): Flow> = groupedMediaItemsFlow @@ -59,9 +60,12 @@ class TimelineMediaGalleryDataSource( private val isStarted = AtomicBoolean(false) + override val isReady: Boolean get() = isStarted.get() && timeline != null + @OptIn(ExperimentalCoroutinesApi::class) override fun start(coroutineScope: CoroutineScope) { if (!isStarted.compareAndSet(false, true)) { + Timber.w("MediaGalleryDataSource for room ${room.roomId} is already started, ignoring subsequent start call") return } flow { @@ -73,10 +77,12 @@ class TimelineMediaGalleryDataSource( } mediaTimeline.getTimeline().fold( { + Timber.d("Timeline media data source flow started for room ${room.roomId}") timeline = it emit(it) }, { + Timber.e(it, "Failed to get media timeline for room ${room.roomId}") groupedMediaItemsFlow.emit(AsyncData.Failure(it)) }, ) @@ -107,13 +113,13 @@ class TimelineMediaGalleryDataSource( } override suspend fun loadMore(direction: Timeline.PaginationDirection) { - timeline?.paginate(direction) + timeline?.paginate(direction) ?: Timber.w("Timeline is not ready yet, cannot load more media items for room ${room.roomId}") } override suspend fun deleteItem(eventId: EventId) { timeline?.redactEvent( eventOrTransactionId = eventId.toEventOrTransactionId(), reason = null, - ) + ) ?: Timber.w("Timeline is not ready yet, cannot delete media item with eventId $eventId for room ${room.roomId}") } } diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryPresenter.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryPresenter.kt index e7caa12f2e..2c2fb31020 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryPresenter.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryPresenter.kt @@ -94,7 +94,9 @@ class MediaGalleryPresenter( mode = event.mode } is MediaGalleryEvent.LoadMore -> coroutineScope.launch { - mediaGalleryDataSource.loadMore(event.direction) + if (mediaGalleryDataSource.isReady) { + mediaGalleryDataSource.loadMore(event.direction) + } } is MediaGalleryEvent.Delete -> coroutineScope.launch { mediaGalleryDataSource.deleteItem(event.eventId) diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSource.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSource.kt index 01ec68a336..b0b81bbde0 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSource.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSource.kt @@ -11,12 +11,13 @@ package io.element.android.libraries.mediaviewer.impl.viewer import androidx.annotation.VisibleForTesting import androidx.compose.runtime.Composable import androidx.compose.runtime.MutableState +import androidx.compose.runtime.ProduceStateScope import androidx.compose.runtime.State -import androidx.compose.runtime.collectAsState import androidx.compose.runtime.mutableStateOf -import androidx.compose.runtime.remember +import androidx.compose.runtime.produceState import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.core.extensions.mapCatchingExceptions +import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.media.MatrixMediaLoader import io.element.android.libraries.matrix.api.media.MediaFile import io.element.android.libraries.matrix.api.media.MediaSource @@ -37,14 +38,17 @@ import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.SharingStarted +import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.stateIn import kotlinx.coroutines.withContext import timber.log.Timber import java.util.concurrent.ConcurrentHashMap class MediaViewerDataSource( mode: MediaViewerMode, + coroutineScope: CoroutineScope, private val dispatcher: CoroutineDispatcher, private val galleryDataSource: MediaGalleryDataSource, private val mediaLoader: MatrixMediaLoader, @@ -76,40 +80,58 @@ class MediaViewerDataSource( localMediaStates.clear() } + /** + * Helper function to translate the [dataFlow] result to a Compose [State] that can be observed in the UI. + */ @Composable - fun collectAsState(): State> { - return remember { dataFlow() }.collectAsState(initialData()) + fun produceState( + producer: suspend ProduceStateScope>.(StateFlow>) -> Unit + ): State> { + return produceState(initialValue = initialData()) { + producer(dataFlow) + } + } + + /** + * Find the index of the page corresponding to the given eventId, or null if not found. + */ + fun findEventIndex(eventId: EventId?): Int? { + if (eventId == null) return null + return dataFlow.value.indexOfFirst { (it as? MediaViewerPageData.MediaViewerData)?.eventId == eventId }.takeIf { it >= 0 } } @VisibleForTesting - internal fun dataFlow(): Flow> { - return galleryDataSource.groupedMediaItemsFlow() - .map { groupedItems -> - when (groupedItems) { - AsyncData.Uninitialized, - is AsyncData.Loading -> { - persistentListOf( - MediaViewerPageData.Loading( - direction = Timeline.PaginationDirection.BACKWARDS, - timestamp = systemClock.epochMillis(), - pagerKey = Long.MIN_VALUE, - ) + internal val dataFlow: StateFlow> = galleryDataSource.groupedMediaItemsFlow() + .map { groupedItems -> + when (groupedItems) { + AsyncData.Uninitialized, + is AsyncData.Loading -> { + persistentListOf( + MediaViewerPageData.Loading( + direction = Timeline.PaginationDirection.BACKWARDS, + timestamp = systemClock.epochMillis(), + pagerKey = Long.MIN_VALUE, ) - } - is AsyncData.Failure -> { - persistentListOf( - MediaViewerPageData.Failure(groupedItems.error), - ) - } - is AsyncData.Success -> { - withContext(dispatcher) { - val mediaItems = groupedItems.data.getItems(galleryMode) - buildMediaViewerPageList(mediaItems) - } + ) + } + is AsyncData.Failure -> { + persistentListOf( + MediaViewerPageData.Failure(groupedItems.error), + ) + } + is AsyncData.Success -> { + withContext(dispatcher) { + val mediaItems = groupedItems.data.getItems(galleryMode) + buildMediaViewerPageList(mediaItems) } } } - } + } + .stateIn( + scope = CoroutineScope(coroutineScope.coroutineContext + dispatcher), + started = SharingStarted.Lazily, + initialValue = initialData(), + ) private fun initialData(): ImmutableList { val initialMediaItems = @@ -148,7 +170,7 @@ class MediaViewerDataSource( is MediaItem.LoadingIndicator -> add( MediaViewerPageData.Loading( direction = mediaItem.direction, - timestamp = systemClock.epochMillis(), + timestamp = mediaItem.timestamp, pagerKey = pagerKeysHandler.getKey(mediaItem), ) ) @@ -161,7 +183,9 @@ class MediaViewerDataSource( } suspend fun loadMore(direction: Timeline.PaginationDirection) { - galleryDataSource.loadMore(direction) + if (galleryDataSource.isReady) { + galleryDataSource.loadMore(direction) + } } suspend fun loadMedia(data: MediaViewerPageData.MediaViewerData) { diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerNode.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerNode.kt index d834534e3e..e594daeb41 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerNode.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerNode.kt @@ -13,6 +13,7 @@ import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.runtime.remember import androidx.compose.ui.Modifier +import androidx.lifecycle.lifecycleScope import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin @@ -118,6 +119,7 @@ class MediaViewerNode( navigator = this, dataSource = MediaViewerDataSource( mode = inputs.mode, + coroutineScope = lifecycleScope, dispatcher = coroutineDispatchers.computation, galleryDataSource = mediaGallerySource, mediaLoader = mediaLoader, diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenter.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenter.kt index d40026dd37..296f20ef4c 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenter.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenter.kt @@ -14,14 +14,12 @@ import androidx.compose.runtime.DisposableEffect import androidx.compose.runtime.IntState import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.State -import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope import androidx.compose.runtime.setValue -import androidx.compose.runtime.snapshotFlow import dev.zacsweers.metro.Assisted import dev.zacsweers.metro.AssistedFactory import dev.zacsweers.metro.AssistedInject @@ -45,10 +43,7 @@ import io.element.android.libraries.mediaviewer.impl.model.mediaPermissions import io.element.android.libraries.ui.strings.CommonStrings import kotlinx.collections.immutable.ImmutableList import kotlinx.coroutines.CoroutineScope -import kotlinx.coroutines.flow.distinctUntilChanged -import kotlinx.coroutines.flow.filter -import kotlinx.coroutines.flow.launchIn -import kotlinx.coroutines.flow.onEach +import kotlinx.coroutines.flow.collectLatest import kotlinx.coroutines.launch import io.element.android.libraries.androidutils.R as UtilsR @@ -75,12 +70,27 @@ class MediaViewerPresenter( @Composable override fun present(): MediaViewerState { val coroutineScope = rememberCoroutineScope() - val data = dataSource.collectAsState() - val currentIndex = remember { mutableIntStateOf(searchIndex(data.value, inputs.eventId)) } + val currentIndex = remember { mutableIntStateOf(dataSource.findEventIndex(inputs.eventId) ?: 0) } + val data = dataSource.produceState { flow -> + flow.collectLatest { new -> + val existingItem = value.getOrNull(currentIndex.intValue) + val newItem = new.getOrNull(currentIndex.intValue) + if (existingItem is MediaViewerPageData.MediaViewerData && existingItem.eventId == inputs.eventId && newItem != existingItem) { + currentIndex.intValue = dataSource.findEventIndex(inputs.eventId) ?: 0 + } else if (currentIndex.intValue > 0 && value.firstOrNull() is MediaViewerPageData.Loading && + new.firstOrNull() !is MediaViewerPageData.Loading) { + // Restore index based on the eventId after the initial items have been loaded + currentIndex.intValue = dataSource.findEventIndex(inputs.eventId) ?: 0 + } + value = new + } + } + val snackbarMessage by snackbarDispatcher.collectSnackbarMessageAsState() - NoMoreItemsBackwardSnackBarDisplayer(currentIndex, data) - NoMoreItemsForwardSnackBarDisplayer(currentIndex, data) + // Add both forward and backward pagination state checks to display a snackbar when there is no more items to load in either direction + NoMoreItemsSnackBarDisplayer(currentIndex, data, Timeline.PaginationDirection.FORWARDS) + NoMoreItemsSnackBarDisplayer(currentIndex, data, Timeline.PaginationDirection.BACKWARDS) val permissions by room.permissionsAsState(MediaPermissions.DEFAULT) { perms -> perms.mediaPermissions() @@ -176,52 +186,39 @@ class MediaViewerPresenter( } @Composable - private fun NoMoreItemsBackwardSnackBarDisplayer( + private fun NoMoreItemsSnackBarDisplayer( currentIndex: IntState, data: State>, + direction: Timeline.PaginationDirection, ) { - // With newest-first ordering, backward loading indicator is at the last index - val isRenderingLoadingBackward by remember { - derivedStateOf { - currentIndex.intValue == data.value.lastIndex && - data.value.size > 1 && - data.value.lastOrNull() is MediaViewerPageData.Loading + var previousIndex by remember { mutableIntStateOf(currentIndex.intValue) } + var previousDataSize by remember { mutableIntStateOf(data.value.size) } + var wasLoading: Boolean? by remember { mutableStateOf(null) } + LaunchedEffect(currentIndex.intValue, data.value) { + fun isLoading(index: Int, data: List, direction: Timeline.PaginationDirection): Boolean { + return when (direction) { + Timeline.PaginationDirection.BACKWARDS -> index == data.lastIndex && data.lastOrNull() is MediaViewerPageData.Loading + Timeline.PaginationDirection.FORWARDS -> index == 0 && data.firstOrNull() is MediaViewerPageData.Loading + } } - } - if (isRenderingLoadingBackward) { - LaunchedEffect(Unit) { - // Observe the loading data vanishing - snapshotFlow { data.value.lastOrNull() is MediaViewerPageData.Loading } - .distinctUntilChanged() - .filter { !it } - .onEach { showNoMoreItemsSnackbar() } - .launchIn(this) + // Reset the effect when the user navigate to another item so we only take into account index changes caused by data changes + if (previousIndex != currentIndex.intValue) { + wasLoading = null + previousIndex = currentIndex.intValue + } + // If we were navigating backwards and the data size grew, we can discard the previous value: it means we received new items + if (direction == Timeline.PaginationDirection.BACKWARDS && previousDataSize < data.value.size) { + wasLoading = null } - } - } - @Composable - private fun NoMoreItemsForwardSnackBarDisplayer( - currentIndex: IntState, - data: State>, - ) { - // With newest-first ordering, forward loading indicator is at the first index - val isRenderingLoadingForward by remember { - derivedStateOf { - currentIndex.intValue == 0 && - data.value.size > 1 && - data.value.firstOrNull() is MediaViewerPageData.Loading - } - } - if (isRenderingLoadingForward) { - LaunchedEffect(Unit) { - // Observe the loading data vanishing - snapshotFlow { data.value.firstOrNull() is MediaViewerPageData.Loading } - .distinctUntilChanged() - .filter { !it } - .onEach { showNoMoreItemsSnackbar() } - .launchIn(this) + val isLoading = isLoading(currentIndex.intValue, data.value, direction) + + if (wasLoading == true && !isLoading) { + showNoMoreItemsSnackbar() } + + previousDataSize = data.value.size + wasLoading = isLoading } } @@ -293,13 +290,4 @@ class MediaViewerPresenter( CommonStrings.error_unknown } } - - private fun searchIndex(data: List, eventId: EventId?): Int { - if (eventId == null) { - return 0 - } - return data.indexOfFirst { - (it as? MediaViewerPageData.MediaViewerData)?.eventId == eventId - }.coerceAtLeast(0) - } } diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerState.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerState.kt index 8a4da2aac3..a51d3c44e9 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerState.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerState.kt @@ -8,6 +8,7 @@ package io.element.android.libraries.mediaviewer.impl.viewer +import androidx.compose.runtime.Immutable import androidx.compose.runtime.State import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.designsystem.utils.snackbar.SnackbarMessage @@ -29,6 +30,7 @@ data class MediaViewerState( val eventSink: (MediaViewerEvent) -> Unit, ) +@Immutable sealed interface MediaViewerPageData { val pagerKey: Long diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerView.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerView.kt index 5e9ced7d77..d52db124ea 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerView.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerView.kt @@ -44,7 +44,6 @@ import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.rememberUpdatedState import androidx.compose.runtime.setValue -import androidx.compose.runtime.snapshotFlow import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier import androidx.compose.ui.draw.alpha @@ -179,9 +178,11 @@ fun MediaViewerView( val pagerState = rememberPagerState(state.currentIndex, 0f) { state.listData.size } - LaunchedEffect(pagerState) { - snapshotFlow { pagerState.currentPage }.collect { page -> - state.eventSink(MediaViewerEvent.OnNavigateTo(page)) + + LaunchedEffect(pagerState.targetPage, state.currentIndex) { + // Only emit an index navigation change when it's triggered by the user scrolling + if (pagerState.targetPage != state.currentIndex && pagerState.isScrollInProgress) { + state.eventSink(MediaViewerEvent.OnNavigateTo(pagerState.targetPage)) } } HorizontalPager( diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/SingleMediaGalleryDataSource.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/SingleMediaGalleryDataSource.kt index 7bbf397171..146ace8620 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/SingleMediaGalleryDataSource.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/SingleMediaGalleryDataSource.kt @@ -30,6 +30,8 @@ class SingleMediaGalleryDataSource( override fun groupedMediaItemsFlow() = flowOf(AsyncData.Success(data)) override fun getLastData(): AsyncData = AsyncData.Success(data) + override val isReady: Boolean = true + override suspend fun loadMore(direction: Timeline.PaginationDirection) = Unit override suspend fun deleteItem(eventId: EventId) = Unit diff --git a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/DefaultMediaViewerEntryPointTest.kt b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/DefaultMediaViewerEntryPointTest.kt index b848ea2a7b..e7d45f433a 100644 --- a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/DefaultMediaViewerEntryPointTest.kt +++ b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/DefaultMediaViewerEntryPointTest.kt @@ -33,16 +33,21 @@ import io.element.android.tests.testutils.lambda.lambdaError import io.element.android.tests.testutils.node.TestParentNode import io.element.android.tests.testutils.testCoroutineDispatchers import io.mockk.mockk +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain import org.junit.Rule import org.junit.Test +@OptIn(ExperimentalCoroutinesApi::class) class DefaultMediaViewerEntryPointTest { @get:Rule val instantTaskExecutorRule = InstantTaskExecutorRule() @Test fun `test node builder`() = runTest { + Dispatchers.setMain(testCoroutineDispatchers().main) val entryPoint = DefaultMediaViewerEntryPoint() val mockMediaUri: Uri = mockk("localMediaUri") val localMediaFactory = FakeLocalMediaFactory(mockMediaUri) @@ -89,6 +94,7 @@ class DefaultMediaViewerEntryPointTest { @Test fun `test node builder avatar`() = runTest { + Dispatchers.setMain(testCoroutineDispatchers().main) val entryPoint = DefaultMediaViewerEntryPoint() val mockMediaUri: Uri = mockk("localMediaUri") val localMediaFactory = FakeLocalMediaFactory(mockMediaUri) diff --git a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/datasource/FakeMediaGalleryDataSource.kt b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/datasource/FakeMediaGalleryDataSource.kt index 4a64f02a33..5839f33ce6 100644 --- a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/datasource/FakeMediaGalleryDataSource.kt +++ b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/datasource/FakeMediaGalleryDataSource.kt @@ -15,18 +15,20 @@ import io.element.android.libraries.mediaviewer.impl.model.GroupedMediaItems import io.element.android.tests.testutils.lambda.lambdaError import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.MutableSharedFlow +import kotlinx.coroutines.flow.MutableStateFlow class FakeMediaGalleryDataSource( + initialData: AsyncData = AsyncData.Uninitialized, + private val isReadyResult: () -> Boolean = { true }, private val startLambda: () -> Unit = { lambdaError() }, private val loadMoreLambda: (Timeline.PaginationDirection) -> Unit = { lambdaError() }, private val deleteItemLambda: (EventId) -> Unit = { lambdaError() }, - ) : MediaGalleryDataSource { +) : MediaGalleryDataSource { override fun start(coroutineScope: CoroutineScope) = startLambda() - private val groupedMediaItemsFlow = MutableSharedFlow>( - replay = 1 - ) + private val groupedMediaItemsFlow = MutableStateFlow(initialData) + + override val isReady: Boolean get() = isReadyResult() override fun groupedMediaItemsFlow(): Flow> { return groupedMediaItemsFlow diff --git a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSourceTest.kt b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSourceTest.kt index c3f1ab9a0d..d7ba547ee4 100644 --- a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSourceTest.kt +++ b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerDataSourceTest.kt @@ -62,15 +62,20 @@ class MediaViewerDataSourceTest { @Test fun `test dataFlow uninitialized, loading and error`() = runTest { - val galleryDataSource = FakeMediaGalleryDataSource() + val galleryDataSource = FakeMediaGalleryDataSource( + initialData = AsyncData.Uninitialized, + ) val sut = createMediaViewerDataSource( galleryDataSource = galleryDataSource, ) - sut.dataFlow().test { + sut.dataFlow.test { + // The flow starts with an empty result + assertThat(awaitItem()).isEmpty() galleryDataSource.emitGroupedMediaItems(AsyncData.Uninitialized) assertThat(awaitItem().first()).isInstanceOf(MediaViewerPageData.Loading::class.java) galleryDataSource.emitGroupedMediaItems(AsyncData.Loading()) - assertThat(awaitItem().first()).isInstanceOf(MediaViewerPageData.Loading::class.java) + // No items emitted, we were already loading data + ensureAllEventsConsumed() galleryDataSource.emitGroupedMediaItems(AsyncData.Failure(AN_EXCEPTION)) assertThat(awaitItem().first()).isEqualTo(MediaViewerPageData.Failure(AN_EXCEPTION)) } @@ -82,7 +87,7 @@ class MediaViewerDataSourceTest { val sut = createMediaViewerDataSource( galleryDataSource = galleryDataSource, ) - sut.dataFlow().test { + sut.dataFlow.test { galleryDataSource.emitGroupedMediaItems( AsyncData.Success( aGroupedMediaItems( @@ -102,7 +107,8 @@ class MediaViewerDataSourceTest { val sut = createMediaViewerDataSource( galleryDataSource = galleryDataSource, ) - sut.dataFlow().test { + sut.dataFlow.test { + skipItems(1) galleryDataSource.emitGroupedMediaItems( AsyncData.Success( aGroupedMediaItems( @@ -141,7 +147,8 @@ class MediaViewerDataSourceTest { mode = MediaViewerMode.TimelineImagesAndVideos(timelineMode = Timeline.Mode.Media), galleryDataSource = galleryDataSource, ) - sut.dataFlow().test { + sut.dataFlow.test { + skipItems(1) galleryDataSource.emitGroupedMediaItems( AsyncData.Success( aGroupedMediaItems( @@ -163,7 +170,8 @@ class MediaViewerDataSourceTest { mode = MediaViewerMode.TimelineFilesAndAudios(timelineMode = Timeline.Mode.Media), galleryDataSource = galleryDataSource, ) - sut.dataFlow().test { + sut.dataFlow.test { + skipItems(1) galleryDataSource.emitGroupedMediaItems( AsyncData.Success( aGroupedMediaItems( @@ -184,7 +192,8 @@ class MediaViewerDataSourceTest { val sut = createMediaViewerDataSource( galleryDataSource = galleryDataSource, ) - sut.dataFlow().test { + sut.dataFlow.test { + skipItems(1) galleryDataSource.emitGroupedMediaItems( AsyncData.Success( aGroupedMediaItems( @@ -217,7 +226,8 @@ class MediaViewerDataSourceTest { val sut = createMediaViewerDataSource( galleryDataSource = galleryDataSource, ) - sut.dataFlow().test { + sut.dataFlow.test { + skipItems(1) galleryDataSource.emitGroupedMediaItems( AsyncData.Success( aGroupedMediaItems( @@ -241,7 +251,8 @@ class MediaViewerDataSourceTest { galleryDataSource = galleryDataSource, mediaLoader = mediaLoader, ) - sut.dataFlow().test { + sut.dataFlow.test { + skipItems(1) galleryDataSource.emitGroupedMediaItems( AsyncData.Success( aGroupedMediaItems( @@ -271,6 +282,7 @@ class MediaViewerDataSourceTest { mediaLoader: MatrixMediaLoader = FakeMatrixMediaLoader(), localMediaFactory: LocalMediaFactory = FakeLocalMediaFactory(mockMediaUrl), ) = MediaViewerDataSource( + coroutineScope = backgroundScope, mode = mode, dispatcher = testCoroutineDispatchers().computation, galleryDataSource = galleryDataSource, diff --git a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenterTest.kt b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenterTest.kt index 86689859ad..459d30b8d5 100644 --- a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenterTest.kt +++ b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerPresenterTest.kt @@ -21,6 +21,7 @@ import io.element.android.libraries.matrix.api.timeline.Timeline import io.element.android.libraries.matrix.api.timeline.item.event.EventOrTransactionId import io.element.android.libraries.matrix.api.timeline.item.event.toEventOrTransactionId import io.element.android.libraries.matrix.test.AN_EVENT_ID +import io.element.android.libraries.matrix.test.AN_EVENT_ID_2 import io.element.android.libraries.matrix.test.A_SESSION_ID_2 import io.element.android.libraries.matrix.test.A_USER_ID import io.element.android.libraries.matrix.test.media.FakeMatrixMediaLoader @@ -51,6 +52,8 @@ import io.mockk.mockk import kotlinx.collections.immutable.persistentListOf import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.advanceUntilIdle +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test @@ -97,6 +100,8 @@ class MediaViewerPresenterTest { assertThat(initialState.snackbarMessage).isNull() assertThat(initialState.canShowInfo).isTrue() assertThat(initialState.mediaBottomSheetState).isEqualTo(MediaBottomSheetState.Hidden) + + cancelAndIgnoreRemainingEvents() } } @@ -120,6 +125,8 @@ class MediaViewerPresenterTest { assertThat(initialState.snackbarMessage).isNull() assertThat(initialState.canShowInfo).isFalse() assertThat(initialState.mediaBottomSheetState).isEqualTo(MediaBottomSheetState.Hidden) + + cancelAndIgnoreRemainingEvents() } } @@ -143,6 +150,8 @@ class MediaViewerPresenterTest { assertThat(initialState.snackbarMessage).isNull() assertThat(initialState.canShowInfo).isTrue() assertThat(initialState.mediaBottomSheetState).isEqualTo(MediaBottomSheetState.Hidden) + + cancelAndIgnoreRemainingEvents() } } @@ -167,6 +176,8 @@ class MediaViewerPresenterTest { assertThat(initialState.snackbarMessage).isNull() assertThat(initialState.canShowInfo).isTrue() assertThat(initialState.mediaBottomSheetState).isEqualTo(MediaBottomSheetState.Hidden) + + cancelAndIgnoreRemainingEvents() } } @@ -555,6 +566,8 @@ class MediaViewerPresenterTest { ) val finalState = awaitItem() assertThat(finalState.currentIndex).isEqualTo(1) + + cancelAndIgnoreRemainingEvents() } } @@ -578,32 +591,34 @@ class MediaViewerPresenterTest { mode: MediaViewerEntryPoint.MediaViewerMode, expectedSnackbarResId: Int, ) = runTest { + val image = anImage.copy(eventId = AN_EVENT_ID) val mediaGalleryDataSource = FakeMediaGalleryDataSource( + initialData = AsyncData.Success( + if (mode is MediaViewerEntryPoint.MediaViewerMode.TimelineFilesAndAudios) { + GroupedMediaItems( + imageAndVideoItems = persistentListOf(), + fileItems = persistentListOf(aForwardLoadingIndicator, image, aBackwardLoadingIndicator), + ) + } else { + GroupedMediaItems( + imageAndVideoItems = persistentListOf(aForwardLoadingIndicator, image, aBackwardLoadingIndicator), + fileItems = persistentListOf(), + ) + } + ), startLambda = { }, ) val presenter = createMediaViewerPresenter( + eventId = AN_EVENT_ID, localMediaFactory = localMediaFactory, mode = mode, mediaGalleryDataSource = mediaGalleryDataSource, ) presenter.test { - awaitFirstItem() - mediaGalleryDataSource.emitGroupedMediaItems( - AsyncData.Success( - if (mode is MediaViewerEntryPoint.MediaViewerMode.TimelineFilesAndAudios) { - GroupedMediaItems( - imageAndVideoItems = persistentListOf(), - fileItems = persistentListOf(aForwardLoadingIndicator, anImage, aBackwardLoadingIndicator), - ) - } else { - GroupedMediaItems( - imageAndVideoItems = persistentListOf(aForwardLoadingIndicator, anImage, aBackwardLoadingIndicator), - fileItems = persistentListOf(), - ) - } - ) - ) - val updatedState = awaitItem() + val updatedState = awaitFirstItem() + + advanceUntilIdle() + runCurrent() // User navigate to the first item (forward loading indicator) updatedState.eventSink( MediaViewerEvent.OnNavigateTo(0) @@ -614,17 +629,17 @@ class MediaViewerPresenterTest { if (mode is MediaViewerEntryPoint.MediaViewerMode.TimelineFilesAndAudios) { GroupedMediaItems( imageAndVideoItems = persistentListOf(), - fileItems = persistentListOf(anImage, aBackwardLoadingIndicator), + fileItems = persistentListOf(image, aBackwardLoadingIndicator), ) } else { GroupedMediaItems( - imageAndVideoItems = persistentListOf(anImage, aBackwardLoadingIndicator), + imageAndVideoItems = persistentListOf(image, aBackwardLoadingIndicator), fileItems = persistentListOf(), ) } ) ) - skipItems(1) + skipItems(2) val stateWithSnackbar = awaitItem() assertThat(stateWithSnackbar.snackbarMessage!!.messageResId).isEqualTo(expectedSnackbarResId) } @@ -707,21 +722,19 @@ class MediaViewerPresenterTest { fun `present - no snackbar displayed when there is no more items but not displaying a loading item`() = runTest { val mediaGalleryDataSource = FakeMediaGalleryDataSource( startLambda = { }, + initialData = AsyncData.Success( + GroupedMediaItems( + imageAndVideoItems = persistentListOf(aForwardLoadingIndicator, anImage, anImage.copy(eventId = AN_EVENT_ID_2), aBackwardLoadingIndicator), + fileItems = persistentListOf(), + ) + ) ) val presenter = createMediaViewerPresenter( + eventId = AN_EVENT_ID, localMediaFactory = localMediaFactory, mediaGalleryDataSource = mediaGalleryDataSource, ) presenter.test { - awaitFirstItem() - mediaGalleryDataSource.emitGroupedMediaItems( - AsyncData.Success( - GroupedMediaItems( - imageAndVideoItems = persistentListOf(aForwardLoadingIndicator, anImage, aBackwardLoadingIndicator), - fileItems = persistentListOf(), - ) - ) - ) val updatedState = awaitItem() // User navigate to the media updatedState.eventSink( @@ -774,6 +787,51 @@ class MediaViewerPresenterTest { } } + @Test + fun `present - receiving loading items with different timestamps emits different items too`() = runTest { + val loadMoreLambda = lambdaRecorder { } + val mediaGalleryDataSource = FakeMediaGalleryDataSource( + startLambda = { }, + loadMoreLambda = loadMoreLambda, + ) + val presenter = createMediaViewerPresenter( + localMediaFactory = localMediaFactory, + mediaGalleryDataSource = mediaGalleryDataSource, + ) + val anImage = aMediaItemImage( + mediaSourceUrl = aUrl, + ) + presenter.test { + awaitFirstItem() + mediaGalleryDataSource.emitGroupedMediaItems( + AsyncData.Success( + GroupedMediaItems( + imageAndVideoItems = persistentListOf(aForwardLoadingIndicator, anImage, aBackwardLoadingIndicator), + fileItems = persistentListOf(), + ) + ) + ) + val updatedState = awaitItem() + + // Get the exact same items, but with new timestamps for the loading indicators + mediaGalleryDataSource.emitGroupedMediaItems( + AsyncData.Success( + GroupedMediaItems( + imageAndVideoItems = persistentListOf( + aForwardLoadingIndicator.copy(timestamp = 1234L), + anImage, + aBackwardLoadingIndicator.copy(timestamp = 1234L), + ), + fileItems = persistentListOf(), + ) + ) + ) + + // We should get a new list of items, which should not be equal to the previous one + assertThat(updatedState.listData).isNotEqualTo(awaitItem().listData) + } + } + @Test fun `present - view in timeline hides the bottom sheet and invokes the navigator`() = runTest { val onViewInTimelineClickLambda = lambdaRecorder { } @@ -794,6 +852,7 @@ class MediaViewerPresenterTest { presenter.test { val initialState = awaitItem() initialState.eventSink(MediaViewerEvent.OpenInfo(aMediaViewerPageData())) + skipItems(1) val withBottomSheetState = awaitItem() assertThat(withBottomSheetState.mediaBottomSheetState).isInstanceOf(MediaBottomSheetState.Details::class.java) initialState.eventSink(MediaViewerEvent.ViewInTimeline(AN_EVENT_ID)) @@ -823,6 +882,7 @@ class MediaViewerPresenterTest { presenter.test { val initialState = awaitItem() initialState.eventSink(MediaViewerEvent.OpenInfo(aMediaViewerPageData())) + skipItems(1) val withBottomSheetState = awaitItem() assertThat(withBottomSheetState.mediaBottomSheetState).isInstanceOf(MediaBottomSheetState.Details::class.java) initialState.eventSink(MediaViewerEvent.Forward(AN_EVENT_ID)) @@ -854,6 +914,7 @@ class MediaViewerPresenterTest { presenter.test { val initialState = awaitItem() initialState.eventSink(MediaViewerEvent.OpenInfo(aMediaViewerPageData())) + skipItems(1) val withBottomSheetState = awaitItem() assertThat(withBottomSheetState.mediaBottomSheetState).isInstanceOf(MediaBottomSheetState.Details::class.java) initialState.eventSink(MediaViewerEvent.Forward(AN_EVENT_ID)) @@ -892,6 +953,7 @@ internal fun TestScope.createMediaViewerPresenter( ), navigator = mediaViewerNavigator, dataSource = MediaViewerDataSource( + coroutineScope = backgroundScope, mode = mode, dispatcher = testCoroutineDispatchers().computation, galleryDataSource = mediaGalleryDataSource, diff --git a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerViewTest.kt b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerViewTest.kt index b426ca50ca..6521f450f5 100644 --- a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerViewTest.kt +++ b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/viewer/MediaViewerViewTest.kt @@ -13,6 +13,11 @@ package io.element.android.libraries.mediaviewer.impl.viewer import android.net.Uri import androidx.activity.ComponentActivity import androidx.annotation.StringRes +import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue import androidx.compose.ui.test.AndroidComposeUiTest import androidx.compose.ui.test.ExperimentalTestApi import androidx.compose.ui.test.assertHasClickAction @@ -23,6 +28,7 @@ import androidx.compose.ui.test.swipeDown import androidx.compose.ui.test.v2.runAndroidComposeUiTest import androidx.test.ext.junit.runners.AndroidJUnit4 import io.element.android.libraries.architecture.AsyncData +import io.element.android.libraries.matrix.api.timeline.Timeline import io.element.android.libraries.mediaviewer.impl.details.aMediaBottomSheetStateDetails import io.element.android.libraries.mediaviewer.test.viewer.aLocalMedia import io.element.android.libraries.ui.strings.CommonStrings @@ -33,9 +39,11 @@ import io.element.android.tests.testutils.ensureCalledOnce import io.element.android.tests.testutils.pressBack import io.element.android.tests.testutils.setSafeContent import io.mockk.mockk +import kotlinx.coroutines.delay import org.junit.Test import org.junit.runner.RunWith import org.robolectric.annotation.Config +import kotlin.time.Duration.Companion.milliseconds @RunWith(AndroidJUnit4::class) class MediaViewerViewTest { @@ -60,7 +68,6 @@ class MediaViewerViewTest { } eventsRecorder.assertList( listOf( - MediaViewerEvent.OnNavigateTo(0), MediaViewerEvent.LoadMedia(state.listData.first() as MediaViewerPageData.MediaViewerData), ) ) @@ -122,7 +129,6 @@ class MediaViewerViewTest { onNodeWithContentDescription(contentDescription).performClick() eventsRecorder.assertList( listOf( - MediaViewerEvent.OnNavigateTo(0), MediaViewerEvent.LoadMedia(data), expectedEvent, ) @@ -178,7 +184,6 @@ class MediaViewerViewTest { clickOn(textRes) eventsRecorder.assertList( listOf( - MediaViewerEvent.OnNavigateTo(0), MediaViewerEvent.LoadMedia(data), expectedEvent, ) @@ -208,7 +213,6 @@ class MediaViewerViewTest { .assertDoesNotExist() eventsRecorder.assertList( listOf( - MediaViewerEvent.OnNavigateTo(0), MediaViewerEvent.LoadMedia(state.listData.first() as MediaViewerPageData.MediaViewerData), ) ) @@ -231,7 +235,6 @@ class MediaViewerViewTest { } eventsRecorder.assertList( listOf( - MediaViewerEvent.OnNavigateTo(0), MediaViewerEvent.LoadMedia(state.listData.first() as MediaViewerPageData.MediaViewerData), ) ) @@ -256,7 +259,6 @@ class MediaViewerViewTest { clickOn(CommonStrings.action_retry) eventsRecorder.assertList( listOf( - MediaViewerEvent.OnNavigateTo(0), MediaViewerEvent.LoadMedia(data), MediaViewerEvent.LoadMedia(data), ) @@ -282,12 +284,65 @@ class MediaViewerViewTest { clickOn(CommonStrings.action_cancel) eventsRecorder.assertList( listOf( - MediaViewerEvent.OnNavigateTo(0), MediaViewerEvent.LoadMedia(data), MediaViewerEvent.ClearLoadingError(data) ) ) } + + @Test + fun `loading event after an error triggers load more Event`() = runAndroidComposeUiTest { + val eventsRecorder = EventsRecorder() + val states = listOf( + aMediaViewerState( + listData = listOf(aMediaViewerPageDataLoading(timestamp = 0L)), + eventSink = eventsRecorder, + ), + aMediaViewerState( + listData = listOf(MediaViewerPageData.Failure(IllegalStateException("error"))), + eventSink = eventsRecorder, + ), + aMediaViewerState( + listData = listOf(aMediaViewerPageDataLoading(timestamp = 0L)), + eventSink = eventsRecorder, + ), + // This one should be ignored since it has the same timestamp as the last one, it should not trigger a recomposition + aMediaViewerState( + listData = listOf(aMediaViewerPageDataLoading(timestamp = 0L)), + eventSink = eventsRecorder, + ), + ) + setSafeContent { + // Iterate over the states with a delay to give the view some time to trigger the `LoadMore` Event + var state by remember { mutableStateOf(states.first()) } + LaunchedEffect(Unit) { + val iterator = states.iterator() + while (iterator.hasNext()) { + delay(200.milliseconds) + state = iterator.next() + } + } + MediaViewerView( + state = state, + textFileViewer = { _, _ -> }, + onBackClick = EnsureNeverCalled(), + audioFocus = null, + ) + } + + // Advance time to let the states update + mainClock.advanceTimeBy(3_000) + + // `LoadMore` should be called twice, once for the first loading state, and once for the second one even though they have the same timestamp because + // of the intermediate error state. + // The third one will be ignored since it has the same timestamp as the second one and it'll be discarded by the Compose's equality diffing. + eventsRecorder.assertList( + listOf( + MediaViewerEvent.LoadMore(direction = Timeline.PaginationDirection.BACKWARDS), + MediaViewerEvent.LoadMore(direction = Timeline.PaginationDirection.BACKWARDS), + ) + ) + } } private fun AndroidComposeUiTest.setMediaViewerView(