From a48ce4b18ca1c9df4909d658e20d745068e46476 Mon Sep 17 00:00:00 2001 From: SpiritCroc Date: Fri, 10 Feb 2023 13:08:48 +0100 Subject: [PATCH] [TESTING] Fight duplicate read markers in main timeline, pt.2 - Make my read-marker experiments backwards-compatible, by introducing a new artificial marker - Fix sometimes the marker going missing, by not deduplicating read markers, then filtering out the other one because duplicates didn't respect the thread id Needs an initial sync to fully apply. Change-Id: Id02ae19d03077016cbeb8d9a8fd5130d77931b2d --- .../src/main/res/values/strings_sc.xml | 1 + .../de/spiritcroc/matrixsdk/util/DbgUtil.kt | 2 ++ .../sdk/api/session/room/read/ReadService.kt | 2 ++ .../database/helper/ChunkEntityHelper.kt | 11 ++++++--- .../database/mapper/TimelineEventMapper.kt | 2 +- .../sync/handler/room/ReadReceiptHandler.kt | 24 +++++++++++++++++-- .../home/room/detail/TimelineFragment.kt | 6 +++++ .../timeline/TimelineEventController.kt | 6 +++-- .../VectorSettingsScDebuggingFragment.kt | 1 + vector/src/main/res/menu/menu_timeline.xml | 3 +++ 10 files changed, 50 insertions(+), 8 deletions(-) diff --git a/library/ui-strings/src/main/res/values/strings_sc.xml b/library/ui-strings/src/main/res/values/strings_sc.xml index f4718909eb..01699e99ee 100644 --- a/library/ui-strings/src/main/res/values/strings_sc.xml +++ b/library/ui-strings/src/main/res/values/strings_sc.xml @@ -167,6 +167,7 @@ Read receipt debugging ViewPager debugging Show ViewPager debugging information + Duplicate read receipts Element features diff --git a/matrix-sdk-android/src/main/java/de/spiritcroc/matrixsdk/util/DbgUtil.kt b/matrix-sdk-android/src/main/java/de/spiritcroc/matrixsdk/util/DbgUtil.kt index 6825be65ac..fb27474fb6 100644 --- a/matrix-sdk-android/src/main/java/de/spiritcroc/matrixsdk/util/DbgUtil.kt +++ b/matrix-sdk-android/src/main/java/de/spiritcroc/matrixsdk/util/DbgUtil.kt @@ -13,6 +13,7 @@ object DbgUtil { const val DBG_SHOW_DISPLAY_INDEX = "DBG_SHOW_DISPLAY_INDEX" const val DBG_VIEW_PAGER = "DBG_VIEW_PAGER" const val DBG_VIEW_PAGER_VISUALS = "DBG_VIEW_PAGER_VISUALS" + const val DBG_SHOW_DUPLICATE_READ_RECEIPTS = "DBG_SHOW_DUPLICATE_READ_RECEIPTS" private val prefs = HashMap() @@ -24,6 +25,7 @@ object DbgUtil { DBG_SHOW_DISPLAY_INDEX, DBG_VIEW_PAGER, DBG_VIEW_PAGER_VISUALS, + DBG_SHOW_DUPLICATE_READ_RECEIPTS, ) fun load(context: Context) { 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 5ebabb093c..40dae18076 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 @@ -89,5 +89,7 @@ interface ReadService { companion object { const val THREAD_ID_MAIN = "main" + // Artificial read marker, updated whenever someone sets the main or null one + const val THREAD_ID_MAIN_OR_NULL = "main_or_null" } } diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/helper/ChunkEntityHelper.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/helper/ChunkEntityHelper.kt index 5d7777480f..9a89eea00f 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/helper/ChunkEntityHelper.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/helper/ChunkEntityHelper.kt @@ -24,6 +24,7 @@ import org.matrix.android.sdk.api.session.events.model.content.EncryptedEventCon import org.matrix.android.sdk.api.session.events.model.toModel import org.matrix.android.sdk.api.session.room.model.RoomMemberContent import org.matrix.android.sdk.api.session.room.read.ReadService.Companion.THREAD_ID_MAIN +import org.matrix.android.sdk.api.session.room.read.ReadService.Companion.THREAD_ID_MAIN_OR_NULL import org.matrix.android.sdk.internal.crypto.model.SessionInfo import org.matrix.android.sdk.internal.database.mapper.asDomain import org.matrix.android.sdk.internal.database.model.ChunkEntity @@ -137,9 +138,13 @@ private fun handleReadReceipts(realm: Realm, roomId: String, eventEntity: EventE val originServerTs = eventEntity.originServerTs if (originServerTs != null) { val timestampOfEvent = originServerTs.toDouble() - // null receipts also update the main receipt - val receiptDestination = eventEntity.rootThreadEventId ?: THREAD_ID_MAIN - setOf(receiptDestination, eventEntity.rootThreadEventId).distinct().forEach { rootThreadEventId -> + // SC: fight duplicate read receipts in main timeline + val receiptDestinations = if (eventEntity.rootThreadEventId in listOf(null, THREAD_ID_MAIN)) { + setOf(eventEntity.rootThreadEventId, THREAD_ID_MAIN_OR_NULL) + } else { + setOf(eventEntity.rootThreadEventId) + } + receiptDestinations.forEach { rootThreadEventId -> val readReceiptOfSender = ReadReceiptEntity.getOrCreate(realm, roomId = roomId, userId = senderId, threadId = rootThreadEventId) // If the synced RR is older, update if (timestampOfEvent > readReceiptOfSender.originServerTs) { diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/TimelineEventMapper.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/TimelineEventMapper.kt index e658622444..ccaa4c09c8 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/TimelineEventMapper.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/mapper/TimelineEventMapper.kt @@ -49,7 +49,7 @@ internal class TimelineEventMapper @Inject constructor(private val readReceiptsS ownedByThreadChunk = timelineEventEntity.ownedByThreadChunk, readReceipts = readReceipts ?.distinctBy { - it.roomMember + Pair(it.roomMember, it.threadId) }?.sortedByDescending { it.originServerTs }.orEmpty() diff --git a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/handler/room/ReadReceiptHandler.kt b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/handler/room/ReadReceiptHandler.kt index 64343689ab..d34806f6c3 100644 --- a/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/handler/room/ReadReceiptHandler.kt +++ b/matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/sync/handler/room/ReadReceiptHandler.kt @@ -19,8 +19,11 @@ package org.matrix.android.sdk.internal.session.sync.handler.room import de.spiritcroc.matrixsdk.util.DbgUtil import de.spiritcroc.matrixsdk.util.Dimber import io.realm.Realm +import org.matrix.android.sdk.api.extensions.orTrue import org.matrix.android.sdk.api.session.events.model.EventType import org.matrix.android.sdk.api.session.room.read.ReadService +import org.matrix.android.sdk.api.session.room.read.ReadService.Companion.THREAD_ID_MAIN +import org.matrix.android.sdk.api.session.room.read.ReadService.Companion.THREAD_ID_MAIN_OR_NULL import org.matrix.android.sdk.internal.database.model.ReadReceiptEntity import org.matrix.android.sdk.internal.database.model.ReadReceiptsSummaryEntity import org.matrix.android.sdk.internal.database.query.createUnmanaged @@ -104,6 +107,9 @@ internal class ReadReceiptHandler @Inject constructor( private fun initialSyncStrategy(realm: Realm, roomId: String, content: ReadReceiptContent) { val readReceiptSummaries = ArrayList() + // SC: fight duplicate read markers + val summariesByEventId = HashMap() + val mainReceiptByUserId = HashMap() for ((eventId, receiptDict) in content) { val userIdsDict = receiptDict[READ_KEY] ?: continue val readReceiptsSummary = ReadReceiptsSummaryEntity(eventId = eventId, roomId = roomId) @@ -114,8 +120,16 @@ internal class ReadReceiptHandler @Inject constructor( val receiptEntity = ReadReceiptEntity.createUnmanaged(roomId, eventId, userId, threadId, ts) rrDimber.i{"Handle initial sync RR $roomId / $userId thread $threadId: event $eventId"} readReceiptsSummary.readReceipts.add(receiptEntity) + // SC: fight duplicate read markers, by unifying main|null into the same marker + if (threadId in listOf(null, THREAD_ID_MAIN) && mainReceiptByUserId[userId]?.originServerTs?.let { it < ts }.orTrue()) { + mainReceiptByUserId[userId] = ReadReceiptEntity.createUnmanaged(roomId, eventId, userId, THREAD_ID_MAIN_OR_NULL, ts) + } } readReceiptSummaries.add(readReceiptsSummary) + summariesByEventId[eventId] = readReceiptsSummary + } + mainReceiptByUserId.forEach { + summariesByEventId[it.value.eventId]?.readReceipts?.add(it.value) } realm.insertOrUpdate(readReceiptSummaries) } @@ -147,8 +161,14 @@ internal class ReadReceiptHandler @Inject constructor( for ((userId, paramsDict) in userIdsDict) { val ts = paramsDict[TIMESTAMP_KEY] as? Double ?: 0.0 val syncedThreadId = paramsDict[THREAD_ID_KEY] as String? - val receiptDestination = syncedThreadId ?: ReadService.THREAD_ID_MAIN - setOf(receiptDestination, syncedThreadId).distinct().forEach { threadId -> + + // SC: fight duplicate read receipts in main timeline + val receiptDestinations = if (syncedThreadId in listOf(null, ReadService.THREAD_ID_MAIN)) { + setOf(syncedThreadId, ReadService.THREAD_ID_MAIN_OR_NULL) + } else { + setOf(syncedThreadId) + } + receiptDestinations.forEach { threadId -> val receiptEntity = ReadReceiptEntity.getOrCreate(realm, roomId, userId, threadId) // ensure new ts is superior to the previous one if (ts > receiptEntity.originServerTs) { diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt index cfb99391d7..3be5816d2c 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/detail/TimelineFragment.kt @@ -921,6 +921,7 @@ class TimelineFragment : menu.findItem(R.id.dev_membership_changes).isChecked = vectorPreferences.showJoinLeaveMessages() menu.findItem(R.id.dev_display_name_changes).isChecked = vectorPreferences.showAvatarDisplayNameChangeMessages() menu.findItem(R.id.dev_redacted).isChecked = vectorPreferences.showRedactedMessages() + menu.findItem(R.id.dev_duplicated_read_receipts).isChecked = DbgUtil.isDbgEnabled(DbgUtil.DBG_SHOW_DUPLICATE_READ_RECEIPTS) // Composer features menu.findItem(R.id.dev_composer_voice_message_button).isChecked = vectorPreferences.useVoiceMessage() @@ -1042,6 +1043,11 @@ class TimelineFragment : reloadTimeline() true } + R.id.dev_duplicated_read_receipts -> item.toggleExec { shouldShow -> + DbgUtil.onPreferenceChanged(requireContext(), DbgUtil.DBG_SHOW_DUPLICATE_READ_RECEIPTS, shouldShow) + reloadTimeline() + true + } R.id.dev_composer_voice_message_button -> item.toggleExec { enabled -> vectorPreferences.setVoiceMessageButtonEnabled(enabled) requireActivity().restart() diff --git a/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/TimelineEventController.kt b/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/TimelineEventController.kt index 1f1b4a5fb4..887433b3ab 100644 --- a/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/TimelineEventController.kt +++ b/vector/src/main/java/im/vector/app/features/home/room/detail/timeline/TimelineEventController.kt @@ -25,6 +25,7 @@ import androidx.recyclerview.widget.RecyclerView import com.airbnb.epoxy.EpoxyController import com.airbnb.epoxy.EpoxyModel import com.airbnb.epoxy.VisibilityState +import de.spiritcroc.matrixsdk.util.DbgUtil import im.vector.app.core.date.DateFormatKind import im.vector.app.core.date.VectorDateFormatter import im.vector.app.core.epoxy.LoadingItem_ @@ -59,7 +60,6 @@ import im.vector.app.features.home.room.detail.timeline.item.MessageInformationD import im.vector.app.features.home.room.detail.timeline.item.ReactionsSummaryEvents import im.vector.app.features.home.room.detail.timeline.item.ReadReceiptData import im.vector.app.features.home.room.detail.timeline.item.ReadReceiptsItem -import im.vector.app.features.home.room.detail.timeline.item.TypingItem_ import im.vector.app.features.home.room.detail.timeline.reply.ReplyPreviewRetriever import im.vector.app.features.home.room.detail.timeline.url.PreviewUrlRetriever import im.vector.app.features.media.AttachmentData @@ -627,8 +627,10 @@ class TimelineEventController @Inject constructor( private fun ReadReceipt.isVisibleInThisThread(): Boolean { return if (partialState.isFromThreadTimeline()) { this.threadId == partialState.rootThreadEventId + } else if (DbgUtil.isDbgEnabled(DbgUtil.DBG_SHOW_DUPLICATE_READ_RECEIPTS)) { + this.threadId in listOf(null, ReadService.THREAD_ID_MAIN, ReadService.THREAD_ID_MAIN_OR_NULL) } else { - this.threadId == ReadService.THREAD_ID_MAIN + this.threadId == ReadService.THREAD_ID_MAIN_OR_NULL } } diff --git a/vector/src/main/java/im/vector/app/features/settings/VectorSettingsScDebuggingFragment.kt b/vector/src/main/java/im/vector/app/features/settings/VectorSettingsScDebuggingFragment.kt index f4f7c63c4a..48ea763812 100644 --- a/vector/src/main/java/im/vector/app/features/settings/VectorSettingsScDebuggingFragment.kt +++ b/vector/src/main/java/im/vector/app/features/settings/VectorSettingsScDebuggingFragment.kt @@ -31,6 +31,7 @@ class VectorSettingsScDebuggingFragment @Inject constructor( DbgPref(DbgUtil.DBG_SHOW_DISPLAY_INDEX, R.string.settings_sc_dbg_show_display_index), DbgPref(DbgUtil.DBG_SHOW_READ_TRACKING, R.string.settings_sc_dbg_show_read_tracking), DbgPref(DbgUtil.DBG_VIEW_PAGER_VISUALS, R.string.settings_sc_dbg_view_pager_visuals), + DbgPref(DbgUtil.DBG_SHOW_DUPLICATE_READ_RECEIPTS, R.string.settings_sc_dbg_show_duplicate_read_receipts), ) val dbgPrefs = dbgLoggingPrefs + dbgVisualsPrefs diff --git a/vector/src/main/res/menu/menu_timeline.xml b/vector/src/main/res/menu/menu_timeline.xml index f7ef814a5a..4a19e19065 100644 --- a/vector/src/main/res/menu/menu_timeline.xml +++ b/vector/src/main/res/menu/menu_timeline.xml @@ -185,6 +185,9 @@ +