From 61ba6fe181b388b7682e600665bef5175d229efb Mon Sep 17 00:00:00 2001 From: Konrad Pozniak Date: Thu, 3 Feb 2022 18:51:15 +0100 Subject: [PATCH] Fix disappearing placeholders (#2309) * add getNextPlaceholderIdAfter to TimelineDao * fix disappearing placeholders * fix disappearing placeholders --- .../viewmodel/CachedTimelineRemoteMediator.kt | 10 +- .../viewmodel/CachedTimelineViewModel.kt | 4 +- .../com/keylesspalace/tusky/db/TimelineDao.kt | 6 ++ .../CachedTimelineRemoteMediatorTest.kt | 69 +++++++++++- .../tusky/components/timeline/StatusMocker.kt | 9 ++ .../keylesspalace/tusky/db/TimelineDaoTest.kt | 101 ++++++++++++++---- 6 files changed, 178 insertions(+), 21 deletions(-) diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt index 98f6452d5..e9d81e59c 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt +++ b/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) diff --git a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt b/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt index c61658a51..1ea616936 100644 --- a/app/src/main/java/com/keylesspalace/tusky/components/timeline/viewmodel/CachedTimelineViewModel.kt +++ b/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) { diff --git a/app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt b/app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt index e2ba80214..5a7000278 100644 --- a/app/src/main/java/com/keylesspalace/tusky/db/TimelineDao.kt +++ b/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? } diff --git a/app/src/test/java/com/keylesspalace/tusky/components/timeline/CachedTimelineRemoteMediatorTest.kt b/app/src/test/java/com/keylesspalace/tusky/components/timeline/CachedTimelineRemoteMediatorTest.kt index dd97b17e4..ad633f3df 100644 --- a/app/src/test/java/com/keylesspalace/tusky/components/timeline/CachedTimelineRemoteMediatorTest.kt +++ b/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) { runBlocking { statuses.forEach { statusWithAccount -> - timelineDao().insertAccount(statusWithAccount.account) + if (statusWithAccount.status.authorServerId != null) { + timelineDao().insertAccount(statusWithAccount.account) + } statusWithAccount.reblogAccount?.let { account -> timelineDao().insertAccount(account) } diff --git a/app/src/test/java/com/keylesspalace/tusky/components/timeline/StatusMocker.kt b/app/src/test/java/com/keylesspalace/tusky/components/timeline/StatusMocker.kt index 5798c90c1..c4ab2faa5 100644 --- a/app/src/test/java/com/keylesspalace/tusky/components/timeline/StatusMocker.kt +++ b/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) + } +} diff --git a/app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt b/app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt index 1f8dfee7e..fa405fe9f 100644 --- a/app/src/test/java/com/keylesspalace/tusky/db/TimelineDaoTest.kt +++ b/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( - statusId = 4, - accountId = 1, - domain = "mastodon.test", - authorServerId = "1" - ) - val status2 = makeStatus( - statusId = 33, - accountId = 1, - domain = "mastodon.test", - authorServerId = "2" - ) - val status3 = makeStatus( - statusId = 22, - accountId = 1, - domain = "mastodon.test", - authorServerId = "2" + val statusData = listOf( + makeStatus( + statusId = 4, + accountId = 1, + domain = "mastodon.test", + authorServerId = "1" + ), + makeStatus( + statusId = 33, + accountId = 1, + domain = "mastodon.test", + authorServerId = "2" + ), + makeStatus( + statusId = 22, + accountId = 1, + 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 { + val placeholder = Placeholder(id.toString(), false).toEntity(accountId) + return Triple(placeholder, null, null) + } + private fun assertStatuses( expected: List>, provided: List