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.

1 files changed, 54 insertions, 59 deletions

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<List<String>?>(null) }
+    val scope = rememberCoroutineScope()
 
-    LaunchedEffect(feedId) {
+    var previousId by remember(articleId) { mutableStateOf<String?>(null) }
+    var nextId by remember(articleId) { mutableStateOf<String?>(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)