From 42c141109fa4b382010b5c41135c5751a276832c Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Wed, 20 May 2026 11:32:32 +0200 Subject: [PATCH] Fix media viewer flickering (#6800) * Fix media viewer flickering This was caused by the data being loaded triggering an index update that got out of sync with the displayed items, and for an instant, the pager index pointed to the wrong data until it was refreshed * Reuse the same 'displayer' function for both forwards and backwards pagination * Make `dataFlow` a property so we don't create a new instance every time we access it * Remove `pageDataComparator` as it prevented new items from being loaded when a pagination returned no valid items to display but has more items to load Make sure we modify the current index when loading new data only if it was pointing to the input event id. * Fix `MediaViewerDataSource` overriding the provided timestamp for `Loading` items Test emitting different loading items from the data source results in the state displaying the different items, so they will trigger a new pagination attempt * Add regression test to check loading -> error -> loading states will still trigger 2 separate `LoadMore` events --- .../impl/datasource/MediaGalleryDataSource.kt | 12 +- .../impl/gallery/MediaGalleryPresenter.kt | 4 +- .../impl/viewer/MediaViewerDataSource.kt | 86 ++++++++----- .../impl/viewer/MediaViewerNode.kt | 2 + .../impl/viewer/MediaViewerPresenter.kt | 104 +++++++-------- .../impl/viewer/MediaViewerState.kt | 2 + .../impl/viewer/MediaViewerView.kt | 9 +- .../viewer/SingleMediaGalleryDataSource.kt | 2 + .../impl/DefaultMediaViewerEntryPointTest.kt | 6 + .../datasource/FakeMediaGalleryDataSource.kt | 12 +- .../impl/viewer/MediaViewerDataSourceTest.kt | 32 +++-- .../impl/viewer/MediaViewerPresenterTest.kt | 120 +++++++++++++----- .../impl/viewer/MediaViewerViewTest.kt | 69 +++++++++- 13 files changed, 312 insertions(+), 148 deletions(-) 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(