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
This commit is contained in:
SpiritCroc 2022-06-22 12:36:16 +02:00
parent 45b7d7882c
commit 3b6b51748e
4 changed files with 33 additions and 6 deletions

View File

@ -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.

View File

@ -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

View File

@ -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<Optional<String>> {

View File

@ -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
}