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.

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<StatusViewData.Concrete>()?.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<Either<Placeholder, Status>>,
         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 =
+            // If we loaded max and didn't find overlap then there might be a gap
-                newStatuses.indexOfFirst {
+            if (index == -1 && fullFetch) {
-                    it.isRight() && it.asRight().id == (statuses[0] as? StatusViewData.Concrete)?.id
+                val placeholderId =
-                }
+                    newStatuses.last { status -> status.isRight() }.asRight().id.inc()
-            if (newIndex == -1) {
+                Log.d(TAG, "updateStatuses: Adding placeholder for ")
-                if (index == -1 && fullFetch) {
+                newStatuses.add(Either.Left(Placeholder(placeholderId)))
-                    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())
             }
+
+            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) {
+            val statuses = loadStatuses(
-                triggerViewUpdate()
+                maxId = null,
-
+                sinceId = null,
-                val statuses = loadStatuses(
+                sinceIdMinusOne = null,
-                    maxId = null,
+                TimelineRequestMode.NETWORK
-                    sinceId = firstOrNull,
+            )
-                    sinceIdMinusOne = secondOrNull,
+            updateStatuses(statuses.toMutableList(), isFullFetch(statuses))
-                    homeMode = TimelineRequestMode.NETWORK
-                )
-
-                val fullFetch = isFullFetch(statuses)
-                updateStatuses(statuses.toMutableList(), fullFetch)
-            } else {
-                loadBelow(null)
-            }
         } finally {
             triggerViewUpdate()
         }

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<Status>()
         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(
+        setInitialRefresh(additionalStatuses + cachedStatuses)
-            timelineRepository.getStatuses(
-                null,
-                "5",
-                "4",
-                LOAD_AT_ONCE,
-                TimelineRequestMode.NETWORK
-            )
-        ).thenReturn(Single.just(additionalStatuses.toEitherList()))
 
         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(
+        setInitialRefresh(additionalStatuses + cachedStatuses)
-            timelineRepository.getStatuses(
-                null,
-                "5",
-                "4",
-                LOAD_AT_ONCE,
-                TimelineRequestMode.NETWORK
-            )
-        ).thenReturn(Single.just(additionalStatuses.toEitherList()))
 
         runBlocking {
             viewModel.loadInitial()
@@ -335,12 +368,12 @@ class TimelineViewModelTest {
         whenever(
             timelineRepository.getStatuses(
                 null,
-                "6",
+                null,
-                "5",
+                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)
+        setInitialRefresh(cachedStatuses)
-        setLoadAbove("5", "4", listOf())
 
         runBlocking {
             viewModel.loadInitial()
@@ -367,8 +399,8 @@ class TimelineViewModelTest {
         whenever(
             timelineRepository.getStatuses(
                 null,
-                "6",
+                null,
-                "5",
+                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)
+        setInitialRefresh(cachedStatuses)
-
-        // Nothing above
-        setLoadAbove("10", "9", listOf())
 
         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)
+        setInitialRefresh(cachedStatuses)
-
-        // Nothing above
-        setLoadAbove("10", "9", listOf())
 
         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)
+        setInitialRefresh(cachedStatuses)
-
-        // Nothing above
-        setLoadAbove("10", "9", listOf())
 
         runBlocking {
             viewModel.loadInitial().join()
@@ -542,10 +565,7 @@ class TimelineViewModelTest {
         )
 
         setCachedResponseWithGaps(cachedStatuses)
-        setInitialRefreshWithGaps("6", cachedStatuses)
+        setInitialRefreshWithGaps(cachedStatuses)
-
-        // Nothing above
-        setLoadAbove("5", items = listOf())
 
         whenever(
             timelineRepository.getStatuses(
@@ -584,9 +604,7 @@ class TimelineViewModelTest {
             Either.Right(status1)
         )
         setCachedResponseWithGaps(cachedStatuses)
-        setInitialRefreshWithGaps("6", cachedStatuses)
+        setInitialRefreshWithGaps(cachedStatuses)
-
-        setLoadAbove("5", items = listOf())
 
         whenever(
             timelineRepository.getStatuses(
@@ -620,8 +638,7 @@ class TimelineViewModelTest {
         val status3 = makeStatus("3")
         val statuses = listOf(status5, status4, status3)
         setCachedResponse(statuses)
-        setInitialRefresh("6", statuses)
+        setInitialRefresh(listOf())
-        setLoadAbove("5", "4", 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)
+        setInitialRefresh(statuses)
-        setLoadAbove("5", "4", listOf())
 
         runBlocking { viewModel.loadInitial() }
 
@@ -668,8 +684,7 @@ class TimelineViewModelTest {
         val status3 = makeStatus("3")
         val statuses = listOf(status5, status4, status3)
         setCachedResponse(statuses)
-        setInitialRefresh("6", statuses)
+        setInitialRefresh(statuses)
-        setLoadAbove("5", "4", listOf())
 
         runBlocking { viewModel.loadInitial() }
 
@@ -702,8 +717,7 @@ class TimelineViewModelTest {
         val status3 = makeStatus("3")
         val statuses = listOf(status5, status4, status3)
         setCachedResponse(statuses)
-        setInitialRefresh("6", statuses)
+        setInitialRefresh(statuses)
-        setLoadAbove("5", "4", listOf())
 
         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<TimelineStatus>
-    ) {
-        whenever(
-            timelineRepository.getStatuses(
-                null,
-                above,
-                aboveMinusOne,
-                LOAD_AT_ONCE,
-                TimelineRequestMode.NETWORK
-            )
-        ).thenReturn(Single.just(items))
-    }
-
     private fun assertHasList(aList: List<StatusViewData>) {
         assertEquals(
             aList,
@@ -747,8 +745,8 @@ class TimelineViewModelTest {
         assertTrue("There were view updates", updates.values().isNotEmpty())
     }
 
-    private fun setInitialRefresh(maxId: String?, statuses: List<Status>) {
+    private fun setInitialRefresh(statuses: List<Status>) {
-        setInitialRefreshWithGaps(maxId, statuses.toEitherList())
+        setInitialRefreshWithGaps(statuses.toEitherList())
     }
 
     private fun setCachedResponse(initialResponse: List<Status>) {
@@ -768,10 +766,10 @@ class TimelineViewModelTest {
             .thenReturn(Single.just(initialResponse))
     }
 
-    private fun setInitialRefreshWithGaps(maxId: String?, statuses: List<TimelineStatus>) {
+    private fun setInitialRefreshWithGaps(statuses: List<TimelineStatus>) {
         whenever(
             timelineRepository.getStatuses(
-                maxId,
+                null,
                 null,
                 null,
                 LOAD_AT_ONCE,