From a71996757ab577373d6f86b99631f687daf5689b Mon Sep 17 00:00:00 2001 From: PoonHarpoon <52467605+PoonHarpoon@users.noreply.github.com> Date: Sun, 11 Oct 2026 19:32:37 +0000 Subject: [PATCH 1/3] Fix reader saving previous chapter's first page as progress Page change callbacks reported adapter positions, which the view model resolved against its latest content. The adapter applies content asynchronously, so after the previous chapter was prepended a stale position (e.g. page 1 of chapter 39) resolved to page 1 of chapter 38, and the chapter change was immediately saved to history. Resolve the visible pages from the adapter's own items and look them up by identity in the view model instead. --- .../futon/reader/ui/ReaderViewModel.kt | 26 ++++++++++++++++--- .../ui/pager/BasePagerReaderFragment.kt | 2 +- .../reader/ui/pager/BaseReaderFragment.kt | 13 ++++++++++ .../pager/doublepage/DoubleReaderFragment.kt | 2 +- .../ReversedDoubleReaderFragment.kt | 4 --- .../pager/reversed/ReversedReaderFragment.kt | 5 ---- .../ui/pager/webtoon/WebtoonReaderFragment.kt | 4 +-- 7 files changed, 39 insertions(+), 17 deletions(-) diff --git a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/ReaderViewModel.kt b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/ReaderViewModel.kt index 9b7ef307ac..df83e94d15 100644 --- a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/ReaderViewModel.kt +++ b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/ReaderViewModel.kt @@ -47,6 +47,7 @@ import io.github.landwarderer.futon.reader.domain.DetectReaderModeUseCase import io.github.landwarderer.futon.reader.domain.PageLoader import io.github.landwarderer.futon.reader.domain.nextChapterPrefetchPages import io.github.landwarderer.futon.reader.ui.config.ReaderSettings +import io.github.landwarderer.futon.reader.ui.pager.ReaderPage import io.github.landwarderer.futon.reader.ui.pager.ReaderUiState import io.github.landwarderer.futon.scrobbling.discord.ui.DiscordRpc import io.github.landwarderer.futon.stats.domain.StatsCollector @@ -346,16 +347,29 @@ class ReaderViewModel @Inject constructor( } } + /** + * Pages are passed by identity, not by position: the adapter applies [content] asynchronously, so its + * positions can refer to an older list. After a previous chapter is prepended, a stale position would + * otherwise resolve to the start of that previous chapter and be saved as the reading progress. + */ @MainThread - fun onCurrentPageChanged(lowerPos: Int, upperPos: Int) { + fun onCurrentPageChanged(lowerPage: ReaderPage?, upperPage: ReaderPage?) { + if (lowerPage == null || upperPage == null) { + return + } val prevJob = stateChangeJob - val pages = content.value.pages // capture immediately stateChangeJob = launchJob(Dispatchers.IO) { prevJob?.cancelAndJoin() loadingJob?.join() - if (pages.size != content.value.pages.size) { - return@launchJob // TODO + val pages = content.value.pages + val lowerIndex = pages.indexOfPage(lowerPage) + val upperIndex = pages.indexOfPage(upperPage) + if (lowerIndex < 0 || upperIndex < 0) { + return@launchJob // page is no longer loaded } + // reversed modes report pages in reverse order + val lowerPos = minOf(lowerIndex, upperIndex) + val upperPos = maxOf(lowerIndex, upperIndex) val centerPos = (lowerPos + upperPos) / 2 pages.getOrNull(centerPos)?.let { page -> val oldChapterId = readingState.value?.chapterId @@ -609,6 +623,10 @@ class ReaderViewModel @Inject constructor( } } + private fun List.indexOfPage(page: ReaderPage): Int = indexOfFirst { + it.chapterId == page.chapterId && it.index == page.index + } + private fun List.trySublist(fromIndex: Int, toIndex: Int): List { val fromIndexBounded = fromIndex.coerceAtMost(lastIndex) val toIndexBounded = toIndex.coerceIn(fromIndexBounded, lastIndex) diff --git a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BasePagerReaderFragment.kt b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BasePagerReaderFragment.kt index 9fa7be5fbb..616950bbe0 100644 --- a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BasePagerReaderFragment.kt +++ b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BasePagerReaderFragment.kt @@ -183,7 +183,7 @@ abstract class BasePagerReaderFragment : BaseReaderFragment : BaseFragment(), ZoomCont super.onDestroyView() } + /** Resolve adapter positions against the adapter's own items, which may lag behind the view model content. */ + protected fun notifyVisiblePagesChanged(lowerPos: Int, upperPos: Int) { + val adapter = readerAdapter ?: return + val lastIndex = adapter.itemCount - 1 + if (lastIndex < 0) { + return + } + viewModel.onCurrentPageChanged( + adapter.getItemOrNull(lowerPos.coerceIn(0, lastIndex)), + adapter.getItemOrNull(upperPos.coerceIn(0, lastIndex)), + ) + } + protected fun requireAdapter() = checkNotNull(readerAdapter) { "Adapter was not created or already destroyed" } diff --git a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/doublepage/DoubleReaderFragment.kt b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/doublepage/DoubleReaderFragment.kt index a14711ec03..474d2eb31b 100644 --- a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/doublepage/DoubleReaderFragment.kt +++ b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/doublepage/DoubleReaderFragment.kt @@ -136,7 +136,7 @@ open class DoubleReaderFragment : BaseReaderFragment() firstVisiblePosition: Int, lastVisiblePosition: Int, ) { - viewModel.onCurrentPageChanged(firstVisiblePosition, lastVisiblePosition) + notifyVisiblePagesChanged(firstVisiblePosition, lastVisiblePosition) } override suspend fun onPagesChanged(pages: List, pendingState: ReaderState?) = coroutineScope { @@ -150,7 +150,7 @@ class WebtoonReaderFragment : BaseReaderFragment() ?.restoreScroll(pendingState.scroll) } } - viewModel.onCurrentPageChanged(position, position) + notifyVisiblePagesChanged(position, position) } else { Snackbar.make(requireView(), R.string.not_found_404, Snackbar.LENGTH_SHORT) .show() From 9643552f495b3329460c552a35039fb8b5bb8ecf Mon Sep 17 00:00:00 2001 From: PoonHarpoon <52467605+PoonHarpoon@users.noreply.github.com> Date: Wed, 4 Nov 2026 05:29:10 +0000 Subject: [PATCH 2/3] Ignore NO_POSITION instead of clamping it to the first page The double-page scroll listener reports NO_POSITION (-1) when nothing is laid out, for example while the reader is resized to zero. Clamping it to 0 resolved to the first page of the earliest loaded chapter, which could then be saved to history as the reading progress and also triggered an unneeded load of the previous chapter. Drop updates with a negative position and clamp only the upper bound, which a double-page spread ending on the last page can exceed by one. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FVCWk6dfgPw4JbUGTiRppj --- .../futon/reader/ui/pager/BaseReaderFragment.kt | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BaseReaderFragment.kt b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BaseReaderFragment.kt index c3589718fa..14f298b0d0 100644 --- a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BaseReaderFragment.kt +++ b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BaseReaderFragment.kt @@ -2,6 +2,7 @@ package io.github.landwarderer.futon.reader.ui.pager import android.os.Bundle import android.view.View +import androidx.annotation.MainThread import androidx.core.view.WindowInsetsCompat import androidx.fragment.app.activityViewModels import androidx.viewbinding.ViewBinding @@ -61,16 +62,25 @@ abstract class BaseReaderFragment : BaseFragment(), ZoomCont super.onDestroyView() } - /** Resolve adapter positions against the adapter's own items, which may lag behind the view model content. */ + /** + * Resolve adapter positions against the adapter's own items, which may lag behind the view model content. + * Must be called on the main thread, while the positions still match the adapter's current list. + */ + @MainThread protected fun notifyVisiblePagesChanged(lowerPos: Int, upperPos: Int) { + // NO_POSITION means nothing is laid out yet; clamping it to 0 would report the first loaded page + if (lowerPos < 0 || upperPos < 0) { + return + } val adapter = readerAdapter ?: return val lastIndex = adapter.itemCount - 1 if (lastIndex < 0) { return } + // only the upper bound is clamped: a double-page spread can end one past the last page viewModel.onCurrentPageChanged( - adapter.getItemOrNull(lowerPos.coerceIn(0, lastIndex)), - adapter.getItemOrNull(upperPos.coerceIn(0, lastIndex)), + adapter.getItemOrNull(lowerPos.coerceAtMost(lastIndex)), + adapter.getItemOrNull(upperPos.coerceAtMost(lastIndex)), ) } From dc8126d64495f78ff539d48f635afd318dd4e1cb Mon Sep 17 00:00:00 2001 From: PoonHarpoon <52467605+PoonHarpoon@users.noreply.github.com> Date: Wed, 4 Nov 2026 05:33:19 +0000 Subject: [PATCH 3/3] Extract visible page resolution and test the prepend scenario Move the adapter position checks and the lookup of visible pages in the view model content into pure functions, so the stale-position case can be unit tested without a fragment or view model. The tests cover a position reported by the adapter after the previous chapter was prepended, NO_POSITION, a double-page spread ending past the last page, reversed modes and pages that are no longer loaded. Also correct the onCurrentPageChanged KDoc: pages are matched by chapter and page index, not by identity. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01FVCWk6dfgPw4JbUGTiRppj --- .../futon/reader/ui/ReaderViewModel.kt | 21 ++--- .../reader/ui/pager/BaseReaderFragment.kt | 14 +-- .../futon/reader/ui/pager/VisiblePages.kt | 33 +++++++ .../futon/reader/ui/pager/VisiblePagesTest.kt | 88 +++++++++++++++++++ 4 files changed, 131 insertions(+), 25 deletions(-) create mode 100644 app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/VisiblePages.kt create mode 100644 app/src/test/kotlin/io/github/landwarderer/futon/reader/ui/pager/VisiblePagesTest.kt diff --git a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/ReaderViewModel.kt b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/ReaderViewModel.kt index df83e94d15..f2474f91e9 100644 --- a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/ReaderViewModel.kt +++ b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/ReaderViewModel.kt @@ -49,6 +49,7 @@ import io.github.landwarderer.futon.reader.domain.nextChapterPrefetchPages import io.github.landwarderer.futon.reader.ui.config.ReaderSettings import io.github.landwarderer.futon.reader.ui.pager.ReaderPage import io.github.landwarderer.futon.reader.ui.pager.ReaderUiState +import io.github.landwarderer.futon.reader.ui.pager.visibleRangeOf import io.github.landwarderer.futon.scrobbling.discord.ui.DiscordRpc import io.github.landwarderer.futon.stats.domain.StatsCollector import kotlinx.coroutines.CancellationException @@ -348,8 +349,8 @@ class ReaderViewModel @Inject constructor( } /** - * Pages are passed by identity, not by position: the adapter applies [content] asynchronously, so its - * positions can refer to an older list. After a previous chapter is prepended, a stale position would + * Pages are passed as pages, not positions, and matched by chapter and page index: the adapter applies + * [content] asynchronously, so its positions can refer to an older list. After a previous chapter is prepended, a stale position would * otherwise resolve to the start of that previous chapter and be saved as the reading progress. */ @MainThread @@ -362,14 +363,10 @@ class ReaderViewModel @Inject constructor( prevJob?.cancelAndJoin() loadingJob?.join() val pages = content.value.pages - val lowerIndex = pages.indexOfPage(lowerPage) - val upperIndex = pages.indexOfPage(upperPage) - if (lowerIndex < 0 || upperIndex < 0) { - return@launchJob // page is no longer loaded - } - // reversed modes report pages in reverse order - val lowerPos = minOf(lowerIndex, upperIndex) - val upperPos = maxOf(lowerIndex, upperIndex) + val visibleRange = pages.visibleRangeOf(lowerPage, upperPage) + ?: return@launchJob // page is no longer loaded + val lowerPos = visibleRange.first + val upperPos = visibleRange.last val centerPos = (lowerPos + upperPos) / 2 pages.getOrNull(centerPos)?.let { page -> val oldChapterId = readingState.value?.chapterId @@ -623,10 +620,6 @@ class ReaderViewModel @Inject constructor( } } - private fun List.indexOfPage(page: ReaderPage): Int = indexOfFirst { - it.chapterId == page.chapterId && it.index == page.index - } - private fun List.trySublist(fromIndex: Int, toIndex: Int): List { val fromIndexBounded = fromIndex.coerceAtMost(lastIndex) val toIndexBounded = toIndex.coerceIn(fromIndexBounded, lastIndex) diff --git a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BaseReaderFragment.kt b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BaseReaderFragment.kt index 14f298b0d0..b29b083a2e 100644 --- a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BaseReaderFragment.kt +++ b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BaseReaderFragment.kt @@ -68,19 +68,11 @@ abstract class BaseReaderFragment : BaseFragment(), ZoomCont */ @MainThread protected fun notifyVisiblePagesChanged(lowerPos: Int, upperPos: Int) { - // NO_POSITION means nothing is laid out yet; clamping it to 0 would report the first loaded page - if (lowerPos < 0 || upperPos < 0) { - return - } val adapter = readerAdapter ?: return - val lastIndex = adapter.itemCount - 1 - if (lastIndex < 0) { - return - } - // only the upper bound is clamped: a double-page spread can end one past the last page + val positions = visibleAdapterPositions(lowerPos, upperPos, adapter.itemCount) ?: return viewModel.onCurrentPageChanged( - adapter.getItemOrNull(lowerPos.coerceAtMost(lastIndex)), - adapter.getItemOrNull(upperPos.coerceAtMost(lastIndex)), + adapter.getItemOrNull(positions.first), + adapter.getItemOrNull(positions.last), ) } diff --git a/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/VisiblePages.kt b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/VisiblePages.kt new file mode 100644 index 0000000000..cf62d93713 --- /dev/null +++ b/app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/VisiblePages.kt @@ -0,0 +1,33 @@ +package io.github.landwarderer.futon.reader.ui.pager + +/** + * Adapter positions to report for the visible range, or null when there is nothing to report. + * + * A negative position is NO_POSITION (nothing is laid out); clamping it to 0 would report the first loaded page. + * Only the upper bound is clamped: a double-page spread can end one past the last page. + */ +internal fun visibleAdapterPositions(lowerPos: Int, upperPos: Int, itemCount: Int): IntRange? { + if (lowerPos < 0 || upperPos < 0 || itemCount <= 0) { + return null + } + val lastIndex = itemCount - 1 + return lowerPos.coerceAtMost(lastIndex)..upperPos.coerceAtMost(lastIndex) +} + +/** + * Indices of [lowerPage] and [upperPage] in this list, matched by chapter and page index rather than by position, + * or null if either page is no longer loaded. Reversed modes report the pages back to front, so the result is + * always ascending. + */ +internal fun List.visibleRangeOf(lowerPage: ReaderPage, upperPage: ReaderPage): IntRange? { + val lowerIndex = indexOfPage(lowerPage) + val upperIndex = indexOfPage(upperPage) + if (lowerIndex < 0 || upperIndex < 0) { + return null + } + return minOf(lowerIndex, upperIndex)..maxOf(lowerIndex, upperIndex) +} + +private fun List.indexOfPage(page: ReaderPage): Int = indexOfFirst { + it.chapterId == page.chapterId && it.index == page.index +} diff --git a/app/src/test/kotlin/io/github/landwarderer/futon/reader/ui/pager/VisiblePagesTest.kt b/app/src/test/kotlin/io/github/landwarderer/futon/reader/ui/pager/VisiblePagesTest.kt new file mode 100644 index 0000000000..2982360c78 --- /dev/null +++ b/app/src/test/kotlin/io/github/landwarderer/futon/reader/ui/pager/VisiblePagesTest.kt @@ -0,0 +1,88 @@ +package io.github.landwarderer.futon.reader.ui.pager + +import io.github.landwarderer.futon.core.model.TestMangaSource +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +class VisiblePagesTest { + + @Test + fun `positions inside the list are reported unchanged`() { + assertEquals(3..4, visibleAdapterPositions(3, 4, itemCount = 10)) + assertEquals(0..0, visibleAdapterPositions(0, 0, itemCount = 10)) + } + + @Test + fun `NO_POSITION is ignored rather than clamped to the first page`() { + assertNull(visibleAdapterPositions(-1, -1, itemCount = 10)) + assertNull(visibleAdapterPositions(-1, 4, itemCount = 10)) + assertNull(visibleAdapterPositions(3, -1, itemCount = 10)) + } + + @Test + fun `an empty adapter reports nothing`() { + assertNull(visibleAdapterPositions(0, 0, itemCount = 0)) + } + + @Test + fun `a spread ending one past the last page is clamped to the last page`() { + assertEquals(9..9, visibleAdapterPositions(9, 10, itemCount = 10)) + } + + @Test + fun `stale position after a prepend resolves to the page on screen, not the previous chapter`() { + // The adapter still shows chapter 39 while the view model has already prepended chapter 38. + val adapterItems = chapter(39, 20) + val content = chapter(38, 15) + chapter(39, 20) + + val positions = checkNotNull(visibleAdapterPositions(0, 0, adapterItems.size)) + val range = checkNotNull( + content.visibleRangeOf(adapterItems[positions.first], adapterItems[positions.last]), + ) + + val current = content[(range.first + range.last) / 2] + assertEquals(39L, current.chapterId) + assertEquals(0, current.index) + // Resolving the raw position against the new content is the original bug. + assertEquals(38L, content[positions.first].chapterId) + } + + @Test + fun `NO_POSITION after a prepend does not report the previous chapter`() { + val adapterItems = chapter(39, 20) + assertNull(visibleAdapterPositions(-1, -1, adapterItems.size)) + } + + @Test + fun `reversed modes report pages back to front but resolve to an ascending range`() { + val content = chapter(39, 20) + val adapterItems = content.reversed() + + val positions = checkNotNull(visibleAdapterPositions(4, 5, adapterItems.size)) + val range = content.visibleRangeOf(adapterItems[positions.first], adapterItems[positions.last]) + + assertEquals(14..15, range) + } + + @Test + fun `a page that is no longer loaded resolves to nothing`() { + val trimmed = chapter(39, 20) + chapter(40, 20) + val removed = chapter(38, 15).last() + + assertNull(trimmed.visibleRangeOf(removed, removed)) + assertNull(trimmed.visibleRangeOf(removed, trimmed.first())) + } + + @Test + fun `pages are matched by chapter and index, not by object`() { + val content = chapter(39, 20) + val copy = content[7].copy(url = "https://example.org/reloaded") + + assertEquals(7..7, content.visibleRangeOf(copy, copy)) + } + + private fun chapter(id: Long, count: Int) = List(count) { index -> + ReaderPage(id * 1000 + index, "https://example.org/$id/$index", null, id, index, TestMangaSource) + } +}