From 72bb34bf27d8d94955a2f90bc75e479ea8b5c7fa Mon Sep 17 00:00:00 2001 From: kyori19 Date: Sat, 12 Mar 2022 17:38:48 +0900 Subject: [PATCH] Fix some network timeline bugs (#2373) * Fix network timeline gap loading * Fix fullReload keeps nextKey * Fix reload after clearing timeline * Improve logic to handle overlapped statuses --- .../viewmodel/NetworkTimelineViewModel.kt | 59 ++++++++++++++----- .../keylesspalace/tusky/util/StringUtils.kt | 13 ++++ .../keylesspalace/tusky/StringUtilsTest.kt | 15 +++++ 3 files changed, 71 insertions(+), 16 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt index bb226292..7a6df9d2 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/NetworkTimelineViewModel.kt @@ -35,8 +35,11 @@ import com.keylesspalace.tusky.entity.Status import com.keylesspalace.tusky.network.FilterModel import com.keylesspalace.tusky.network.MastodonApi import com.keylesspalace.tusky.network.TimelineCases +import com.keylesspalace.tusky.util.dec import com.keylesspalace.tusky.util.getDomain import com.keylesspalace.tusky.util.inc +import com.keylesspalace.tusky.util.isLessThan +import com.keylesspalace.tusky.util.isLessThanOrEqual import com.keylesspalace.tusky.util.toViewData import com.keylesspalace.tusky.viewdata.StatusViewData import kotlinx.coroutines.flow.map @@ -135,6 +138,10 @@ class NetworkTimelineViewModel @Inject constructor( override fun loadMore(placeholderId: String) { viewModelScope.launch { try { + val placeholderIndex = + statusData.indexOfFirst { it is StatusViewData.Placeholder && it.id == placeholderId } + statusData[placeholderIndex] = StatusViewData.Placeholder(placeholderId, isLoading = true) + val statusResponse = fetchStatusesForKind( fromId = placeholderId.inc(), uptoId = null, @@ -147,28 +154,47 @@ class NetworkTimelineViewModel @Inject constructor( return@launch } + statusData.removeAt(placeholderIndex) + val activeAccount = accountManager.activeAccount!! - val data = statuses.map { status -> - val oldStatus = statusData.find { s -> - s.asStatusOrNull()?.id == status.id - }?.asStatusOrNull() - - val contentShowing = oldStatus?.isShowingContent ?: activeAccount.alwaysShowSensitiveMedia || !status.actionableStatus.sensitive - val expanded = oldStatus?.isExpanded ?: activeAccount.alwaysOpenSpoiler - val contentCollapsed = oldStatus?.isCollapsed ?: true - status.toViewData( - isShowingContent = contentShowing, - isExpanded = expanded, - isCollapsed = contentCollapsed + isShowingContent = activeAccount.alwaysShowSensitiveMedia || !status.actionableStatus.sensitive, + isExpanded = activeAccount.alwaysOpenSpoiler, + isCollapsed = true ) + }.toMutableList() + + if (statuses.isNotEmpty()) { + val firstId = statuses.first().id + val lastId = statuses.last().id + val overlappedFrom = statusData.indexOfFirst { it.asStatusOrNull()?.id?.isLessThanOrEqual(firstId) ?: false } + val overlappedTo = statusData.indexOfFirst { it.asStatusOrNull()?.id?.isLessThan(lastId) ?: false } + + if (overlappedFrom < overlappedTo) { + data.mapIndexed { i, status -> i to statusData.firstOrNull { it.asStatusOrNull()?.id == status.id }?.asStatusOrNull() } + .filter { (_, oldStatus) -> oldStatus != null } + .forEach { (i, oldStatus) -> + data[i] = data[i] + .copy( + isShowingContent = oldStatus!!.isShowingContent, + isExpanded = oldStatus.isExpanded, + isCollapsed = oldStatus.isCollapsed, + ) + } + + statusData.removeAll { status -> + when (status) { + is StatusViewData.Placeholder -> lastId.isLessThan(status.id) && status.id.isLessThanOrEqual(firstId) + is StatusViewData.Concrete -> lastId.isLessThan(status.id) && status.id.isLessThanOrEqual(firstId) + } + } + } else { + statusData.add(overlappedFrom, StatusViewData.Placeholder(statuses.last().id.dec(), isLoading = false)) + } } - val index = - statusData.indexOfFirst { it is StatusViewData.Placeholder && it.id == placeholderId } - statusData.removeAt(index) - statusData.addAll(index, data) + statusData.addAll(placeholderIndex, data) currentSource?.invalidate() } catch (e: Exception) { @@ -214,6 +240,7 @@ class NetworkTimelineViewModel @Inject constructor( } override fun fullReload() { + nextKey = statusData.firstOrNull { it is StatusViewData.Concrete }?.asStatusOrNull()?.id?.inc() statusData.clear() currentSource?.invalidate() } diff --git a/app/src/main/java/com/keylesspalace/tusky/util/StringUtils.kt b/app/src/main/java/com/keylesspalace/tusky/util/StringUtils.kt index f28e09c5..4ce67511 100644 --- a/app/src/main/java/com/keylesspalace/tusky/util/StringUtils.kt +++ b/app/src/main/java/com/keylesspalace/tusky/util/StringUtils.kt @@ -78,6 +78,19 @@ fun String.isLessThan(other: String): Boolean { } } +/** + * A <= B (strictly) by length and then by content. + * Examples: + * "abc" <= "bcd" + * "ab" <= "abc" + * "cb" <= "abc" + * "ab" <= "ab" + * not: "abc" > "cb" + */ +fun String.isLessThanOrEqual(other: String): Boolean { + return this == other || isLessThan(other) +} + fun Spanned.trimTrailingWhitespace(): Spanned { var i = length do { diff --git a/app/src/test/java/com/keylesspalace/tusky/StringUtilsTest.kt b/app/src/test/java/com/keylesspalace/tusky/StringUtilsTest.kt index 30cc971a..c2809eb8 100644 --- a/app/src/test/java/com/keylesspalace/tusky/StringUtilsTest.kt +++ b/app/src/test/java/com/keylesspalace/tusky/StringUtilsTest.kt @@ -3,6 +3,7 @@ package com.keylesspalace.tusky import com.keylesspalace.tusky.util.dec import com.keylesspalace.tusky.util.inc import com.keylesspalace.tusky.util.isLessThan +import com.keylesspalace.tusky.util.isLessThanOrEqual import org.junit.Assert.assertEquals import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue @@ -24,6 +25,20 @@ class StringUtilsTest { notLessList.forEach { (l, r) -> assertFalse("not $l < $r", l.isLessThan(r)) } } + @Test + fun isLessThanOrEqual() { + val lessList = listOf( + "abc" to "bcd", + "ab" to "abc", + "cb" to "abc", + "1" to "2", + "abc" to "abc", + ) + lessList.forEach { (l, r) -> assertTrue("$l < $r", l.isLessThanOrEqual(r)) } + val notLessList = lessList.filterNot { (l, r) -> l == r }.map { (l, r) -> r to l } + notLessList.forEach { (l, r) -> assertFalse("not $l < $r", l.isLessThanOrEqual(r)) } + } + @Test fun inc() { listOf(