From 6fedfe54baef11f35e70f003d8bbcdb1b48d2e6b Mon Sep 17 00:00:00 2001 From: Nik Clayton Date: Fri, 29 Sep 2023 11:10:55 +0200 Subject: [PATCH] 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 --- app/lint-baseline.xml | 2 +- .../timeline/CachedTimelineRepository.kt | 2 +- .../components/timeline/TimelineFragment.kt | 39 +++---------------- .../viewmodel/CachedTimelineRemoteMediator.kt | 18 ++++----- .../main/java/app/pachli/db/AppDatabase.kt | 1 + 5 files changed, 17 insertions(+), 45 deletions(-) diff --git a/app/lint-baseline.xml b/app/lint-baseline.xml index 9a7183128..c91440b4c 100644 --- a/app/lint-baseline.xml +++ b/app/lint-baseline.xml @@ -3688,7 +3688,7 @@ errorLine2=" ~~~~~~~~~"> diff --git a/app/src/main/java/app/pachli/components/timeline/CachedTimelineRepository.kt b/app/src/main/java/app/pachli/components/timeline/CachedTimelineRepository.kt index b99da1119..ed9a43e36 100644 --- a/app/src/main/java/app/pachli/components/timeline/CachedTimelineRepository.kt +++ b/app/src/main/java/app/pachli/components/timeline/CachedTimelineRepository.kt @@ -85,7 +85,7 @@ class CachedTimelineRepository @Inject constructor( Log.d(TAG, "initialKey: $initialKey is row: $row") return Pager( - config = PagingConfig(pageSize = pageSize), + config = PagingConfig(pageSize = pageSize, jumpThreshold = PAGE_SIZE * 3, enablePlaceholders = true), initialKey = row, remoteMediator = CachedTimelineRemoteMediator( initialKey, diff --git a/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt b/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt index e9dc5f8f9..fad9d54ac 100644 --- a/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt +++ b/app/src/main/java/app/pachli/components/timeline/TimelineFragment.kt @@ -131,9 +131,6 @@ class TimelineFragment : 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?) { super.onCreate(savedInstanceState) @@ -141,8 +138,6 @@ class TimelineFragment : timelineKind = arguments.getParcelable(KIND_ARG)!! - shouldRestoreReadingPosition = timelineKind == TimelineKind.Home - viewModel.init(timelineKind) isSwipeToRefreshEnabled = arguments.getBoolean(ARG_ENABLE_SWIPE_TO_REFRESH, true) @@ -359,28 +354,6 @@ class TimelineFragment : if (userRefreshState == UserRefreshState.COMPLETE) { // 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 // if one them has not already caused a peek. if (peeked) return@collect @@ -528,13 +501,14 @@ class TimelineFragment : * previous first status always remains visible. */ fun saveVisibleId(statusId: String? = null) { - statusId ?: layoutManager.findFirstCompletelyVisibleItemPosition() + val id = statusId ?: layoutManager.findFirstCompletelyVisibleItemPosition() .takeIf { it != RecyclerView.NO_POSITION } ?.let { adapter.snapshot().getOrNull(it)?.id } - ?.let { - Log.d(TAG, "Saving ID: $it") - viewModel.accept(InfallibleUiAction.SaveVisibleId(visibleId = it)) - } + + id?.let { + Log.d(TAG, "Saving ID: $it") + viewModel.accept(InfallibleUiAction.SaveVisibleId(visibleId = it)) + } } private fun setupSwipeRefreshLayout() { @@ -569,7 +543,6 @@ class TimelineFragment : } override fun onRefresh() { - shouldRestoreReadingPosition = timelineKind == TimelineKind.Home binding.statusView.hide() snackbar?.dismiss() diff --git a/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt b/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt index 712dea39a..5441f0751 100644 --- a/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt +++ b/app/src/main/java/app/pachli/components/timeline/viewmodel/CachedTimelineRemoteMediator.kt @@ -71,10 +71,12 @@ class CachedTimelineRemoteMediator( return try { val response = when (loadType) { LoadType.REFRESH -> { - val closestItem = state.anchorPosition?.let { state.closestItemToPosition(it) }?.status?.serverId - val key = closestItem ?: initialKey - Log.d(TAG, "Loading from item: $key") - getInitialPage(key, state.config.pageSize) + val closestItem = state.anchorPosition?.let { + state.closestItemToPosition(maxOf(0, it - (state.config.pageSize / 2))) + }?.status?.serverId + val statusId = closestItem ?: initialKey + Log.d(TAG, "Loading from item: $statusId") + getInitialPage(statusId, state.config.pageSize) } LoadType.APPEND -> { 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 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 deferredNextPage = async { api.homeTimeline(maxId = statusId, limit = pageSize) } - val deferredPrevPage = async { - api.homeTimeline(minId = statusId, limit = pageSize) - } deferredStatus.await().getOrNull()?.let { status -> val statuses = buildList { - deferredPrevPage.await().body()?.let { this.addAll(it) } this.add(status) 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 // of statuses immediately newer than their desired status. This page must // *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.body().isNullOrEmpty()) return@coroutineScope response } diff --git a/app/src/main/java/app/pachli/db/AppDatabase.kt b/app/src/main/java/app/pachli/db/AppDatabase.kt index 0032793fd..320433c30 100644 --- a/app/src/main/java/app/pachli/db/AppDatabase.kt +++ b/app/src/main/java/app/pachli/db/AppDatabase.kt @@ -24,6 +24,7 @@ import androidx.room.RoomDatabase import androidx.room.migration.AutoMigrationSpec import app.pachli.components.conversation.ConversationEntity +@Suppress("ClassName") @Database( entities = [ DraftEntity::class,