From 635e9b9edded1a2ffba4132e8a49c5cacb0570eb Mon Sep 17 00:00:00 2001 From: ganfra Date: Wed, 12 Jul 2023 22:43:35 +0200 Subject: [PATCH 1/4] Timeline: avoid pagination when timeline is not ready --- .../impl/timeline/TimelinePresenter.kt | 17 +++---------- .../impl/timeline/TimelineStateProvider.kt | 2 +- .../messages/impl/timeline/TimelineView.kt | 2 +- .../timeline/TimelinePresenterTest.kt | 6 ++--- .../api/timeline/CannotPaginateException.kt | 21 ++++++++++++++++ .../matrix/api/timeline/MatrixTimeline.kt | 6 +++-- .../timeline/MatrixTimelineDiffProcessor.kt | 7 +----- .../impl/timeline/RustMatrixTimeline.kt | 25 +++++++++++++++---- .../test/timeline/FakeMatrixTimeline.kt | 2 +- 9 files changed, 55 insertions(+), 33 deletions(-) create mode 100644 libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/CannotPaginateException.kt diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt index 765becfd11..913a878b12 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt @@ -31,14 +31,12 @@ import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.room.MatrixRoom import io.element.android.libraries.matrix.api.room.MessageEventType -import io.element.android.libraries.matrix.api.timeline.MatrixTimeline import io.element.android.libraries.matrix.ui.room.canSendEventAsState import kotlinx.collections.immutable.ImmutableList import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch -import timber.log.Timber import javax.inject.Inject private const val backPaginationEventLimit = 20 @@ -69,7 +67,7 @@ class TimelinePresenter @Inject constructor( fun handleEvents(event: TimelineEvents) { when (event) { - TimelineEvents.LoadMore -> localCoroutineScope.loadMore(paginationState) + TimelineEvents.LoadMore -> localCoroutineScope.paginateBackwards() is TimelineEvents.SetHighlightedEvent -> highlightedEventId.value = event.eventId is TimelineEvents.OnScrollFinished -> { // Get last valid EventId seen by the user, as the first index might refer to a Virtual item @@ -87,11 +85,6 @@ class TimelinePresenter @Inject constructor( timeline .timelineItems .onEach(timelineItemsFactory::replaceWith) - .onEach { timelineItems -> - if (timelineItems.isEmpty()) { - loadMore(paginationState) - } - } .launchIn(this) } @@ -113,12 +106,8 @@ class TimelinePresenter @Inject constructor( return null } - private fun CoroutineScope.loadMore(paginationState: MatrixTimeline.PaginationState) = launch { - if (paginationState.canBackPaginate && !paginationState.isBackPaginating) { - timeline.paginateBackwards(backPaginationEventLimit, backPaginationPageSize) - } else { - Timber.v("Can't back paginate as paginationState = $paginationState") - } + private fun CoroutineScope.paginateBackwards() = launch { + timeline.paginateBackwards(backPaginationEventLimit, backPaginationPageSize) } private fun CoroutineScope.sendReadReceipt(eventId: EventId) = launch { diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineStateProvider.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineStateProvider.kt index e96d7ac0f3..e17852ee0e 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineStateProvider.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineStateProvider.kt @@ -40,7 +40,7 @@ import kotlin.random.Random fun aTimelineState(timelineItems: ImmutableList = persistentListOf()) = TimelineState( timelineItems = timelineItems, - paginationState = MatrixTimeline.PaginationState(isBackPaginating = false, canBackPaginate = true), + paginationState = MatrixTimeline.PaginationState(isBackPaginating = false, hasMoreToLoadBackwards = true), highlightedEventId = null, canReply = true, eventSink = {} diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineView.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineView.kt index bb33981a64..ee7a57c70e 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineView.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineView.kt @@ -134,7 +134,7 @@ fun TimelineView( onSwipeToReply = onSwipeToReply, ) } - if (state.paginationState.canBackPaginate) { + if (state.paginationState.hasMoreToLoadBackwards) { // Do not use key parameter to avoid wrong positioning item(contentType = "TimelineLoadingMoreIndicator") { TimelineLoadingMoreIndicator() diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/timeline/TimelinePresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/timeline/TimelinePresenterTest.kt index 9151376d4d..dc3f28d527 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/timeline/TimelinePresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/timeline/TimelinePresenterTest.kt @@ -59,14 +59,14 @@ class TimelinePresenterTest { presenter.present() }.test { val initialState = awaitItem() - assertThat(initialState.paginationState.canBackPaginate).isTrue() + assertThat(initialState.paginationState.hasMoreToLoadBackwards).isTrue() assertThat(initialState.paginationState.isBackPaginating).isFalse() initialState.eventSink.invoke(TimelineEvents.LoadMore) val inPaginationState = awaitItem() assertThat(inPaginationState.paginationState.isBackPaginating).isTrue() - assertThat(inPaginationState.paginationState.canBackPaginate).isTrue() + assertThat(inPaginationState.paginationState.hasMoreToLoadBackwards).isTrue() val postPaginationState = awaitItem() - assertThat(postPaginationState.paginationState.canBackPaginate).isTrue() + assertThat(postPaginationState.paginationState.hasMoreToLoadBackwards).isTrue() assertThat(postPaginationState.paginationState.isBackPaginating).isFalse() } } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/CannotPaginateException.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/CannotPaginateException.kt new file mode 100644 index 0000000000..82a87631c3 --- /dev/null +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/CannotPaginateException.kt @@ -0,0 +1,21 @@ +/* + * Copyright (c) 2023 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.matrix.api.timeline + +sealed class TimelineException: Exception() { + object CannotPaginate: TimelineException() +} diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/MatrixTimeline.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/MatrixTimeline.kt index b74a34bd01..af411216e5 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/MatrixTimeline.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/MatrixTimeline.kt @@ -24,8 +24,10 @@ interface MatrixTimeline { data class PaginationState( val isBackPaginating: Boolean, - val canBackPaginate: Boolean - ) + val hasMoreToLoadBackwards: Boolean + ) { + val canBackPaginate = !isBackPaginating && hasMoreToLoadBackwards + } val paginationState: StateFlow val timelineItems: Flow> diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/MatrixTimelineDiffProcessor.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/MatrixTimelineDiffProcessor.kt index dd0198cf18..b14d459697 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/MatrixTimelineDiffProcessor.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/MatrixTimelineDiffProcessor.kt @@ -17,7 +17,6 @@ package io.element.android.libraries.matrix.impl.timeline import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem -import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -30,20 +29,16 @@ internal class MatrixTimelineDiffProcessor( private val timelineItemFactory: MatrixTimelineItemMapper, ) { - private val initLatch = CompletableDeferred() private val mutex = Mutex() suspend fun postItems(items: List) { updateTimelineItems { val mappedItems = items.map { it.asMatrixTimelineItem() } - addAll(mappedItems) + addAll(0, mappedItems) } - initLatch.complete(Unit) } suspend fun postDiff(diff: TimelineDiff) { - // Makes sure to process first items before diff. - initLatch.await() updateTimelineItems { applyDiff(diff) } diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt index 2c3a579b2f..de97d48a8a 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt @@ -20,10 +20,12 @@ import io.element.android.libraries.matrix.api.core.EventId import io.element.android.libraries.matrix.api.room.MatrixRoom import io.element.android.libraries.matrix.api.timeline.MatrixTimeline import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem +import io.element.android.libraries.matrix.api.timeline.TimelineException import io.element.android.libraries.matrix.impl.timeline.item.event.EventMessageMapper import io.element.android.libraries.matrix.impl.timeline.item.event.EventTimelineItemMapper import io.element.android.libraries.matrix.impl.timeline.item.event.TimelineEventContentMapper import io.element.android.libraries.matrix.impl.timeline.item.virtual.VirtualTimelineItemMapper +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.FlowPreview @@ -40,6 +42,8 @@ import org.matrix.rustcomponents.sdk.Room import org.matrix.rustcomponents.sdk.TimelineDiff import org.matrix.rustcomponents.sdk.TimelineItem import timber.log.Timber +import java.util.concurrent.atomic.AtomicBoolean + class RustMatrixTimeline( roomCoroutineScope: CoroutineScope, @@ -48,11 +52,14 @@ class RustMatrixTimeline( private val dispatcher: CoroutineDispatcher, ) : MatrixTimeline { + private val initLatch = CompletableDeferred() + private val isInit = AtomicBoolean(false) + private val _timelineItems: MutableStateFlow> = MutableStateFlow(emptyList()) private val _paginationState = MutableStateFlow( - MatrixTimeline.PaginationState(canBackPaginate = true, isBackPaginating = false) + MatrixTimeline.PaginationState(hasMoreToLoadBackwards = true, isBackPaginating = false) ) private val timelineItemFactory = MatrixTimelineItemMapper( @@ -78,9 +85,12 @@ class RustMatrixTimeline( internal suspend fun postItems(items: List) { timelineDiffProcessor.postItems(items) + isInit.set(true) + initLatch.complete(Unit) } internal suspend fun postDiff(timelineDiff: TimelineDiff) { + initLatch.await() timelineDiffProcessor.postDiff(timelineDiff) } @@ -90,19 +100,19 @@ class RustMatrixTimeline( BackPaginationStatus.IDLE -> { currentPaginationState.copy( isBackPaginating = false, - canBackPaginate = true + hasMoreToLoadBackwards = true ) } BackPaginationStatus.PAGINATING -> { currentPaginationState.copy( isBackPaginating = true, - canBackPaginate = true + hasMoreToLoadBackwards = true ) } BackPaginationStatus.TIMELINE_START_REACHED -> { currentPaginationState.copy( isBackPaginating = false, - canBackPaginate = false + hasMoreToLoadBackwards = false ) } } @@ -117,6 +127,7 @@ class RustMatrixTimeline( override suspend fun paginateBackwards(requestSize: Int, untilNumberOfItems: Int): Result = withContext(dispatcher) { runCatching { + if (!canBackPaginate()) throw TimelineException.CannotPaginate Timber.v("Start back paginating for room ${matrixRoom.roomId} ") val paginationOptions = PaginationOptions.UntilNumItems( eventLimit = requestSize.toUShort(), @@ -125,12 +136,16 @@ class RustMatrixTimeline( ) innerRoom.paginateBackwards(paginationOptions) }.onFailure { - Timber.e(it, "Fail to paginate for room ${matrixRoom.roomId}") + Timber.d(it, "Fail to paginate for room ${matrixRoom.roomId}") }.onSuccess { Timber.v("Success back paginating for room ${matrixRoom.roomId}") } } + private fun canBackPaginate(): Boolean { + return isInit.get() && paginationState.value.canBackPaginate + } + override suspend fun sendReadReceipt(eventId: EventId) = withContext(dispatcher) { runCatching { innerRoom.sendReadReceipt(eventId = eventId.value) diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/timeline/FakeMatrixTimeline.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/timeline/FakeMatrixTimeline.kt index db31fbd8f0..b49a5490ce 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/timeline/FakeMatrixTimeline.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/timeline/FakeMatrixTimeline.kt @@ -27,7 +27,7 @@ import kotlinx.coroutines.flow.getAndUpdate class FakeMatrixTimeline( initialTimelineItems: List = emptyList(), - initialPaginationState: MatrixTimeline.PaginationState = MatrixTimeline.PaginationState(canBackPaginate = true, isBackPaginating = false) + initialPaginationState: MatrixTimeline.PaginationState = MatrixTimeline.PaginationState(hasMoreToLoadBackwards = true, isBackPaginating = false) ) : MatrixTimeline { private val _paginationState: MutableStateFlow = MutableStateFlow(initialPaginationState) From 9784d8c87444632ce19cf55ebc842134f3f59e29 Mon Sep 17 00:00:00 2001 From: ganfra Date: Wed, 12 Jul 2023 22:44:42 +0200 Subject: [PATCH 2/4] Timeline: split the initial item list in small chunks to post items asap --- .../libraries/matrix/impl/timeline/RustMatrixTimeline.kt | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt index de97d48a8a..2c245c7164 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt @@ -44,6 +44,7 @@ import org.matrix.rustcomponents.sdk.TimelineItem import timber.log.Timber import java.util.concurrent.atomic.AtomicBoolean +private const val INITIAL_MAX_SIZE = 50 class RustMatrixTimeline( roomCoroutineScope: CoroutineScope, @@ -84,7 +85,10 @@ class RustMatrixTimeline( override val timelineItems: Flow> = _timelineItems.sample(50) internal suspend fun postItems(items: List) { - timelineDiffProcessor.postItems(items) + // Split the initial items in multiple list as there is no pagination in the cached data, so we can post timelineItems asap. + items.chunked(INITIAL_MAX_SIZE).reversed().forEach { + timelineDiffProcessor.postItems(it) + } isInit.set(true) initLatch.complete(Unit) } From 1a0dc8266bc095492915b4b1ddf62bfa73ed7ca2 Mon Sep 17 00:00:00 2001 From: ganfra Date: Wed, 12 Jul 2023 22:48:57 +0200 Subject: [PATCH 3/4] Timeline: rename file --- .../{CannotPaginateException.kt => TimelineException.kt} | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/{CannotPaginateException.kt => TimelineException.kt} (87%) diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/CannotPaginateException.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/TimelineException.kt similarity index 87% rename from libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/CannotPaginateException.kt rename to libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/TimelineException.kt index 82a87631c3..b7c155a5aa 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/CannotPaginateException.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/TimelineException.kt @@ -16,6 +16,6 @@ package io.element.android.libraries.matrix.api.timeline -sealed class TimelineException: Exception() { - object CannotPaginate: TimelineException() +sealed class TimelineException : Exception() { + object CannotPaginate : TimelineException() } From f6b29c3700e6a91963d4c67281df3e37ad8bc3a6 Mon Sep 17 00:00:00 2001 From: ganfra Date: Thu, 13 Jul 2023 11:23:04 +0200 Subject: [PATCH 4/4] Timeline: revert back the paginateBackwards in the TimelinePresenter so it's now blocked in the Timeline if needed --- .../features/messages/impl/timeline/TimelinePresenter.kt | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt index 913a878b12..0833ff2205 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt @@ -85,6 +85,11 @@ class TimelinePresenter @Inject constructor( timeline .timelineItems .onEach(timelineItemsFactory::replaceWith) + .onEach { timelineItems -> + if (timelineItems.isEmpty()) { + paginateBackwards() + } + } .launchIn(this) }