fix: Restore the user's reading position under all circumstances (#133)

The previous code did not always work when the user returned to the app
after a lengthy absence (e.g., overnight).

Instead of restoring by scrolling in `TimelineFragment`, restore by
working with the platform.

Determine the initial page to fetch by looking half a page ahead of the
saved saved status ID, and fetch that status and the page immediately
prior. This seems to match the view's expectations about what will be
immediately available.

Set `jumpThreshold` and `enablePlaceholders` in the `PagingConfig` so
the paging system will jump to the saved status.

Remove the restoration code in `TimelineFragment`.

Fixes #53
This commit is contained in:
Nik Clayton 2023-09-29 11:10:55 +02:00 committed by GitHub
parent d434144922
commit 6fedfe54ba
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 17 additions and 45 deletions

View File

@ -3688,7 +3688,7 @@
errorLine2=" ~~~~~~~~~"> errorLine2=" ~~~~~~~~~">
<location <location
file="src/main/java/app/pachli/components/timeline/TimelineFragment.kt" file="src/main/java/app/pachli/components/timeline/TimelineFragment.kt"
line="185" line="180"
column="47"/> column="47"/>
</issue> </issue>

View File

@ -85,7 +85,7 @@ class CachedTimelineRepository @Inject constructor(
Log.d(TAG, "initialKey: $initialKey is row: $row") Log.d(TAG, "initialKey: $initialKey is row: $row")
return Pager( return Pager(
config = PagingConfig(pageSize = pageSize), config = PagingConfig(pageSize = pageSize, jumpThreshold = PAGE_SIZE * 3, enablePlaceholders = true),
initialKey = row, initialKey = row,
remoteMediator = CachedTimelineRemoteMediator( remoteMediator = CachedTimelineRemoteMediator(
initialKey, initialKey,

View File

@ -131,9 +131,6 @@ class TimelineFragment :
private var isSwipeToRefreshEnabled = true private var isSwipeToRefreshEnabled = true
/** True if the reading position should be restored when new data is submitted to the adapter */
private var shouldRestoreReadingPosition = false
override fun onCreate(savedInstanceState: Bundle?) { override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState) super.onCreate(savedInstanceState)
@ -141,8 +138,6 @@ class TimelineFragment :
timelineKind = arguments.getParcelable(KIND_ARG)!! timelineKind = arguments.getParcelable(KIND_ARG)!!
shouldRestoreReadingPosition = timelineKind == TimelineKind.Home
viewModel.init(timelineKind) viewModel.init(timelineKind)
isSwipeToRefreshEnabled = arguments.getBoolean(ARG_ENABLE_SWIPE_TO_REFRESH, true) isSwipeToRefreshEnabled = arguments.getBoolean(ARG_ENABLE_SWIPE_TO_REFRESH, true)
@ -359,28 +354,6 @@ class TimelineFragment :
if (userRefreshState == UserRefreshState.COMPLETE) { if (userRefreshState == UserRefreshState.COMPLETE) {
// Refresh has finished, pages are being prepended. // Refresh has finished, pages are being prepended.
// Restore the user's reading position, if appropriate.
if (shouldRestoreReadingPosition) {
Log.d(
TAG,
"Page updated, should restore reading position to ${viewModel.readingPositionId}",
)
adapter.snapshot()
.indexOfFirst { it?.id == viewModel.readingPositionId }
.takeIf { it != -1 }
?.let { pos ->
Log.d(TAG, "restored reading position")
binding.recyclerView.post {
getView() ?: return@post
(binding.recyclerView.layoutManager as? LinearLayoutManager)?.scrollToPositionWithOffset(
pos,
0,
)
}
shouldRestoreReadingPosition = false
}
}
// There might be multiple prepends after a refresh, only continue // There might be multiple prepends after a refresh, only continue
// if one them has not already caused a peek. // if one them has not already caused a peek.
if (peeked) return@collect if (peeked) return@collect
@ -528,10 +501,11 @@ class TimelineFragment :
* previous first status always remains visible. * previous first status always remains visible.
*/ */
fun saveVisibleId(statusId: String? = null) { fun saveVisibleId(statusId: String? = null) {
statusId ?: layoutManager.findFirstCompletelyVisibleItemPosition() val id = statusId ?: layoutManager.findFirstCompletelyVisibleItemPosition()
.takeIf { it != RecyclerView.NO_POSITION } .takeIf { it != RecyclerView.NO_POSITION }
?.let { adapter.snapshot().getOrNull(it)?.id } ?.let { adapter.snapshot().getOrNull(it)?.id }
?.let {
id?.let {
Log.d(TAG, "Saving ID: $it") Log.d(TAG, "Saving ID: $it")
viewModel.accept(InfallibleUiAction.SaveVisibleId(visibleId = it)) viewModel.accept(InfallibleUiAction.SaveVisibleId(visibleId = it))
} }
@ -569,7 +543,6 @@ class TimelineFragment :
} }
override fun onRefresh() { override fun onRefresh() {
shouldRestoreReadingPosition = timelineKind == TimelineKind.Home
binding.statusView.hide() binding.statusView.hide()
snackbar?.dismiss() snackbar?.dismiss()

View File

@ -71,10 +71,12 @@ class CachedTimelineRemoteMediator(
return try { return try {
val response = when (loadType) { val response = when (loadType) {
LoadType.REFRESH -> { LoadType.REFRESH -> {
val closestItem = state.anchorPosition?.let { state.closestItemToPosition(it) }?.status?.serverId val closestItem = state.anchorPosition?.let {
val key = closestItem ?: initialKey state.closestItemToPosition(maxOf(0, it - (state.config.pageSize / 2)))
Log.d(TAG, "Loading from item: $key") }?.status?.serverId
getInitialPage(key, state.config.pageSize) val statusId = closestItem ?: initialKey
Log.d(TAG, "Loading from item: $statusId")
getInitialPage(statusId, state.config.pageSize)
} }
LoadType.APPEND -> { LoadType.APPEND -> {
val rke = db.withTransaction { val rke = db.withTransaction {
@ -200,18 +202,14 @@ class CachedTimelineRemoteMediator(
// You can fetch the page immediately before the key, or the page immediately after, but // You can fetch the page immediately before the key, or the page immediately after, but
// you can not fetch the page itself. // you can not fetch the page itself.
// Fetch the requested status, and the pages immediately before (prev) and after (next) // Fetch the requested status, and the page immediately after (next)
val deferredStatus = async { api.status(statusId = statusId) } val deferredStatus = async { api.status(statusId = statusId) }
val deferredNextPage = async { val deferredNextPage = async {
api.homeTimeline(maxId = statusId, limit = pageSize) api.homeTimeline(maxId = statusId, limit = pageSize)
} }
val deferredPrevPage = async {
api.homeTimeline(minId = statusId, limit = pageSize)
}
deferredStatus.await().getOrNull()?.let { status -> deferredStatus.await().getOrNull()?.let { status ->
val statuses = buildList { val statuses = buildList {
deferredPrevPage.await().body()?.let { this.addAll(it) }
this.add(status) this.add(status)
deferredNextPage.await().body()?.let { this.addAll(it) } deferredNextPage.await().body()?.let { this.addAll(it) }
} }
@ -243,7 +241,7 @@ class CachedTimelineRemoteMediator(
// There were no statuses older than the user's desired status. Return the page // There were no statuses older than the user's desired status. Return the page
// of statuses immediately newer than their desired status. This page must // of statuses immediately newer than their desired status. This page must
// *not* be empty (as noted earlier, if it is, paging stops). // *not* be empty (as noted earlier, if it is, paging stops).
deferredPrevPage.await().let { response -> api.homeTimeline(minId = statusId, limit = pageSize).let { response ->
if (response.isSuccessful) { if (response.isSuccessful) {
if (!response.body().isNullOrEmpty()) return@coroutineScope response if (!response.body().isNullOrEmpty()) return@coroutineScope response
} }

View File

@ -24,6 +24,7 @@ import androidx.room.RoomDatabase
import androidx.room.migration.AutoMigrationSpec import androidx.room.migration.AutoMigrationSpec
import app.pachli.components.conversation.ConversationEntity import app.pachli.components.conversation.ConversationEntity
@Suppress("ClassName")
@Database( @Database(
entities = [ entities = [
DraftEntity::class, DraftEntity::class,