From 5c7f9d0d306b66740642cf4111f2a9ee26897ac5 Mon Sep 17 00:00:00 2001 From: Peter Stone Date: Sat, 22 Aug 2026 01:02:31 +0000 Subject: Fix reader rotation bug: replace-nav instead of a swipe-queue snapshot The reader used to snapshot the article-id order of its list ONCE at entry and page a HorizontalPager across that fixed queue. Two real bugs traced back to one root cause: the snapshot lived in plain remember state that did NOT survive an orientation change, while the pager's own page index did -- rotating mid-session left the index pointing into a freshly re-fetched (and by then shorter, since paging past an article marks it read and drops it from the Unread query) list, landing on a different article than the one actually being read. Back was confusing for the same reason: it always returned to wherever the ORIGINAL initialArticleId pointed, not wherever the pager had since scrolled to. Fixed by not having a separate queue/index at all: each Next/Previous now replaces this screen's own back-stack entry with a fresh navigation to the neighboring article's id (MainActivity wires onNavigateToArticle with the same popUpTo-replace pattern LinkReaderScreen already uses for its own link chain). "Which article is showing" is now the nav route argument itself -- Android's own Saved State mechanism, not custom state that can drift out of sync with it -- and the back stack is always exactly [list, one reader entry] no matter how many articles were paged through. --- .../java/org/terst/feedreader/ui/MainActivity.kt | 16 ++- .../java/org/terst/feedreader/ui/ReaderScreen.kt | 113 ++++++++++----------- 2 files changed, 67 insertions(+), 62 deletions(-) (limited to 'app/src') diff --git a/app/src/main/java/org/terst/feedreader/ui/MainActivity.kt b/app/src/main/java/org/terst/feedreader/ui/MainActivity.kt index f7ca043..9add10a 100644 --- a/app/src/main/java/org/terst/feedreader/ui/MainActivity.kt +++ b/app/src/main/java/org/terst/feedreader/ui/MainActivity.kt @@ -89,13 +89,23 @@ fun FeedReaderApp() { val feedId = decodeNavArg(backStackEntry.arguments?.getString("feedId") ?: UNREAD_FEED_ID) val articleId = decodeNavArg(backStackEntry.arguments?.getString("articleId") ?: "") // feedId here is the list the article was opened from (Unread/Saved/a specific - // feed) -- ReaderScreen uses it to build the swipe queue of sibling articles. + // feed) -- ReaderScreen uses it to look up the current article's neighbors. ReaderScreen( feedId = feedId, - initialArticleId = articleId, + articleId = articleId, onBack = { navController.popBackStack() }, onOpenLink = { url -> navController.navigate("linkReader/${encodeNavArg(url)}") }, - onOpenBrowser = { url -> navController.navigate("browser/${encodeNavArg(url)}") } + onOpenBrowser = { url -> navController.navigate("browser/${encodeNavArg(url)}") }, + // Replaces this screen's own back-stack entry rather than stacking a new + // one per article (same popUpTo-replace pattern linkReader's own link-chain + // navigation already uses below) -- so the stack is always exactly + // [list, this one reader entry], and back always returns to the list in a + // single press no matter how many articles were paged through via Next/Previous. + onNavigateToArticle = { nextArticleId -> + navController.navigate("reader/${encodeNavArg(feedId)}/${encodeNavArg(nextArticleId)}") { + popUpTo("reader/{feedId}/{articleId}") { inclusive = true } + } + } ) } composable( diff --git a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt index ae63258..4317073 100644 --- a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt +++ b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt @@ -5,7 +5,6 @@ import android.content.ClipboardManager import android.content.Context import android.content.Intent import android.net.Uri -import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row @@ -14,8 +13,6 @@ import androidx.compose.foundation.layout.fillMaxSize import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.height import androidx.compose.foundation.layout.padding -import androidx.compose.foundation.pager.HorizontalPager -import androidx.compose.foundation.pager.rememberPagerState import androidx.compose.material.icons.Icons import androidx.compose.material.icons.filled.Article import androidx.compose.material.icons.filled.ArrowBack @@ -67,26 +64,47 @@ import org.terst.feedreader.readability.ReaderExtractor import java.net.URLEncoder /** - * Swipeable wrapper: snapshots the article-id order of the list this reader - * was opened from (Unread/Saved/a specific feed) ONCE at entry, then pages - * across that fixed queue. Deliberately a snapshot, not a live Flow -- opening - * an article marks it read, which would otherwise drop it out of the live - * Unread query mid-swipe and shift every subsequent page underneath the user. + * Not a swipeable multi-article queue anymore -- it was, via a HorizontalPager paging + * across a snapshot of sibling article ids taken once at entry, but that design had two + * real bugs traced back to the same root cause: the snapshot lived in plain `remember` + * state that did NOT survive an orientation change, while the pager's own page index DID + * (Compose's PagerState saves that part) -- rotating mid-session left the index pointing + * into a freshly re-fetched (and by then shorter/shifted, since paging past an article + * marks it read and drops it from the Unread query) list, landing on a different article + * than the one actually being read. And because the visually-displayed article and the + * nav route argument backing the whole screen instance could drift apart that way, "back" + * got confusing too -- it was always popping back to wherever the ORIGINAL initialArticleId + * pointed, not wherever the pager had since scrolled to. + * + * Fixed by not having a separate queue/index at all: each Next/Previous replaces this + * screen's own back-stack entry with a fresh navigation to the neighboring article's id + * (MainActivity wires onNavigateToArticle with the same popUpTo-replace pattern + * LinkReaderScreen already uses for its own link chain), so "which article is showing" IS + * the nav route argument -- Android's own Saved State mechanism, not custom state that can + * fall out of sync with it -- and the back stack is always exactly [list, this one reader + * entry] no matter how many articles you've paged through, so back always returns to the + * list in one press, deterministically. */ -@OptIn(ExperimentalFoundationApi::class) @Composable fun ReaderScreen( feedId: String, - initialArticleId: String, + articleId: String, onBack: () -> Unit, onOpenLink: (String) -> Unit, - onOpenBrowser: (String) -> Unit + onOpenBrowser: (String) -> Unit, + onNavigateToArticle: (String) -> Unit ) { val context = LocalContext.current val db = remember { AppDatabase.get(context) } - var articleIds by remember(feedId) { mutableStateOf?>(null) } + val scope = rememberCoroutineScope() - LaunchedEffect(feedId) { + var previousId by remember(articleId) { mutableStateOf(null) } + var nextId by remember(articleId) { mutableStateOf(null) } + + // Looked up fresh every time articleId changes (i.e. every navigation), not cached -- + // always reflects the CURRENT live Unread/Older/Saved/feed ordering, so there's no + // stale-snapshot state to fall out of sync with anything, on rotation or otherwise. + LaunchedEffect(articleId) { val cutoff = System.currentTimeMillis() - RECENT_WINDOW_MS val flow = when (feedId) { UNREAD_FEED_ID -> db.articleDao().observeUnread(cutoff) @@ -94,70 +112,47 @@ fun ReaderScreen( SAVED_FEED_ID -> db.articleDao().observeSaved() else -> db.articleDao().observeByFeed(feedId) } - articleIds = flow.first().map { it.id } - } - - val ids = articleIds - if (ids == null) { - Box(modifier = Modifier.fillMaxSize()) { - CircularProgressIndicator(modifier = Modifier.align(Alignment.Center)) - } - return + val ids = flow.first().map { it.id } + val idx = ids.indexOf(articleId) + previousId = if (idx > 0) ids[idx - 1] else null + nextId = if (idx in 0 until ids.size - 1) ids[idx + 1] else null } - // The article may have been opened from somewhere not in this queue (shouldn't normally - // happen, but fall back to a single-item queue rather than crashing on indexOfFirst = -1). - val safeIds = if (ids.contains(initialArticleId)) ids else listOf(initialArticleId) - val initialIndex = safeIds.indexOf(initialArticleId).coerceAtLeast(0) - val pagerState = rememberPagerState(initialPage = initialIndex) { safeIds.size } - val scope = rememberCoroutineScope() - // Swipe-to-next-article is gone -- two different approaches to keeping it from - // interfering with WebView scrolling (an orientation-aware Compose gesture detector, - // then a passive native touch listener) were tried and neither actually resolved the - // reported jerkiness, and the second one broke swipe outright. Explicit Previous/Next - // buttons in the bottom bar replace it: no ambiguity with scrolling at all, since - // there's no gesture to disambiguate in the first place. - val currentPage = pagerState.currentPage - val hasPrevious = currentPage > 0 - val hasNext = currentPage < safeIds.size - 1 // Marking read happens HERE (moving to another article), not on open -- backing out // to the list without paging past an article leaves it unread. Only Next/Previous // count as "moved away from it"; exiting via the back arrow deliberately doesn't. fun goToPreviousArticle() { - val leavingId = safeIds[pagerState.currentPage] + val target = previousId ?: return scope.launch { - db.articleDao().setRead(leavingId, true) - pagerState.animateScrollToPage((pagerState.currentPage - 1).coerceAtLeast(0)) + db.articleDao().setRead(articleId, true) + onNavigateToArticle(target) } } fun goToNextArticle() { - val leavingId = safeIds[pagerState.currentPage] + val target = nextId ?: return scope.launch { - db.articleDao().setRead(leavingId, true) - pagerState.animateScrollToPage((pagerState.currentPage + 1).coerceAtMost(safeIds.size - 1)) + db.articleDao().setRead(articleId, true) + onNavigateToArticle(target) } } // "I looked, but I'm not done with this one" -- advances the same as Next, just without // the implicit setRead(true) that moving on normally carries. fun goToNextArticleKeepUnread() { - scope.launch { - pagerState.animateScrollToPage((pagerState.currentPage + 1).coerceAtMost(safeIds.size - 1)) - } + val target = nextId ?: return + onNavigateToArticle(target) } - HorizontalPager(state = pagerState, userScrollEnabled = false, modifier = Modifier.fillMaxSize()) { page -> - ReaderContent( - articleId = safeIds[page], - onBack = onBack, - onOpenLink = onOpenLink, - onOpenBrowser = onOpenBrowser, - hasPrevious = hasPrevious, - hasNext = hasNext, - onPrevious = ::goToPreviousArticle, - onNext = ::goToNextArticle, - onNextKeepUnread = ::goToNextArticleKeepUnread - ) - } + ReaderContent( + articleId = articleId, + onBack = onBack, + onOpenLink = onOpenLink, + onOpenBrowser = onOpenBrowser, + hasPrevious = previousId != null, + hasNext = nextId != null, + onPrevious = ::goToPreviousArticle, + onNext = ::goToNextArticle, + onNextKeepUnread = ::goToNextArticleKeepUnread + ) } @OptIn(ExperimentalMaterial3Api::class) -- cgit v1.2.3