diff --git a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesPresenter.kt b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesPresenter.kt index 986dba4709..a37f6419a4 100644 --- a/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesPresenter.kt +++ b/features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/MessagesPresenter.kt @@ -159,10 +159,10 @@ class MessagesPresenter @AssistedInject constructor( } LaunchedEffect(Unit) { - // Mark the room as read on entering but don't send read receipts + // Remove the unread flag on entering but don't send read receipts // as those will be handled by the timeline. withContext(dispatchers.io) { - room.markAsRead(null) + room.setUnreadFlag(isUnread = false) } } 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 c044d96f5c..b976b14960 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 @@ -22,7 +22,6 @@ import androidx.compose.runtime.MutableState import androidx.compose.runtime.collectAsState import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue -import androidx.compose.runtime.mutableIntStateOf import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.rememberCoroutineScope @@ -90,7 +89,6 @@ class TimelinePresenter @AssistedInject constructor( mutableStateOf(null) } - val lastReadReceiptIndex = rememberSaveable { mutableIntStateOf(Int.MAX_VALUE) } val lastReadReceiptId = rememberSaveable { mutableStateOf(null) } val timelineItems by timelineItemsFactory.collectItemsAsState() @@ -128,7 +126,6 @@ class TimelinePresenter @AssistedInject constructor( appScope.sendReadReceiptIfNeeded( firstVisibleIndex = event.firstIndex, timelineItems = timelineItems, - lastReadReceiptIndex = lastReadReceiptIndex, lastReadReceiptId = lastReadReceiptId, readReceiptType = if (isSendPublicReadReceiptsEnabled) ReceiptType.READ else ReceiptType.READ_PRIVATE, ) @@ -228,16 +225,19 @@ class TimelinePresenter @AssistedInject constructor( private fun CoroutineScope.sendReadReceiptIfNeeded( firstVisibleIndex: Int, timelineItems: ImmutableList, - lastReadReceiptIndex: MutableState, lastReadReceiptId: MutableState, readReceiptType: ReceiptType, ) = 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 = eventId, receiptType = readReceiptType) + // If we are at the bottom of timeline, we mark the room as read. + if (firstVisibleIndex == 0) { + room.markAsRead(receiptType = readReceiptType) + } else { + // 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 && eventId != lastReadReceiptId.value) { + lastReadReceiptId.value = eventId + timeline.sendReadReceipt(eventId = eventId, receiptType = readReceiptType) + } } } diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/MessagesPresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/MessagesPresenterTest.kt index 44dc96d804..e1236b805d 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/MessagesPresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/MessagesPresenterTest.kt @@ -134,7 +134,7 @@ class MessagesPresenterTest { @OptIn(ExperimentalCoroutinesApi::class) @Test - fun `present - check that the room is marked as read`() = runTest { + fun `present - check that the room's unread flag is removed`() = runTest { val room = FakeMatrixRoom() assertThat(room.markAsReadCalls).isEmpty() val presenter = createMessagesPresenter(matrixRoom = room) @@ -142,7 +142,7 @@ class MessagesPresenterTest { presenter.present() }.test { runCurrent() - assertThat(room.markAsReadCalls).isEqualTo(listOf(null)) + assertThat(room.setUnreadFlagCalls).isEqualTo(listOf(false)) cancelAndIgnoreRemainingEvents() } } diff --git a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt index c01135fb5f..5ee6ca8aa0 100644 --- a/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt +++ b/features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt @@ -45,6 +45,7 @@ import io.element.android.libraries.matrix.api.timeline.item.event.ReactionSende import io.element.android.libraries.matrix.api.timeline.item.event.Receipt 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.AN_EVENT_ID_2 import io.element.android.libraries.matrix.test.A_USER_ID import io.element.android.libraries.matrix.test.encryption.FakeEncryptionService import io.element.android.libraries.matrix.test.room.FakeMatrixRoom @@ -60,8 +61,11 @@ import io.element.android.tests.testutils.awaitWithLatch import io.element.android.tests.testutils.consumeItemsUntilPredicate import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.collections.immutable.persistentListOf +import kotlinx.coroutines.ExperimentalCoroutinesApi import kotlinx.coroutines.delay +import kotlinx.coroutines.test.StandardTestDispatcher import kotlinx.coroutines.test.TestScope +import kotlinx.coroutines.test.runCurrent import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test @@ -69,6 +73,7 @@ import java.util.Date import kotlin.time.Duration.Companion.seconds private const val FAKE_UNIQUE_ID = "FAKE_UNIQUE_ID" +private const val FAKE_UNIQUE_ID_2 = "FAKE_UNIQUE_ID_2" class TimelinePresenterTest { @get:Rule @@ -125,13 +130,47 @@ class TimelinePresenterTest { } } + @OptIn(ExperimentalCoroutinesApi::class) @Test - fun `present - on scroll finished send read receipt if an event is before the index`() = runTest { + fun `present - on scroll finished mark a room as read if the first visible index is 0`() = runTest(StandardTestDispatcher()) { val timeline = FakeMatrixTimeline( initialTimelineItems = listOf( MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem()) ) ) + val sessionPreferencesStore = InMemorySessionPreferencesStore(isSendPublicReadReceiptsEnabled = false) + val room = FakeMatrixRoom(matrixTimeline = timeline) + val presenter = createTimelinePresenter( + timeline = timeline, + room = room, + sessionPreferencesStore = sessionPreferencesStore, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + assertThat(timeline.sentReadReceipts).isEmpty() + val initialState = awaitFirstItem() + initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(0)) + runCurrent() + assertThat(room.markAsReadCalls).isNotEmpty() + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `present - on scroll finished send read receipt if an event is before the index`() = runTest { + val timeline = FakeMatrixTimeline( + initialTimelineItems = listOf( + MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem()), + MatrixTimelineItem.Event( + uniqueId = FAKE_UNIQUE_ID_2, + event = anEventTimelineItem( + eventId = AN_EVENT_ID_2, + content = aMessageContent("Test message") + ) + ) + ) + ) val presenter = createTimelinePresenter(timeline) moleculeFlow(RecompositionMode.Immediate) { presenter.present() @@ -140,7 +179,7 @@ class TimelinePresenterTest { val initialState = awaitFirstItem() awaitWithLatch { latch -> timeline.sendReadReceiptLatch = latch - initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(0)) + initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(1)) } assertThat(timeline.sentReadReceipts).isNotEmpty() assertThat(timeline.sentReadReceipts.first().second).isEqualTo(ReceiptType.READ) @@ -149,10 +188,17 @@ class TimelinePresenterTest { } @Test - fun `present - on scroll finished send a private read receipt if an event is before the index and public read receipts are disabled`() = runTest { + fun `present - on scroll finished send a private read receipt if an event is at an index other than 0 and public read receipts are disabled`() = runTest { val timeline = FakeMatrixTimeline( initialTimelineItems = listOf( - MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem()) + MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem()), + MatrixTimelineItem.Event( + uniqueId = FAKE_UNIQUE_ID_2, + event = anEventTimelineItem( + eventId = AN_EVENT_ID_2, + content = aMessageContent("Test message") + ) + ) ) ) val sessionPreferencesStore = InMemorySessionPreferencesStore(isSendPublicReadReceiptsEnabled = false) @@ -168,6 +214,7 @@ class TimelinePresenterTest { awaitWithLatch { latch -> timeline.sendReadReceiptLatch = latch initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(0)) + initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(1)) } assertThat(timeline.sentReadReceipts).isNotEmpty() assertThat(timeline.sentReadReceipts.first().second).isEqualTo(ReceiptType.READ_PRIVATE) @@ -176,10 +223,17 @@ class TimelinePresenterTest { } @Test - fun `present - on scroll finished will not send read receipt if no event is before the index`() = runTest { + fun `present - on scroll finished will not send read receipt the first visible event is the same as before`() = runTest { val timeline = FakeMatrixTimeline( initialTimelineItems = listOf( - MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem()) + MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem()), + MatrixTimelineItem.Event( + uniqueId = FAKE_UNIQUE_ID_2, + event = anEventTimelineItem( + eventId = AN_EVENT_ID_2, + content = aMessageContent("Test message") + ) + ) ) ) val presenter = createTimelinePresenter(timeline) @@ -191,8 +245,9 @@ class TimelinePresenterTest { awaitWithLatch { latch -> timeline.sendReadReceiptLatch = latch initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(1)) + initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(1)) } - assertThat(timeline.sentReadReceipts).isEmpty() + assertThat(timeline.sentReadReceipts).hasSize(1) cancelAndIgnoreRemainingEvents() } } @@ -201,6 +256,7 @@ class TimelinePresenterTest { fun `present - on scroll finished will not send read receipt only virtual events exist before the index`() = runTest { val timeline = FakeMatrixTimeline( initialTimelineItems = listOf( + MatrixTimelineItem.Virtual(FAKE_UNIQUE_ID, VirtualTimelineItem.ReadMarker), MatrixTimelineItem.Virtual(FAKE_UNIQUE_ID, VirtualTimelineItem.ReadMarker) ) ) @@ -212,7 +268,7 @@ class TimelinePresenterTest { val initialState = awaitFirstItem() awaitWithLatch { latch -> timeline.sendReadReceiptLatch = latch - initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(0)) + initialState.eventSink.invoke(TimelineEvents.OnScrollFinished(1)) } assertThat(timeline.sentReadReceipts).isEmpty() cancelAndIgnoreRemainingEvents() diff --git a/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt b/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt index fa841ff88f..e14a2b9aaa 100644 --- a/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt +++ b/features/roomlist/impl/src/main/kotlin/io/element/android/features/roomlist/impl/RoomListPresenter.kt @@ -145,15 +145,20 @@ class RoomListPresenter @Inject constructor( is RoomListEvents.HideContextMenu -> contextMenu = RoomListState.ContextMenu.Hidden is RoomListEvents.LeaveRoom -> leaveRoomState.eventSink(LeaveRoomEvent.ShowConfirmation(event.roomId)) is RoomListEvents.MarkAsRead -> coroutineScope.launch { - val receiptType = if (sessionPreferencesStore.isSendPublicReadReceiptsEnabled().first()) { - ReceiptType.READ - } else { - ReceiptType.READ_PRIVATE + client.getRoom(event.roomId)?.use { room -> + room.setUnreadFlag(isUnread = false) + val receiptType = if (sessionPreferencesStore.isSendPublicReadReceiptsEnabled().first()) { + ReceiptType.READ + } else { + ReceiptType.READ_PRIVATE + } + room.markAsRead(receiptType) } - client.getRoom(event.roomId)?.markAsRead(receiptType) } is RoomListEvents.MarkAsUnread -> coroutineScope.launch { - client.getRoom(event.roomId)?.markAsUnread() + client.getRoom(event.roomId)?.use { room -> + room.setUnreadFlag(isUnread = true) + } } } } diff --git a/features/roomlist/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTests.kt b/features/roomlist/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTests.kt index 49d7eb2974..6ff8df00f0 100644 --- a/features/roomlist/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTests.kt +++ b/features/roomlist/impl/src/test/kotlin/io/element/android/features/roomlist/impl/RoomListPresenterTests.kt @@ -487,18 +487,18 @@ class RoomListPresenterTests { }.test { val initialState = awaitItem() assertThat(room.markAsReadCalls).isEmpty() - assertThat(room.markAsUnreadReadCallCount).isEqualTo(0) + assertThat(room.setUnreadFlagCalls).isEmpty() initialState.eventSink.invoke(RoomListEvents.MarkAsRead(A_ROOM_ID)) assertThat(room.markAsReadCalls).isEqualTo(listOf(ReceiptType.READ)) - assertThat(room.markAsUnreadReadCallCount).isEqualTo(0) + assertThat(room.setUnreadFlagCalls).isEqualTo(listOf(false)) initialState.eventSink.invoke(RoomListEvents.MarkAsUnread(A_ROOM_ID)) assertThat(room.markAsReadCalls).isEqualTo(listOf(ReceiptType.READ)) - assertThat(room.markAsUnreadReadCallCount).isEqualTo(1) + assertThat(room.setUnreadFlagCalls).isEqualTo(listOf(false, true)) // Test again with private read receipts sessionPreferencesStore.setSendPublicReadReceipts(false) initialState.eventSink.invoke(RoomListEvents.MarkAsRead(A_ROOM_ID)) assertThat(room.markAsReadCalls).isEqualTo(listOf(ReceiptType.READ, ReceiptType.READ_PRIVATE)) - assertThat(room.markAsUnreadReadCallCount).isEqualTo(1) + assertThat(room.setUnreadFlagCalls).isEqualTo(listOf(false, true, false)) cancelAndIgnoreRemainingEvents() scope.cancel() } diff --git a/gradle/libs.versions.toml b/gradle/libs.versions.toml index 255f8cbae6..1aeaffa3f2 100644 --- a/gradle/libs.versions.toml +++ b/gradle/libs.versions.toml @@ -152,7 +152,7 @@ jsoup = "org.jsoup:jsoup:1.17.2" appyx_core = { module = "com.bumble.appyx:core", version.ref = "appyx" } molecule-runtime = "app.cash.molecule:molecule-runtime:1.3.2" timber = "com.jakewharton.timber:timber:5.0.1" -matrix_sdk = "org.matrix.rustcomponents:sdk-android:0.2.0" +matrix_sdk = "org.matrix.rustcomponents:sdk-android:0.2.1" matrix_richtexteditor = { module = "io.element.android:wysiwyg", version.ref = "wysiwyg" } matrix_richtexteditor_compose = { module = "io.element.android:wysiwyg-compose", version.ref = "wysiwyg" } sqldelight-driver-android = { module = "app.cash.sqldelight:android-driver", version.ref = "sqldelight" } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt index db5cd6c260..22d8f56415 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/room/MatrixRoom.kt @@ -153,15 +153,17 @@ interface MatrixRoom : Closeable { suspend fun reportContent(eventId: EventId, reason: String, blockUserId: UserId?): Result /** - * Reverts a previously set unread flag, and eventually send a Read Receipt. - * @param receiptType The type of receipt to send. If null, no Read Receipt will be sent. + * Mark the room as read by trying to attach an unthreaded read receipt to the latest room event. + * @param receiptType The type of receipt to send. */ - suspend fun markAsRead(receiptType: ReceiptType?): Result + suspend fun markAsRead(receiptType: ReceiptType): Result /** - * Sets a flag on the room to indicate that the user has explicitly marked it as unread. + * Sets a flag on the room to indicate that the user has explicitly marked it as unread, or reverts the flag. + * @param isUnread true to mark the room as unread, false to remove the flag. + * */ - suspend fun markAsUnread(): Result + suspend fun setUnreadFlag(isUnread: Boolean): Result /** * Share a location message in the room. diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt index 6ab29c1840..3ce35cfaa6 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/room/RustMatrixRoom.kt @@ -442,19 +442,15 @@ class RustMatrixRoom( } } - override suspend fun markAsRead(receiptType: ReceiptType?): Result = withContext(roomDispatcher) { + override suspend fun markAsRead(receiptType: ReceiptType): Result = withContext(roomDispatcher) { runCatching { - if (receiptType != null) { - innerRoom.markAsReadAndSendReadReceipt(receiptType.toRustReceiptType()) - } else { - innerRoom.markAsRead() - } + innerRoom.markAsRead(receiptType.toRustReceiptType()) } } - override suspend fun markAsUnread(): Result = withContext(roomDispatcher) { + override suspend fun setUnreadFlag(isUnread: Boolean): Result = withContext(roomDispatcher) { runCatching { - innerRoom.markAsUnread() + innerRoom.setUnreadFlag(isUnread) } } diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt index 60969cfd0b..b3bfa66328 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/room/FakeMatrixRoom.kt @@ -378,17 +378,18 @@ class FakeMatrixRoom( return reportContentResult } - val markAsReadCalls = mutableListOf() - override suspend fun markAsRead(receiptType: ReceiptType?): Result { + val markAsReadCalls = mutableListOf() + + override suspend fun markAsRead(receiptType: ReceiptType): Result { markAsReadCalls.add(receiptType) return Result.success(Unit) } - var markAsUnreadReadCallCount = 0 + var setUnreadFlagCalls = mutableListOf() private set - override suspend fun markAsUnread(): Result { - markAsUnreadReadCallCount++ + override suspend fun setUnreadFlag(isUnread: Boolean): Result { + setUnreadFlagCalls.add(isUnread) return Result.success(Unit) }