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..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 @@ -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 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. + * 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" } 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/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() 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) + } +}