Merge pull request #868 from vector-im/feature/fga/better_timeline_scroll

Feature/fga/better timeline scroll

Author: ganfra

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

@@ -22,21 +22,24 @@ import androidx.compose.runtime.MutableState
 import androidx.compose.runtime.collectAsState
 import androidx.compose.runtime.getValue
 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.setValue
 import io.element.android.features.messages.impl.timeline.factories.TimelineItemsFactory
 import io.element.android.features.messages.impl.timeline.model.TimelineItem
 import io.element.android.libraries.architecture.Presenter
+import io.element.android.libraries.core.coroutine.CoroutineDispatchers
 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.item.event.TimelineItemEventOrigin
 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 kotlinx.coroutines.withContext
 import javax.inject.Inject
 
 private const val backPaginationEventLimit = 20
@@ -45,42 +48,52 @@ private const val backPaginationPageSize = 50
 class TimelinePresenter @Inject constructor(
     private val timelineItemsFactory: TimelineItemsFactory,
     private val room: MatrixRoom,
+    private val dispatchers: CoroutineDispatchers,
+    private val appScope: CoroutineScope,
 ) : Presenter<TimelineState> {
 
     private val timeline = room.timeline
 
     @Composable
     override fun present(): TimelineState {
-        val localCoroutineScope = rememberCoroutineScope()
+        val localScope = rememberCoroutineScope()
         val highlightedEventId: MutableState<EventId?> = rememberSaveable {
             mutableStateOf(null)
         }
 
-        var lastReadMarkerIndex by rememberSaveable { mutableStateOf(Int.MAX_VALUE) }
-        var lastReadMarkerId by rememberSaveable { mutableStateOf<EventId?>(null) }
+        val lastReadReceiptIndex = rememberSaveable { mutableStateOf(Int.MAX_VALUE) }
+        val lastReadReceiptId = rememberSaveable { mutableStateOf<EventId?>(null) }
 
         val timelineItems by timelineItemsFactory.collectItemsAsState()
         val paginationState by timeline.paginationState.collectAsState()
-
         val syncUpdateFlow = room.syncUpdateFlow.collectAsState()
         val userHasPermissionToSendMessage by room.canSendEventAsState(type = MessageEventType.ROOM_MESSAGE, updateKey = syncUpdateFlow.value)
 
+        val prevMostRecentItemId = rememberSaveable { mutableStateOf<String?>(null) }
+        val hasNewItems = remember { mutableStateOf(false) }
+
         fun handleEvents(event: TimelineEvents) {
             when (event) {
-                TimelineEvents.LoadMore -> localCoroutineScope.paginateBackwards()
+                TimelineEvents.LoadMore -> localScope.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
-                    val eventId = getLastEventIdBeforeOrAt(event.firstIndex, timelineItems) ?: return
-                    if (event.firstIndex <= lastReadMarkerIndex && eventId != lastReadMarkerId) {
-                        lastReadMarkerIndex = event.firstIndex
-                        lastReadMarkerId = eventId
-                        localCoroutineScope.sendReadReceipt(eventId)
+                    if (event.firstIndex == 0) {
+                        hasNewItems.value = false
                     }
+                    appScope.sendReadReceiptIfNeeded(
+                        firstVisibleIndex = event.firstIndex,
+                        timelineItems = timelineItems,
+                        lastReadReceiptIndex = lastReadReceiptIndex,
+                        lastReadReceiptId = lastReadReceiptId
+                    )
                 }
             }
         }
 
+        LaunchedEffect(timelineItems.size) {
+            computeHasNewItems(timelineItems, prevMostRecentItemId, hasNewItems)
+        }
+
         LaunchedEffect(Unit) {
             timeline
                 .timelineItems
@@ -98,10 +111,49 @@ class TimelinePresenter @Inject constructor(
             canReply = userHasPermissionToSendMessage,
             paginationState = paginationState,
             timelineItems = timelineItems,
+            hasNewItems = hasNewItems.value,
             eventSink = ::handleEvents
         )
     }
 
+    /**
+     * This method compute the hasNewItem state passed as a [MutableState] each time the timeline items size changes.
+     * Basically, if we got new timeline event from sync or local, either from us or another user, we update the state so we tell we have new items.
+     * The state never goes back to false from this method, but need to be reset from somewhere else.
+     */
+    private suspend fun computeHasNewItems(
+        timelineItems: ImmutableList<TimelineItem>,
+        prevMostRecentItemId: MutableState<String?>,
+        hasNewItemsState: MutableState<Boolean>
+    ) = withContext(dispatchers.computation) {
+        val newMostRecentItem = timelineItems.firstOrNull()
+        val prevMostRecentItemIdValue = prevMostRecentItemId.value
+        val newMostRecentItemId = newMostRecentItem?.identifier()
+        val hasNewItems = prevMostRecentItemIdValue != null &&
+            newMostRecentItem is TimelineItem.Event &&
+            newMostRecentItem.origin != TimelineItemEventOrigin.PAGINATION &&
+            newMostRecentItemId != prevMostRecentItemIdValue
+        if (hasNewItems) {
+            hasNewItemsState.value = true
+        }
+        prevMostRecentItemId.value = newMostRecentItemId
+    }
+
+    private fun CoroutineScope.sendReadReceiptIfNeeded(
+        firstVisibleIndex: Int,
+        timelineItems: ImmutableList<TimelineItem>,
+        lastReadReceiptIndex: MutableState<Int>,
+        lastReadReceiptId: MutableState<EventId?>,
+    ) = launch(dispatchers.computation) {
+        // Get last valid EventId seen by the user, as the first index might refer to a Virtual item
+        val eventId = getLastEventIdBeforeOrAt(firstVisibleIndex, timelineItems)
+        if (eventId != null && firstVisibleIndex <= lastReadReceiptIndex.value && eventId != lastReadReceiptId.value) {
+            lastReadReceiptIndex.value = firstVisibleIndex
+            lastReadReceiptId.value = eventId
+            timeline.sendReadReceipt(eventId)
+        }
+    }
+
     private fun getLastEventIdBeforeOrAt(index: Int, items: ImmutableList<TimelineItem>): EventId? {
         for (item in items.subList(index, items.count())) {
             if (item is TimelineItem.Event) {
@@ -114,8 +166,4 @@ class TimelinePresenter @Inject constructor(
     private fun CoroutineScope.paginateBackwards() = launch {
         timeline.paginateBackwards(backPaginationEventLimit, backPaginationPageSize)
     }
-
-    private fun CoroutineScope.sendReadReceipt(eventId: EventId) = launch {
-        timeline.sendReadReceipt(eventId)
-    }
 }

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

@@ -28,5 +28,6 @@ data class TimelineState(
     val highlightedEventId: EventId?,
     val canReply: Boolean,
     val paginationState: MatrixTimeline.PaginationState,
+    val hasNewItems: Boolean,
     val eventSink: (TimelineEvents) -> Unit
 )

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

@@ -31,8 +31,8 @@ import io.element.android.libraries.matrix.api.core.TransactionId
 import io.element.android.libraries.matrix.api.core.UserId
 import io.element.android.libraries.matrix.api.timeline.MatrixTimeline
 import io.element.android.libraries.matrix.api.timeline.item.TimelineItemDebugInfo
-import io.element.android.libraries.matrix.api.timeline.item.event.LocalEventSendState
 import io.element.android.libraries.matrix.api.timeline.item.event.InReplyTo
+import io.element.android.libraries.matrix.api.timeline.item.event.LocalEventSendState
 import kotlinx.collections.immutable.ImmutableList
 import kotlinx.collections.immutable.persistentListOf
 import kotlinx.collections.immutable.toImmutableList
@@ -45,7 +45,8 @@ fun aTimelineState(timelineItems: ImmutableList<TimelineItem> = persistentListOf
     paginationState = MatrixTimeline.PaginationState(isBackPaginating = false, hasMoreToLoadBackwards = true),
     highlightedEventId = null,
     canReply = true,
-    eventSink = {}
+    hasNewItems = false,
+    eventSink = {},
 )
 
 internal fun aTimelineItemList(content: TimelineItemEventContent): ImmutableList<TimelineItem> {
@@ -127,6 +128,7 @@ internal fun aTimelineItemEvent(
         localSendState = sendState,
         inReplyTo = inReplyTo,
         debugInfo = debugInfo,
+        origin = null
     )
 }
 
@@ -153,13 +155,14 @@ internal fun aTimelineItemDebugInfo(
     model, originalJson, latestEditedJson
 )
 
-fun aGroupedEvents(): TimelineItem.GroupedEvents {
+fun aGroupedEvents(id: Long = 0): TimelineItem.GroupedEvents {
     val event = aTimelineItemEvent(
         isMine = true,
         content = aTimelineItemStateEventContent(),
         groupPosition = TimelineItemGroupPosition.None
     )
     return TimelineItem.GroupedEvents(
+        id = id.toString(),
         events = listOf(
             event,
             event,

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

@@ -21,6 +21,7 @@ package io.element.android.features.messages.impl.timeline
 import androidx.compose.animation.AnimatedVisibility
 import androidx.compose.animation.ExperimentalAnimationApi
 import androidx.compose.animation.animateContentSize
+import androidx.compose.animation.core.tween
 import androidx.compose.animation.scaleIn
 import androidx.compose.animation.scaleOut
 import androidx.compose.foundation.layout.Box
@@ -48,7 +49,6 @@ import androidx.compose.runtime.rememberCoroutineScope
 import androidx.compose.runtime.saveable.rememberSaveable
 import androidx.compose.ui.Alignment
 import androidx.compose.ui.Modifier
-import androidx.compose.ui.draw.shadow
 import androidx.compose.ui.platform.LocalInspectionMode
 import androidx.compose.ui.res.pluralStringResource
 import androidx.compose.ui.tooling.preview.Preview
@@ -64,17 +64,13 @@ import io.element.android.features.messages.impl.timeline.model.TimelineItem
 import io.element.android.features.messages.impl.timeline.model.event.TimelineItemEventContent
 import io.element.android.features.messages.impl.timeline.model.event.TimelineItemEventContentProvider
 import io.element.android.features.messages.impl.timeline.model.event.TimelineItemStateContent
-import io.element.android.features.messages.impl.timeline.model.virtual.TimelineItemEncryptedHistoryBannerVirtualModel
 import io.element.android.libraries.designsystem.preview.ElementPreviewDark
 import io.element.android.libraries.designsystem.preview.ElementPreviewLight
 import io.element.android.libraries.designsystem.theme.components.FloatingActionButton
 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 io.element.android.libraries.theme.ElementTheme
-import kotlinx.collections.immutable.ImmutableList
-import kotlinx.collections.immutable.persistentListOf
-import kotlinx.collections.immutable.toPersistentList
 import kotlinx.coroutines.launch
 
 @Composable
@@ -103,13 +99,6 @@ fun TimelineView(
         // TODO implement this logic once we have support to 'jump to event X' in sliding sync
     }
 
-    // Send an event to the presenter when the scrolling is finished, with the first visible index at the bottom.
-    val firstVisibleIndex by remember { derivedStateOf { lazyListState.firstVisibleItemIndex } }
-    val isScrollFinished by remember { derivedStateOf { !lazyListState.isScrollInProgress } }
-    LaunchedEffect(firstVisibleIndex, isScrollFinished) {
-        if (!isScrollFinished) return@LaunchedEffect
-        state.eventSink(TimelineEvents.OnScrollFinished(firstVisibleIndex))
-    }
 
     Box(modifier = modifier) {
         LazyColumn(
@@ -150,8 +139,8 @@ fun TimelineView(
 
         TimelineScrollHelper(
             lazyListState = lazyListState,
-            timelineItems = state.timelineItems,
-            onScrollFinishedAt = ::onScrollFinishedAt,
+            hasNewItems = state.hasNewItems,
+            onScrollFinishedAt = ::onScrollFinishedAt
         )
     }
 }
@@ -247,63 +236,66 @@ fun TimelineItemRow(
 }
 
 @Composable
-internal fun BoxScope.TimelineScrollHelper(
+private fun BoxScope.TimelineScrollHelper(
     lazyListState: LazyListState,
-    timelineItems: ImmutableList<TimelineItem>,
-    onScrollFinishedAt: (Int) -> Unit = {},
+    hasNewItems: Boolean,
+    onScrollFinishedAt: (Int) -> Unit,
 ) {
     val coroutineScope = rememberCoroutineScope()
-    val firstVisibleItemIndex by remember { derivedStateOf { lazyListState.firstVisibleItemIndex } }
     val isScrollFinished by remember { derivedStateOf { !lazyListState.isScrollInProgress } }
-    val shouldAutoScrollToBottom by remember { derivedStateOf { lazyListState.firstVisibleItemIndex < 2 } }
-    val showScrollToBottomButton by remember { derivedStateOf { lazyListState.firstVisibleItemIndex > 0 } }
+    val canAutoScroll by remember { derivedStateOf { lazyListState.firstVisibleItemIndex < 3 } }
 
-    LaunchedEffect(timelineItems, firstVisibleItemIndex) {
-        if (!isScrollFinished) return@LaunchedEffect
-
-        // Auto-scroll when new timeline items appear
-        if (shouldAutoScrollToBottom) {
+    LaunchedEffect(canAutoScroll, hasNewItems) {
+        val shouldAutoScroll = isScrollFinished && canAutoScroll && hasNewItems
+        if (shouldAutoScroll) {
             coroutineScope.launch {
                 lazyListState.animateScrollToItem(0)
             }
         }
     }
-    LaunchedEffect(isScrollFinished) {
-        if (!isScrollFinished) return@LaunchedEffect
 
-        // Notify the parent composable about the first visible item index when scrolling finishes
-        onScrollFinishedAt(firstVisibleItemIndex)
+    LaunchedEffect(isScrollFinished) {
+        if (isScrollFinished) {
+            // Notify the parent composable about the first visible item index when scrolling finishes
+            onScrollFinishedAt(lazyListState.firstVisibleItemIndex)
+        }
     }
 
-    // Jump to bottom button (display also in previews)
-    AnimatedVisibility(
+    JumpToBottomButton(
+        // Use inverse of canAutoScroll otherwise we might briefly see the before the scroll animation is triggered
+        isVisible = !canAutoScroll,
         modifier = Modifier
             .align(Alignment.BottomEnd)
             .padding(end = 24.dp, bottom = 12.dp),
-        visible = showScrollToBottomButton || LocalInspectionMode.current,
-        enter = scaleIn(),
-        exit = scaleOut(),
+        onClick = {
+            coroutineScope.launch {
+                if (lazyListState.firstVisibleItemIndex > 10) {
+                    lazyListState.scrollToItem(0)
+                } else {
+                    lazyListState.animateScrollToItem(0)
+                }
+            }
+        }
+    )
+}
+
+@Composable
+private fun JumpToBottomButton(
+    isVisible: Boolean,
+    onClick: () -> Unit,
+    modifier: Modifier = Modifier,
+) {
+    AnimatedVisibility(
+        modifier = modifier,
+        visible = isVisible || LocalInspectionMode.current,
+        enter = scaleIn(animationSpec = tween(100)),
+        exit = scaleOut(animationSpec = tween(100)),
     ) {
         FloatingActionButton(
-            onClick = {
-                coroutineScope.launch {
-                    if (firstVisibleItemIndex > 10) {
-                        lazyListState.scrollToItem(0)
-                    } else {
-                        lazyListState.animateScrollToItem(0)
-                    }
-                }
-            },
+            onClick = onClick,
             elevation = FloatingActionButtonDefaults.elevation(4.dp, 4.dp, 4.dp, 4.dp),
             shape = CircleShape,
-            modifier = Modifier
-                .shadow(
-                    elevation = 4.dp,
-                    shape = CircleShape,
-                    ambientColor = ElementTheme.materialColors.primary,
-                    spotColor = ElementTheme.materialColors.primary,
-                )
-                .size(36.dp),
+            modifier = Modifier.size(36.dp),
             containerColor = ElementTheme.colors.bgSubtleSecondary,
             contentColor = ElementTheme.colors.iconSecondary
         ) {

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

@@ -85,6 +85,7 @@ class TimelineItemEventFactory @Inject constructor(
             localSendState = currentTimelineItem.event.localSendState,
             inReplyTo = currentTimelineItem.event.inReplyTo(),
             debugInfo = currentTimelineItem.event.debugInfo,
+            origin = currentTimelineItem.event.origin,
         )
     }