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,