letro/letro-android
1
0
Fork 0
Code Issues Pull Requests Actions 6 Packages Projects Releases Wiki Activity

Merge pull request #2041 from element-hq/feature/fga/fix_timeline_back_pagination_loop

Browse Source 
Fix timeline back pagination loop in encrypted room.
This commit is contained in:
Benoit Marty
2023-12-21 15:42:23 +01:00
committed by GitHub
parent 15e61d3bd9 a47e654601
commit 6b16995812
10 changed files with 84 additions and 123 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/event/TimelineItemEventFactory.kt
View File

@@ -80,7 +80,7 @@ class TimelineItemEventFactory @Inject constructor(
)
currentTimelineItem.event
return TimelineItem.Event(
id = currentTimelineItem.uniqueId.toString(),
id = currentTimelineItem.uniqueId,
eventId = currentTimelineItem.eventId,
transactionId = currentTimelineItem.transactionId,
senderId = currentSender,

7
features/messages/impl/src/main/kotlin/io/element/android/features/messages/impl/timeline/factories/virtual/TimelineItemVirtualFactory.kt
View File

@@ -31,13 +31,8 @@ class TimelineItemVirtualFactory @Inject constructor(
fun create(
virtualTimelineItem: MatrixTimelineItem.Virtual,
): TimelineItem.Virtual {
val id = if (virtualTimelineItem.virtual is VirtualTimelineItem.EncryptedHistoryBanner) {
"encrypted_history_banner"
} else {
virtualTimelineItem.uniqueId.toString()
}
return TimelineItem.Virtual(
id = id,
id = virtualTimelineItem.uniqueId,
model = virtualTimelineItem.computeModel()
)
}

18
features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/timeline/TimelinePresenterTest.kt
View File

@@ -60,6 +60,8 @@ import org.junit.Rule
import org.junit.Test
import java.util.Date
private const val FAKE_UNIQUE_ID = "FAKE_UNIQUE_ID"
class TimelinePresenterTest {
@get:Rule
@@ -120,7 +122,7 @@ class TimelinePresenterTest {
fun `present - on scroll finished send read receipt if an event is before the index`() = runTest {
val timeline = FakeMatrixTimeline(
initialTimelineItems = listOf(
MatrixTimelineItem.Event(0, anEventTimelineItem())
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem())
)
)
val presenter = createTimelinePresenter(timeline)
@@ -144,7 +146,7 @@ class TimelinePresenterTest {
fun `present - on scroll finished will not send read receipt if no event is before the index`() = runTest {
val timeline = FakeMatrixTimeline(
initialTimelineItems = listOf(
MatrixTimelineItem.Event(0, anEventTimelineItem())
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem())
)
)
val presenter = createTimelinePresenter(timeline)
@@ -168,7 +170,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(0, VirtualTimelineItem.ReadMarker)
MatrixTimelineItem.Virtual(FAKE_UNIQUE_ID, VirtualTimelineItem.ReadMarker)
)
)
val presenter = createTimelinePresenter(timeline)
@@ -199,13 +201,13 @@ class TimelinePresenterTest {
assertThat(initialState.newEventState).isEqualTo(NewEventState.None)
assertThat(initialState.timelineItems.size).isEqualTo(0)
timeline.updateTimelineItems {
listOf(MatrixTimelineItem.Event(0, anEventTimelineItem(content = aMessageContent())))
listOf(MatrixTimelineItem.Event("0", anEventTimelineItem(content = aMessageContent())))
}
consumeItemsUntilPredicate { it.timelineItems.size == 1 }
// Mimics sending a message, and assert newEventState is FromMe
timeline.updateTimelineItems { items ->
val event = anEventTimelineItem(content = aMessageContent(), isOwn = true)
items + listOf(MatrixTimelineItem.Event(1, event))
items + listOf(MatrixTimelineItem.Event("1", event))
}
consumeItemsUntilPredicate { it.timelineItems.size == 2 }
awaitLastSequentialItem().also { state ->
@@ -214,7 +216,7 @@ class TimelinePresenterTest {
// Mimics receiving a message without clearing the previous FromMe
timeline.updateTimelineItems { items ->
val event = anEventTimelineItem(content = aMessageContent())
items + listOf(MatrixTimelineItem.Event(2, event))
items + listOf(MatrixTimelineItem.Event("2", event))
}
consumeItemsUntilPredicate { it.timelineItems.size == 3 }
@@ -226,7 +228,7 @@ class TimelinePresenterTest {
// Mimics receiving a message and assert newEventState is FromOther
timeline.updateTimelineItems { items ->
val event = anEventTimelineItem(content = aMessageContent())
items + listOf(MatrixTimelineItem.Event(3, event))
items + listOf(MatrixTimelineItem.Event("3", event))
}
consumeItemsUntilPredicate { it.timelineItems.size == 4 }
awaitLastSequentialItem().also { state ->
@@ -267,7 +269,7 @@ class TimelinePresenterTest {
),
)
timeline.updateTimelineItems {
listOf(MatrixTimelineItem.Event(0, anEventTimelineItem(reactions = oneReaction)))
listOf(MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem(reactions = oneReaction)))
}
skipItems(1)
val item = awaitItem().timelineItems.first()

2
features/messages/impl/src/test/kotlin/io/element/android/features/messages/impl/voicemessages/timeline/RedactedVoiceMessageManagerTest.kt
View File

@@ -80,7 +80,7 @@ fun TestScope.aDefaultRedactedVoiceMessageManager(
fun aRedactedMatrixTimeline(eventId: EventId) = listOf<MatrixTimelineItem>(
MatrixTimelineItem.Event(
uniqueId = 0,
uniqueId = "0",
event = EventTimelineItem(
eventId = eventId,
transactionId = null,

2
features/poll/impl/src/test/kotlin/io/element/android/features/poll/impl/PollFixtures.kt
View File

@@ -31,7 +31,7 @@ fun aPollTimeline(
return FakeMatrixTimeline(
initialTimelineItems = polls.map { entry ->
MatrixTimelineItem.Event(
entry.key.hashCode().toLong(),
entry.key.value,
anEventTimelineItem(
eventId = entry.key,
content = entry.value,

4
libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/timeline/MatrixTimelineItem.kt
View File

@@ -22,12 +22,12 @@ import io.element.android.libraries.matrix.api.timeline.item.event.EventTimeline
import io.element.android.libraries.matrix.api.timeline.item.virtual.VirtualTimelineItem
sealed interface MatrixTimelineItem {
data class Event(val uniqueId: Long, val event: EventTimelineItem) : MatrixTimelineItem {
data class Event(val uniqueId: String, val event: EventTimelineItem) : MatrixTimelineItem {
val eventId: EventId? = event.eventId
val transactionId: TransactionId? = event.transactionId
}
data class Virtual(val uniqueId: Long, val virtual: VirtualTimelineItem) : MatrixTimelineItem
data class Virtual(val uniqueId: String, val virtual: VirtualTimelineItem) : MatrixTimelineItem
data object Other : MatrixTimelineItem
}

2
libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/MatrixTimelineItemMapper.kt
View File

@@ -32,7 +32,7 @@ class MatrixTimelineItemMapper(
) {
fun map(timelineItem: TimelineItem): MatrixTimelineItem = timelineItem.use {
val uniqueId = timelineItem.uniqueId().toLong()
val uniqueId = timelineItem.uniqueId().toString()
val asEvent = it.asEvent()
if (asEvent != null) {
val eventTimelineItem = eventTimelineItemMapper.map(asEvent)

92
libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/RustMatrixTimeline.kt
View File

@@ -38,7 +38,7 @@ import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.asStateFlow
import kotlinx.coroutines.flow.getAndUpdate
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.mapLatest
import kotlinx.coroutines.flow.onEach
@@ -80,7 +80,6 @@ class RustMatrixTimeline(
lastLoginTimestamp = lastLoginTimestamp,
isRoomEncrypted = matrixRoom.isEncrypted,
isKeyBackupEnabled = isKeyBackupEnabled,
paginationStateFlow = _paginationState,
dispatcher = dispatcher,
)
@@ -102,6 +101,11 @@ class RustMatrixTimeline(
override val paginationState: StateFlow<MatrixTimeline.PaginationState> = _paginationState.asStateFlow()
@OptIn(ExperimentalCoroutinesApi::class)
override val timelineItems: Flow<List<MatrixTimelineItem>> = _timelineItems.mapLatest { items ->
encryptedHistoryPostProcessor.process(items)
}
init {
Timber.d("Initialize timeline for room ${matrixRoom.roomId}")
@@ -115,9 +119,9 @@ class RustMatrixTimeline(
postDiffs(diffs)
}.launchIn(this)
innerTimeline.backPaginationStatusFlow()
paginationStateFlow()
.onEach {
postPaginationStatus(it)
_paginationState.value = it
}
.launchIn(this)
@@ -125,6 +129,44 @@ class RustMatrixTimeline(
}
}
private fun paginationStateFlow(): Flow<MatrixTimeline.PaginationState> {
return combine(
innerTimeline.backPaginationStatusFlow(),
timelineItems,
) { paginationStatus, filteredItems ->
if (filteredItems.hasEncryptionHistoryBanner()) {
return@combine MatrixTimeline.PaginationState(
isBackPaginating = false,
hasMoreToLoadBackwards = false,
beginningOfRoomReached = false,
)
}
when (paginationStatus) {
BackPaginationStatus.IDLE -> {
MatrixTimeline.PaginationState(
isBackPaginating = false,
hasMoreToLoadBackwards = true,
beginningOfRoomReached = false,
)
}
BackPaginationStatus.PAGINATING -> {
MatrixTimeline.PaginationState(
isBackPaginating = true,
hasMoreToLoadBackwards = true,
beginningOfRoomReached = false,
)
}
BackPaginationStatus.TIMELINE_START_REACHED -> {
MatrixTimeline.PaginationState(
isBackPaginating = false,
hasMoreToLoadBackwards = false,
beginningOfRoomReached = true,
)
}
}
}
}
private suspend fun fetchMembers() = withContext(dispatcher) {
initLatch.await()
try {
@@ -134,11 +176,6 @@ class RustMatrixTimeline(
}
}
@OptIn(ExperimentalCoroutinesApi::class)
override val timelineItems: Flow<List<MatrixTimelineItem>> = _timelineItems.mapLatest { items ->
encryptedHistoryPostProcessor.process(items)
}
private suspend fun postItems(items: List<TimelineItem>) = coroutineScope {
// Split the initial items in multiple list as there is no pagination in the cached data, so we can post timelineItems asap.
items.chunked(INITIAL_MAX_SIZE).reversed().forEach {
@@ -154,39 +191,6 @@ class RustMatrixTimeline(
timelineDiffProcessor.postDiffs(diffs)
}
private fun postPaginationStatus(status: BackPaginationStatus) {
_paginationState.getAndUpdate { currentPaginationState ->
if (hasEncryptionHistoryBanner()) {
return@getAndUpdate currentPaginationState.copy(
isBackPaginating = false,
hasMoreToLoadBackwards = false,
beginningOfRoomReached = false,
)
}
when (status) {
BackPaginationStatus.IDLE -> {
currentPaginationState.copy(
isBackPaginating = false,
hasMoreToLoadBackwards = true
)
}
BackPaginationStatus.PAGINATING -> {
currentPaginationState.copy(
isBackPaginating = true,
hasMoreToLoadBackwards = true
)
}
BackPaginationStatus.TIMELINE_START_REACHED -> {
currentPaginationState.copy(
isBackPaginating = false,
hasMoreToLoadBackwards = false,
beginningOfRoomReached = true,
)
}
}
}
}
override suspend fun fetchDetailsForEvent(eventId: EventId): Result<Unit> = withContext(dispatcher) {
runCatching {
innerTimeline.fetchDetailsForEvent(eventId.value)
@@ -251,8 +255,8 @@ class RustMatrixTimeline(
return _timelineItems.value.firstOrNull { (it as? MatrixTimelineItem.Event)?.eventId == eventId } as? MatrixTimelineItem.Event
}
private fun hasEncryptionHistoryBanner(): Boolean {
val firstItem = _timelineItems.value.firstOrNull()
private fun List<MatrixTimelineItem>.hasEncryptionHistoryBanner(): Boolean {
val firstItem = firstOrNull()
return firstItem is MatrixTimelineItem.Virtual
&& firstItem.virtual is VirtualTimelineItem.EncryptedHistoryBanner
}

21
libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/timeline/postprocessor/TimelineEncryptedHistoryPostProcessor.kt
View File

@@ -16,12 +16,9 @@
package io.element.android.libraries.matrix.impl.timeline.postprocessor
import io.element.android.libraries.matrix.api.timeline.MatrixTimeline
import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem
import io.element.android.libraries.matrix.api.timeline.item.virtual.VirtualTimelineItem
import kotlinx.coroutines.CoroutineDispatcher
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.flow.getAndUpdate
import kotlinx.coroutines.withContext
import timber.log.Timber
import java.util.Date
@@ -31,26 +28,12 @@ class TimelineEncryptedHistoryPostProcessor(
private val lastLoginTimestamp: Date?,
private val isRoomEncrypted: Boolean,
private val isKeyBackupEnabled: Boolean,
private val paginationStateFlow: MutableStateFlow<MatrixTimeline.PaginationState>,
) {
suspend fun process(items: List<MatrixTimelineItem>): List<MatrixTimelineItem> = withContext(dispatcher) {
Timber.d("Process on Thread=${Thread.currentThread()}")
if (!isRoomEncrypted || isKeyBackupEnabled || lastLoginTimestamp == null) return@withContext items
val filteredItems = replaceWithEncryptionHistoryBannerIfNeeded(items)
// Disable back pagination
val wasFiltered = filteredItems !== items
if (wasFiltered) {
paginationStateFlow.getAndUpdate {
it.copy(
isBackPaginating = false,
hasMoreToLoadBackwards = false,
beginningOfRoomReached = false,
)
}
}
filteredItems
replaceWithEncryptionHistoryBannerIfNeeded(items)
}
private fun replaceWithEncryptionHistoryBannerIfNeeded(list: List<MatrixTimelineItem>): List<MatrixTimelineItem> {
@@ -62,7 +45,7 @@ class TimelineEncryptedHistoryPostProcessor(
}
return if (lastEncryptedHistoryBannerIndex >= 0) {
val sublist = list.drop(lastEncryptedHistoryBannerIndex + 1).toMutableList()
sublist.add(0, MatrixTimelineItem.Virtual(0L, VirtualTimelineItem.EncryptedHistoryBanner))
sublist.add(0, MatrixTimelineItem.Virtual(VirtualTimelineItem.EncryptedHistoryBanner.toString(), VirtualTimelineItem.EncryptedHistoryBanner))
sublist
} else {
list

57
libraries/matrix/impl/src/test/kotlin/io/element/android/libraries/matrix/impl/timeline/postprocessor/TimelineEncryptedHistoryPostProcessorTest.kt
View File

@@ -17,17 +17,17 @@
package io.element.android.libraries.matrix.impl.timeline.postprocessor
import com.google.common.truth.Truth.assertThat
import io.element.android.libraries.matrix.api.timeline.MatrixTimeline
import io.element.android.libraries.matrix.api.timeline.MatrixTimelineItem
import io.element.android.libraries.matrix.api.timeline.item.virtual.VirtualTimelineItem
import io.element.android.libraries.matrix.test.timeline.anEventTimelineItem
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.test.StandardTestDispatcher
import kotlinx.coroutines.test.TestScope
import kotlinx.coroutines.test.runTest
import org.junit.Test
import java.util.Date
private const val FAKE_UNIQUE_ID = "FAKE_UNIQUE_ID"
class TimelineEncryptedHistoryPostProcessorTest {
private val defaultLastLoginTimestamp = Date(1_689_061_264L)
@@ -36,7 +36,7 @@ class TimelineEncryptedHistoryPostProcessorTest {
fun `given an unencrypted room, nothing is done`() = runTest {
val processor = createPostProcessor(isRoomEncrypted = false)
val items = listOf(
MatrixTimelineItem.Event(0L, anEventTimelineItem())
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem())
)
assertThat(processor.process(items)).isSameInstanceAs(items)
}
@@ -45,7 +45,7 @@ class TimelineEncryptedHistoryPostProcessorTest {
fun `given an encrypted room, and key backup enabled, nothing is done`() = runTest {
val processor = createPostProcessor(isKeyBackupEnabled = true)
val items = listOf(
MatrixTimelineItem.Event(0L, anEventTimelineItem())
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem())
)
assertThat(processor.process(items)).isSameInstanceAs(items)
}
@@ -54,7 +54,7 @@ class TimelineEncryptedHistoryPostProcessorTest {
fun `given a null lastLoginTimestamp, nothing is done`() = runTest {
val processor = createPostProcessor(lastLoginTimestamp = null)
val items = listOf(
MatrixTimelineItem.Event(0L, anEventTimelineItem())
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem())
)
assertThat(processor.process(items)).isSameInstanceAs(items)
}
@@ -70,7 +70,7 @@ class TimelineEncryptedHistoryPostProcessorTest {
fun `given a list with no items before lastLoginTimestamp, nothing is done`() = runTest {
val processor = createPostProcessor()
val items = listOf(
MatrixTimelineItem.Event(0L, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time + 1))
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time + 1))
)
assertThat(processor.process(items)).isSameInstanceAs(items)
}
@@ -79,49 +79,35 @@ class TimelineEncryptedHistoryPostProcessorTest {
fun `given a list with an item with equal timestamp as lastLoginTimestamp, it's replaced`() = runTest {
val processor = createPostProcessor()
val items = listOf(
MatrixTimelineItem.Event(0L, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time))
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time))
)
assertThat(processor.process(items))
.isEqualTo(listOf(MatrixTimelineItem.Virtual(0L, VirtualTimelineItem.EncryptedHistoryBanner)))
.isEqualTo(listOf(MatrixTimelineItem.Virtual(VirtualTimelineItem.EncryptedHistoryBanner.toString(), VirtualTimelineItem.EncryptedHistoryBanner)))
}
@Test
fun `given a list with an item with a lower timestamp than lastLoginTimestamp, it's replaced`() = runTest {
val processor = createPostProcessor()
val items = listOf(
MatrixTimelineItem.Event(0L, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time - 1))
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time - 1))
)
assertThat(processor.process(items)).isEqualTo(
listOf(MatrixTimelineItem.Virtual(0L, VirtualTimelineItem.EncryptedHistoryBanner))
listOf(MatrixTimelineItem.Virtual(VirtualTimelineItem.EncryptedHistoryBanner.toString(), VirtualTimelineItem.EncryptedHistoryBanner))
)
}
@Test
fun `given a list with several with lower or equal timestamps than lastLoginTimestamp, they're replaced and the user can't back paginate`() = runTest {
val paginationStateFlow = MutableStateFlow(
MatrixTimeline.PaginationState(
hasMoreToLoadBackwards = true,
isBackPaginating = false,
beginningOfRoomReached = false,
)
)
val processor = createPostProcessor(paginationStateFlow = paginationStateFlow)
fun `given a list with several with lower or equal timestamps than lastLoginTimestamp, then they're replaced`() = runTest {
val processor = createPostProcessor()
val items = listOf(
MatrixTimelineItem.Event(0L, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time - 1)),
MatrixTimelineItem.Event(0L, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time)),
MatrixTimelineItem.Event(0L, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time + 1)),
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time - 1)),
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time)),
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time + 1)),
)
assertThat(processor.process(items)).isEqualTo(
listOf(
MatrixTimelineItem.Virtual(0L, VirtualTimelineItem.EncryptedHistoryBanner),
MatrixTimelineItem.Event(0L, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time + 1))
)
)
assertThat(paginationStateFlow.value).isEqualTo(
MatrixTimeline.PaginationState(
hasMoreToLoadBackwards = false,
isBackPaginating = false,
beginningOfRoomReached = false,
MatrixTimelineItem.Virtual(VirtualTimelineItem.EncryptedHistoryBanner.toString(), VirtualTimelineItem.EncryptedHistoryBanner),
MatrixTimelineItem.Event(FAKE_UNIQUE_ID, anEventTimelineItem(timestamp = defaultLastLoginTimestamp.time + 1))
)
)
}
@@ -130,19 +116,10 @@ class TimelineEncryptedHistoryPostProcessorTest {
lastLoginTimestamp: Date? = defaultLastLoginTimestamp,
isRoomEncrypted: Boolean = true,
isKeyBackupEnabled: Boolean = false,
paginationStateFlow: MutableStateFlow<MatrixTimeline.PaginationState> =
MutableStateFlow(
MatrixTimeline.PaginationState(
hasMoreToLoadBackwards = true,
isBackPaginating = false,
beginningOfRoomReached = false,
)
)
) = TimelineEncryptedHistoryPostProcessor(
lastLoginTimestamp = lastLoginTimestamp,
isRoomEncrypted = isRoomEncrypted,
isKeyBackupEnabled = isKeyBackupEnabled,
paginationStateFlow = paginationStateFlow,
dispatcher = StandardTestDispatcher(testScheduler)
)
}