Merge pull request #1879 from vector-im/feature/fga/timeline_always_s…

…croll_send

Timeline : Scroll to end of timeline when sending a new message #1877

changelog.d/1877.feature

@@ -0,0 +1 @@
+Scroll to end of timeline when sending a new message.

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

@@ -34,6 +34,7 @@ import im.vector.app.features.analytics.plan.PollEnd
 import im.vector.app.features.analytics.plan.PollVote
 import io.element.android.features.messages.impl.MessagesNavigator
 import io.element.android.features.messages.impl.timeline.factories.TimelineItemsFactory
+import io.element.android.features.messages.impl.timeline.model.NewEventState
 import io.element.android.features.messages.impl.timeline.model.TimelineItem
 import io.element.android.features.messages.impl.timeline.session.SessionState
 import io.element.android.features.messages.impl.voicemessages.timeline.RedactedVoiceMessageManager
@@ -98,7 +99,7 @@ class TimelinePresenter @AssistedInject constructor(
         val userHasPermissionToSendMessage by room.canSendMessageAsState(type = MessageEventType.ROOM_MESSAGE, updateKey = syncUpdateFlow.value)
 
         val prevMostRecentItemId = rememberSaveable { mutableStateOf<String?>(null) }
-        val hasNewItems = remember { mutableStateOf(false) }
+        val newItemState = remember { mutableStateOf(NewEventState.None) }
 
         val sessionVerifiedStatus by verificationService.sessionVerifiedStatus.collectAsState()
         val keyBackupState by encryptionService.backupStateStateFlow.collectAsState()
@@ -121,7 +122,7 @@ class TimelinePresenter @AssistedInject constructor(
                 is TimelineEvents.SetHighlightedEvent -> highlightedEventId.value = event.eventId
                 is TimelineEvents.OnScrollFinished -> {
                     if (event.firstIndex == 0) {
-                        hasNewItems.value = false
+                        newItemState.value = NewEventState.None
                     }
                     appScope.sendReadReceiptIfNeeded(
                         firstVisibleIndex = event.firstIndex,
@@ -150,7 +151,7 @@ class TimelinePresenter @AssistedInject constructor(
         }
 
         LaunchedEffect(timelineItems.size) {
-            computeHasNewItems(timelineItems, prevMostRecentItemId, hasNewItems)
+            computeNewItemState(timelineItems, prevMostRecentItemId, newItemState)
         }
 
         LaunchedEffect(Unit) {
@@ -182,7 +183,7 @@ class TimelinePresenter @AssistedInject constructor(
             paginationState = paginationState,
             timelineItems = timelineItems,
             showReadReceipts = readReceiptsEnabled,
-            hasNewItems = hasNewItems.value,
+            newEventState = newItemState.value,
             sessionState = sessionState,
             eventSink = ::handleEvents
         )
@@ -191,22 +192,32 @@ class TimelinePresenter @AssistedInject constructor(
     /**
      * 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.
+     * The state never goes back to None from this method, but need to be reset from somewhere else.
      */
-    private suspend fun computeHasNewItems(
+    private suspend fun computeNewItemState(
         timelineItems: ImmutableList<TimelineItem>,
         prevMostRecentItemId: MutableState<String?>,
-        hasNewItemsState: MutableState<Boolean>
+        newEventState: MutableState<NewEventState>
     ) = withContext(dispatchers.computation) {
+        // FromMe is prioritized over FromOther, so skip if we already have a FromMe
+        if (newEventState.value == NewEventState.FromMe) {
+            return@withContext
+        }
         val newMostRecentItem = timelineItems.firstOrNull()
         val prevMostRecentItemIdValue = prevMostRecentItemId.value
         val newMostRecentItemId = newMostRecentItem?.identifier()
-        val hasNewItems = prevMostRecentItemIdValue != null &&
+        val hasNewEvent = prevMostRecentItemIdValue != null &&
             newMostRecentItem is TimelineItem.Event &&
             newMostRecentItem.origin != TimelineItemEventOrigin.PAGINATION &&
             newMostRecentItemId != prevMostRecentItemIdValue
-        if (hasNewItems) {
-            hasNewItemsState.value = true
+        if (hasNewEvent) {
+            val newMostRecentEvent = newMostRecentItem as? TimelineItem.Event
+            val fromMe = newMostRecentEvent?.localSendState != null
+            newEventState.value = if (fromMe) {
+                NewEventState.FromMe
+            } else {
+                NewEventState.FromOther
+            }
         }
         prevMostRecentItemId.value = newMostRecentItemId
     }

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

@@ -17,6 +17,7 @@
 package io.element.android.features.messages.impl.timeline
 
 import androidx.compose.runtime.Immutable
+import io.element.android.features.messages.impl.timeline.model.NewEventState
 import io.element.android.features.messages.impl.timeline.model.TimelineItem
 import io.element.android.features.messages.impl.timeline.session.SessionState
 import io.element.android.libraries.matrix.api.core.EventId
@@ -30,7 +31,7 @@ data class TimelineState(
     val highlightedEventId: EventId?,
     val userHasPermissionToSendMessage: Boolean,
     val paginationState: MatrixTimeline.PaginationState,
-    val hasNewItems: Boolean,
+    val newEventState: NewEventState,
     val sessionState: SessionState,
     val eventSink: (TimelineEvents) -> Unit
 )

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

@@ -17,6 +17,7 @@
 package io.element.android.features.messages.impl.timeline
 
 import io.element.android.features.messages.impl.timeline.model.InReplyToDetails
+import io.element.android.features.messages.impl.timeline.model.NewEventState
 import io.element.android.features.messages.impl.timeline.model.ReadReceiptData
 import io.element.android.features.messages.impl.timeline.model.TimelineItem
 import io.element.android.features.messages.impl.timeline.model.TimelineItemGroupPosition
@@ -53,7 +54,7 @@ fun aTimelineState(timelineItems: ImmutableList<TimelineItem> = persistentListOf
     ),
     highlightedEventId = null,
     userHasPermissionToSendMessage = true,
-    hasNewItems = false,
+    newEventState = NewEventState.None,
     sessionState = aSessionState(
         isSessionVerified = true,
         isKeyBackupEnabled = true,

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

@@ -66,6 +66,7 @@ import io.element.android.features.messages.impl.timeline.components.virtual.Tim
 import io.element.android.features.messages.impl.timeline.components.virtual.TimelineLoadingMoreIndicator
 import io.element.android.features.messages.impl.timeline.di.LocalTimelineItemPresenterFactories
 import io.element.android.features.messages.impl.timeline.di.aFakeTimelineItemPresenterFactories
+import io.element.android.features.messages.impl.timeline.model.NewEventState
 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
@@ -167,7 +168,7 @@ fun TimelineView(
         TimelineScrollHelper(
             isTimelineEmpty = state.timelineItems.isEmpty(),
             lazyListState = lazyListState,
-            hasNewItems = state.hasNewItems,
+            newEventState = state.newEventState,
             onScrollFinishedAt = ::onScrollFinishedAt
         )
     }
@@ -286,22 +287,34 @@ private fun TimelineItemRow(
 private fun BoxScope.TimelineScrollHelper(
     isTimelineEmpty: Boolean,
     lazyListState: LazyListState,
-    hasNewItems: Boolean,
+    newEventState: NewEventState,
     onScrollFinishedAt: (Int) -> Unit,
 ) {
     val coroutineScope = rememberCoroutineScope()
     val isScrollFinished by remember { derivedStateOf { !lazyListState.isScrollInProgress } }
-    val canAutoScroll by remember { derivedStateOf { lazyListState.firstVisibleItemIndex < 3 } }
+    val canAutoScroll by remember {
+        derivedStateOf {
+            lazyListState.firstVisibleItemIndex < 3
+        }
+    }
 
-    LaunchedEffect(canAutoScroll, hasNewItems) {
-        val shouldAutoScroll = isScrollFinished && canAutoScroll && hasNewItems
-        if (shouldAutoScroll) {
-            coroutineScope.launch {
+    fun scrollToBottom() {
+        coroutineScope.launch {
+            if (lazyListState.firstVisibleItemIndex > 10) {
+                lazyListState.scrollToItem(0)
+            } else {
                 lazyListState.animateScrollToItem(0)
             }
         }
     }
 
+    LaunchedEffect(canAutoScroll, newEventState) {
+        val shouldAutoScroll = isScrollFinished && (canAutoScroll || newEventState == NewEventState.FromMe)
+        if (shouldAutoScroll) {
+            scrollToBottom()
+        }
+    }
+
     LaunchedEffect(isScrollFinished, isTimelineEmpty) {
         if (isScrollFinished && !isTimelineEmpty) {
             // Notify the parent composable about the first visible item index when scrolling finishes
@@ -315,15 +328,7 @@ private fun BoxScope.TimelineScrollHelper(
         modifier = Modifier
             .align(Alignment.BottomEnd)
             .padding(end = 24.dp, bottom = 12.dp),
-        onClick = {
-            coroutineScope.launch {
-                if (lazyListState.firstVisibleItemIndex > 10) {
-                    lazyListState.scrollToItem(0)
-                } else {
-                    lazyListState.animateScrollToItem(0)
-                }
-            }
-        }
+        onClick = ::scrollToBottom,
     )
 }
 

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

@@ -0,0 +1,25 @@
+/*
+ * 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.features.messages.impl.timeline.model
+
+/**
+ * Model if there is a new event in the timeline and if it is from me or from other.
+ * This can be used to scroll to the bottom of the list when a new event is added.
+ */
+enum class NewEventState {
+    None, FromMe, FromOther
+}

features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt

@@ -26,6 +26,7 @@ import io.element.android.features.messages.impl.FakeMessagesNavigator
 import io.element.android.features.messages.impl.fixtures.aMessageEvent
 import io.element.android.features.messages.impl.fixtures.aTimelineItemsFactory
 import io.element.android.features.messages.impl.timeline.factories.TimelineItemsFactory
+import io.element.android.features.messages.impl.timeline.model.NewEventState
 import io.element.android.features.messages.impl.timeline.model.TimelineItem
 import io.element.android.features.messages.impl.timeline.session.SessionState
 import io.element.android.features.messages.impl.voicemessages.timeline.FakeRedactedVoiceMessageManager
@@ -36,6 +37,7 @@ 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.item.event.EventReaction
+import io.element.android.libraries.matrix.api.timeline.item.event.LocalEventSendState
 import io.element.android.libraries.matrix.api.timeline.item.event.ReactionSender
 import io.element.android.libraries.matrix.api.timeline.item.virtual.VirtualTimelineItem
 import io.element.android.libraries.matrix.test.AN_EVENT_ID
@@ -48,7 +50,9 @@ import io.element.android.libraries.matrix.test.verification.FakeSessionVerifica
 import io.element.android.libraries.matrix.ui.components.aMatrixUserList
 import io.element.android.services.analytics.test.FakeAnalyticsService
 import io.element.android.tests.testutils.WarmUpRule
+import io.element.android.tests.testutils.awaitLastSequentialItem
 import io.element.android.tests.testutils.awaitWithLatch
+import io.element.android.tests.testutils.consumeItemsUntilPredicate
 import io.element.android.tests.testutils.testCoroutineDispatchers
 import io.element.android.tests.testutils.waitForPredicate
 import kotlinx.coroutines.delay
@@ -187,28 +191,49 @@ class TimelinePresenterTest {
     }
 
     @Test
-    fun `present - covers hasNewItems scenarios`() = runTest {
+    fun `present - covers newEventState scenarios`() = runTest {
         val timeline = FakeMatrixTimeline()
         val presenter = createTimelinePresenter(timeline)
         moleculeFlow(RecompositionMode.Immediate) {
             presenter.present()
         }.test {
             val initialState = awaitItem()
-            assertThat(initialState.hasNewItems).isFalse()
+            assertThat(initialState.newEventState).isEqualTo(NewEventState.None)
             assertThat(initialState.timelineItems.size).isEqualTo(0)
             timeline.updateTimelineItems {
                 listOf(MatrixTimelineItem.Event(0, anEventTimelineItem(content = aMessageContent())))
             }
-            skipItems(1)
-            assertThat(awaitItem().timelineItems.size).isEqualTo(1)
+            consumeItemsUntilPredicate { it.timelineItems.size == 1 }
+            // Mimics sending a message, and assert newEventState is FromMe
             timeline.updateTimelineItems { items ->
-                items + listOf(MatrixTimelineItem.Event(1, anEventTimelineItem(content = aMessageContent())))
+                val event = anEventTimelineItem(content = aMessageContent(), localSendState = LocalEventSendState.Sent(AN_EVENT_ID))
+                items + listOf(MatrixTimelineItem.Event(1, event))
             }
-            skipItems(1)
-            assertThat(awaitItem().timelineItems.size).isEqualTo(2)
-            assertThat(awaitItem().hasNewItems).isTrue()
+            consumeItemsUntilPredicate { it.timelineItems.size == 2 }
+            awaitLastSequentialItem().also { state ->
+                assertThat(state.newEventState).isEqualTo(NewEventState.FromMe)
+            }
+            // Mimics receiving a message without clearing the previous FromMe
+            timeline.updateTimelineItems { items ->
+                val event = anEventTimelineItem(content = aMessageContent())
+                items + listOf(MatrixTimelineItem.Event(2, event))
+            }
+            consumeItemsUntilPredicate { it.timelineItems.size == 3 }
+
+            // Scroll to bottom to clear previous FromMe
             initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(0))
-            assertThat(awaitItem().hasNewItems).isFalse()
+            awaitLastSequentialItem().also { state ->
+                assertThat(state.newEventState).isEqualTo(NewEventState.None)
+            }
+            // Mimics receiving a message and assert newEventState is FromOther
+            timeline.updateTimelineItems { items ->
+                val event = anEventTimelineItem(content = aMessageContent())
+                items + listOf(MatrixTimelineItem.Event(3, event))
+            }
+            consumeItemsUntilPredicate { it.timelineItems.size == 4 }
+            awaitLastSequentialItem().also { state ->
+                assertThat(state.newEventState).isEqualTo(NewEventState.FromOther)
+            }
             cancelAndIgnoreRemainingEvents()
         }
     }
@@ -221,7 +246,7 @@ class TimelinePresenterTest {
             presenter.present()
         }.test {
             val initialState = awaitItem()
-            assertThat(initialState.hasNewItems).isFalse()
+            assertThat(initialState.newEventState).isEqualTo(NewEventState.None)
             assertThat(initialState.timelineItems.size).isEqualTo(0)
             val now = Date().time
             val minuteInMillis = 60 * 1000