Feature/fga/simplify timeline logic

changelog.d/6318.bugfix

@@ -0,0 +1 @@
+Fix loop in timeline and simplify management of chunks and timeline events.

matrix-sdk-android/src/androidTest/java/org/matrix/android/sdk/session/room/timeline/ChunkEntityTest.kt

@@ -22,21 +22,18 @@ import io.realm.Realm
 import io.realm.RealmConfiguration
 import io.realm.kotlin.createObject
 import org.amshove.kluent.shouldBeEqualTo
-import org.amshove.kluent.shouldBeTrue
 import org.junit.Before
 import org.junit.Test
 import org.junit.runner.RunWith
 import org.matrix.android.sdk.InstrumentedTest
 import org.matrix.android.sdk.api.session.events.model.Event
 import org.matrix.android.sdk.api.session.room.send.SendState
 import org.matrix.android.sdk.internal.database.helper.addTimelineEvent
-import org.matrix.android.sdk.internal.database.helper.merge
 import org.matrix.android.sdk.internal.database.mapper.toEntity
 import org.matrix.android.sdk.internal.database.model.ChunkEntity
 import org.matrix.android.sdk.internal.database.model.SessionRealmModule
 import org.matrix.android.sdk.internal.session.room.timeline.PaginationDirection
 import org.matrix.android.sdk.internal.util.time.DefaultClock
-import org.matrix.android.sdk.session.room.timeline.RoomDataHelper.createFakeListOfEvents
 import org.matrix.android.sdk.session.room.timeline.RoomDataHelper.createFakeMessageEvent
 
 @RunWith(AndroidJUnit4::class)
@@ -97,63 +94,6 @@ internal class ChunkEntityTest : InstrumentedTest {
         }
     }
 
-    @Test
-    fun merge_shouldAddEvents_whenMergingBackward() {
-        monarchy.runTransactionSync { realm ->
-            val chunk1: ChunkEntity = realm.createObject()
-            val chunk2: ChunkEntity = realm.createObject()
-            chunk1.addAll(ROOM_ID, createFakeListOfEvents(30), PaginationDirection.BACKWARDS)
-            chunk2.addAll(ROOM_ID, createFakeListOfEvents(30), PaginationDirection.BACKWARDS)
-            chunk1.merge(ROOM_ID, chunk2, PaginationDirection.BACKWARDS)
-            chunk1.timelineEvents.size shouldBeEqualTo 60
-        }
-    }
-
-    @Test
-    fun merge_shouldAddOnlyDifferentEvents_whenMergingBackward() {
-        monarchy.runTransactionSync { realm ->
-            val chunk1: ChunkEntity = realm.createObject()
-            val chunk2: ChunkEntity = realm.createObject()
-            val eventsForChunk1 = createFakeListOfEvents(30)
-            val eventsForChunk2 = eventsForChunk1 + createFakeListOfEvents(10)
-            chunk1.isLastForward = true
-            chunk2.isLastForward = false
-            chunk1.addAll(ROOM_ID, eventsForChunk1, PaginationDirection.FORWARDS)
-            chunk2.addAll(ROOM_ID, eventsForChunk2, PaginationDirection.BACKWARDS)
-            chunk1.merge(ROOM_ID, chunk2, PaginationDirection.BACKWARDS)
-            chunk1.timelineEvents.size shouldBeEqualTo 40
-            chunk1.isLastForward.shouldBeTrue()
-        }
-    }
-
-    @Test
-    fun merge_shouldPrevTokenMerged_whenMergingForwards() {
-        monarchy.runTransactionSync { realm ->
-            val chunk1: ChunkEntity = realm.createObject()
-            val chunk2: ChunkEntity = realm.createObject()
-            val prevToken = "prev_token"
-            chunk1.prevToken = prevToken
-            chunk1.addAll(ROOM_ID, createFakeListOfEvents(30), PaginationDirection.BACKWARDS)
-            chunk2.addAll(ROOM_ID, createFakeListOfEvents(30), PaginationDirection.BACKWARDS)
-            chunk1.merge(ROOM_ID, chunk2, PaginationDirection.FORWARDS)
-            chunk1.prevToken shouldBeEqualTo prevToken
-        }
-    }
-
-    @Test
-    fun merge_shouldNextTokenMerged_whenMergingBackwards() {
-        monarchy.runTransactionSync { realm ->
-            val chunk1: ChunkEntity = realm.createObject()
-            val chunk2: ChunkEntity = realm.createObject()
-            val nextToken = "next_token"
-            chunk1.nextToken = nextToken
-            chunk1.addAll(ROOM_ID, createFakeListOfEvents(30), PaginationDirection.BACKWARDS)
-            chunk2.addAll(ROOM_ID, createFakeListOfEvents(30), PaginationDirection.BACKWARDS)
-            chunk1.merge(ROOM_ID, chunk2, PaginationDirection.BACKWARDS)
-            chunk1.nextToken shouldBeEqualTo nextToken
-        }
-    }
-
     private fun ChunkEntity.addAll(
             roomId: String,
             events: List<Event>,

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/RealmSessionStoreMigration.kt

@@ -47,6 +47,7 @@ import org.matrix.android.sdk.internal.database.migration.MigrateSessionTo026
 import org.matrix.android.sdk.internal.database.migration.MigrateSessionTo027
 import org.matrix.android.sdk.internal.database.migration.MigrateSessionTo028
 import org.matrix.android.sdk.internal.database.migration.MigrateSessionTo029
+import org.matrix.android.sdk.internal.database.migration.MigrateSessionTo030
 import org.matrix.android.sdk.internal.util.Normalizer
 import timber.log.Timber
 import javax.inject.Inject
@@ -61,7 +62,7 @@ internal class RealmSessionStoreMigration @Inject constructor(
     override fun equals(other: Any?) = other is RealmSessionStoreMigration
     override fun hashCode() = 1000
 
-    val schemaVersion = 29L
+    val schemaVersion = 30L
 
     override fun migrate(realm: DynamicRealm, oldVersion: Long, newVersion: Long) {
         Timber.d("Migrating Realm Session from $oldVersion to $newVersion")
@@ -95,5 +96,6 @@ internal class RealmSessionStoreMigration @Inject constructor(
         if (oldVersion < 27) MigrateSessionTo027(realm).perform()
         if (oldVersion < 28) MigrateSessionTo028(realm).perform()
         if (oldVersion < 29) MigrateSessionTo029(realm).perform()
+        if (oldVersion < 30) MigrateSessionTo030(realm).perform()
     }
 }

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/helper/ChunkEntityHelper.kt

@@ -17,7 +17,6 @@
 package org.matrix.android.sdk.internal.database.helper
 
 import io.realm.Realm
-import io.realm.Sort
 import io.realm.kotlin.createObject
 import org.matrix.android.sdk.api.session.room.model.RoomMemberContent
 import org.matrix.android.sdk.internal.database.model.ChunkEntity
@@ -34,32 +33,9 @@ import org.matrix.android.sdk.internal.database.model.TimelineEventEntityFields
 import org.matrix.android.sdk.internal.database.query.find
 import org.matrix.android.sdk.internal.database.query.getOrCreate
 import org.matrix.android.sdk.internal.database.query.where
-import org.matrix.android.sdk.internal.database.query.whereRoomId
-import org.matrix.android.sdk.internal.extensions.assertIsManaged
 import org.matrix.android.sdk.internal.session.room.timeline.PaginationDirection
 import timber.log.Timber
 
-internal fun ChunkEntity.merge(roomId: String, chunkToMerge: ChunkEntity, direction: PaginationDirection) {
-    assertIsManaged()
-    val localRealm = this.realm
-    val eventsToMerge: List<TimelineEventEntity>
-    if (direction == PaginationDirection.FORWARDS) {
-        this.nextToken = chunkToMerge.nextToken
-        this.isLastForward = chunkToMerge.isLastForward
-        eventsToMerge = chunkToMerge.timelineEvents.sort(TimelineEventEntityFields.DISPLAY_INDEX, Sort.ASCENDING)
-    } else {
-        this.prevToken = chunkToMerge.prevToken
-        this.isLastBackward = chunkToMerge.isLastBackward
-        eventsToMerge = chunkToMerge.timelineEvents.sort(TimelineEventEntityFields.DISPLAY_INDEX, Sort.DESCENDING)
-    }
-    chunkToMerge.stateEvents.forEach { stateEvent ->
-        addStateEvent(roomId, stateEvent, direction)
-    }
-    eventsToMerge.forEach {
-        addTimelineEventFromMerge(localRealm, it, direction)
-    }
-}
-
 internal fun ChunkEntity.addStateEvent(roomId: String, stateEvent: EventEntity, direction: PaginationDirection) {
     if (direction == PaginationDirection.BACKWARDS) {
         Timber.v("We don't keep chunk state events when paginating backward")
@@ -144,40 +120,6 @@ internal fun computeIsUnique(
     }
 }
 
-private fun ChunkEntity.addTimelineEventFromMerge(realm: Realm, timelineEventEntity: TimelineEventEntity, direction: PaginationDirection) {
-    val eventId = timelineEventEntity.eventId
-    if (timelineEvents.find(eventId) != null) {
-        return
-    }
-    val displayIndex = nextDisplayIndex(direction)
-    val localId = TimelineEventEntity.nextId(realm)
-    val copied = realm.createObject<TimelineEventEntity>().apply {
-        this.localId = localId
-        this.root = timelineEventEntity.root
-        this.eventId = timelineEventEntity.eventId
-        this.roomId = timelineEventEntity.roomId
-        this.annotations = timelineEventEntity.annotations
-        this.readReceipts = timelineEventEntity.readReceipts
-        this.displayIndex = displayIndex
-        this.senderAvatar = timelineEventEntity.senderAvatar
-        this.senderName = timelineEventEntity.senderName
-        this.isUniqueDisplayName = timelineEventEntity.isUniqueDisplayName
-    }
-    handleThreadSummary(realm, eventId, copied)
-    timelineEvents.add(copied)
-}
-
-/**
- * Upon copy of the timeline events we should update the latestMessage TimelineEventEntity with the new one.
- */
-private fun handleThreadSummary(realm: Realm, oldEventId: String, newTimelineEventEntity: TimelineEventEntity) {
-    EventEntity
-            .whereRoomId(realm, newTimelineEventEntity.roomId)
-            .equalTo(EventEntityFields.IS_ROOT_THREAD, true)
-            .equalTo(EventEntityFields.THREAD_SUMMARY_LATEST_MESSAGE.EVENT_ID, oldEventId)
-            .findFirst()?.threadSummaryLatestMessage = newTimelineEventEntity
-}
-
 private fun handleReadReceipts(realm: Realm, roomId: String, eventEntity: EventEntity, senderId: String): ReadReceiptsSummaryEntity {
     val readReceiptsSummaryEntity = ReadReceiptsSummaryEntity.where(realm, eventEntity.eventId).findFirst()
             ?: realm.createObject<ReadReceiptsSummaryEntity>(eventEntity.eventId).apply {

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/migration/MigrateSessionTo030.kt

@@ -0,0 +1,48 @@
+/*
+ * Copyright (c) 2022 The Matrix.org Foundation C.I.C.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.matrix.android.sdk.internal.database.migration
+
+import io.realm.DynamicRealm
+import org.matrix.android.sdk.internal.database.model.ChunkEntityFields
+import org.matrix.android.sdk.internal.database.model.TimelineEventEntityFields
+import org.matrix.android.sdk.internal.extensions.clearWith
+import org.matrix.android.sdk.internal.util.database.RealmMigrator
+
+/**
+ * Migrating to:
+ * Cleaning old chunks which may have broken links.
+ */
+internal class MigrateSessionTo030(realm: DynamicRealm) : RealmMigrator(realm, 30) {
+
+    override fun doMigrate(realm: DynamicRealm) {
+        // Delete all previous chunks
+        val chunks = realm.where("ChunkEntity")
+                .equalTo(ChunkEntityFields.IS_LAST_FORWARD, false)
+                .findAll()
+
+        chunks.forEach { chunk ->
+            chunk.getList(ChunkEntityFields.TIMELINE_EVENTS.`$`).clearWith { timelineEvent ->
+                // Don't delete state events
+                if (timelineEvent.isNull(TimelineEventEntityFields.ROOT.TYPE)) {
+                    timelineEvent.getObject(TimelineEventEntityFields.ROOT.`$`)?.deleteFromRealm()
+                    timelineEvent.deleteFromRealm()
+                }
+            }
+            chunk.deleteFromRealm()
+        }
+    }
+}

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/database/query/ChunkEntityQueries.kt

@@ -31,6 +31,7 @@ internal fun ChunkEntity.Companion.where(realm: Realm, roomId: String): RealmQue
 
 internal fun ChunkEntity.Companion.find(realm: Realm, roomId: String, prevToken: String? = null, nextToken: String? = null): ChunkEntity? {
     val query = where(realm, roomId)
+    if (prevToken == null && nextToken == null) return null
     if (prevToken != null) {
         query.equalTo(ChunkEntityFields.PREV_TOKEN, prevToken)
     }
@@ -40,7 +41,7 @@ internal fun ChunkEntity.Companion.find(realm: Realm, roomId: String, prevToken:
     return query.findFirst()
 }
 
-internal fun ChunkEntity.Companion.findAll(realm: Realm, roomId: String, prevToken: String? = null, nextToken: String? = null): RealmResults<ChunkEntity>? {
+internal fun ChunkEntity.Companion.findAll(realm: Realm, roomId: String, prevToken: String? = null, nextToken: String? = null): RealmResults<ChunkEntity> {
     val query = where(realm, roomId)
     if (prevToken != null) {
         query.equalTo(ChunkEntityFields.PREV_TOKEN, prevToken)

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/DefaultTimeline.kt

@@ -18,6 +18,7 @@ package org.matrix.android.sdk.internal.session.room.timeline
 
 import io.realm.Realm
 import io.realm.RealmConfiguration
+import kotlinx.coroutines.CancellationException
 import kotlinx.coroutines.CoroutineScope
 import kotlinx.coroutines.SupervisorJob
 import kotlinx.coroutines.android.asCoroutineDispatcher
@@ -235,11 +236,15 @@ internal class DefaultTimeline(
         val loadMoreResult = try {
             strategy.loadMore(count, direction, fetchOnServerIfNeeded)
         } catch (throwable: Throwable) {
-            // Timeline could not be loaded with a (likely) permanent issue, such as the
-            // server now knowing the initialEventId, so we want to show an error message
-            // and possibly restart without initialEventId.
-            onTimelineFailure(throwable)
-            return false
+            if (throwable is CancellationException) {
+                LoadMoreResult.FAILURE
+            } else {
+                // Timeline could not be loaded with a (likely) permanent issue, such as the
+                // server now knowing the initialEventId, so we want to show an error message
+                // and possibly restart without initialEventId.
+                onTimelineFailure(throwable)
+                return false
+            }
         }
         Timber.v("$baseLogMessage: result $loadMoreResult")
         val hasMoreToLoad = loadMoreResult != LoadMoreResult.REACHED_END

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/TimelineChunk.kt

@@ -70,6 +70,7 @@ internal class TimelineChunk(
 
     private val isLastForward = AtomicBoolean(chunkEntity.isLastForward)
     private val isLastBackward = AtomicBoolean(chunkEntity.isLastBackward)
+    private val nextToken = chunkEntity.nextToken
     private var prevChunkLatch: CompletableDeferred<Unit>? = null
     private var nextChunkLatch: CompletableDeferred<Unit>? = null
 
@@ -136,8 +137,10 @@ internal class TimelineChunk(
             val prevEvents = prevChunk?.builtItems(includesNext = false, includesPrev = true).orEmpty()
             deepBuiltItems.addAll(prevEvents)
         }
-
-        return deepBuiltItems
+        // In some scenario (permalink) we might end up with duplicate timeline events, so we want to be sure we only expose one.
+        return deepBuiltItems.distinctBy {
+            it.eventId
+        }
     }
 
     /**
@@ -154,10 +157,6 @@ internal class TimelineChunk(
         val loadFromStorage = loadFromStorage(count, direction).also {
             logLoadedFromStorage(it, direction)
         }
-        if (loadFromStorage.numberOfEvents == 6) {
-            Timber.i("here")
-        }
-
         val offsetCount = count - loadFromStorage.numberOfEvents
 
         return if (offsetCount == 0) {
@@ -251,10 +250,6 @@ internal class TimelineChunk(
     }
 
     fun getBuiltEventIndex(eventId: String, searchInNext: Boolean, searchInPrev: Boolean): Int? {
-        val builtEventIndex = builtEventsIndexes[eventId]
-        if (builtEventIndex != null) {
-            return getOffsetIndex() + builtEventIndex
-        }
         if (searchInNext) {
             val nextBuiltEventIndex = nextChunk?.getBuiltEventIndex(eventId, searchInNext = true, searchInPrev = false)
             if (nextBuiltEventIndex != null) {
@@ -267,7 +262,12 @@ internal class TimelineChunk(
                 return prevBuiltEventIndex
             }
         }
-        return null
+        val builtEventIndex = builtEventsIndexes[eventId]
+        return if (builtEventIndex != null) {
+            getOffsetIndex() + builtEventIndex
+        } else {
+            null
+        }
     }
 
     fun getBuiltEvent(eventId: String, searchInNext: Boolean, searchInPrev: Boolean): TimelineEvent? {
@@ -445,7 +445,7 @@ internal class TimelineChunk(
             Timber.e(failure, "Failed to fetch from server")
             LoadMoreResult.FAILURE
         }
-        return if (loadMoreResult == LoadMoreResult.SUCCESS) {
+        return if (loadMoreResult != LoadMoreResult.FAILURE) {
             latch?.await()
             loadMore(count, direction, fetchOnServerIfNeeded = false)
         } else {
@@ -470,11 +470,15 @@ internal class TimelineChunk(
     }
 
     private fun getOffsetIndex(): Int {
+        if (nextToken == null) return 0
         var offset = 0
         var currentNextChunk = nextChunk
         while (currentNextChunk != null) {
             offset += currentNextChunk.builtEvents.size
-            currentNextChunk = currentNextChunk.nextChunk
+            currentNextChunk = currentNextChunk.nextChunk?.takeIf {
+                // In case of permalink we can end up with a linked nextChunk (which is the lastForward Chunk) but no nextToken
+                it.nextToken != null
+            }
         }
         return offset
     }

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/TimelineEventDataSource.kt

@@ -56,7 +56,8 @@ internal class TimelineEventDataSource @Inject constructor(
         // TODO pretty bad query.. maybe we should denormalize clear type in base?
         return realmSessionProvider.withRealm { realm ->
             TimelineEventEntity.whereRoomId(realm, roomId)
-                    .sort(TimelineEventEntityFields.DISPLAY_INDEX, Sort.ASCENDING)
+                    .sort(TimelineEventEntityFields.ROOT.ORIGIN_SERVER_TS, Sort.ASCENDING)
+                    .distinct(TimelineEventEntityFields.EVENT_ID)
                     .findAll()
                     ?.mapNotNull { timelineEventMapper.map(it).takeIf { it.root.isImageMessage() || it.root.isVideoMessage() } }
                     .orEmpty()

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/room/timeline/TokenChunkEvent.kt

@@ -24,5 +24,5 @@ internal interface TokenChunkEvent {
     val events: List<Event>
     val stateEvents: List<Event>?
 
-    fun hasMore() = start != end
+    fun hasMore() = end != null && start != end
 }