Fix incorrectly incrementing IDs before sending to server. (#1026)

* Fix incorrectly incrementing IDs before sending to server.

* Add TimelineRepositoryTest, fix adding placeholder, fix String#dec()

* Add more TimelineRepository tests, fix bugs

* Add tests for adding statuses from DB.
This commit is contained in:
Ivan Kupalov 2019-02-05 20:06:00 +01:00 committed by Konrad Pozniak
commit 63952813c8
11 changed files with 631 additions and 208 deletions

View file

@ -59,7 +59,11 @@ LIMIT :limit""")
}
@Query("""DELETE FROM TimelineStatusEntity WHERE authorServerId = null
AND timelineUserId = :acccount AND serverId > :sinceId AND serverId < :maxId""")
AND timelineUserId = :acccount AND
(LENGTH(serverId) < LENGTH(:maxId) OR LENGTH(serverId) == LENGTH(:maxId) AND serverId < :maxId)
AND
(LENGTH(serverId) > LENGTH(:sinceId) OR LENGTH(serverId) == LENGTH(:sinceId) AND serverId > :sinceId)
""")
abstract fun removeAllPlaceholdersBetween(acccount: Long, maxId: String, sinceId: String)
@Query("""UPDATE TimelineStatusEntity SET favourited = :favourited

View file

@ -29,6 +29,8 @@ import com.keylesspalace.tusky.db.AppDatabase
import com.keylesspalace.tusky.network.MastodonApi
import com.keylesspalace.tusky.network.TimelineCases
import com.keylesspalace.tusky.network.TimelineCasesImpl
import com.keylesspalace.tusky.util.HtmlConverter
import com.keylesspalace.tusky.util.HtmlConverterImpl
import dagger.Module
import dagger.Provides
import javax.inject.Singleton
@ -77,4 +79,10 @@ class AppModule {
fun providesDatabase(app: TuskyApplication): AppDatabase {
return app.serviceLocator.get(AppDatabase::class.java)
}
@Provides
@Singleton
fun providesHtmlConverter(): HtmlConverter {
return HtmlConverterImpl()
}
}

View file

@ -6,6 +6,7 @@ import com.keylesspalace.tusky.db.AppDatabase
import com.keylesspalace.tusky.network.MastodonApi
import com.keylesspalace.tusky.repository.TimelineRepository
import com.keylesspalace.tusky.repository.TimelineRepositoryImpl
import com.keylesspalace.tusky.util.HtmlConverter
import dagger.Module
import dagger.Provides
@ -13,7 +14,9 @@ import dagger.Provides
class RepositoryModule {
@Provides
fun providesTimelineRepository(db: AppDatabase, mastodonApi: MastodonApi,
accountManager: AccountManager, gson: Gson): TimelineRepository {
return TimelineRepositoryImpl(db.timelineDao(), mastodonApi, accountManager, gson)
accountManager: AccountManager, gson: Gson,
htmlConverter: HtmlConverter): TimelineRepository {
return TimelineRepositoryImpl(db.timelineDao(), mastodonApi, accountManager, gson,
htmlConverter)
}
}

View file

@ -244,14 +244,14 @@ public class TimelineFragment extends SFragment implements
if (this.kind == Kind.HOME) {
this.tryCache();
} else {
sendFetchTimelineRequest(null, null, FetchEnd.BOTTOM, -1);
sendFetchTimelineRequest(null, null, null, FetchEnd.BOTTOM, -1);
}
}
private void tryCache() {
// Request timeline from disk to make it quick, then replace it with timeline from
// the server to update it
this.timelineRepo.getStatuses(null, null, LOAD_AT_ONCE,
this.timelineRepo.getStatuses(null, null, null, LOAD_AT_ONCE,
TimelineRequestMode.DISK)
.observeOn(AndroidSchedulers.mainThread())
.as(autoDisposable(from(this, Lifecycle.Event.ON_DESTROY)))
@ -278,7 +278,7 @@ public class TimelineFragment extends SFragment implements
} else {
topId = CollectionsKt.first(statuses, Either::isRight).asRight().getId();
}
this.timelineRepo.getStatuses(topId, null, LOAD_AT_ONCE,
this.timelineRepo.getStatuses(topId, null, null, LOAD_AT_ONCE,
TimelineRequestMode.NETWORK)
.observeOn(AndroidSchedulers.mainThread())
.as(autoDisposable(from(this, Lifecycle.Event.ON_DESTROY)))
@ -520,12 +520,22 @@ public class TimelineFragment extends SFragment implements
}
private void loadAbove() {
Either<Placeholder, Status> firstOrNull =
CollectionsKt.firstOrNull(this.statuses, Either::isRight);
String firstOrNull = null;
String secondOrNull = null;
for (int i = 0; i < this.statuses.size(); i++) {
Either<Placeholder, Status> status = this.statuses.get(i);
if (status.isRight()) {
firstOrNull = status.asRight().getId();
if (i + 1 < statuses.size() && statuses.get(i + 1).isRight()) {
secondOrNull = statuses.get(i + 1).asRight().getId();
}
break;
}
}
if (firstOrNull != null) {
this.sendFetchTimelineRequest(null, firstOrNull.asRight().getId(), FetchEnd.TOP, -1);
this.sendFetchTimelineRequest(null, firstOrNull, secondOrNull, FetchEnd.TOP, -1);
} else {
this.sendFetchTimelineRequest(null, null, FetchEnd.BOTTOM, -1);
this.sendFetchTimelineRequest(null, null, null, FetchEnd.BOTTOM, -1);
}
}
@ -631,11 +641,16 @@ public class TimelineFragment extends SFragment implements
if (statuses.size() >= position && position > 0) {
Status fromStatus = statuses.get(position - 1).asRightOrNull();
Status toStatus = statuses.get(position + 1).asRightOrNull();
String maxMinusOne =
statuses.size() > position + 1 && statuses.get(position + 2).isRight()
? statuses.get(position + 1).asRight().getId()
: null;
if (fromStatus == null || toStatus == null) {
Log.e(TAG, "Failed to load more at " + position + ", wrong placeholder position");
return;
}
sendFetchTimelineRequest(fromStatus.getId(), toStatus.getId(), FetchEnd.MIDDLE, position);
sendFetchTimelineRequest(fromStatus.getId(), toStatus.getId(), maxMinusOne,
FetchEnd.MIDDLE, position);
Placeholder placeholder = statuses.get(position).asLeft();
StatusViewData newViewData = new StatusViewData.Placeholder(placeholder.getId(), true);
@ -810,14 +825,14 @@ public class TimelineFragment extends SFragment implements
break;
}
}
sendFetchTimelineRequest(bottomId, null, FetchEnd.BOTTOM, -1);
sendFetchTimelineRequest(bottomId, null, null, FetchEnd.BOTTOM, -1);
}
private void fullyRefresh() {
statuses.clear();
updateAdapter();
bottomLoading = true;
sendFetchTimelineRequest(null, null, FetchEnd.BOTTOM, -1);
sendFetchTimelineRequest(null, null, null, FetchEnd.BOTTOM, -1);
}
private boolean jumpToTopAllowed() {
@ -861,7 +876,8 @@ public class TimelineFragment extends SFragment implements
}
}
private void sendFetchTimelineRequest(@Nullable String fromId, @Nullable String uptoId,
private void sendFetchTimelineRequest(@Nullable String maxId, @Nullable String sinceId,
@Nullable String sinceIdMinusOne,
final FetchEnd fetchEnd, final int pos) {
if (kind == Kind.HOME) {
TimelineRequestMode mode;
@ -871,7 +887,7 @@ public class TimelineFragment extends SFragment implements
} else {
mode = TimelineRequestMode.NETWORK;
}
timelineRepo.getStatuses(fromId, uptoId, LOAD_AT_ONCE, mode)
timelineRepo.getStatuses(maxId, sinceId, sinceIdMinusOne, LOAD_AT_ONCE, mode)
.observeOn(AndroidSchedulers.mainThread())
.as(autoDisposable(from(this, Lifecycle.Event.ON_DESTROY)))
.subscribe(
@ -895,7 +911,7 @@ public class TimelineFragment extends SFragment implements
}
};
Call<List<Status>> listCall = getFetchCallByTimelineType(kind, hashtagOrId, fromId, uptoId);
Call<List<Status>> listCall = getFetchCallByTimelineType(kind, hashtagOrId, maxId, sinceId);
callList.add(listCall);
listCall.enqueue(callback);
}

View file

@ -11,10 +11,7 @@ import com.keylesspalace.tusky.entity.Status
import com.keylesspalace.tusky.network.MastodonApi
import com.keylesspalace.tusky.repository.TimelineRequestMode.DISK
import com.keylesspalace.tusky.repository.TimelineRequestMode.NETWORK
import com.keylesspalace.tusky.util.Either
import com.keylesspalace.tusky.util.HtmlUtils
import com.keylesspalace.tusky.util.dec
import com.keylesspalace.tusky.util.inc
import com.keylesspalace.tusky.util.*
import io.reactivex.Single
import io.reactivex.schedulers.Schedulers
import java.io.IOException
@ -30,7 +27,7 @@ enum class TimelineRequestMode {
}
interface TimelineRepository {
fun getStatuses(maxId: String?, sinceId: String?, limit: Int,
fun getStatuses(maxId: String?, sinceId: String?, sincedIdMinusOne: String?, limit: Int,
requestMode: TimelineRequestMode): Single<out List<TimelineStatus>>
companion object {
@ -42,15 +39,17 @@ class TimelineRepositoryImpl(
private val timelineDao: TimelineDao,
private val mastodonApi: MastodonApi,
private val accountManager: AccountManager,
private val gson: Gson
private val gson: Gson,
private val htmlConverter: HtmlConverter
) : TimelineRepository {
init {
this.cleanup()
}
override fun getStatuses(maxId: String?, sinceId: String?, limit: Int,
requestMode: TimelineRequestMode): Single<out List<TimelineStatus>> {
override fun getStatuses(maxId: String?, sinceId: String?, sincedIdMinusOne: String?,
limit: Int, requestMode: TimelineRequestMode
): Single<out List<TimelineStatus>> {
val acc = accountManager.activeAccount ?: throw IllegalStateException()
val accountId = acc.id
val instance = acc.domain
@ -58,21 +57,19 @@ class TimelineRepositoryImpl(
return if (requestMode == DISK) {
this.getStatusesFromDb(accountId, maxId, sinceId, limit)
} else {
getStatusesFromNetwork(maxId, sinceId, limit, instance, accountId, requestMode)
getStatusesFromNetwork(maxId, sinceId, sincedIdMinusOne, limit, instance, accountId,
requestMode)
}
}
private fun getStatusesFromNetwork(maxId: String?, sinceId: String?, limit: Int,
instance: String, accountId: Long,
requestMode: TimelineRequestMode
private fun getStatusesFromNetwork(maxId: String?, sinceId: String?,
sinceIdMinusOne: String?, limit: Int, instance: String,
accountId: Long, requestMode: TimelineRequestMode
): Single<out List<TimelineStatus>> {
val maxIdInc = maxId?.let(String::inc)
val sinceIdDec = sinceId?.let(String::dec)
return mastodonApi.homeTimelineSingle(maxIdInc, sinceIdDec, limit + 2)
.doAfterSuccess { statuses ->
return mastodonApi.homeTimelineSingle(maxId, sinceIdMinusOne, limit + 1)
.map { statuses ->
this.saveStatusesToDb(instance, accountId, statuses, maxId, sinceId)
}
.map { statuses -> this.removePlaceholdersAndMap(statuses, maxId, sinceId) }
.flatMap { statuses ->
this.addFromDbIfNeeded(accountId, statuses, maxId, sinceId, limit, requestMode)
}
@ -85,22 +82,6 @@ class TimelineRepositoryImpl(
}
}
private fun removePlaceholdersAndMap(statuses: List<Status>, maxId: String?,
sinceId: String?
): List<Either.Right<Placeholder, Status>> {
val statusesCopy = statuses.toMutableList()
// Remove first and last statuses if they were used used just for overlap
if (maxId != null && statusesCopy.firstOrNull()?.id == maxId) {
statusesCopy.removeAt(0)
}
if (sinceId != null && statusesCopy.lastOrNull()?.id == sinceId) {
statusesCopy.removeAt(statusesCopy.size - 1)
}
return statusesCopy.map { s -> Either.Right<Placeholder, Status>(s) }
}
private fun addFromDbIfNeeded(accountId: Long, statuses: List<Either<Placeholder, Status>>,
maxId: String?, sinceId: String?, limit: Int,
requestMode: TimelineRequestMode
@ -109,8 +90,7 @@ class TimelineRepositoryImpl(
val newMaxID = if (statuses.isEmpty()) {
maxId
} else {
// It's statuses from network. They're always Right
statuses.last().asRight().id
statuses.last { it.isRight() }.asRight().id
}
this.getStatusesFromDb(accountId, newMaxID, sinceId, limit)
.map { fromDb ->
@ -137,71 +117,64 @@ class TimelineRepositoryImpl(
}
private fun saveStatusesToDb(instance: String, accountId: Long, statuses: List<Status>,
maxId: String?, sinceId: String?) {
maxId: String?, sinceId: String?
): List<Either<Placeholder, Status>> {
var placeholderToInsert: Placeholder? = null
// Look for overlap
val resultStatuses = if (statuses.isNotEmpty() && sinceId != null) {
val indexOfSince = statuses.indexOfLast { it.id == sinceId }
if (indexOfSince == -1) {
// We didn't find the status which must be there. Add a placeholder
placeholderToInsert = Placeholder(sinceId.inc())
statuses.mapTo(mutableListOf(), Status::lift)
.apply {
add(Either.Left(placeholderToInsert))
}
} else {
// There was an overlap. Remove all overlapped statuses. No need for a placeholder.
statuses.mapTo(mutableListOf(), Status::lift)
.apply {
subList(indexOfSince, size).clear()
}
}
} else {
// Just a normal case.
statuses.map(Status::lift)
}
Single.fromCallable {
val (prepend, append) = calculatePlaceholders(maxId, sinceId, statuses)
if (prepend != null) {
timelineDao.insertStatusIfNotThere(prepend.toEntity(accountId))
}
if (append != null) {
timelineDao.insertStatusIfNotThere(append.toEntity(accountId))
}
for (status in statuses) {
timelineDao.insertInTransaction(
status.toEntity(accountId, instance),
status.account.toEntity(instance, accountId),
status.reblog?.account?.toEntity(instance, accountId)
status.toEntity(accountId, instance, htmlConverter, gson),
status.account.toEntity(instance, accountId, gson),
status.reblog?.account?.toEntity(instance, accountId, gson)
)
}
placeholderToInsert?.let {
timelineDao.insertStatusIfNotThere(placeholderToInsert.toEntity(accountId))
}
// If we're loading in the bottom insert placeholder after every load
// (for requests on next launches) but not return it.
if (sinceId == null && statuses.isNotEmpty()) {
timelineDao.insertStatusIfNotThere(
Placeholder(statuses.last().id.dec()).toEntity(accountId))
}
// There may be placeholders which we thought could be from our TL but they are not
if (statuses.size > 2) {
timelineDao.removeAllPlaceholdersBetween(accountId, statuses.first().id,
statuses.last().id)
} else if (maxId != null && sinceId != null) {
} else if (placeholderToInsert == null && maxId != null && sinceId != null) {
timelineDao.removeAllPlaceholdersBetween(accountId, maxId, sinceId)
}
}
.subscribeOn(Schedulers.io())
.subscribe()
}
private fun calculatePlaceholders(maxId: String?, sinceId: String?,
statuses: List<Status>
): Pair<Placeholder?, Placeholder?> {
if (statuses.isEmpty()) return null to null
val firstId = statuses.first().id
val prepend = if (maxId != null) {
if (maxId > firstId) {
val decMax = maxId.dec()
if (decMax != firstId) {
Placeholder(decMax)
} else null
} else null
} else {
// Placeholders never overwrite real values so it's safe
Placeholder(firstId.inc())
}
val lastId = statuses.last().id
val append = if (sinceId != null) {
if (sinceId < lastId) {
val incSince = sinceId.inc()
if (incSince != lastId) {
Placeholder(incSince)
} else null
} else null
} else {
// Placeholders never overwrite real values so it's safe
Placeholder(lastId.dec())
}
return prepend to append
return resultStatuses
}
private fun cleanup() {
@ -215,42 +188,6 @@ class TimelineRepositoryImpl(
.subscribe()
}
private fun Account.toEntity(instance: String, accountId: Long): TimelineAccountEntity {
return TimelineAccountEntity(
serverId = id,
timelineUserId = accountId,
instance = instance,
localUsername = localUsername,
username = username,
displayName = displayName,
url = url,
avatar = avatar,
emojis = gson.toJson(emojis)
)
}
private fun TimelineAccountEntity.toAccount(): Account {
return Account(
id = serverId,
localUsername = localUsername,
username = username,
displayName = displayName,
note = SpannedString(""),
url = url,
avatar = avatar,
header = "",
locked = false,
followingCount = 0,
followersCount = 0,
statusesCount = 0,
source = null,
bot = false,
emojis = gson.fromJson(this.emojis, emojisListTypeToken.type),
fields = null,
moved = null
)
}
private fun TimelineStatusWithAccount.toStatus(): TimelineStatus {
if (this.status.authorServerId == null) {
return Either.Left(Placeholder(this.status.serverId))
@ -268,11 +205,11 @@ class TimelineRepositoryImpl(
Status(
id = id,
url = status.url,
account = account.toAccount(),
account = account.toAccount(gson),
inReplyToId = status.inReplyToId,
inReplyToAccountId = status.inReplyToAccountId,
reblog = null,
content = HtmlUtils.fromHtml(status.content),
content = status.content?.let(htmlConverter::fromHtml) ?: SpannedString(""),
createdAt = Date(status.createdAt),
emojis = emojis,
reblogsCount = status.reblogsCount,
@ -293,7 +230,7 @@ class TimelineRepositoryImpl(
Status(
id = status.serverId,
url = null, // no url for reblogs
account = this.reblogAccount!!.toAccount(),
account = this.reblogAccount!!.toAccount(gson),
inReplyToId = null,
inReplyToAccountId = null,
reblog = reblog,
@ -316,11 +253,11 @@ class TimelineRepositoryImpl(
Status(
id = status.serverId,
url = status.url,
account = account.toAccount(),
account = account.toAccount(gson),
inReplyToId = status.inReplyToId,
inReplyToAccountId = status.inReplyToAccountId,
reblog = null,
content = HtmlUtils.fromHtml(status.content),
content = status.content?.let(htmlConverter::fromHtml) ?: SpannedString(""),
createdAt = Date(status.createdAt),
emojis = emojis,
reblogsCount = status.reblogsCount,
@ -338,64 +275,103 @@ class TimelineRepositoryImpl(
}
return Either.Right(status)
}
}
private fun Status.toEntity(timelineUserId: Long, instance: String): TimelineStatusEntity {
val actionable = actionableStatus
return TimelineStatusEntity(
serverId = this.id,
url = actionable.url!!,
instance = instance,
timelineUserId = timelineUserId,
authorServerId = actionable.account.id,
inReplyToId = actionable.inReplyToId,
inReplyToAccountId = actionable.inReplyToAccountId,
content = HtmlUtils.toHtml(actionable.content),
createdAt = actionable.createdAt.time,
emojis = actionable.emojis.let(gson::toJson),
reblogsCount = actionable.reblogsCount,
favouritesCount = actionable.favouritesCount,
reblogged = actionable.reblogged,
favourited = actionable.favourited,
sensitive = actionable.sensitive,
spoilerText = actionable.spoilerText,
visibility = actionable.visibility,
attachments = actionable.attachments.let(gson::toJson),
mentions = actionable.mentions.let(gson::toJson),
application = actionable.let(gson::toJson),
reblogServerId = reblog?.id,
reblogAccountId = reblog?.let { this.account.id }
)
}
private val emojisListTypeToken = object : TypeToken<List<Emoji>>() {}
private fun Placeholder.toEntity(timelineUserId: Long): TimelineStatusEntity {
return TimelineStatusEntity(
serverId = this.id,
url = null,
instance = null,
timelineUserId = timelineUserId,
authorServerId = null,
inReplyToId = null,
inReplyToAccountId = null,
content = null,
createdAt = 0L,
emojis = null,
reblogsCount = 0,
favouritesCount = 0,
reblogged = false,
favourited = false,
sensitive = false,
spoilerText = null,
visibility = null,
attachments = null,
mentions = null,
application = null,
reblogServerId = null,
reblogAccountId = null
fun Account.toEntity(instance: String, accountId: Long, gson: Gson): TimelineAccountEntity {
return TimelineAccountEntity(
serverId = id,
timelineUserId = accountId,
instance = instance,
localUsername = localUsername,
username = username,
displayName = displayName,
url = url,
avatar = avatar,
emojis = gson.toJson(emojis)
)
}
)
}
fun TimelineAccountEntity.toAccount(gson: Gson): Account {
return Account(
id = serverId,
localUsername = localUsername,
username = username,
displayName = displayName,
note = SpannedString(""),
url = url,
avatar = avatar,
header = "",
locked = false,
followingCount = 0,
followersCount = 0,
statusesCount = 0,
source = null,
bot = false,
emojis = gson.fromJson(this.emojis, emojisListTypeToken.type),
fields = null,
moved = null
)
}
companion object {
private val emojisListTypeToken = object : TypeToken<List<Emoji>>() {}
}
}
fun Placeholder.toEntity(timelineUserId: Long): TimelineStatusEntity {
return TimelineStatusEntity(
serverId = this.id,
url = null,
instance = null,
timelineUserId = timelineUserId,
authorServerId = null,
inReplyToId = null,
inReplyToAccountId = null,
content = null,
createdAt = 0L,
emojis = null,
reblogsCount = 0,
favouritesCount = 0,
reblogged = false,
favourited = false,
sensitive = false,
spoilerText = null,
visibility = null,
attachments = null,
mentions = null,
application = null,
reblogServerId = null,
reblogAccountId = null
)
}
fun Status.toEntity(timelineUserId: Long, instance: String,
htmlConverter: HtmlConverter,
gson: Gson): TimelineStatusEntity {
val actionable = actionableStatus
return TimelineStatusEntity(
serverId = this.id,
url = actionable.url!!,
instance = instance,
timelineUserId = timelineUserId,
authorServerId = actionable.account.id,
inReplyToId = actionable.inReplyToId,
inReplyToAccountId = actionable.inReplyToAccountId,
content = htmlConverter.toHtml(actionable.content),
createdAt = actionable.createdAt.time,
emojis = actionable.emojis.let(gson::toJson),
reblogsCount = actionable.reblogsCount,
favouritesCount = actionable.favouritesCount,
reblogged = actionable.reblogged,
favourited = actionable.favourited,
sensitive = actionable.sensitive,
spoilerText = actionable.spoilerText,
visibility = actionable.visibility,
attachments = actionable.attachments.let(gson::toJson),
mentions = actionable.mentions.let(gson::toJson),
application = actionable.let(gson::toJson),
reblogServerId = reblog?.id,
reblogAccountId = reblog?.let { this.account.id }
)
}
fun Status.lift(): Either<Placeholder, Status> = Either.Right(this)

View file

@ -0,0 +1,22 @@
package com.keylesspalace.tusky.util
import android.text.Spanned
/**
* Abstracting away Android-specific things.
*/
interface HtmlConverter {
fun fromHtml(html: String): Spanned
fun toHtml(text: Spanned): String
}
internal class HtmlConverterImpl : HtmlConverter {
override fun fromHtml(html: String): Spanned {
return HtmlUtils.fromHtml(html)
}
override fun toHtml(text: Spanned): String {
return HtmlUtils.toHtml(text)
}
}

View file

@ -25,7 +25,7 @@ fun randomAlphanumericString(count: Int): String {
fun String.inc(): String {
// We assume that we will stay in the safe range for now
val builder = this.toCharArray()
builder.last().inc()
builder[lastIndex] = builder[lastIndex].inc()
return String(builder)
}
@ -34,24 +34,25 @@ fun String.inc(): String {
* "Decrement" string so that during sorting it's smaller than [this].
*/
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()
break
return String(builder)
} else {
builder[i] = 'z'
}
i--
}
// All characters were '0'
if (i == 0 && this.isNotEmpty()) {
// Remove one character
return String(builder.copyOfRange(1, builder.size))
return if (builder[0] > '1') {
builder[0] = builder[0].dec()
String(builder)
} else {
String(builder.copyOfRange(1, builder.size))
}
return String(builder)
}
/**