Merge pull request #731 from vector-im/feature/fga/timeline_back_pagination

Feature/fga/timeline back pagination

appnav/src/main/kotlin/io/element/android/appnav/RoomFlowNode.kt

@@ -18,6 +18,7 @@ package io.element.android.appnav
 
 import android.os.Parcelable
 import androidx.compose.runtime.Composable
+import androidx.compose.runtime.DisposableEffect
 import androidx.compose.ui.Modifier
 import androidx.lifecycle.lifecycleScope
 import com.bumble.appyx.core.composable.Children
@@ -88,14 +89,12 @@ class RoomFlowNode @AssistedInject constructor(
         lifecycle.subscribe(
             onCreate = {
                 Timber.v("OnCreate")
-                inputs.room.open()
                 plugins<LifecycleCallback>().forEach { it.onFlowCreated(id, inputs.room) }
                 appNavigationStateService.onNavigateToRoom(id, inputs.room.roomId)
                 fetchRoomMembers()
             },
             onDestroy = {
                 Timber.v("OnDestroy")
-                inputs.room.close()
                 plugins<LifecycleCallback>().forEach { it.onFlowReleased(id, inputs.room) }
                 appNavigationStateService.onLeavingRoom(id)
             }
@@ -161,6 +160,15 @@ class RoomFlowNode @AssistedInject constructor(
 
     @Composable
     override fun View(modifier: Modifier) {
+        // Rely on the View Lifecycle instead of the Node Lifecycle,
+        // because this node enters 'onDestroy' before his children, so it can leads to
+        // using the room in a child node where it's already closed.
+        DisposableEffect(Unit) {
+            inputs.room.open()
+            onDispose {
+                inputs.room.close()
+            }
+        }
         Children(
             navModel = backstack,
             modifier = modifier,

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenter.kt

@@ -59,21 +59,16 @@ class TimelinePresenter @Inject constructor(
         var lastReadMarkerIndex by rememberSaveable { mutableStateOf(Int.MAX_VALUE) }
         var lastReadMarkerId by rememberSaveable { mutableStateOf<EventId?>(null) }
 
-        val timelineItems = timelineItemsFactory
+        val timelineItems by timelineItemsFactory.collectItemsAsState()
-            .flow()
+        val paginationState by timeline.paginationState.collectAsState()
-            .collectAsState()
-
-        val paginationState = timeline
-            .paginationState()
-            .collectAsState()
 
         fun handleEvents(event: TimelineEvents) {
             when (event) {
-                TimelineEvents.LoadMore -> localCoroutineScope.loadMore(paginationState.value)
+                TimelineEvents.LoadMore -> localCoroutineScope.loadMore(paginationState)
                 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
-                    val eventId = getLastEventIdBeforeOrAt(event.firstIndex, timelineItems.value) ?: return
+                    val eventId = getLastEventIdBeforeOrAt(event.firstIndex, timelineItems) ?: return
                     if (event.firstIndex <= lastReadMarkerIndex && eventId != lastReadMarkerId) {
                         lastReadMarkerIndex = event.firstIndex
                         lastReadMarkerId = eventId
@@ -85,11 +80,11 @@ class TimelinePresenter @Inject constructor(
 
         LaunchedEffect(Unit) {
             timeline
-                .timelineItems()
+                .timelineItems
                 .onEach(timelineItemsFactory::replaceWith)
                 .onEach { timelineItems ->
                     if (timelineItems.isEmpty()) {
-                        loadMore(paginationState.value)
+                        loadMore(paginationState)
                     }
                 }
                 .launchIn(this)
@@ -97,8 +92,8 @@ class TimelinePresenter @Inject constructor(
 
         return TimelineState(
             highlightedEventId = highlightedEventId.value,
-            paginationState = paginationState.value,
+            paginationState = paginationState,
-            timelineItems = timelineItems.value,
+            timelineItems = timelineItems,
             eventSink = ::handleEvents
         )
     }

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineView.kt

@@ -39,7 +39,6 @@ import androidx.compose.runtime.mutableStateOf
 import androidx.compose.runtime.remember
 import androidx.compose.runtime.rememberCoroutineScope
 import androidx.compose.runtime.saveable.rememberSaveable
-import androidx.compose.runtime.snapshotFlow
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
 import androidx.compose.ui.res.pluralStringResource
@@ -62,7 +61,6 @@ import io.element.android.libraries.designsystem.theme.components.Icon
 import io.element.android.libraries.matrix.api.core.EventId
 import io.element.android.libraries.matrix.api.core.UserId
 import kotlinx.collections.immutable.ImmutableList
-import kotlinx.coroutines.flow.distinctUntilChanged
 import kotlinx.coroutines.launch
 
 @Composable
@@ -123,8 +121,7 @@ fun TimelineView(
 
         TimelineScrollHelper(
             lazyListState = lazyListState,
-            timelineItems = state.timelineItems,
+            timelineItems = state.timelineItems
-            onLoadMore = ::onReachedLoadMore
         )
     }
 }
@@ -222,7 +219,6 @@ fun TimelineItemRow(
 internal fun BoxScope.TimelineScrollHelper(
     lazyListState: LazyListState,
     timelineItems: ImmutableList<TimelineItem>,
-    onLoadMore: () -> Unit = {},
 ) {
     val coroutineScope = rememberCoroutineScope()
     val firstVisibleItemIndex by remember { derivedStateOf { lazyListState.firstVisibleItemIndex } }
@@ -236,24 +232,6 @@ internal fun BoxScope.TimelineScrollHelper(
         }
     }
 
-    // Handle load more preloading
-    val loadMore by remember {
-        derivedStateOf {
-            val layoutInfo = lazyListState.layoutInfo
-            val totalItemsNumber = layoutInfo.totalItemsCount
-            val lastVisibleItemIndex = (layoutInfo.visibleItemsInfo.lastOrNull()?.index ?: 0) + 1
-            lastVisibleItemIndex > (totalItemsNumber - 30)
-        }
-    }
-
-    LaunchedEffect(loadMore) {
-        snapshotFlow { loadMore }
-            .distinctUntilChanged()
-            .collect {
-                onLoadMore()
-            }
-    }
-
     // Jump to bottom button
     if (firstVisibleItemIndex > 2) {
         FloatingActionButton(

features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/TimelineItemsFactory.kt

@@ -16,6 +16,9 @@
 
 package io.element.android.features.messages.impl.timeline.factories
 
+import androidx.compose.runtime.Composable
+import androidx.compose.runtime.State
+import androidx.compose.runtime.collectAsState
 import androidx.recyclerview.widget.DiffUtil
 import io.element.android.features.messages.impl.timeline.diff.CacheInvalidator
 import io.element.android.features.messages.impl.timeline.diff.MatrixTimelineItemsDiffCallback
@@ -27,11 +30,8 @@ import io.element.android.libraries.core.coroutine.CoroutineDispatchers
 import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem
 import kotlinx.collections.immutable.ImmutableList
 import kotlinx.collections.immutable.persistentListOf
-import kotlinx.collections.immutable.toImmutableList
 import kotlinx.collections.immutable.toPersistentList
 import kotlinx.coroutines.flow.MutableStateFlow
-import kotlinx.coroutines.flow.StateFlow
-import kotlinx.coroutines.flow.asStateFlow
 import kotlinx.coroutines.sync.Mutex
 import kotlinx.coroutines.sync.withLock
 import kotlinx.coroutines.withContext
@@ -55,7 +55,10 @@ class TimelineItemsFactory @Inject constructor(
     private val lock = Mutex()
     private val cacheInvalidator = CacheInvalidator(timelineItemsCache)
 
-    fun flow(): StateFlow<ImmutableList<TimelineItem>> = timelineItems.asStateFlow()
+    @Composable
+    fun collectItemsAsState(): State<ImmutableList<TimelineItem>> {
+        return timelineItems.collectAsState()
+    }
 
     suspend fun replaceWith(
         timelineItems: List<MatrixTimelineItem>,

libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/MatrixTimeline.kt

@@ -27,8 +27,8 @@ interface MatrixTimeline {
         val canBackPaginate: Boolean
     )
 
-    fun paginationState(): StateFlow<PaginationState>
+    val paginationState: StateFlow<PaginationState>
-    fun timelineItems(): Flow<List<MatrixTimelineItem>>
+    val timelineItems: Flow<List<MatrixTimelineItem>>
 
     suspend fun paginateBackwards(requestSize: Int, untilNumberOfItems: Int): Result<Unit>
     suspend fun fetchDetailsForEvent(eventId: EventId): Result<Unit>

libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt

@@ -30,6 +30,7 @@ import kotlinx.coroutines.FlowPreview
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
+import kotlinx.coroutines.flow.asStateFlow
 import kotlinx.coroutines.flow.sample
 import kotlinx.coroutines.withContext
 import org.matrix.rustcomponents.sdk.PaginationOptions
@@ -45,10 +46,10 @@ class RustMatrixTimeline(
     private val coroutineDispatchers: CoroutineDispatchers,
 ) : MatrixTimeline {
 
-    private val timelineItems: MutableStateFlow<List<MatrixTimelineItem>> =
+    private val _timelineItems: MutableStateFlow<List<MatrixTimelineItem>> =
         MutableStateFlow(emptyList())
 
-    private val paginationState = MutableStateFlow(
+    private val _paginationState = MutableStateFlow(
         MatrixTimeline.PaginationState(canBackPaginate = true, isBackPaginating = false)
     )
 
@@ -64,19 +65,15 @@ class RustMatrixTimeline(
     )
 
     private val timelineDiffProcessor = MatrixTimelineDiffProcessor(
-        paginationState = paginationState,
+        paginationState = _paginationState,
-        timelineItems = timelineItems,
+        timelineItems = _timelineItems,
         timelineItemFactory = timelineItemFactory,
     )
 
-    override fun paginationState(): StateFlow<MatrixTimeline.PaginationState> {
+    override val paginationState: StateFlow<MatrixTimeline.PaginationState> = _paginationState.asStateFlow()
-        return paginationState
-    }
 
     @OptIn(FlowPreview::class)
-    override fun timelineItems(): Flow<List<MatrixTimelineItem>> {
+    override val timelineItems: Flow<List<MatrixTimelineItem>> = _timelineItems.sample(50)
-        return timelineItems.sample(50)
-    }
 
     internal suspend fun postItems(items: List<TimelineItem>) {
         timelineDiffProcessor.postItems(items)

libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/timeline/FakeMatrixTimeline.kt

@@ -23,33 +23,30 @@ import kotlinx.coroutines.delay
 import kotlinx.coroutines.flow.Flow
 import kotlinx.coroutines.flow.MutableStateFlow
 import kotlinx.coroutines.flow.StateFlow
+import kotlinx.coroutines.flow.getAndUpdate
 
 class FakeMatrixTimeline(
     initialTimelineItems: List<MatrixTimelineItem> = emptyList(),
     initialPaginationState: MatrixTimeline.PaginationState = MatrixTimeline.PaginationState(canBackPaginate = true, isBackPaginating = false)
 ) : MatrixTimeline {
 
-    private val paginationState: MutableStateFlow<MatrixTimeline.PaginationState> = MutableStateFlow(initialPaginationState)
+    private val _paginationState: MutableStateFlow<MatrixTimeline.PaginationState> = MutableStateFlow(initialPaginationState)
-    private val timelineItems: MutableStateFlow<List<MatrixTimelineItem>> = MutableStateFlow(initialTimelineItems)
+    private val _timelineItems: MutableStateFlow<List<MatrixTimelineItem>> = MutableStateFlow(initialTimelineItems)
 
     var sendReadReceiptCount = 0
         private set
 
     fun updatePaginationState(update: (MatrixTimeline.PaginationState.() -> MatrixTimeline.PaginationState)) {
-        paginationState.value = update(paginationState.value)
+        _paginationState.getAndUpdate(update)
     }
 
     fun updateTimelineItems(update: (items: List<MatrixTimelineItem>) -> List<MatrixTimelineItem>) {
-        timelineItems.value = update(timelineItems.value)
+        _timelineItems.getAndUpdate(update)
     }
 
-    override fun paginationState(): StateFlow<MatrixTimeline.PaginationState> {
+    override val paginationState: StateFlow<MatrixTimeline.PaginationState> = _paginationState
-        return paginationState
-    }
 
-    override fun timelineItems(): Flow<List<MatrixTimelineItem>> {
+    override val timelineItems: Flow<List<MatrixTimelineItem>> = _timelineItems
-        return timelineItems
-    }
 
     override suspend fun paginateBackwards(requestSize: Int, untilNumberOfItems: Int): Result<Unit> {
         updatePaginationState {

samples/minimal/src/main/kotlin/io/element/android/samples/minimal/RoomListScreen.kt

@@ -82,11 +82,7 @@ class RoomListScreen(
                 withContext(coroutineDispatchers.io) {
                     matrixClient.getRoom(roomId)!!.use { room ->
                         room.open()
-                        val timeline = room.timeline
+                        room.timeline.paginateBackwards(20, 50)
-                        timeline.apply {
-                            // TODO This doesn't work reliably as initialize is asynchronous, and the timeline can't be used until it's finished
-                            paginateBackwards(20, 50)
-                        }
                     }
                 }
             }