From 453a000d490856a29c5c3add2cde324f528c2c19 Mon Sep 17 00:00:00 2001 From: charlag Date: Sat, 3 Jul 2021 00:06:17 +0200 Subject: [PATCH] Fix refreshing timeline Before on initial load we tried to get current statuses using some ID math plus we've been fetching latest statuses. Updating current doesn't actually work so instead now we are doing what Mastodon itself is doing: we just fetch latest and update current if they overlap. There were also a few issues with finding overlap index (using indexOfLast using ===) so it wasn't actually working. --- .../components/timeline/TimelineViewModel.kt | 116 ++++--------- .../timeline/TimelineViewModelTest.kt | 164 +++++++++--------- 2 files changed, 114 insertions(+), 166 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineViewModel.kt index e24e09102..d6a6e10a3 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineViewModel.kt +++ b/app/src/main/java/com/keylesspalace/tusky/components/timeline/TimelineViewModel.kt @@ -126,47 +126,6 @@ class TimelineViewModel @Inject constructor( reloadFilters() } - private suspend fun updateCurrent() { - val topId = statuses.firstIsInstanceOrNull()?.id ?: return - // Request statuses including current top to refresh all of them - val topIdMinusOne = topId.inc() - val statuses = try { - loadStatuses( - maxId = topIdMinusOne, - sinceId = null, - sinceIdMinusOne = null, - TimelineRequestMode.NETWORK, - ) - } catch (t: Exception) { - initialUpdateFailed = true - if (isExpectedRequestException(t)) { - Log.d(TAG, "Failed updating timeline", t) - triggerViewUpdate() - return - } else { - throw t - } - } - - initialUpdateFailed = false - - // When cached timeline is too old, we would replace it with nothing - if (statuses.isNotEmpty()) { - val mutableStatuses = statuses.toMutableList() - filterStatuses(mutableStatuses) - this.statuses.removeAll { item -> - val id = when (item) { - is StatusViewData.Concrete -> item.id - is StatusViewData.Placeholder -> item.id - } - - id == topId || id.isLessThan(topId) - } - this.statuses.addAll(mutableStatuses.toViewData()) - } - triggerViewUpdate() - } - private fun isExpectedRequestException(t: Exception) = t is IOException || t is HttpException fun refresh(): Job { @@ -176,7 +135,6 @@ class TimelineViewModel @Inject constructor( triggerViewUpdate() try { - if (initialUpdateFailed) updateCurrent() loadAbove() } catch (e: Exception) { if (isExpectedRequestException(e)) { @@ -458,34 +416,45 @@ class TimelineViewModel @Inject constructor( return statuses } + /** Insert statuses from above taking overlaps into account and adding gap if needed. */ private fun updateStatuses( newStatuses: MutableList>, fullFetch: Boolean ) { + // What we want to do here is: + // 1. Find out if there's an overlap between old and new. If there is one, take overlap + // from the server: + // server: [5, 4, 3, 2] + // cached: [3, 2, 1] + // result: [5, 4, 3, 2, 1], index = 2 + // + // 2. If there's none and it was full fetch then insert a gap: + // server: [9, 8, 7, 6] + // cached: [4, 3, 2, 1] + // result: [9, 8, 7, 6, 5, 4, 3, 2, 1] + // ^ placeholder + // (assuming LOAD_AT_ONE = 4) + if (statuses.isEmpty()) { statuses.addAll(newStatuses.toViewData()) } else { val lastOfNew = newStatuses.lastOrNull() val index = if (lastOfNew == null) -1 - else statuses.indexOfLast { it.asStatusOrNull()?.id === lastOfNew.asRightOrNull()?.id } + else statuses.indexOfFirst { it.asStatusOrNull()?.id == lastOfNew.asRightOrNull()?.id } + Log.d(TAG, "updateStatuses: clearing up to $index") if (index >= 0) { - statuses.subList(0, index).clear() + statuses.subList(0, index + 1).clear() } - val newIndex = - newStatuses.indexOfFirst { - it.isRight() && it.asRight().id == (statuses[0] as? StatusViewData.Concrete)?.id - } - if (newIndex == -1) { - if (index == -1 && fullFetch) { - val placeholderId = - newStatuses.last { status -> status.isRight() }.asRight().id.inc() - newStatuses.add(Either.Left(Placeholder(placeholderId))) - } - statuses.addAll(0, newStatuses.toViewData()) - } else { - statuses.addAll(0, newStatuses.subList(0, newIndex).toViewData()) + // If we loaded max and didn't find overlap then there might be a gap + if (index == -1 && fullFetch) { + val placeholderId = + newStatuses.last { status -> status.isRight() }.asRight().id.inc() + Log.d(TAG, "updateStatuses: Adding placeholder for ") + newStatuses.add(Either.Left(Placeholder(placeholderId))) } + + statuses.addAll(0, newStatuses.toViewData()) } // Remove all consecutive placeholders removeConsecutivePlaceholders() @@ -526,7 +495,6 @@ class TimelineViewModel @Inject constructor( .await() val mutableStatusResponse = statuses.toMutableList() - filterStatuses(mutableStatusResponse) if (statuses.size > 1) { clearPlaceholdersForResponse(mutableStatusResponse) this.statuses.clear() @@ -548,7 +516,6 @@ class TimelineViewModel @Inject constructor( tryCache() isLoadingInitially = statuses.isEmpty() triggerViewUpdate() - updateCurrent() try { loadAbove() } catch (e: Exception) { @@ -580,31 +547,14 @@ class TimelineViewModel @Inject constructor( } private suspend fun loadAbove() { - var firstOrNull: String? = null - var secondOrNull: String? = null - for (i in statuses.indices) { - val status = statuses[i].asStatusOrNull() ?: continue - firstOrNull = status.id - secondOrNull = statuses.getOrNull(i + 1)?.asStatusOrNull()?.id - break - } - try { - if (firstOrNull != null) { - triggerViewUpdate() - - val statuses = loadStatuses( - maxId = null, - sinceId = firstOrNull, - sinceIdMinusOne = secondOrNull, - homeMode = TimelineRequestMode.NETWORK - ) - - val fullFetch = isFullFetch(statuses) - updateStatuses(statuses.toMutableList(), fullFetch) - } else { - loadBelow(null) - } + val statuses = loadStatuses( + maxId = null, + sinceId = null, + sinceIdMinusOne = null, + TimelineRequestMode.NETWORK + ) + updateStatuses(statuses.toMutableList(), isFullFetch(statuses)) } finally { triggerViewUpdate() } diff --git a/app/src/test/java/com/keylesspalace/tusky/components/timeline/TimelineViewModelTest.kt b/app/src/test/java/com/keylesspalace/tusky/components/timeline/TimelineViewModelTest.kt index a5665de92..ba78f7cb5 100644 --- a/app/src/test/java/com/keylesspalace/tusky/components/timeline/TimelineViewModelTest.kt +++ b/app/src/test/java/com/keylesspalace/tusky/components/timeline/TimelineViewModelTest.kt @@ -12,6 +12,7 @@ import com.keylesspalace.tusky.network.FilterModel import com.keylesspalace.tusky.network.MastodonApi import com.keylesspalace.tusky.network.TimelineCases import com.keylesspalace.tusky.util.Either +import com.keylesspalace.tusky.util.inc import com.keylesspalace.tusky.util.toViewData import com.keylesspalace.tusky.viewdata.StatusViewData import com.nhaarman.mockitokotlin2.clearInvocations @@ -85,13 +86,13 @@ class TimelineViewModelTest { val initialResponse = listOf() setCachedResponse(initialResponse) - // loadAbove -> loadBelow + // loadAbove whenever( timelineRepository.getStatuses( maxId = null, sinceId = null, sincedIdMinusOne = null, - requestMode = TimelineRequestMode.ANY, + requestMode = TimelineRequestMode.NETWORK, limit = LOAD_AT_ONCE ) ).thenReturn(Single.just(listOf())) @@ -105,7 +106,7 @@ class TimelineViewModelTest { null, null, LOAD_AT_ONCE, - TimelineRequestMode.ANY + TimelineRequestMode.NETWORK ) } @@ -120,7 +121,7 @@ class TimelineViewModelTest { isNull(), isNull(), eq(LOAD_AT_ONCE), - eq(TimelineRequestMode.ANY) + eq(TimelineRequestMode.NETWORK) ) ).thenReturn( Single.just( @@ -141,7 +142,7 @@ class TimelineViewModelTest { isNull(), isNull(), eq(LOAD_AT_ONCE), - eq(TimelineRequestMode.ANY) + eq(TimelineRequestMode.NETWORK) ) assertViewUpdated(updates) @@ -193,7 +194,7 @@ class TimelineViewModelTest { sinceId = null, sincedIdMinusOne = null, limit = LOAD_AT_ONCE, - TimelineRequestMode.ANY, + TimelineRequestMode.NETWORK, ) ).thenReturn(Single.error(IOException("test"))) @@ -208,7 +209,7 @@ class TimelineViewModelTest { isNull(), isNull(), eq(LOAD_AT_ONCE), - eq(TimelineRequestMode.ANY) + eq(TimelineRequestMode.NETWORK) ) assertViewUpdated(updates) @@ -221,7 +222,7 @@ class TimelineViewModelTest { fun `loadInitial, home, with cache, error on load above`() { val statuses = (5 downTo 1).map { makeStatus(it.toString()) } setCachedResponse(statuses) - setInitialRefresh("6", statuses) + setInitialRefresh(statuses) whenever( timelineRepository.getStatuses( @@ -263,7 +264,7 @@ class TimelineViewModelTest { ).thenReturn(Single.error(IOException("test"))) // Empty on loading above - setLoadAbove("5", "4", listOf()) + setInitialRefresh(listOf()) val updates = viewModel.viewUpdates.test() @@ -277,24 +278,65 @@ class TimelineViewModelTest { assertNull(viewModel.failure) } + @Test + fun `loadInitial but there's a gap above now`() { + val cachedStatuses = (5 downTo 1).map { makeStatus(it.toString()) } + val newStatuses = (100 downTo 100 - LOAD_AT_ONCE).map { makeStatus(it.toString()) } + setCachedResponse(cachedStatuses) + setInitialRefresh(newStatuses) + + runBlocking { + viewModel.loadInitial() + } + + clearInvocations(timelineRepository) + + runBlocking { + viewModel.refresh().join() + } + + val expected = newStatuses.toViewData() + + listOf(StatusViewData.Placeholder(newStatuses.last().id.inc(), false)) + + cachedStatuses.toViewData() + + assertHasList(expected) + assertFalse("refreshing", viewModel.isRefreshing) + assertNull("failure is not set", viewModel.failure) + } + + @Test + fun `loadInitial but there's overlap`() { + val cachedStatuses = (5 downTo 1).map { makeStatus(it.toString()) } + val newStatuses = (3 + LOAD_AT_ONCE downTo 3).map { makeStatus(it.toString()) } + setCachedResponse(cachedStatuses) + setInitialRefresh(newStatuses) + + runBlocking { + viewModel.loadInitial() + } + + clearInvocations(timelineRepository) + + runBlocking { + viewModel.refresh().join() + } + + val expected = (3 + LOAD_AT_ONCE downTo 1).map { makeStatus(it.toString()) }.toViewData() + + assertHasList(expected) + assertFalse("refreshing", viewModel.isRefreshing) + assertNull("failure is not set", viewModel.failure) + } + @Test fun `loads above cached`() { val cachedStatuses = (5 downTo 1).map { makeStatus(it.toString()) } setCachedResponse(cachedStatuses) - setInitialRefresh("6", cachedStatuses) val additionalStatuses = (10 downTo 6) .map { makeStatus(it.toString()) } - whenever( - timelineRepository.getStatuses( - null, - "5", - "4", - LOAD_AT_ONCE, - TimelineRequestMode.NETWORK - ) - ).thenReturn(Single.just(additionalStatuses.toEitherList())) + setInitialRefresh(additionalStatuses + cachedStatuses) runBlocking { viewModel.loadInitial() @@ -309,19 +351,10 @@ class TimelineViewModelTest { fun refresh() { val cachedStatuses = (5 downTo 1).map { makeStatus(it.toString()) } setCachedResponse(cachedStatuses) - setInitialRefresh("6", cachedStatuses) val additionalStatuses = listOf(makeStatus("6")) - whenever( - timelineRepository.getStatuses( - null, - "5", - "4", - LOAD_AT_ONCE, - TimelineRequestMode.NETWORK - ) - ).thenReturn(Single.just(additionalStatuses.toEitherList())) + setInitialRefresh(additionalStatuses + cachedStatuses) runBlocking { viewModel.loadInitial() @@ -335,12 +368,12 @@ class TimelineViewModelTest { whenever( timelineRepository.getStatuses( null, - "6", - "5", + null, + null, LOAD_AT_ONCE, TimelineRequestMode.NETWORK ) - ).thenReturn(Single.just(newStatuses.toEitherList())) + ).thenReturn(Single.just((newStatuses + additionalStatuses + cachedStatuses).toEitherList())) runBlocking { viewModel.refresh() @@ -354,8 +387,7 @@ class TimelineViewModelTest { fun `refresh failed`() { val cachedStatuses = (5 downTo 1).map { makeStatus(it.toString()) } setCachedResponse(cachedStatuses) - setInitialRefresh("6", cachedStatuses) - setLoadAbove("5", "4", listOf()) + setInitialRefresh(cachedStatuses) runBlocking { viewModel.loadInitial() @@ -367,8 +399,8 @@ class TimelineViewModelTest { whenever( timelineRepository.getStatuses( null, - "6", - "5", + null, + null, LOAD_AT_ONCE, TimelineRequestMode.NETWORK ) @@ -387,10 +419,7 @@ class TimelineViewModelTest { fun loadMore() { val cachedStatuses = (10 downTo 5).map { makeStatus(it.toString()) } setCachedResponse(cachedStatuses) - setInitialRefresh("11", cachedStatuses) - - // Nothing above - setLoadAbove("10", "9", listOf()) + setInitialRefresh(cachedStatuses) runBlocking { viewModel.loadInitial().join() @@ -423,10 +452,7 @@ class TimelineViewModelTest { fun `loadMore parallel`() { val cachedStatuses = (10 downTo 5).map { makeStatus(it.toString()) } setCachedResponse(cachedStatuses) - setInitialRefresh("11", cachedStatuses) - - // Nothing above - setLoadAbove("10", "9", listOf()) + setInitialRefresh(cachedStatuses) runBlocking { viewModel.loadInitial().join() @@ -477,10 +503,7 @@ class TimelineViewModelTest { fun `loadMore failed`() { val cachedStatuses = (10 downTo 5).map { makeStatus(it.toString()) } setCachedResponse(cachedStatuses) - setInitialRefresh("11", cachedStatuses) - - // Nothing above - setLoadAbove("10", "9", listOf()) + setInitialRefresh(cachedStatuses) runBlocking { viewModel.loadInitial().join() @@ -542,10 +565,7 @@ class TimelineViewModelTest { ) setCachedResponseWithGaps(cachedStatuses) - setInitialRefreshWithGaps("6", cachedStatuses) - - // Nothing above - setLoadAbove("5", items = listOf()) + setInitialRefreshWithGaps(cachedStatuses) whenever( timelineRepository.getStatuses( @@ -584,9 +604,7 @@ class TimelineViewModelTest { Either.Right(status1) ) setCachedResponseWithGaps(cachedStatuses) - setInitialRefreshWithGaps("6", cachedStatuses) - - setLoadAbove("5", items = listOf()) + setInitialRefreshWithGaps(cachedStatuses) whenever( timelineRepository.getStatuses( @@ -620,8 +638,7 @@ class TimelineViewModelTest { val status3 = makeStatus("3") val statuses = listOf(status5, status4, status3) setCachedResponse(statuses) - setInitialRefresh("6", statuses) - setLoadAbove("5", "4", listOf()) + setInitialRefresh(listOf()) runBlocking { viewModel.loadInitial() } @@ -644,8 +661,7 @@ class TimelineViewModelTest { val status3 = makeStatus("3") val statuses = listOf(status5, status4, status3) setCachedResponse(statuses) - setInitialRefresh("6", statuses) - setLoadAbove("5", "4", listOf()) + setInitialRefresh(statuses) runBlocking { viewModel.loadInitial() } @@ -668,8 +684,7 @@ class TimelineViewModelTest { val status3 = makeStatus("3") val statuses = listOf(status5, status4, status3) setCachedResponse(statuses) - setInitialRefresh("6", statuses) - setLoadAbove("5", "4", listOf()) + setInitialRefresh(statuses) runBlocking { viewModel.loadInitial() } @@ -702,8 +717,7 @@ class TimelineViewModelTest { val status3 = makeStatus("3") val statuses = listOf(status5, status4, status3) setCachedResponse(statuses) - setInitialRefresh("6", statuses) - setLoadAbove("5", "4", listOf()) + setInitialRefresh(statuses) runBlocking { viewModel.loadInitial() } @@ -720,22 +734,6 @@ class TimelineViewModelTest { assertHasList(listOf(status5, status4.copy(poll = votedPoll), status3).toViewData()) } - private fun setLoadAbove( - above: String, - aboveMinusOne: String? = null, - items: List - ) { - whenever( - timelineRepository.getStatuses( - null, - above, - aboveMinusOne, - LOAD_AT_ONCE, - TimelineRequestMode.NETWORK - ) - ).thenReturn(Single.just(items)) - } - private fun assertHasList(aList: List) { assertEquals( aList, @@ -747,8 +745,8 @@ class TimelineViewModelTest { assertTrue("There were view updates", updates.values().isNotEmpty()) } - private fun setInitialRefresh(maxId: String?, statuses: List) { - setInitialRefreshWithGaps(maxId, statuses.toEitherList()) + private fun setInitialRefresh(statuses: List) { + setInitialRefreshWithGaps(statuses.toEitherList()) } private fun setCachedResponse(initialResponse: List) { @@ -768,10 +766,10 @@ class TimelineViewModelTest { .thenReturn(Single.just(initialResponse)) } - private fun setInitialRefreshWithGaps(maxId: String?, statuses: List) { + private fun setInitialRefreshWithGaps(statuses: List) { whenever( timelineRepository.getStatuses( - maxId, + null, null, null, LOAD_AT_ONCE,