Fix: reader saving the previous chapter’s first page as reading progress

app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/ReaderViewModel.kt

@@ -47,7 +47,9 @@ 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.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
@@ -346,16 +348,25 @@ class ReaderViewModel @Inject constructor(
         }
     }
 
+    /**
+     * 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
-    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 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

app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/BasePagerReaderFragment.kt

@@ -183,7 +183,7 @@ abstract class BasePagerReaderFragment : BaseReaderFragment<FragmentReaderPagerB
 	}
 
 	protected open fun notifyPageChanged(page: Int) {
-		viewModel.onCurrentPageChanged(page, page)
+		notifyVisiblePagesChanged(page, page)
 	}
 
 	companion object {

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,6 +62,20 @@ abstract class BaseReaderFragment<B : ViewBinding> : BaseFragment<B>(), ZoomCont
 		super.onDestroyView()
 	}
 
+	/**
+	 * 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) {
+		val adapter = readerAdapter ?: return
+		val positions = visibleAdapterPositions(lowerPos, upperPos, adapter.itemCount) ?: return
+		viewModel.onCurrentPageChanged(
+			adapter.getItemOrNull(positions.first),
+			adapter.getItemOrNull(positions.last),
+		)
+	}
+
 	protected fun requireAdapter() = checkNotNull(readerAdapter) {
 		"Adapter was not created or already destroyed"
 	}

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<ReaderPage>.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<ReaderPage>.indexOfPage(page: ReaderPage): Int = indexOfFirst {
+	it.chapterId == page.chapterId && it.index == page.index
+}

app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/doublepage/DoubleReaderFragment.kt

@@ -136,7 +136,7 @@ open class DoubleReaderFragment : BaseReaderFragment<FragmentReaderDoubleBinding
 	}
 
 	protected open fun notifyPageChanged(lowerPos: Int, upperPos: Int) {
-		viewModel.onCurrentPageChanged(lowerPos, upperPos)
+		notifyVisiblePagesChanged(lowerPos, upperPos)
 	}
 
 	private fun getCurrentItem() = (requireViewBinding().recyclerView.layoutManager as LinearLayoutManager)

app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/doublereversed/ReversedDoubleReaderFragment.kt

@@ -18,10 +18,6 @@ class ReversedDoubleReaderFragment : DoubleReaderFragment() {
 		super.onPagesChanged(pages.reversed(), pendingState)
 	}
 
-	override fun notifyPageChanged(lowerPos: Int, upperPos: Int) {
-		viewModel.onCurrentPageChanged(reversed(upperPos), reversed(lowerPos))
-	}
-
 	private fun reversed(position: Int): Int {
 		return ((readerAdapter?.itemCount ?: 0) - position - 1).coerceAtLeast(0)
 	}

app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/reversed/ReversedReaderFragment.kt

@@ -41,11 +41,6 @@ class ReversedReaderFragment : BasePagerReaderFragment() {
 		super.onPagesChanged(pages.reversed(), pendingState)
 	}
 
-	override fun notifyPageChanged(page: Int) {
-		val pos = reversed(page)
-		viewModel.onCurrentPageChanged(pos, pos)
-	}
-
 	private fun reversed(position: Int): Int {
 		return ((readerAdapter?.itemCount ?: 0) - position - 1).coerceAtLeast(0)
 	}

app/src/main/kotlin/io/github/landwarderer/futon/reader/ui/pager/webtoon/WebtoonReaderFragment.kt

@@ -126,7 +126,7 @@ class WebtoonReaderFragment : BaseReaderFragment<FragmentReaderWebtoonBinding>()
 		firstVisiblePosition: Int,
 		lastVisiblePosition: Int,
 	) {
-		viewModel.onCurrentPageChanged(firstVisiblePosition, lastVisiblePosition)
+		notifyVisiblePagesChanged(firstVisiblePosition, lastVisiblePosition)
 	}
 
 	override suspend fun onPagesChanged(pages: List<ReaderPage>, pendingState: ReaderState?) = coroutineScope {
@@ -150,7 +150,7 @@ class WebtoonReaderFragment : BaseReaderFragment<FragmentReaderWebtoonBinding>()
 							?.restoreScroll(pendingState.scroll)
 					}
 				}
-				viewModel.onCurrentPageChanged(position, position)
+				notifyVisiblePagesChanged(position, position)
 			} else {
 				Snackbar.make(requireView(), R.string.not_found_404, Snackbar.LENGTH_SHORT)
 					.show()

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)
+    }
+}