From 3b6b51748efee9cbf996dedfd72e146bfd24e88b Mon Sep 17 00:00:00 2001 From: SpiritCroc Date: Wed, 22 Jun 2022 12:36:16 +0200 Subject: [PATCH] Fix clearing read but not synced messages in notifications When lots (> 10) of messages arrive in a chat, such that not all get /sync'ed, the app couldn't properly check if events that were not synced are before or after the read marker. The previous way to handle this was just to always assume these events would be unread, which caused some old messages show in notifications and not dismiss themselves when appropriate. Unfortunately, we can not safely assume that if the read marker is in the latest chunk, the missing events would be read, since we may be showing "fastlane" notifications from fcm/push before the /sync finished. Thus, with this commit, we now remove messages that weren't synced or paginated if they meet the following heuristic: 1. The read marker is in the latest chunk and 2. The read marker points at an event with a timestamp later than the one of the missing event. Change-Id: I8053252e95a3b2142512f93244647a86b6f1a231 --- .../sdk/api/session/room/read/ReadService.kt | 2 +- .../internal/database/query/ReadQueries.kt | 29 +++++++++++++++++-- .../session/room/read/DefaultReadService.kt | 4 +-- .../notifications/OutdatedEventDetector.kt | 4 ++- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/read/ReadService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/read/ReadService.kt index 51ce5a6046..8b66de67f4 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/read/ReadService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/room/read/ReadService.kt @@ -59,7 +59,7 @@ interface ReadService { /** * Check if an event is already read, ie. your read receipt is set on a more recent event. */ - fun isEventRead(eventId: String): Boolean + fun isEventRead(eventId: String, eventTs: Long? = null): Boolean /** * Returns a live read marker id for the room. diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/query/ReadQueries.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/query/ReadQueries.kt index ee770901cb..53e2eb5ad0 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/query/ReadQueries.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/query/ReadQueries.kt @@ -18,6 +18,7 @@ package org.matrix.android.sdk.internal.database.query import de.spiritcroc.matrixsdk.util.Dimber import io.realm.Realm import io.realm.RealmConfiguration +import org.matrix.android.sdk.api.extensions.orFalse import org.matrix.android.sdk.api.session.events.model.LocalEcho import org.matrix.android.sdk.internal.database.helper.isMoreRecentThan import org.matrix.android.sdk.internal.database.model.ChunkEntity @@ -30,6 +31,7 @@ internal fun isEventRead(realmConfiguration: RealmConfiguration, userId: String?, roomId: String?, eventId: String?, + eventTs: Long? = null, ignoreSenderId: Boolean = false): Boolean { if (userId.isNullOrBlank() || roomId.isNullOrBlank() || eventId.isNullOrBlank()) { return false @@ -41,8 +43,11 @@ internal fun isEventRead(realmConfiguration: RealmConfiguration, return Realm.getInstance(realmConfiguration).use { realm -> val eventToCheck = TimelineEventEntity.where(realm, roomId, eventId).findFirst() when { - // The event doesn't exist locally, let's assume it hasn't been read - eventToCheck == null -> false + // The event doesn't exist locally, let's assume it hasn't been read unless we know all unread events + eventToCheck == null -> isReadMarkerMoreRecentThanMissingEvent(realm, roomId, userId, eventTs) + //.also { + //Timber.i("isEventRead: eventToCheck ($eventId) == null -> assume read: $it") + //} !ignoreSenderId && eventToCheck.root?.sender == userId -> true // If new event exists and the latest event is from ourselves we can infer the event is read !ignoreSenderId && latestEventIsFromSelf(realm, roomId, userId) -> true @@ -52,6 +57,26 @@ internal fun isEventRead(realmConfiguration: RealmConfiguration, } } +private fun isReadMarkerMoreRecentThanMissingEvent(realm: Realm, roomId: String, userId: String, eventTs: Long?): Boolean { + if (eventTs == null) { + // We don't have enough information to do an educated guess without timestamp: + // Case 1: a fastlane event came through for which we didn't sync yet + // -> the read marker may be very well in the latest chunk, but the missing event is still unread + // Case 2: We synced all recent events, but have some gap where the missing event would be + // -> if the read marker is at the bottom, the missing event should be marked as read in this case + // => fallback to showing the notification either way + return false + } + // Assume a missing event is read if: + // - The read receipt is in the last forward chunk and + // - The timestamp of the notification is smaller than the read receipt's one + return ReadReceiptEntity.where(realm, roomId, userId).findFirst()?.let { readReceipt -> + val readReceiptEvent = TimelineEventEntity.where(realm, roomId, readReceipt.eventId).findFirst() + //Timber.i("isReadMarkerMoreRecentThanMissing? ${readReceiptEvent?.chunk?.firstOrNull()?.isLastForward} && ${(readReceiptEvent?.root?.originServerTs ?: 0) > eventTs} <- ${(readReceiptEvent?.root?.originServerTs ?: 0)} > $eventTs") + readReceiptEvent?.chunk?.firstOrNull()?.isLastForward.orFalse() && (readReceiptEvent?.root?.originServerTs ?: 0) > eventTs + } ?: false +} + private fun latestEventIsFromSelf(realm: Realm, roomId: String, userId: String) = TimelineEventEntity.latestEvent(realm, roomId, true) ?.root?.sender == userId diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/read/DefaultReadService.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/read/DefaultReadService.kt index 29ef957df8..3e3ba194ea 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/read/DefaultReadService.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/read/DefaultReadService.kt @@ -85,8 +85,8 @@ internal class DefaultReadService @AssistedInject constructor( setMarkedUnreadTask.execute(params) } - override fun isEventRead(eventId: String): Boolean { - return isEventRead(monarchy.realmConfiguration, userId, roomId, eventId) + override fun isEventRead(eventId: String, eventTs: Long?): Boolean { + return isEventRead(monarchy.realmConfiguration, userId, roomId, eventId, eventTs) } override fun getReadMarkerLive(): LiveData> { diff --git a/vector/src/main/java/im/vector/app/features/notifications/OutdatedEventDetector.kt b/vector/src/main/java/im/vector/app/features/notifications/OutdatedEventDetector.kt index bb45d74708..de1a18f580 100644 --- a/vector/src/main/java/im/vector/app/features/notifications/OutdatedEventDetector.kt +++ b/vector/src/main/java/im/vector/app/features/notifications/OutdatedEventDetector.kt @@ -35,7 +35,9 @@ class OutdatedEventDetector @Inject constructor( val eventID = notifiableEvent.eventId val roomID = notifiableEvent.roomId val room = session.getRoom(roomID) ?: return false - return room.readService().isEventRead(eventID) + // Also pass the notification timestamp, so we can use some heuristics in case of + // fastlane notifications while the read marker is in the latest chunk + return room.readService().isEventRead(eventID, notifiableEvent.timestamp.takeIf { it > 0 }) } return false }