From d7c05609692153670dc136082da4e0566d7838a6 Mon Sep 17 00:00:00 2001
From: Peter Stone <thepeterstone@gmail.com>
Date: Wed, 22 Jul 2026 21:07:05 +0000
Subject: Remove reader-mode heuristics entirely; explicit per-feed opt-in only

Reader mode was still switching kottke articles even after the v3
opt-out and the "un-hide only if thin" fallback -- both were still
heuristic (content-length-based), and kottke's posts are routinely short
by design, not paywalled, so the heuristics kept firing on ordinary
content. Removed the auto-extract-on-thin-content logic entirely: reader
mode now NEVER turns on by itself, under any condition.

- FeedEntity.readerModeDisabled -> readerModeEnabled (opt-in, default off
  for every feed -- schema v4, full table rebuild since Room's migration
  validation requires an exact column match and a bare ADD COLUMN left
  the old column orphaned; reproduced live as a crash, "Migration didn't
  properly handle: feeds").
- The reader's toggle icon now sets reader mode for the article's WHOLE
  FEED, persists it immediately, and confirms via snackbar ("Reader mode
  enabled/disabled for X") -- both in the reader screen and the Feeds
  list's own toggle. This is the only way reader mode ever turns on.
- ReaderExtractor: replaced the "un-hide everything if thin" paywall
  fallback (which is what pulled kottke's site footer into the result --
  confirmed live, twice) with one that only un-hides elements whose
  class/id actually names them as paywall/premium/subscriber content
  (West Hawaii Today's hidden div is literally class="paywall
  paywall_<id>"). This turned out not to be the kottke bug's actual cause
  either -- see below -- but it's a real, independent correctness
  improvement over the previous blanket version, so it stays.
- Also added: noscript-wrapped lazy-load image fallback (a common
  pattern pairs a placeholder <img> with a real one inside <noscript>,
  which our JS-enabled extraction WebView never parses into DOM per the
  HTML spec) and lazy data-src/srcset resolution generally.

Root cause of the kottke footer bug, found by fetching and inspecting
the actual page structure rather than guessing again: it's not a
CSS-hidden-element issue at all. kottke's real post content lives in a
plain, undecorated <div class="post"> that's a sibling (not ancestor) of
an ads container and a "Socials & More" footer widget, all under one
shared <div class="grid-container">. Because kottke's posts are so
short, Readability's generic paragraph-density scoring doesn't find
enough signal in .post alone and climbs to a broader container that
happens to include the neighboring footer. No un-hide logic is involved
at any point -- this reproduces identically on the plain, unmodified
Readability pass. This is a genuine limit of heuristic content
extraction on very short-form posts, not something to keep patching with
more heuristics. Reset kottke's setting back to off (its correct
default) after testing.
---
 .../java/org/terst/feedreader/data/AppDatabase.kt  | 45 ++++++++++-
 .../java/org/terst/feedreader/data/Entities.kt     |  4 +-
 .../feedreader/readability/ReaderExtractor.kt      | 76 ++++++++++++++-----
 .../java/org/terst/feedreader/ui/FeedListScreen.kt | 23 ++++--
 .../java/org/terst/feedreader/ui/ReaderScreen.kt   | 87 ++++++++++++----------
 5 files changed, 167 insertions(+), 68 deletions(-)

diff --git a/app/src/main/java/org/terst/feedreader/data/AppDatabase.kt b/app/src/main/java/org/terst/feedreader/data/AppDatabase.kt
index 4cf0ecd..441556b 100644
--- a/app/src/main/java/org/terst/feedreader/data/AppDatabase.kt
+++ b/app/src/main/java/org/terst/feedreader/data/AppDatabase.kt
@@ -9,7 +9,7 @@ import androidx.room.migration.Migration
 
 @Database(
     entities = [FeedEntity::class, ArticleEntity::class, FilterRuleEntity::class],
-    version = 3,
+    version = 4,
     exportSchema = false
 )
 abstract class AppDatabase : RoomDatabase() {
@@ -31,19 +31,58 @@ abstract class AppDatabase : RoomDatabase() {
             }
         }
 
-        // v3: added FeedEntity.readerModeDisabled (per-feed reader-mode opt-out).
+        // v3: added FeedEntity.readerModeDisabled (per-feed reader-mode opt-out, paired with
+        // an auto-extract-on-thin-content heuristic). Superseded by v4 below.
         private val MIGRATION_2_3 = object : Migration(2, 3) {
             override fun migrate(db: SupportSQLiteDatabase) {
                 db.execSQL("ALTER TABLE feeds ADD COLUMN readerModeDisabled INTEGER NOT NULL DEFAULT 0")
             }
         }
 
+        // v4: replaced readerModeDisabled with readerModeEnabled -- reader mode is no longer
+        // ever auto-triggered by a content-length heuristic (that's what kept "auto-switching"
+        // kottke articles even after the v3 opt-out); it's now a plain per-feed opt-in, set
+        // only by explicitly tapping the reader-mode toggle. Starts everyone fresh at off
+        // rather than trying to translate the old opt-out semantics, which don't map cleanly
+        // onto "opt in". A plain ADD COLUMN + leaving the old one in place (as v1->v2 and
+        // v2->v3 do) crashes here: Room's post-migration validation requires the table's
+        // columns to match FeedEntity's fields EXACTLY, and readerModeDisabled isn't a field
+        // anymore -- confirmed live ("Migration didn't properly handle: feeds"). SQLite has no
+        // DROP COLUMN on the versions bundled with older Android, so this rebuilds the table
+        // instead: create with the new shape, copy the columns that still exist, swap in.
+        private val MIGRATION_3_4 = object : Migration(3, 4) {
+            override fun migrate(db: SupportSQLiteDatabase) {
+                db.execSQL(
+                    """
+                    CREATE TABLE feeds_new (
+                        id TEXT NOT NULL PRIMARY KEY,
+                        title TEXT NOT NULL,
+                        siteUrl TEXT,
+                        lastFetchedAt INTEGER,
+                        etag TEXT,
+                        lastModified TEXT,
+                        lastError TEXT,
+                        readerModeEnabled INTEGER NOT NULL DEFAULT 0
+                    )
+                    """.trimIndent()
+                )
+                db.execSQL(
+                    """
+                    INSERT INTO feeds_new (id, title, siteUrl, lastFetchedAt, etag, lastModified, lastError)
+                    SELECT id, title, siteUrl, lastFetchedAt, etag, lastModified, lastError FROM feeds
+                    """.trimIndent()
+                )
+                db.execSQL("DROP TABLE feeds")
+                db.execSQL("ALTER TABLE feeds_new RENAME TO feeds")
+            }
+        }
+
         fun get(context: Context): AppDatabase = instance ?: synchronized(this) {
             instance ?: Room.databaseBuilder(
                 context.applicationContext,
                 AppDatabase::class.java,
                 "feedreader.db"
-            ).addMigrations(MIGRATION_1_2, MIGRATION_2_3).build().also { instance = it }
+            ).addMigrations(MIGRATION_1_2, MIGRATION_2_3, MIGRATION_3_4).build().also { instance = it }
         }
     }
 }
diff --git a/app/src/main/java/org/terst/feedreader/data/Entities.kt b/app/src/main/java/org/terst/feedreader/data/Entities.kt
index f943f78..f4be19e 100644
--- a/app/src/main/java/org/terst/feedreader/data/Entities.kt
+++ b/app/src/main/java/org/terst/feedreader/data/Entities.kt
@@ -13,7 +13,9 @@ data class FeedEntity(
     val etag: String? = null,            // conditional-GET caching (If-None-Match) to avoid re-downloading unchanged feeds
     val lastModified: String? = null,    // conditional-GET caching (If-Modified-Since)
     val lastError: String? = null,       // set when a fetch fails; cleared on the next successful fetch
-    val readerModeDisabled: Boolean = false // per-feed opt-out: never auto/manually switch to Readability for this feed's articles (e.g. a feed whose own formatting is better than what extraction produces)
+    val readerModeEnabled: Boolean = false // per-feed opt-IN, set only by explicitly tapping the reader-mode
+    // toggle (never automatic/heuristic) -- once on, articles from this feed default to the
+    // extracted view and auto-extract on open instead of requiring a per-article tap each time.
 )
 
 @Entity(tableName = "articles")
diff --git a/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt b/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt
index 82300ac..3eaa158 100644
--- a/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt
+++ b/app/src/main/java/org/terst/feedreader/readability/ReaderExtractor.kt
@@ -74,6 +74,37 @@ object ReaderExtractor {
                                     return (article && article.textContent) ? article.textContent.trim().length : 0;
                                 }
 
+                                // A common lazy-load pattern pairs a placeholder <img> with a
+                                // sibling <noscript> holding the REAL <img> tag, meant for
+                                // browsers with JS off. But our WebView runs WITH JS enabled
+                                // (Readability.js itself needs it) -- and per the HTML spec,
+                                // when scripting IS enabled, a <noscript> element's contents are
+                                // parsed as opaque text, not as DOM, so document.querySelectorAll
+                                // never even sees that real <img>. Pull it out manually: parse
+                                // each <noscript>'s raw text as markup and promote any <img>
+                                // inside to a real, visible sibling (replacing the placeholder if
+                                // one immediately precedes it).
+                                var noscripts = document.querySelectorAll('noscript');
+                                for (var i = 0; i < noscripts.length; i++) {
+                                    var ns = noscripts[i];
+                                    var raw = ns.textContent || '';
+                                    if (raw.indexOf('<img') === -1) continue;
+                                    var temp = document.createElement('div');
+                                    temp.innerHTML = raw;
+                                    var realImg = temp.querySelector('img');
+                                    if (!realImg) continue;
+                                    var prev = ns.previousElementSibling;
+                                    if (prev && prev.tagName === 'IMG') {
+                                        prev.setAttribute('src', realImg.getAttribute('src') || '');
+                                        if (realImg.getAttribute('srcset')) prev.setAttribute('srcset', realImg.getAttribute('srcset'));
+                                        prev.removeAttribute('data-src');
+                                        prev.removeAttribute('data-original');
+                                        prev.removeAttribute('data-lazy-src');
+                                    } else {
+                                        ns.parentNode.insertBefore(realImg, ns);
+                                    }
+                                }
+
                                 // Many sites lazy-load media via JS: the real URL sits in a
                                 // data-src/data-srcset attribute and only gets copied into
                                 // src/srcset once the site's own script runs -- which never
@@ -117,31 +148,40 @@ object ReaderExtractor {
                                 // majority of sites -- it's what keeps collapsed nav/footer/sidebar
                                 // chrome out of the result. Try it first, unmodified.
                                 var normalArticle = new Readability(document.cloneNode(true)).parse();
-                                if (plainLength(normalArticle) >= 500) {
-                                    return JSON.stringify(normalArticle);
-                                }
 
-                                // Only reached when the normal pass came back thin. Some "soft"
-                                // paywalls deliver the full article in the initial HTML response to
-                                // every visitor, then hide it with CSS (inline or via a stylesheet
-                                // rule) pending a client-side subscription check -- the content isn't
-                                // withheld by the server, only its visibility is, and Readability's
-                                // hidden-element skip discards it the same as any other hidden chrome.
-                                // Un-hide and retry; keep whichever pass actually found more text,
-                                // since un-hiding everything indiscriminately can just as easily pull
-                                // in an unrelated hidden footer/notice on a normal (non-paywalled) page.
-                                var all = document.querySelectorAll('*');
-                                for (var i = 0; i < all.length; i++) {
-                                    var el = all[i];
+                                // "Thin normal-pass result" is NOT a reliable paywall signal on its
+                                // own -- plenty of ordinary, non-paywalled posts (kottke's linkblog
+                                // style, for one) are also genuinely just short, and un-hiding
+                                // EVERYTHING for those pulled in an unrelated hidden footer/social-
+                                // links block instead of any real article content (reproduced live:
+                                // the same kottke article kept coming back wrapped in site-footer
+                                // boilerplate even with reader mode explicitly enabled for the feed).
+                                // Target the actual, specific signal instead: elements whose class/id
+                                // names the page itself labels as paywall/premium/subscriber content.
+                                // West Hawaii Today's hidden div is literally class="paywall
+                                // paywall_<id>" -- this only ever fires for genuine paywall markup,
+                                // never for a plain short post.
+                                var paywallEls = document.querySelectorAll(
+                                    '[class*="paywall" i], [id*="paywall" i], [class*="premium-content" i], ' +
+                                    '[class*="subscriber-content" i], [class*="metered-content" i]'
+                                );
+                                var unhidAny = false;
+                                for (var i = 0; i < paywallEls.length; i++) {
+                                    var el = paywallEls[i];
                                     var cs = window.getComputedStyle(el);
                                     if (cs.display === 'none' || cs.visibility === 'hidden') {
                                         el.style.setProperty('display', 'block', 'important');
                                         el.style.setProperty('visibility', 'visible', 'important');
+                                        unhidAny = true;
+                                    }
+                                }
+                                if (unhidAny) {
+                                    var unhiddenArticle = new Readability(document.cloneNode(true)).parse();
+                                    if (plainLength(unhiddenArticle) > plainLength(normalArticle)) {
+                                        return JSON.stringify(unhiddenArticle);
                                     }
                                 }
-                                var unhiddenArticle = new Readability(document.cloneNode(true)).parse();
-                                var best = plainLength(unhiddenArticle) > plainLength(normalArticle) ? unhiddenArticle : normalArticle;
-                                return JSON.stringify(best);
+                                return JSON.stringify(normalArticle);
                             } catch (e) {
                                 return JSON.stringify({error: e.toString()});
                             }
diff --git a/app/src/main/java/org/terst/feedreader/ui/FeedListScreen.kt b/app/src/main/java/org/terst/feedreader/ui/FeedListScreen.kt
index f8ea504..47f51fc 100644
--- a/app/src/main/java/org/terst/feedreader/ui/FeedListScreen.kt
+++ b/app/src/main/java/org/terst/feedreader/ui/FeedListScreen.kt
@@ -27,6 +27,8 @@ import androidx.compose.material3.Icon
 import androidx.compose.material3.IconButton
 import androidx.compose.material3.ListItem
 import androidx.compose.material3.Scaffold
+import androidx.compose.material3.SnackbarHost
+import androidx.compose.material3.SnackbarHostState
 import androidx.compose.material3.Text
 import androidx.compose.material3.TextButton
 import androidx.compose.material3.TopAppBar
@@ -73,6 +75,7 @@ fun FeedListScreen(
     val olderUnreadCount by remember { db.articleDao().observeOlderUnreadCount(cutoff) }.collectAsState(initial = 0)
     var menuExpanded by remember { mutableStateOf(false) }
     var pendingDelete by remember { mutableStateOf<FeedEntity?>(null) }
+    val snackbarHostState = remember { SnackbarHostState() }
 
     pendingDelete?.let { feed ->
         AlertDialog(
@@ -121,7 +124,8 @@ fun FeedListScreen(
             FloatingActionButton(onClick = onAddFeed) {
                 Icon(Icons.Default.Add, contentDescription = "Add feed")
             }
-        }
+        },
+        snackbarHost = { SnackbarHost(snackbarHostState) }
     ) { padding ->
         LazyColumn(modifier = Modifier.fillMaxSize().padding(padding)) {
             item {
@@ -152,7 +156,14 @@ fun FeedListScreen(
                     onClick = { onOpenFeed(feed.id) },
                     onDelete = { pendingDelete = feed },
                     onToggleReaderMode = {
-                        scope.launch { db.feedDao().update(feed.copy(readerModeDisabled = !feed.readerModeDisabled)) }
+                        val newEnabled = !feed.readerModeEnabled
+                        scope.launch {
+                            db.feedDao().update(feed.copy(readerModeEnabled = newEnabled))
+                            val label = feed.title.ifBlank { "this feed" }
+                            snackbarHostState.showSnackbar(
+                                if (newEnabled) "Reader mode enabled for $label" else "Reader mode disabled for $label"
+                            )
+                        }
                     }
                 )
                 HorizontalDivider()
@@ -184,11 +195,11 @@ private fun FeedRow(
                 }
                 IconButton(onClick = onToggleReaderMode) {
                     Icon(
-                        if (feed.readerModeDisabled) Icons.Outlined.Article else Icons.Filled.Article,
-                        contentDescription = if (feed.readerModeDisabled)
-                            "Reader mode disabled for this feed -- tap to re-enable"
-                        else
+                        if (feed.readerModeEnabled) Icons.Filled.Article else Icons.Outlined.Article,
+                        contentDescription = if (feed.readerModeEnabled)
                             "Reader mode enabled for this feed -- tap to disable"
+                        else
+                            "Reader mode disabled for this feed -- tap to enable"
                     )
                 }
                 IconButton(onClick = onDelete) {
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 4becea6..806926c 100644
--- a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt
+++ b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt
@@ -59,10 +59,6 @@ import org.terst.feedreader.stash.StashClient
 import org.terst.feedreader.stash.StashSaveResult
 import java.net.URLEncoder
 
-// Below this length (of the plain feed-provided HTML), assume it's a teaser/summary,
-// not the real article body, and auto-fall-back to on-device Readability extraction.
-private const val THIN_CONTENT_THRESHOLD = 500
-
 /**
  * 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
@@ -191,44 +187,53 @@ private fun ReaderContent(
 
         feedContent = loaded.feedContentHtml
         readerContent = loaded.extractedContent
-        val plainLength = feedContent?.replace(Regex("<[^>]*>"), "")?.trim()?.length ?: 0
 
-        if (loadedFeed?.readerModeDisabled != true && readerContent == null && plainLength < THIN_CONTENT_THRESHOLD) {
-            // Feed content is too thin to be the real article -- auto-extract, since in this
-            // case reader mode isn't really an alternative view, it's the only real content.
-            isExtracting = true
-            val extracted = ReaderExtractor.extract(context, loaded.link)
-            isExtracting = false
-            extracted?.content?.let {
-                readerContent = it
-                db.articleDao().setExtractedContent(articleId, it)
-            }
+        // Reader mode is NEVER heuristic/automatic (no more "content looks thin, switch
+        // for you") -- it strictly follows the feed's persisted, explicitly-set opt-in.
+        // Once a feed has it on, articles from it default to the extracted view and
+        // auto-extract on open (that's the point of persisting it per-feed: no re-tapping
+        // per article), but nothing here ever turns it on by itself.
+        if (loadedFeed?.readerModeEnabled == true) {
             isReaderMode = true
+            if (readerContent == null) {
+                isExtracting = true
+                val extracted = ReaderExtractor.extract(context, loaded.link)
+                isExtracting = false
+                extracted?.content?.let {
+                    readerContent = it
+                    db.articleDao().setExtractedContent(articleId, it)
+                }
+            }
         }
     }
 
-    fun toggleReaderMode() {
+    // The toggle now sets reader mode for the ARTICLE'S FEED, persisted immediately, with
+    // a confirmation snackbar -- not a per-article, this-session-only view flip. Tapping it
+    // is the one and only way reader mode ever turns on.
+    fun toggleFeedReaderMode() {
+        val f = feed ?: return
         val a = article ?: return
-        if (feed?.readerModeDisabled == true) return
-        if (isReaderMode) {
-            isReaderMode = false
-            return
-        }
-        if (readerContent != null) {
-            isReaderMode = true
-            return
-        }
+        val newEnabled = !f.readerModeEnabled
+        feed = f.copy(readerModeEnabled = newEnabled)
         scope.launch {
-            isExtracting = true
-            val extracted = ReaderExtractor.extract(context, a.link)
-            isExtracting = false
-            val content = extracted?.content
-            if (content != null) {
-                readerContent = content
-                db.articleDao().setExtractedContent(articleId, content)
+            db.feedDao().update(f.copy(readerModeEnabled = newEnabled))
+            val feedLabel = f.title.ifBlank { "this feed" }
+            if (newEnabled) {
+                if (readerContent == null) {
+                    isExtracting = true
+                    val extracted = ReaderExtractor.extract(context, a.link)
+                    isExtracting = false
+                    val content = extracted?.content
+                    if (content != null) {
+                        readerContent = content
+                        db.articleDao().setExtractedContent(articleId, content)
+                    }
+                }
                 isReaderMode = true
+                snackbarHostState.showSnackbar("Reader mode enabled for $feedLabel")
             } else {
-                snackbarHostState.showSnackbar("Couldn't extract a reader view for this article")
+                isReaderMode = false
+                snackbarHostState.showSnackbar("Reader mode disabled for $feedLabel")
             }
         }
     }
@@ -257,14 +262,16 @@ private fun ReaderContent(
                 },
                 actions = {
                     val currentArticle = article
-                    // Only offer the toggle when there's an actual feed-provided alternative to
-                    // switch back to (if we already had to auto-extract for thin feed content,
-                    // reader mode IS the only content) and the feed hasn't opted out of it.
-                    if (feedContent != null && feed?.readerModeDisabled != true) {
-                        IconButton(enabled = !isExtracting, onClick = { toggleReaderMode() }) {
+                    // Sets reader mode for the whole feed (persisted), not just this article --
+                    // shown whenever the article belongs to an actual subscribed feed.
+                    if (feed != null) {
+                        IconButton(enabled = !isExtracting, onClick = { toggleFeedReaderMode() }) {
                             Icon(
-                                if (isReaderMode) Icons.Filled.Article else Icons.Outlined.Article,
-                                contentDescription = "Toggle reader view"
+                                if (feed?.readerModeEnabled == true) Icons.Filled.Article else Icons.Outlined.Article,
+                                contentDescription = if (feed?.readerModeEnabled == true)
+                                    "Reader mode enabled for this feed -- tap to disable"
+                                else
+                                    "Reader mode disabled for this feed -- tap to enable"
                             )
                         }
                     }
-- 
cgit v1.2.3