From 469b54f2042923d1a493bd3e99454464348279e4 Mon Sep 17 00:00:00 2001 From: Jorge Martin Espinosa Date: Thu, 29 Jun 2023 12:08:19 +0200 Subject: [PATCH] Send read receipts (#713) * Send read receipts * Add changelog * Add tests. * Optimise how sending read receipts work in the timeline. * Move the check for send read receipts to the presenter Also improve the way we find the first visible `Event` if we have `Virtual` ones in the timeline. --- changelog.d/671.feature | 1 + .../messages/impl/timeline/TimelineEvents.kt | 1 + .../impl/timeline/TimelinePresenter.kt | 29 ++++++++ .../messages/impl/timeline/TimelineView.kt | 8 ++ .../timeline/TimelinePresenterTest.kt | 73 +++++++++++++++++++ .../matrix/api/timeline/MatrixTimeline.kt | 2 + .../impl/timeline/RustMatrixTimeline.kt | 6 ++ .../test/timeline/FakeMatrixTimeline.kt | 8 ++ 8 files changed, 128 insertions(+) create mode 100644 changelog.d/671.feature diff --git a/changelog.d/671.feature b/changelog.d/671.feature new file mode 100644 index 0000000000..94f7e25093 --- /dev/null +++ b/changelog.d/671.feature @@ -0,0 +1 @@ +Send read receipts for rooms. diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineEvents.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineEvents.kt index ff64441198..2bfed45470 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineEvents.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/TimelineEvents.kt @@ -21,4 +21,5 @@ import io.element.android.libraries.matrix.api.core.EventId sealed interface TimelineEvents { object LoadMore : TimelineEvents data class SetHighlightedEvent(val eventId: EventId?) : TimelineEvents + data class OnScrollFinished(val firstIndex: Int) : TimelineEvents } 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 f34bc284e1..8513d8e605 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 @@ -20,14 +20,18 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.MutableState import androidx.compose.runtime.collectAsState +import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf 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.matrix.api.core.EventId import io.element.android.libraries.matrix.api.room.MatrixRoom import io.element.android.libraries.matrix.api.timeline.MatrixTimeline +import kotlinx.collections.immutable.ImmutableList import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach @@ -52,6 +56,9 @@ class TimelinePresenter @Inject constructor( mutableStateOf(null) } + var lastReadMarkerIndex by rememberSaveable { mutableStateOf(Int.MAX_VALUE) } + var lastReadMarkerId by rememberSaveable { mutableStateOf(null) } + val timelineItems = timelineItemsFactory .flow() .collectAsState() @@ -64,6 +71,15 @@ class TimelinePresenter @Inject constructor( when (event) { TimelineEvents.LoadMore -> localCoroutineScope.loadMore(paginationState.value) 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 + if (event.firstIndex <= lastReadMarkerIndex && eventId != lastReadMarkerId) { + lastReadMarkerIndex = event.firstIndex + lastReadMarkerId = eventId + localCoroutineScope.sendReadReceipt(eventId) + } + } } } @@ -87,6 +103,15 @@ class TimelinePresenter @Inject constructor( ) } + private fun getLastEventIdBeforeOrAt(index: Int, items: ImmutableList): EventId? { + for (item in items.subList(index, items.count())) { + if (item is TimelineItem.Event) { + return item.eventId + } + } + return null + } + private fun CoroutineScope.loadMore(paginationState: MatrixTimeline.PaginationState) = launch { if (paginationState.canBackPaginate && !paginationState.isBackPaginating) { timeline.paginateBackwards(backPaginationEventLimit, backPaginationPageSize) @@ -94,4 +119,8 @@ class TimelinePresenter @Inject constructor( Timber.v("Can't back paginate as paginationState = $paginationState") } } + + private fun CoroutineScope.sendReadReceipt(eventId: EventId) = launch { + timeline.sendReadReceipt(eventId) + } } 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 6e50d8e638..4d7f2b9978 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 @@ -85,6 +85,14 @@ 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( modifier = Modifier.fillMaxSize(), 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 a88b2251a4..9151376d4d 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 @@ -23,8 +23,12 @@ import com.google.common.truth.Truth.assertThat import io.element.android.features.messages.fixtures.aTimelineItemsFactory import io.element.android.features.messages.impl.timeline.TimelineEvents import io.element.android.features.messages.impl.timeline.TimelinePresenter +import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem +import io.element.android.libraries.matrix.api.timeline.item.virtual.VirtualTimelineItem import io.element.android.libraries.matrix.test.AN_EVENT_ID import io.element.android.libraries.matrix.test.room.FakeMatrixRoom +import io.element.android.libraries.matrix.test.room.anEventTimelineItem +import io.element.android.libraries.matrix.test.timeline.FakeMatrixTimeline import kotlinx.coroutines.test.runTest import org.junit.Test @@ -87,4 +91,73 @@ class TimelinePresenterTest { assertThat(withoutHighlightedState.highlightedEventId).isNull() } } + + @Test + fun `present - on scroll finished send read receipt if an event is before the index`() = runTest { + val timeline = FakeMatrixTimeline() + val timelineItemsFactory = aTimelineItemsFactory().apply { + replaceWith(listOf(MatrixTimelineItem.Event(anEventTimelineItem()))) + } + val room = FakeMatrixRoom(matrixTimeline = timeline) + val presenter = TimelinePresenter( + timelineItemsFactory = timelineItemsFactory, + room = room, + ) + moleculeFlow(RecompositionClock.Immediate) { + presenter.present() + }.test { + assertThat(timeline.sendReadReceiptCount).isEqualTo(0) + val initialState = awaitItem() + + initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(0)) + + assertThat(timeline.sendReadReceiptCount).isEqualTo(1) + } + } + + @Test + fun `present - on scroll finished will not send read receipt no event is before the index`() = runTest { + val timeline = FakeMatrixTimeline() + val timelineItemsFactory = aTimelineItemsFactory().apply { + replaceWith(listOf(MatrixTimelineItem.Event(anEventTimelineItem()))) + } + val room = FakeMatrixRoom(matrixTimeline = timeline) + val presenter = TimelinePresenter( + timelineItemsFactory = timelineItemsFactory, + room = room, + ) + moleculeFlow(RecompositionClock.Immediate) { + presenter.present() + }.test { + assertThat(timeline.sendReadReceiptCount).isEqualTo(0) + val initialState = awaitItem() + + initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(1)) + + assertThat(timeline.sendReadReceiptCount).isEqualTo(0) + } + } + + @Test + fun `present - on scroll finished will not send read receipt only virtual events exist before the index`() = runTest { + val timeline = FakeMatrixTimeline() + val timelineItemsFactory = aTimelineItemsFactory().apply { + replaceWith(listOf(MatrixTimelineItem.Virtual(VirtualTimelineItem.ReadMarker))) + } + val room = FakeMatrixRoom(matrixTimeline = timeline) + val presenter = TimelinePresenter( + timelineItemsFactory = timelineItemsFactory, + room = room, + ) + moleculeFlow(RecompositionClock.Immediate) { + presenter.present() + }.test { + assertThat(timeline.sendReadReceiptCount).isEqualTo(0) + val initialState = awaitItem() + + initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(1)) + + assertThat(timeline.sendReadReceiptCount).isEqualTo(0) + } + } } 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 1f45a0ddac..ff7b3e1cad 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 @@ -32,4 +32,6 @@ interface MatrixTimeline { suspend fun paginateBackwards(requestSize: Int, untilNumberOfItems: Int): Result suspend fun fetchDetailsForEvent(eventId: EventId): Result + + suspend fun sendReadReceipt(eventId: EventId): Result } 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 1893c452bb..8c96e48fd7 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 @@ -107,4 +107,10 @@ class RustMatrixTimeline( Timber.v("Success back paginating for room ${matrixRoom.roomId}") } } + + override suspend fun sendReadReceipt(eventId: EventId) = withContext(coroutineDispatchers.io) { + 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 f26905f5b1..6bad63b441 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 @@ -32,6 +32,9 @@ class FakeMatrixTimeline( private val paginationState: MutableStateFlow = MutableStateFlow(initialPaginationState) private val timelineItems: MutableStateFlow> = MutableStateFlow(initialTimelineItems) + var sendReadReceiptCount = 0 + private set + fun updatePaginationState(update: (MatrixTimeline.PaginationState.() -> MatrixTimeline.PaginationState)) { paginationState.value = update(paginationState.value) } @@ -66,4 +69,9 @@ class FakeMatrixTimeline( override suspend fun fetchDetailsForEvent(eventId: EventId): Result { return Result.success(Unit) } + + override suspend fun sendReadReceipt(eventId: EventId): Result { + sendReadReceiptCount++ + return Result.success(Unit) + } }