From a4a8fc2268508a2420fe238d8a8f898aa4b9b630 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 10 Dec 2024 14:39:10 +0100 Subject: [PATCH] Media timeline: improve pagination logic. --- .../matrix/impl/timeline/RustTimeline.kt | 57 ++++++++----------- .../impl/gallery/MediaGalleryPresenter.kt | 8 --- .../impl/gallery/TimelineMediaItemsFactory.kt | 25 -------- .../gallery/FakeTimelineMediaItemsFactory.kt | 5 -- .../impl/gallery/MediaGalleryPresenterTest.kt | 1 - 5 files changed, 23 insertions(+), 73 deletions(-) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt index 1a9da807a0..c8e8b77b32 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustTimeline.kt @@ -56,6 +56,7 @@ import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.filter import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.getAndUpdate import kotlinx.coroutines.flow.launchIn @@ -64,8 +65,6 @@ import kotlinx.coroutines.flow.onCompletion import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.launch -import kotlinx.coroutines.sync.Mutex -import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext import org.matrix.rustcomponents.sdk.EditedContent import org.matrix.rustcomponents.sdk.FormattedBody @@ -172,36 +171,26 @@ class RustTimeline( } } - private val backwardsPaginationMutex = Mutex() - private val forwardsPaginationMutex = Mutex() - - private fun getPaginationMutex(direction: Timeline.PaginationDirection) = when (direction) { - Timeline.PaginationDirection.BACKWARDS -> backwardsPaginationMutex - Timeline.PaginationDirection.FORWARDS -> forwardsPaginationMutex - } - // Use NonCancellable to avoid breaking the timeline when the coroutine is cancelled. override suspend fun paginate(direction: Timeline.PaginationDirection): Result = withContext(NonCancellable) { withContext(dispatcher) { initLatch.await() - getPaginationMutex(direction).withLock { - runCatching { - if (!canPaginate(direction)) throw TimelineException.CannotPaginate - updatePaginationStatus(direction) { it.copy(isPaginating = true) } - when (direction) { - Timeline.PaginationDirection.BACKWARDS -> inner.paginateBackwards(PAGINATION_SIZE.toUShort()) - Timeline.PaginationDirection.FORWARDS -> inner.focusedPaginateForwards(PAGINATION_SIZE.toUShort()) - } - }.onFailure { error -> - updatePaginationStatus(direction) { it.copy(isPaginating = false) } - if (error is TimelineException.CannotPaginate) { - Timber.d("Can't paginate $direction on room ${matrixRoom.roomId} with paginationStatus: ${backPaginationStatus.value}") - } else { - Timber.e(error, "Error paginating $direction on room ${matrixRoom.roomId}") - } - }.onSuccess { hasReachedEnd -> - updatePaginationStatus(direction) { it.copy(isPaginating = false, hasMoreToLoad = !hasReachedEnd) } + runCatching { + if (!canPaginate(direction)) throw TimelineException.CannotPaginate + updatePaginationStatus(direction) { it.copy(isPaginating = true) } + when (direction) { + Timeline.PaginationDirection.BACKWARDS -> inner.paginateBackwards(PAGINATION_SIZE.toUShort()) + Timeline.PaginationDirection.FORWARDS -> inner.focusedPaginateForwards(PAGINATION_SIZE.toUShort()) } + }.onFailure { error -> + if (error is TimelineException.CannotPaginate) { + Timber.d("Can't paginate $direction on room ${matrixRoom.roomId} with paginationStatus: ${backPaginationStatus.value}") + } else { + updatePaginationStatus(direction) { it.copy(isPaginating = false) } + Timber.e(error, "Error paginating $direction on room ${matrixRoom.roomId}") + } + }.onSuccess { hasReachedEnd -> + updatePaginationStatus(direction) { it.copy(isPaginating = false, hasMoreToLoad = !hasReachedEnd) } } } } @@ -223,13 +212,13 @@ class RustTimeline( override val timelineItems: Flow> = combine( _timelineItems, - backPaginationStatus.map { it.hasMoreToLoad }.distinctUntilChanged(), - forwardPaginationStatus.map { it.hasMoreToLoad }.distinctUntilChanged(), + backPaginationStatus.filter { !it.isPaginating }.distinctUntilChanged(), + forwardPaginationStatus.filter { !it.isPaginating }.distinctUntilChanged(), matrixRoom.roomInfoFlow.map { it.creator }, isTimelineInitialized, ) { timelineItems, - hasMoreToLoadBackward, - hasMoreToLoadForward, + backwardPaginationStatus, + forwardPaginationStatus, roomCreator, isTimelineInitialized -> withContext(dispatcher) { @@ -239,15 +228,15 @@ class RustTimeline( items = items, isDm = matrixRoom.isDm, roomCreator = roomCreator, - hasMoreToLoadBackwards = hasMoreToLoadBackward, + hasMoreToLoadBackwards = backwardPaginationStatus.hasMoreToLoad, ) } .let { items -> loadingIndicatorsPostProcessor.process( items = items, isTimelineInitialized = isTimelineInitialized, - hasMoreToLoadBackward = hasMoreToLoadBackward, - hasMoreToLoadForward = hasMoreToLoadForward + hasMoreToLoadBackward = backwardPaginationStatus.hasMoreToLoad, + hasMoreToLoadForward = forwardPaginationStatus.hasMoreToLoad, ) } .let { items -> 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 68f2aa98f0..c122e95447 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 @@ -183,14 +183,6 @@ class MediaGalleryPresenter @AssistedInject constructor( } .launchIn(this) - timeline.data.paginationStatus(Timeline.PaginationDirection.BACKWARDS) - .onEach { backwardPaginationStatus -> - if (backwardPaginationStatus.canPaginate) { - timelineMediaItemsFactory.onCanPaginate() - } - } - .launchIn(this) - timelineMediaItemsFactory.timelineItems.map { timelineItems -> AsyncData.Success(timelineItems) } diff --git a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/TimelineMediaItemsFactory.kt b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/TimelineMediaItemsFactory.kt index aca0e6e477..abad9d2052 100644 --- a/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/TimelineMediaItemsFactory.kt +++ b/libraries/mediaviewer/impl/src/main/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/TimelineMediaItemsFactory.kt @@ -14,7 +14,6 @@ import io.element.android.libraries.androidutils.diff.MutableListDiffCache import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.di.RoomScope import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem -import io.element.android.services.toolbox.api.systemclock.SystemClock import kotlinx.collections.immutable.ImmutableList import kotlinx.collections.immutable.toPersistentList import kotlinx.coroutines.flow.Flow @@ -23,14 +22,11 @@ import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext -import timber.log.Timber import javax.inject.Inject interface TimelineMediaItemsFactory { val timelineItems: Flow> - suspend fun replaceWith(timelineItems: List) - suspend fun onCanPaginate() } @ContributesBinding(RoomScope::class) @@ -38,7 +34,6 @@ class DefaultTimelineMediaItemsFactory @Inject constructor( private val dispatchers: CoroutineDispatchers, private val virtualItemFactory: VirtualItemFactory, private val eventItemFactory: EventItemFactory, - private val systemClock: SystemClock, ) : TimelineMediaItemsFactory { private val _timelineItems = MutableSharedFlow>(replay = 1) private val lock = Mutex() @@ -66,26 +61,6 @@ class DefaultTimelineMediaItemsFactory @Inject constructor( } } - /** - * Update the timestamp of the loading indicator, so that it may trigger a new pagination request. - */ - override suspend fun onCanPaginate() { - lock.withLock { - val values = _timelineItems.replayCache.firstOrNull() ?: return@withLock - val lastItem = values.lastOrNull() - if (lastItem is MediaItem.LoadingIndicator) { - val newList = values.toMutableList().apply { - removeAt(size - 1) - val newTs = systemClock.epochMillis() - add(lastItem.copy(timestamp = newTs)) - } - _timelineItems.emit(newList.toPersistentList()) - } else { - Timber.w("onCanPaginate called but last item is not a loading indicator") - } - } - } - private suspend fun buildAndEmitTimelineItemStates( timelineItems: List, ) { diff --git a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/FakeTimelineMediaItemsFactory.kt b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/FakeTimelineMediaItemsFactory.kt index 618ba855ad..c9c95230db 100644 --- a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/FakeTimelineMediaItemsFactory.kt +++ b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/FakeTimelineMediaItemsFactory.kt @@ -16,7 +16,6 @@ import kotlinx.coroutines.flow.flowOf class FakeTimelineMediaItemsFactory( private val replaceWithLambda: (List) -> Unit = { lambdaError() }, - private val onCanPaginateLambda: () -> Unit = { lambdaError() } ) : TimelineMediaItemsFactory { override val timelineItems: Flow> get() = flowOf(emptyList().toImmutableList()) @@ -24,8 +23,4 @@ class FakeTimelineMediaItemsFactory( override suspend fun replaceWith(timelineItems: List) { replaceWithLambda(timelineItems) } - - override suspend fun onCanPaginate() { - onCanPaginateLambda() - } } diff --git a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryPresenterTest.kt b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryPresenterTest.kt index a5fe335fd3..39c88a5597 100644 --- a/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryPresenterTest.kt +++ b/libraries/mediaviewer/impl/src/test/kotlin/io/element/android/libraries/mediaviewer/impl/gallery/MediaGalleryPresenterTest.kt @@ -247,7 +247,6 @@ class MediaGalleryPresenterTest { room = room, timelineMediaItemsFactory = FakeTimelineMediaItemsFactory( replaceWithLambda = lambdaRecorder, Unit> { _ -> }, - onCanPaginateLambda = lambdaRecorder { }, ), localMediaFactory = localMediaFactory, mediaLoader = matrixMediaLoader,