Fix disappearing placeholders (#2309)

* add getNextPlaceholderIdAfter to TimelineDao

* fix disappearing placeholders

* fix disappearing placeholders

app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt

@@ -53,11 +53,19 @@ class CachedTimelineRemoteMediator(
 
         try {
             var dbEmpty = false
+
+            val topPlaceholderId = if (loadType == LoadType.REFRESH) {
+                timelineDao.getTopPlaceholderId(activeAccount.id)
+            } else {
+                null // don't execute the query if it is not needed
+            }
+
             if (!initialRefresh && loadType == LoadType.REFRESH) {
                 val topId = timelineDao.getTopId(activeAccount.id)
                 topId?.let { cachedTopId ->
                     val statusResponse = api.homeTimeline(
                         maxId = cachedTopId,
+                        sinceId = topPlaceholderId, // so already existing placeholders don't get accidentally overwritten
                         limit = state.config.pageSize
                     ).await()
 
@@ -74,7 +82,7 @@ class CachedTimelineRemoteMediator(
 
             val statusResponse = when (loadType) {
                 LoadType.REFRESH -> {
-                    api.homeTimeline(limit = state.config.pageSize).await()
+                    api.homeTimeline(sinceId = topPlaceholderId, limit = state.config.pageSize).await()
                 }
                 LoadType.PREPEND -> {
                     return MediatorResult.Success(endOfPaginationReached = true)

app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt

@@ -128,7 +128,9 @@ class CachedTimelineViewModel @Inject constructor(
 
                 timelineDao.insertStatus(Placeholder(placeholderId, loading = true).toEntity(activeAccount.id))
 
-                val response = api.homeTimeline(maxId = placeholderId.inc(), limit = 20).await()
+                val nextPlaceholderId = timelineDao.getNextPlaceholderIdAfter(activeAccount.id, placeholderId)
+
+                val response = api.homeTimeline(maxId = placeholderId.inc(), sinceId = nextPlaceholderId, limit = 20).await()
 
                 val statuses = response.body()
                 if (!response.isSuccessful || statuses == null) {

app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt

@@ -142,4 +142,10 @@ AND timelineUserId = :accountId
 
     @Query("SELECT serverId FROM TimelineStatusEntity WHERE timelineUserId = :accountId ORDER BY LENGTH(serverId) DESC, serverId DESC LIMIT 1")
     abstract suspend fun getTopId(accountId: Long): String?
+
+    @Query("SELECT serverId FROM TimelineStatusEntity WHERE timelineUserId = :accountId AND authorServerId IS NULL ORDER BY LENGTH(serverId) DESC, serverId DESC LIMIT 1")
+    abstract suspend fun getTopPlaceholderId(accountId: Long): String?
+
+    @Query("SELECT serverId FROM TimelineStatusEntity WHERE timelineUserId = :accountId AND authorServerId IS NULL AND (LENGTH(:serverId) > LENGTH(serverId) OR (LENGTH(:serverId) = LENGTH(serverId) AND :serverId > serverId)) ORDER BY LENGTH(serverId) DESC, serverId DESC LIMIT 1")
+    abstract suspend fun getNextPlaceholderIdAfter(accountId: Long, serverId: String): String?
 }

app/src/test/java/com/keylesspalace/tusky/components/timeline/CachedTimelineRemoteMediatorTest.kt

@@ -26,6 +26,7 @@ import kotlinx.coroutines.runBlocking
 import okhttp3.ResponseBody.Companion.toResponseBody
 import org.junit.After
 import org.junit.Assert.assertEquals
+import org.junit.Assert.assertFalse
 import org.junit.Assert.assertTrue
 import org.junit.Before
 import org.junit.Test
@@ -367,6 +368,70 @@ class CachedTimelineRemoteMediatorTest {
         )
     }
 
+    @Test
+    @ExperimentalPagingApi
+    fun `should not remove placeholder in timeline`() {
+
+        val statusesAlreadyInDb = listOf(
+            mockStatusEntityWithAccount("8"),
+            mockStatusEntityWithAccount("7"),
+            mockPlaceholderEntityWithAccount("6"),
+            mockStatusEntityWithAccount("1"),
+        )
+
+        db.insert(statusesAlreadyInDb)
+
+        val remoteMediator = CachedTimelineRemoteMediator(
+            accountManager = accountManager,
+            api = mock {
+                on { homeTimeline(sinceId = "6", limit = 20) } doReturn Single.just(
+                    Response.success(
+                        listOf(
+                            mockStatus("9"),
+                            mockStatus("8"),
+                            mockStatus("7")
+                        )
+                    )
+                )
+                on { homeTimeline(maxId = "8", sinceId = "6", limit = 20) } doReturn Single.just(
+                    Response.success(
+                        listOf(
+                            mockStatus("8"),
+                            mockStatus("7")
+                        )
+                    )
+                )
+            },
+            db = db,
+            gson = Gson()
+        )
+
+        val state = state(
+            listOf(
+                PagingSource.LoadResult.Page(
+                    data = statusesAlreadyInDb,
+                    prevKey = null,
+                    nextKey = 0
+                )
+            )
+        )
+
+        val result = runBlocking { remoteMediator.load(LoadType.REFRESH, state) }
+
+        assertTrue(result is RemoteMediator.MediatorResult.Success)
+        assertFalse((result as RemoteMediator.MediatorResult.Success).endOfPaginationReached)
+
+        db.assertStatuses(
+            listOf(
+                mockStatusEntityWithAccount("9"),
+                mockStatusEntityWithAccount("8"),
+                mockStatusEntityWithAccount("7"),
+                mockPlaceholderEntityWithAccount("6"),
+                mockStatusEntityWithAccount("1"),
+            )
+        )
+    }
+
     @Test
     @ExperimentalPagingApi
     fun `should append statuses`() {
@@ -434,7 +499,9 @@ class CachedTimelineRemoteMediatorTest {
     private fun AppDatabase.insert(statuses: List<TimelineStatusWithAccount>) {
         runBlocking {
             statuses.forEach { statusWithAccount ->
-                timelineDao().insertAccount(statusWithAccount.account)
+                if (statusWithAccount.status.authorServerId != null) {
+                    timelineDao().insertAccount(statusWithAccount.account)
+                }
                 statusWithAccount.reblogAccount?.let { account ->
                     timelineDao().insertAccount(account)
                 }

app/src/test/java/com/keylesspalace/tusky/components/timeline/StatusMocker.kt

@@ -77,3 +77,12 @@ fun mockStatusEntityWithAccount(
         )
     }
 }
+
+fun mockPlaceholderEntityWithAccount(
+    id: String,
+    userId: Long = 1,
+): TimelineStatusWithAccount {
+    return TimelineStatusWithAccount().apply {
+        status = Placeholder(id, false).toEntity(userId)
+    }
+}

app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt

@@ -6,6 +6,8 @@ import androidx.test.ext.junit.runners.AndroidJUnit4
 import androidx.test.platform.app.InstrumentationRegistry
 import com.google.gson.Gson
 import com.keylesspalace.tusky.appstore.CacheUpdater
+import com.keylesspalace.tusky.components.timeline.Placeholder
+import com.keylesspalace.tusky.components.timeline.toEntity
 import com.keylesspalace.tusky.entity.Status
 import kotlinx.coroutines.runBlocking
 import org.junit.After
@@ -219,26 +221,28 @@ class TimelineDaoTest {
     @Test
     fun `should return correct topId`() = runBlocking {
 
-        val status1 = makeStatus(
+        val statusData = listOf(
-            statusId = 4,
+            makeStatus(
-            accountId = 1,
+                statusId = 4,
-            domain = "mastodon.test",
+                accountId = 1,
-            authorServerId = "1"
+                domain = "mastodon.test",
-        )
+                authorServerId = "1"
-        val status2 = makeStatus(
+            ),
-            statusId = 33,
+            makeStatus(
-            accountId = 1,
+                statusId = 33,
-            domain = "mastodon.test",
+                accountId = 1,
-            authorServerId = "2"
+                domain = "mastodon.test",
-        )
+                authorServerId = "2"
-        val status3 = makeStatus(
+            ),
-            statusId = 22,
+            makeStatus(
-            accountId = 1,
+                statusId = 22,
-            domain = "mastodon.test",
+                accountId = 1,
-            authorServerId = "2"
+                domain = "mastodon.test",
+                authorServerId = "2"
+            )
         )
 
-        for ((status, author, reblogAuthor) in listOf(status1, status2, status3)) {
+        for ((status, author, reblogAuthor) in statusData) {
             timelineDao.insertAccount(author)
             reblogAuthor?.let {
                 timelineDao.insertAccount(it)
@@ -249,6 +253,59 @@ class TimelineDaoTest {
         assertEquals("33", timelineDao.getTopId(1))
     }
 
+    @Test
+    fun `should return correct placeholderId after other ids`() = runBlocking {
+
+        val statusData = listOf(
+            makeStatus(statusId = 1000),
+            makePlaceholder(id = 99),
+            makeStatus(statusId = 97),
+            makeStatus(statusId = 95),
+            makePlaceholder(id = 94),
+            makeStatus(statusId = 90)
+        )
+
+        for ((status, author, reblogAuthor) in statusData) {
+            author?.let {
+                timelineDao.insertAccount(it)
+            }
+            reblogAuthor?.let {
+                timelineDao.insertAccount(it)
+            }
+            timelineDao.insertStatus(status)
+        }
+
+        assertEquals("99", timelineDao.getNextPlaceholderIdAfter(1, "1000"))
+        assertEquals("94", timelineDao.getNextPlaceholderIdAfter(1, "97"))
+        assertNull(timelineDao.getNextPlaceholderIdAfter(1, "90"))
+    }
+
+    @Test
+    fun `should return correct top placeholderId`() = runBlocking {
+
+        val statusData = listOf(
+            makeStatus(statusId = 1000),
+            makePlaceholder(id = 99),
+            makeStatus(statusId = 97),
+            makePlaceholder(id = 96),
+            makeStatus(statusId = 90),
+            makePlaceholder(id = 80),
+            makeStatus(statusId = 77)
+        )
+
+        for ((status, author, reblogAuthor) in statusData) {
+            author?.let {
+                timelineDao.insertAccount(it)
+            }
+            reblogAuthor?.let {
+                timelineDao.insertAccount(it)
+            }
+            timelineDao.insertStatus(status)
+        }
+
+        assertEquals("99", timelineDao.getTopPlaceholderId(1))
+    }
+
     private fun makeStatus(
         accountId: Long = 1,
         statusId: Long = 10,
@@ -317,6 +374,14 @@ class TimelineDaoTest {
         return Triple(status, author, reblogAuthor)
     }
 
+    private fun makePlaceholder(
+        accountId: Long = 1,
+        id: Long
+    ): Triple<TimelineStatusEntity, TimelineAccountEntity?, TimelineAccountEntity?> {
+        val placeholder = Placeholder(id.toString(), false).toEntity(accountId)
+        return Triple(placeholder, null, null)
+    }
+
     private fun assertStatuses(
         expected: List<Triple<TimelineStatusEntity, TimelineAccountEntity, TimelineAccountEntity?>>,
         provided: List<TimelineStatusWithAccount>