Fix Timeline not loading (#2398)

* fix cached timeline

* fix network timeline

* delete unused inc / dec extensions

* fix tests and bug in network timeline

* add db migration

* remove unused import

* commit 31.json

* improve placeholder inserting logic, add comment

* fix tests

* improve tests
This commit is contained in:
Konrad Pozniak 2022-03-28 18:39:16 +02:00 committed by GitHub
commit f2529a8e61
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
12 changed files with 938 additions and 120 deletions

View file

@ -30,7 +30,6 @@ import com.keylesspalace.tusky.db.TimelineStatusEntity
import com.keylesspalace.tusky.db.TimelineStatusWithAccount
import com.keylesspalace.tusky.entity.Status
import com.keylesspalace.tusky.network.MastodonApi
import com.keylesspalace.tusky.util.dec
import kotlinx.coroutines.rx3.await
import retrofit2.HttpException
@ -102,9 +101,14 @@ class CachedTimelineRemoteMediator(
db.withTransaction {
val overlappedStatuses = replaceStatusRange(statuses, state)
if (loadType == LoadType.REFRESH && overlappedStatuses == 0 && statuses.isNotEmpty() && !dbEmpty) {
/* In case we loaded a whole page and there was no overlap with existing statuses,
we insert a placeholder because there might be even more unknown statuses */
if (loadType == LoadType.REFRESH && overlappedStatuses == 0 && statuses.size == state.config.pageSize && !dbEmpty) {
/* This overrides the last of the newly loaded statuses with a placeholder
to guarantee the placeholder has an id that exists on the server as not all
servers handle client generated ids as expected */
timelineDao.insertStatus(
Placeholder(statuses.last().id.dec(), loading = false).toEntity(activeAccount.id)
Placeholder(statuses.last().id, loading = false).toEntity(activeAccount.id)
)
}
}

View file

@ -41,8 +41,6 @@ import com.keylesspalace.tusky.entity.Poll
import com.keylesspalace.tusky.network.FilterModel
import com.keylesspalace.tusky.network.MastodonApi
import com.keylesspalace.tusky.network.TimelineCases
import com.keylesspalace.tusky.util.dec
import com.keylesspalace.tusky.util.inc
import com.keylesspalace.tusky.viewdata.StatusViewData
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.map
@ -149,9 +147,11 @@ class CachedTimelineViewModel @Inject constructor(
timelineDao.insertStatus(Placeholder(placeholderId, loading = true).toEntity(activeAccount.id))
val nextPlaceholderId = timelineDao.getNextPlaceholderIdAfter(activeAccount.id, placeholderId)
val response = api.homeTimeline(maxId = placeholderId.inc(), sinceId = nextPlaceholderId, limit = LOAD_AT_ONCE).await()
val response = db.withTransaction {
val idAbovePlaceholder = timelineDao.getIdAbove(activeAccount.id, placeholderId)
val nextPlaceholderId = timelineDao.getNextPlaceholderIdAfter(activeAccount.id, placeholderId)
api.homeTimeline(maxId = idAbovePlaceholder, sinceId = nextPlaceholderId, limit = LOAD_AT_ONCE)
}.await()
val statuses = response.body()
if (!response.isSuccessful || statuses == null) {
@ -185,9 +185,14 @@ class CachedTimelineViewModel @Inject constructor(
)
}
if (overlappedStatuses == 0 && statuses.isNotEmpty()) {
/* In case we loaded a whole page and there was no overlap with existing statuses,
we insert a placeholder because there might be even more unknown statuses */
if (overlappedStatuses == 0 && statuses.size == LOAD_AT_ONCE) {
/* This overrides the last of the newly loaded statuses with a placeholder
to guarantee the placeholder has an id that exists on the server as not all
servers handle client generated ids as expected */
timelineDao.insertStatus(
Placeholder(statuses.last().id.dec(), loading = false).toEntity(activeAccount.id)
Placeholder(statuses.last().id, loading = false).toEntity(activeAccount.id)
)
}
}

View file

@ -22,7 +22,6 @@ import androidx.paging.RemoteMediator
import com.keylesspalace.tusky.components.timeline.util.ifExpected
import com.keylesspalace.tusky.db.AccountManager
import com.keylesspalace.tusky.util.HttpHeaderLink
import com.keylesspalace.tusky.util.dec
import com.keylesspalace.tusky.util.toViewData
import com.keylesspalace.tusky.viewdata.StatusViewData
import retrofit2.HttpException
@ -93,7 +92,7 @@ class NetworkTimelineRemoteMediator(
viewModel.statusData.addAll(0, data)
if (insertPlaceholder) {
viewModel.statusData.add(statuses.size, StatusViewData.Placeholder(statuses.last().id.dec(), false))
viewModel.statusData[statuses.size - 1] = StatusViewData.Placeholder(statuses.last().id, false)
}
} else {
val linkHeader = statusResponse.headers()["Link"]

View file

@ -35,9 +35,7 @@ import com.keylesspalace.tusky.entity.Status
import com.keylesspalace.tusky.network.FilterModel
import com.keylesspalace.tusky.network.MastodonApi
import com.keylesspalace.tusky.network.TimelineCases
import com.keylesspalace.tusky.util.dec
import com.keylesspalace.tusky.util.getDomain
import com.keylesspalace.tusky.util.inc
import com.keylesspalace.tusky.util.isLessThan
import com.keylesspalace.tusky.util.isLessThanOrEqual
import com.keylesspalace.tusky.util.toViewData
@ -142,8 +140,10 @@ class NetworkTimelineViewModel @Inject constructor(
statusData.indexOfFirst { it is StatusViewData.Placeholder && it.id == placeholderId }
statusData[placeholderIndex] = StatusViewData.Placeholder(placeholderId, isLoading = true)
val idAbovePlaceholder = statusData.getOrNull(placeholderIndex - 1)?.id
val statusResponse = fetchStatusesForKind(
fromId = placeholderId.inc(),
fromId = idAbovePlaceholder,
uptoId = null,
limit = 20
)
@ -157,7 +157,7 @@ class NetworkTimelineViewModel @Inject constructor(
statusData.removeAt(placeholderIndex)
val activeAccount = accountManager.activeAccount!!
val data = statuses.map { status ->
val data: MutableList<StatusViewData> = statuses.map { status ->
status.toViewData(
isShowingContent = activeAccount.alwaysShowSensitiveMedia || !status.actionableStatus.sensitive,
isExpanded = activeAccount.alwaysOpenSpoiler,
@ -175,7 +175,7 @@ class NetworkTimelineViewModel @Inject constructor(
data.mapIndexed { i, status -> i to statusData.firstOrNull { it.asStatusOrNull()?.id == status.id }?.asStatusOrNull() }
.filter { (_, oldStatus) -> oldStatus != null }
.forEach { (i, oldStatus) ->
data[i] = data[i]
data[i] = data[i].asStatusOrNull()!!
.copy(
isShowingContent = oldStatus!!.isShowingContent,
isExpanded = oldStatus.isExpanded,
@ -190,7 +190,7 @@ class NetworkTimelineViewModel @Inject constructor(
}
}
} else {
statusData.add(overlappedFrom, StatusViewData.Placeholder(statuses.last().id.dec(), isLoading = false))
data[data.size - 1] = StatusViewData.Placeholder(statuses.last().id, isLoading = false)
}
}
@ -240,7 +240,7 @@ class NetworkTimelineViewModel @Inject constructor(
}
override fun fullReload() {
nextKey = statusData.firstOrNull { it is StatusViewData.Concrete }?.asStatusOrNull()?.id?.inc()
nextKey = statusData.firstOrNull { it is StatusViewData.Concrete }?.asStatusOrNull()?.id
statusData.clear()
currentSource?.invalidate()
}

View file

@ -29,10 +29,9 @@ import java.io.File;
/**
* DB version & declare DAO
*/
@Database(entities = { DraftEntity.class, AccountEntity.class, InstanceEntity.class, TimelineStatusEntity.class,
TimelineAccountEntity.class, ConversationEntity.class
}, version = 30)
}, version = 31)
public abstract class AppDatabase extends RoomDatabase {
public abstract AccountDao accountDao();
@ -474,4 +473,14 @@ public abstract class AppDatabase extends RoomDatabase {
database.execSQL("ALTER TABLE `InstanceEntity` ADD COLUMN `maxPollDuration` INTEGER");
}
};
public static final Migration MIGRATION_30_31 = new Migration(30, 31) {
@Override
public void migrate(@NonNull SupportSQLiteDatabase database) {
// no actual scheme change, but placeholder ids are now used differently so the cache needs to be cleared to avoid bugs
database.execSQL("DELETE FROM `TimelineAccountEntity`");
database.execSQL("DELETE FROM `TimelineStatusEntity`");
}
};
}

View file

@ -186,6 +186,15 @@ AND timelineUserId = :accountId
@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?
/**
* Returns the id directly above [serverId], or null if [serverId] is the id of the top status
*/
@Query("SELECT serverId FROM TimelineStatusEntity WHERE timelineUserId = :accountId AND (LENGTH(:serverId) < LENGTH(serverId) OR (LENGTH(:serverId) = LENGTH(serverId) AND :serverId < serverId)) ORDER BY LENGTH(serverId) ASC, serverId ASC LIMIT 1")
abstract suspend fun getIdAbove(accountId: Long, serverId: String): String?
/**
* Returns the id of the next placeholder after [serverId]
*/
@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?
}

View file

@ -62,7 +62,7 @@ class AppModule {
AppDatabase.MIGRATION_22_23, AppDatabase.MIGRATION_23_24, AppDatabase.MIGRATION_24_25,
AppDatabase.Migration25_26(appContext.getExternalFilesDir("Tusky")),
AppDatabase.MIGRATION_26_27, AppDatabase.MIGRATION_27_28, AppDatabase.MIGRATION_28_29,
AppDatabase.MIGRATION_29_30
AppDatabase.MIGRATION_29_30, AppDatabase.MIGRATION_30_31
)
.build()
}

View file

@ -16,51 +16,6 @@ fun randomAlphanumericString(count: Int): String {
return String(chars)
}
// We sort statuses by ID. Something we need to invent some ID for placeholder.
/**
* "Increment" string so that during sorting it's bigger than [this]. Inverse operation to [dec].
*/
fun String.inc(): String {
val builder = this.toCharArray()
var i = builder.lastIndex
while (i >= 0) {
if (builder[i] < 'z') {
builder[i] = builder[i].inc()
return String(builder)
} else {
builder[i] = '0'
}
i--
}
return String(
CharArray(builder.size + 1) { index ->
if (index == 0) '0' else builder[index - 1]
}
)
}
/**
* "Decrement" string so that during sorting it's smaller than [this]. Inverse operation to [inc].
*/
fun String.dec(): String {
if (this.isEmpty()) return this
val builder = this.toCharArray()
var i = builder.lastIndex
while (i >= 0) {
if (builder[i] > '0') {
builder[i] = builder[i].dec()
return String(builder)
} else {
builder[i] = 'z'
}
i--
}
return String(builder.copyOfRange(1, builder.size))
}
/**
* A < B (strictly) by length and then by content.
* Examples: