diff options
Diffstat (limited to 'app')
15 files changed, 853 insertions, 18 deletions
diff --git a/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt b/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt index 3a791cc..63614d1 100644 --- a/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt +++ b/app/src/main/java/org/terst/feedreader/data/CachedExtraction.kt @@ -1,6 +1,7 @@ package org.terst.feedreader.data import org.terst.feedreader.readability.ConsentNotice +import org.terst.feedreader.readability.RecirculationWidgets import org.terst.feedreader.readability.UnrenderedTex /** What a cached `articles.extractedContent` value turned out to be worth. */ @@ -58,7 +59,14 @@ suspend fun loadCachedExtraction( discardedUnrenderedTex = true ) else -> null - } ?: return CachedExtraction(content = cached, discardedConsentNotice = false) + } ?: return CachedExtraction( + // Recirculation widgets are deliberately not in the `discard` list above. A cached + // article carrying the publisher's promo blocks is a good article with litter in it, + // not a poisoned row: cleaning it in place shows the right thing immediately, where + // discarding it would throw away a usable body and fetch the identical one back. + content = RecirculationWidgets.strip(cached), + discardedConsentNotice = false + ) clearCache() return discard diff --git a/app/src/main/java/org/terst/feedreader/data/FeedDiscovery.kt b/app/src/main/java/org/terst/feedreader/data/FeedDiscovery.kt new file mode 100644 index 0000000..98ce5b3 --- /dev/null +++ b/app/src/main/java/org/terst/feedreader/data/FeedDiscovery.kt @@ -0,0 +1,88 @@ +package org.terst.feedreader.data + +import java.net.URI + +/** + * Finds the feed a web page advertises. + * + * Reported live: "Can't subscribe to https://heathercoxrichardson.substack.com/". Adding a feed + * fetched whatever URL it was given and handed it straight to [FeedParser], so a site's home + * page -- the only URL a reader can actually see and copy -- failed on the XML parse with a + * message about an unrecognised root element. + * + * Nothing exotic is needed to fix it. Sites have advertised their feeds in the document head + * since RSS autodiscovery was agreed in 2002, and that Substack page does: + * + * ``` + * <link rel="alternate" type="application/rss+xml" href="/feed" title="Letters from an American"/> + * ``` + * + * Only what the page itself declares is used. Guessing paths (`/feed`, `/rss`, `/index.xml`) + * would add a burst of 404s on every failed add and would eventually subscribe somebody to the + * wrong thing; a site that declares nothing is reported as having no feed, which is the truth. + */ +object FeedDiscovery { + + /** Feed content types, as `type=` gives them. */ + private val FEED_TYPES = listOf("application/rss+xml", "application/atom+xml", "application/rdf+xml", "text/xml") + + private val LINK_TAG = Regex("<link\\b[^>]*>", RegexOption.IGNORE_CASE) + + /** `name="value"`, `name='value'` or `name=value`, in any order within the tag. */ + private val ATTRIBUTE = + Regex("""(?<name>[a-zA-Z-]+)\s*=\s*("(?<dq>[^"]*)"|'(?<sq>[^']*)'|(?<bare>[^\s">]+))""") + + /** + * The feed URL [html] advertises, absolute, or null when it advertises none. + * + * [pageUrl] is the address the HTML came from, which a relative href resolves against -- + * against the page, not the site root, since a feed link on `/blog/` routinely says + * `feed.xml` and means `/blog/feed.xml`. + */ + fun findFeedUrl(html: String, pageUrl: String): String? { + // Already a feed: there is nothing to discover, and scanning XML for <link> tags would + // only find Atom's own self-links and send the caller round in a circle. + if (html.trimStart().startsWith("<?xml", ignoreCase = true)) return null + + return LINK_TAG.findAll(html) + .map { attributesOf(it.value) } + .firstOrNull { attrs -> + attrs["rel"]?.contains("alternate", ignoreCase = true) == true && + attrs["type"]?.lowercase()?.trim() in FEED_TYPES + } + ?.get("href") + ?.takeIf { it.isNotBlank() } + ?.let { resolve(it, pageUrl) } + } + + private fun attributesOf(tag: String): Map<String, String> = + ATTRIBUTE.findAll(tag).associate { match -> + // Named rather than numbered: which of the three quoting alternatives matched is + // the whole question here, and "groupValues[4]" does not say that to a reader. + val value = match.groups["dq"]?.value + ?: match.groups["sq"]?.value + ?: match.groups["bare"]?.value.orEmpty() + match.groups["name"]!!.value.lowercase() to value + } + + /** + * [href] as an absolute https URL. + * + * The https upgrade is the same rule the add-feed screen already applies to what the user + * types, for the same reason: Android blocks cleartext, so an http feed would fail every + * fetch forever. A site that advertises an http feed almost always serves it over https too. + */ + private fun resolve(href: String, pageUrl: String): String? = try { + val absolute = URI(pageUrl).resolve(href.trim()).toString() + if (absolute.startsWith("http://", ignoreCase = true)) { + "https://" + absolute.removePrefix("http://").removePrefix("HTTP://") + } else { + absolute + } + } catch (ignored: IllegalArgumentException) { + // A malformed href is a page problem, not a crash: report no feed and let the caller + // say so, the same as a page that advertises none. Nothing is lost by not reporting + // it -- "no feed found at that address" is exactly as true and as useful either way. + null + } +} diff --git a/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt b/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt index ed0033d..dd77014 100644 --- a/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt +++ b/app/src/main/java/org/terst/feedreader/readability/EvalDecision.kt @@ -39,7 +39,14 @@ internal fun decideEvalOutcome( !isConsentNotice return when { - usable -> EvalDecision.Done(ExtractionResult.Success(article!!)) + // Cleaned on the way out rather than at the point of display: this is the one place + // every fresh extraction passes through, so the widgets never reach the screen and + // never reach `articles.extractedContent` either. + usable -> EvalDecision.Done( + ExtractionResult.Success( + article!!.copy(content = RecirculationWidgets.strip(article.content)) + ) + ) // Readability ran and objected: that verdict will not change. readabilityError != null -> diff --git a/app/src/main/java/org/terst/feedreader/readability/RecirculationWidgets.kt b/app/src/main/java/org/terst/feedreader/readability/RecirculationWidgets.kt new file mode 100644 index 0000000..a6515c1 --- /dev/null +++ b/app/src/main/java/org/terst/feedreader/readability/RecirculationWidgets.kt @@ -0,0 +1,128 @@ +package org.terst.feedreader.readability + +/** + * Removes "recirculation" widgets -- the promo blocks a publisher's widget platform injects + * into the middle of the article body to send the reader off to other stories. + * + * Reported live: West Hawaii Today articles "get a bunch of stray 'Trending' headers". + * Confirmed by running the vendored Readability.js over the rendered page for the 12 articles + * in that feed: 9 carried two surviving widgets each. The three that didn't were paywalled + * stubs too short for the widgets to be reached. + * + * Readability keeps them because they are not in a sidebar -- they are inside the article's own + * container, between real paragraphs, so every structural signal says they belong. What arrives + * is a heading labelling nothing, because the list of links under it is filled in later by a + * script that never runs in the reader: + * + * ``` + * <div data-mrf-recirculation="IL_..." data-testid="text-html"><p><h3><span>Trending</span></h3></p></div> + * ``` + * + * The signal is the attribute, not the word "Trending": `data-mrf-recirculation` says what the + * block is for, in the widget platform's own vocabulary, and no article body is ever inside one. + * Matching on the heading text instead would be both narrower (the label varies by site and + * section) and more dangerous (an article may legitimately discuss what is trending). + * + * [MAX_WIDGET_TEXT_LENGTH] is the same safety valve [ConsentNotice] uses: however the attribute + * is applied, nothing with real writing in it is ever thrown away. + */ +object RecirculationWidgets { + + /** + * Text length past which a block is kept whatever attribute it carries. A widget is a label + * and a few headlines; anything longer is somebody's writing and is not ours to delete. + */ + private const val MAX_WIDGET_TEXT_LENGTH = 600 + + /** + * Attributes marking a block as recirculation. Only ones whose own name says so -- this list + * grows on evidence from a real article, never on a guess about what a vendor prefix means. + */ + private val WIDGET_ATTRIBUTES = listOf("data-mrf-recirculation") + + private val OPENING_DIV = Regex("<div\\b[^>]*>", RegexOption.IGNORE_CASE) + private val TAG_REGEX = Regex("<[^>]*>") + private val WHITESPACE_REGEX = Regex("\\s+") + + private const val OPEN_DIV = "<div" + private const val CLOSE_DIV = "</div" + + /** [html] with any recirculation widget removed. */ + fun strip(html: String?): String? = + if (html.isNullOrBlank() || WIDGET_ATTRIBUTES.none { html.contains(it, ignoreCase = true) }) { + html + } else { + removeWidgets(html) + } + + private fun removeWidgets(html: String): String { + val out = StringBuilder(html.length) + var cursor = 0 + var open = OPENING_DIV.find(html, cursor) + while (open != null) { + val end = widgetEnd(html, open) + if (end == null) { + // Not a widget, or not safe to remove. Copy up to and including this opening + // tag: an ordinary div that merely sits before a widget must not go with it. + out.append(html, cursor, open.range.last + 1) + cursor = open.range.last + 1 + } else { + out.append(html, cursor, open.range.first) + cursor = end + } + open = OPENING_DIV.find(html, cursor) + } + out.append(html, cursor, html.length) + return out.toString() + } + + /** + * Where the block opened at [open] ends, or null when it should be left exactly as it is -- + * because it is not a widget, because the markup never closes it, or because it holds more + * text than a widget ever does and is therefore somebody's writing. + */ + private fun widgetEnd(html: String, open: MatchResult): Int? = + if (!marksRecirculation(open.value)) { + null + } else { + matchingClose(html, open.range.last + 1) + ?.takeIf { !tooLongToBeAWidget(html.substring(open.range.first, it)) } + } + + private fun marksRecirculation(openingTag: String) = + WIDGET_ATTRIBUTES.any { openingTag.contains(it, ignoreCase = true) } + + /** + * The index just past the `</div>` closing the div that opened before [from], or null when + * the markup never closes it. + */ + private fun matchingClose(html: String, from: Int): Int? { + var depth = 1 + var i = from + var end: Int? = null + while (end == null && i < html.length) { + val next = html.indexOf('<', i) + when { + next < 0 -> i = html.length + html.startsWith(CLOSE_DIV, next, ignoreCase = true) -> { + val close = html.indexOf('>', next) + depth-- + when { + close < 0 -> i = html.length + depth == 0 -> end = close + 1 + else -> i = close + 1 + } + } + html.startsWith(OPEN_DIV, next, ignoreCase = true) -> { + depth++ + i = next + OPEN_DIV.length + } + else -> i = next + 1 + } + } + return end + } + + private fun tooLongToBeAWidget(block: String): Boolean = + block.replace(TAG_REGEX, " ").replace(WHITESPACE_REGEX, " ").trim().length > MAX_WIDGET_TEXT_LENGTH +} diff --git a/app/src/main/java/org/terst/feedreader/ui/AddFeedScreen.kt b/app/src/main/java/org/terst/feedreader/ui/AddFeedScreen.kt index 1e8814c..9f817ab 100644 --- a/app/src/main/java/org/terst/feedreader/ui/AddFeedScreen.kt +++ b/app/src/main/java/org/terst/feedreader/ui/AddFeedScreen.kt @@ -33,11 +33,13 @@ import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import okhttp3.Request import org.terst.feedreader.data.AppDatabase +import org.terst.feedreader.data.FeedDiscovery import org.terst.feedreader.data.FeedEntity import org.terst.feedreader.data.FeedParser import org.terst.feedreader.data.Net import org.terst.feedreader.opml.Opml import org.terst.feedreader.work.FeedFetchWorker +import java.io.ByteArrayInputStream @OptIn(ExperimentalMaterial3Api::class) @Composable @@ -168,25 +170,60 @@ fun AddFeedScreen(onDone: () -> Unit) { private fun preferHttps(url: String): String = if (url.startsWith("http://", ignoreCase = true)) "https://" + url.removePrefix("http://") else url +/** + * Fetches [url] and returns its bytes, or null when the server refused. + * + * Read into memory rather than parsed from the live stream because the bytes may need looking + * at twice: once as a feed, and -- when that is not what the URL turned out to be -- again as a + * web page advertising where its feed actually lives. A response body is a one-shot stream, so + * the second look is only possible if the first did not consume it. + */ +private class Fetched(val bytes: ByteArray?, val error: String?) + +private fun fetchBytes(url: String): Fetched = + Net.client.newCall(Request.Builder().url(url).build()).execute().use { response -> + when { + !response.isSuccessful -> Fetched(null, "Couldn't fetch feed: HTTP ${response.code}") + else -> response.body?.let { Fetched(it.bytes(), null) } ?: Fetched(null, "Empty response") + } + } + private suspend fun addFeedByUrl(context: android.content.Context, db: AppDatabase, feedUrl: String): String = withContext(Dispatchers.IO) { - val url = preferHttps(feedUrl) + val typedUrl = preferHttps(feedUrl) try { - val response = Net.client.newCall(Request.Builder().url(url).build()).execute() - response.use { - if (!it.isSuccessful) return@withContext "Couldn't fetch feed: HTTP ${it.code}" - val body = it.body ?: return@withContext "Empty response" - val parsed = FeedParser.parse(body.byteStream()) - // Same REPLACE-wipes-other-fields concern as the OPML import path. - val existing = db.feedDao().getById(url) - val entity = existing?.copy( - title = parsed.title.ifBlank { url }, - siteUrl = parsed.siteUrl ?: existing.siteUrl - ) ?: FeedEntity(id = url, title = parsed.title.ifBlank { url }, siteUrl = parsed.siteUrl) - db.feedDao().upsert(entity) + val fetched = fetchBytes(typedUrl) + val bytes = fetched.bytes ?: return@withContext fetched.error ?: "Empty response" + + // What the user had to hand is usually the site, not the feed -- a site advertises + // its feed in its head, it does not print the URL anywhere a reader can copy. So a + // failed parse is not the answer yet: look for what the page says its feed is and + // try that, once. See FeedDiscovery. + var url = typedUrl + var parsed = runCatching { FeedParser.parse(ByteArrayInputStream(bytes)) }.getOrNull() + if (parsed == null) { + val discovered = FeedDiscovery.findFeedUrl(String(bytes, Charsets.UTF_8), typedUrl) + ?: return@withContext "No feed found at that address" + val feed = fetchBytes(discovered) + val feedBytes = feed.bytes ?: return@withContext feed.error ?: "Empty response" + // One hop only: whatever the page pointed at has to be a feed. Following a + // second page would be a redirect chain nobody asked for. + parsed = FeedParser.parse(ByteArrayInputStream(feedBytes)) + url = discovered } + + // Same REPLACE-wipes-other-fields concern as the OPML import path. + val existing = db.feedDao().getById(url) + val entity = existing?.copy( + title = parsed.title.ifBlank { url }, + siteUrl = parsed.siteUrl ?: existing.siteUrl + ) ?: FeedEntity(id = url, title = parsed.title.ifBlank { url }, siteUrl = parsed.siteUrl) + db.feedDao().upsert(entity) + FeedFetchWorker.fetchNow(context) - "Added" + // Says which feed was added when it is not the URL that was typed, so a site URL + // quietly becoming a different feed URL is visible rather than surprising later. + if (url == typedUrl) "Added" else "Added ${parsed.title.ifBlank { url }}" } catch (e: Exception) { "Couldn't add feed: ${e.message}" } diff --git a/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt b/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt index 72f6c1f..9f9f74a 100644 --- a/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt +++ b/app/src/main/java/org/terst/feedreader/ui/LinkReaderScreen.kt @@ -258,6 +258,7 @@ fun LinkReaderScreen(url: String, onBack: () -> Unit, onOpenLink: (String) -> Un ReaderWebViewCallbacks( onOpenLink = onOpenLink, onLongPressLink = { longPressedLink = it }, + onOpenYouTubeExternally = { context.startActivity(Intent(Intent.ACTION_VIEW, Uri.parse(it))) }, onYouTubeVideo = { youtubeVideoId = it }, onImageTap = { zoomedImageUrl = it }, currentFontSizePx = { latestFontSizePx }, 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 9952dc5..0d326ea 100644 --- a/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt +++ b/app/src/main/java/org/terst/feedreader/ui/ReaderScreen.kt @@ -599,6 +599,7 @@ private fun ReaderContent( ReaderWebViewCallbacks( onOpenLink = onOpenLink, onLongPressLink = { longPressedLink = it }, + onOpenYouTubeExternally = { context.startActivity(Intent(Intent.ACTION_VIEW, Uri.parse(it))) }, onYouTubeVideo = { youtubeVideoId = it }, onImageTap = { zoomedImageUrl = it }, onScrollChanged = { scrollY -> ReaderScrollPositions.set(articleId, scrollY) }, diff --git a/app/src/main/java/org/terst/feedreader/ui/ReaderWebView.kt b/app/src/main/java/org/terst/feedreader/ui/ReaderWebView.kt index f1400d3..78020a9 100644 --- a/app/src/main/java/org/terst/feedreader/ui/ReaderWebView.kt +++ b/app/src/main/java/org/terst/feedreader/ui/ReaderWebView.kt @@ -74,6 +74,11 @@ private fun extractYouTubeVideoId(uri: Uri): String? { * WebView's own default long-press behavior (a native "open/copy link" popup) -- * callers show their own save/share/open action menu instead. * + * onOpenYouTubeExternally fires for a LONG-press on a YouTube link, with the watch URL, instead + * of the ordinary onLongPressLink actions menu -- callers hand it to the system, which is the + * YouTube app on a phone that has one. A tap plays the video in the reader; a long-press is how + * you say you wanted it somewhere else. + * * onYouTubeVideo fires for a tap on a YouTube watch link (what ReaderExtractor turns * an embedded video into a thumbnail link to) with just the video ID, instead of the * old behavior of handing off to the YouTube app/browser -- callers show an in-app @@ -124,6 +129,7 @@ private fun extractYouTubeVideoId(uri: Uri): String? { data class ReaderWebViewCallbacks( val onOpenLink: (String) -> Unit, val onLongPressLink: (String) -> Unit = {}, + val onOpenYouTubeExternally: (String) -> Unit = {}, val onYouTubeVideo: (String) -> Unit = {}, val onImageTap: (String) -> Unit = {}, val onScrollChanged: (Int) -> Unit = {}, @@ -245,7 +251,19 @@ fun newReaderWebView(context: Context, callbacks: ReaderWebViewCallbacks): Reade // Every article image now sits inside a zoom link, so a long-press on one // reports that link's internal scheme -- strip it back to the image's own URL // so the actions menu shows, and acts on, something real. - callbacks.onLongPressLink(withoutImageZoomScheme(url)) + val target = withoutImageZoomScheme(url) + // A long-press on a video reports the thumbnail's URL rather than the link's -- + // see youTubeTargetForLongPress, which resolves it back to the video. Straight + // out to whatever handles YouTube on the phone, usually the app: a tap already + // plays the video in the reader, so the only thing a long-press can usefully + // mean is "not in here, out there", and picking that off a menu would be a menu + // with one useful item. + val video = youTubeTargetForLongPress(target) + if (video != null) { + callbacks.onOpenYouTubeExternally(video) + } else { + callbacks.onLongPressLink(target) + } true } else { false @@ -380,7 +398,17 @@ fun YouTubePlayerDialog(videoId: String, onDismiss: () -> Unit) { } } webChromeClient = WebChromeClient() - loadUrl("https://www.youtube-nocookie.com/embed/$videoId?autoplay=1&playsinline=1") + // A host page with the embed in an iframe, not the embed itself as the + // top-level document. YouTube answers a top-level embed with "Video player + // configuration error -- Error 153", which is the reported "YouTube play is + // misconfigured"; framed, the same URL plays. See YouTubeLinks. + loadDataWithBaseURL( + YOUTUBE_EMBED_BASE_URL, + youTubeEmbedPageHtml(videoId), + "text/html", + "utf-8", + null + ) } }, onRelease = { it.destroy() } diff --git a/app/src/main/java/org/terst/feedreader/ui/YouTubeLinks.kt b/app/src/main/java/org/terst/feedreader/ui/YouTubeLinks.kt new file mode 100644 index 0000000..e2b3b62 --- /dev/null +++ b/app/src/main/java/org/terst/feedreader/ui/YouTubeLinks.kt @@ -0,0 +1,127 @@ +package org.terst.feedreader.ui + +/** + * Recognising a YouTube placeholder link, and building a page its embed will actually play in. + * + * Split out of ReaderWebView for the usual reason: this is string work with no WebView, no + * gestures and no Compose in it, so it can be exercised without a device -- which matters here + * because the bug it fixes is invisible until a video is tapped. + * + * **Why the embed needs a page at all.** The player dialog used to call + * `loadUrl("https://www.youtube-nocookie.com/embed/<id>")`, loading the embed as the WebView's + * own top-level document. YouTube refuses that: the player comes up showing "Video player + * configuration error -- Error 153", which is reported as "YouTube play is misconfigured". + * Confirmed by loading the same URL three ways in a real browser on an Android user agent: + * + * | how the embed is loaded | result | + * |------------------------------------------|-----------------------------| + * | top-level, as the app did | Error 153 | + * | top-level with `&origin=` | Error 153 | + * | in an `<iframe>` on an ordinary https page | plays | + * + * An embed is meant to be embedded. Given a host page it behaves exactly as it does on any + * site that includes a video, so that is what [youTubeEmbedPageHtml] provides. + */ + +/** Video id characters, per YouTube: 11 of these and nothing else. */ +private val VIDEO_ID_CHARS = Regex("[^A-Za-z0-9_-]") + +/** Hosts whose links are YouTube's. Matched whole, so `notyoutube.com` is not one of them. */ +private val YOUTUBE_HOSTS = setOf("youtube.com", "www.youtube.com", "m.youtube.com", "youtu.be", "www.youtu.be") + +/** Thumbnail hosts. YouTube serves these from i.ytimg.com and numbered variants of it. */ +private val THUMBNAIL_HOST = Regex("^i[0-9]*\\.ytimg\\.com$") + +/** `/vi/<id>/...` or `/vi_webp/<id>/...` -- the id is the first segment after the vi folder. */ +private val THUMBNAIL_PATH = Regex("^/vi(?:_webp)?/([A-Za-z0-9_-]{11})/") + +/** `?v=<id>` anywhere in the query. */ +private val WATCH_QUERY = Regex("[?&]v=([A-Za-z0-9_-]{11})(?:&|$)") + +/** `/<id>` as the whole path, which is what youtu.be links are. */ +private val SHORT_PATH = Regex("^/([A-Za-z0-9_-]{11})/?$") + +/** + * The origin the embed is hosted on. + * + * Deliberately not a youtube.com domain: the embed checks that it is framed by a page other + * than its own, which is the condition the top-level load failed. Nothing is ever fetched from + * this address -- it is the base URL for `loadDataWithBaseURL`, so it only has to be a + * well-formed https origin for the document to have one. + */ +const val YOUTUBE_EMBED_BASE_URL = "https://reader.feedreader.invalid/" + +/** + * The video a long-press reported, as a watch URL, or null when the long-press was not on one. + * + * **What a long-press actually reports.** Not the link's href. The reader's video placeholder is + * `<a href="https://www.youtube.com/watch?v=ID"><img src="https://i.ytimg.com/vi/ID/hq720.jpg"></a>`, + * and for an image inside a link WebView's hit test gives SRC_IMAGE_ANCHOR_TYPE with the + * *image's* URL in `getExtra()`, not the anchor's. Confirmed on the device: long-pressing the + * thumbnail opened the ordinary link menu showing the i.ytimg.com JPEG. So the thumbnail URL is + * the shape that has to be recognised, and it carries the video id in its path. + * + * Watch and youtu.be links are recognised too, for wherever one does arrive -- a plain text link + * to a video in an article, which is not wrapped in an image. + * + * Anything on YouTube that is not one specific video -- a channel, a playlist -- returns null and + * gets the ordinary actions menu, since "open this directly" has no single video to mean. + */ +fun youTubeTargetForLongPress(url: String): String? { + val host = hostOf(url) ?: return null + val path = pathOf(url) + val id = when { + THUMBNAIL_HOST.matches(host) -> THUMBNAIL_PATH.find(path)?.groupValues?.get(1) + host !in YOUTUBE_HOSTS -> null + host.endsWith("youtu.be") -> SHORT_PATH.find(path)?.groupValues?.get(1) + else -> WATCH_QUERY.find(url)?.groupValues?.get(1) + } + return id?.let { youTubeWatchUrl(it) } +} + +/** Where a video lives, in the form a phone hands to the YouTube app. */ +fun youTubeWatchUrl(videoId: String): String = "https://www.youtube.com/watch?v=${scrub(videoId)}" + +/** + * A minimal page holding nothing but the player, filling it. + * + * `allow="autoplay"` is required for the autoplay in the src to be honoured inside an iframe -- + * without it the embed loads and then sits there, which would look like a different bug with + * the same symptom. + */ +fun youTubeEmbedPageHtml(videoId: String): String { + val id = scrub(videoId) + return """<!doctype html><html><head><meta name="viewport" content="width=device-width,initial-scale=1">""" + + """<style>html,body{margin:0;padding:0;height:100%;background:#000;overflow:hidden}""" + + """iframe{display:block;border:0;width:100%;height:100%}</style></head><body>""" + + """<iframe src="https://www.youtube-nocookie.com/embed/$id?autoplay=1&playsinline=1&rel=0" """ + + """allow="autoplay; encrypted-media; picture-in-picture; fullscreen" allowfullscreen></iframe>""" + + """</body></html>""" +} + +/** + * The id with anything outside YouTube's own alphabet removed. + * + * The ids reaching here are already matched against `[a-zA-Z0-9_-]{11}` upstream, so this + * changes nothing today. It exists because the value is spliced straight into an HTML + * attribute, and that should be safe on its own terms rather than because of a check in a + * different file that a later edit might move. + */ +private fun scrub(videoId: String): String = VIDEO_ID_CHARS.replace(videoId, "") + +/** The path of [url], or "/" when it has none. */ +private fun pathOf(url: String): String { + val afterScheme = url.substringAfter("://", missingDelimiterValue = "") + val afterAuthority = afterScheme.substringAfter('/', missingDelimiterValue = "") + val path = "/" + afterAuthority.substringBefore('?').substringBefore('#') + return path +} + +/** The host of [url], lowercased, or null when it is not a URL with one. */ +private fun hostOf(url: String): String? { + val afterScheme = url.substringAfter("://", missingDelimiterValue = "") + if (afterScheme.isEmpty()) return null + val authority = afterScheme.substringBefore('/').substringBefore('?').substringBefore('#') + val host = authority.substringAfterLast('@').substringBefore(':') + return host.lowercase().ifEmpty { null } +} diff --git a/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt b/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt index fc9c32e..442663f 100644 --- a/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt +++ b/app/src/test/java/org/terst/feedreader/data/CachedExtractionTest.kt @@ -96,4 +96,23 @@ class CachedExtractionTest { assertFalse(result.discardedConsentNotice) assertFalse(cleared) } + + /** + * A cached article carrying the publisher's "Trending" promo widgets. Unlike a cookie + * notice, the article itself is fine -- only the widgets have to go, and re-extracting + * would be a pointless round trip that produced the same body again. + */ + @Test + fun `a cached article has its recirculation widgets stripped without being re-extracted`() = runTest { + var cleared = false + val body = "<p>" + "The actual article body. ".repeat(60) + "</p>" + val widget = "<div data-mrf-recirculation=\"IL_x\"><p><h3><span>Trending</span></h3></p></div>" + + val result = loadCachedExtraction(body + widget + body) { cleared = true } + + assertFalse("the stray header survived: ${result.content}", result.content!!.contains("Trending")) + assertTrue(result.content!!.contains("The actual article body.")) + assertFalse("the article was fine; nothing should be thrown away", result.discarded) + assertFalse("re-extracting would fetch the same body again", cleared) + } } diff --git a/app/src/test/java/org/terst/feedreader/data/FeedDiscoveryTest.kt b/app/src/test/java/org/terst/feedreader/data/FeedDiscoveryTest.kt new file mode 100644 index 0000000..eee238a --- /dev/null +++ b/app/src/test/java/org/terst/feedreader/data/FeedDiscoveryTest.kt @@ -0,0 +1,93 @@ +package org.terst.feedreader.data + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Test + +/** + * Finding the feed when what you were given is a site. + * + * Reported live: "Can't subscribe to https://heathercoxrichardson.substack.com/". Pasting a + * site's home page was the only thing to hand -- a site does not advertise its feed URL + * anywhere a reader can see, it advertises it in a <link rel="alternate"> that every other + * reader looks for and this one did not. + */ +class FeedDiscoveryTest { + + /** Verbatim from the reported page, 2026-10-21. */ + private val substackHead = + """<!doctype html><html><head><title>Letters from an American</title>""" + + """<link rel="alternate" type="application/rss+xml" href="/feed" """ + + """title="Letters from an American"/></head><body>...</body></html>""" + + @Test + fun `finds the feed a Substack home page advertises, resolved against the site`() { + assertEquals( + "https://heathercoxrichardson.substack.com/feed", + FeedDiscovery.findFeedUrl(substackHead, "https://heathercoxrichardson.substack.com/") + ) + } + + @Test + fun `finds an Atom feed too`() { + val html = """<link rel="alternate" type="application/atom+xml" href="/atom.xml">""" + + assertEquals("https://example.com/atom.xml", FeedDiscovery.findFeedUrl(html, "https://example.com/")) + } + + @Test + fun `keeps an absolute href as it stands`() { + val html = """<link rel="alternate" type="application/rss+xml" href="https://feeds.example.net/x.xml">""" + + assertEquals("https://feeds.example.net/x.xml", FeedDiscovery.findFeedUrl(html, "https://example.com/blog/")) + } + + @Test + fun `resolves a relative href against the page, not the site root`() { + val html = """<link rel="alternate" type="application/rss+xml" href="feed.xml">""" + + assertEquals("https://example.com/blog/feed.xml", FeedDiscovery.findFeedUrl(html, "https://example.com/blog/")) + } + + @Test + fun `copes with the attributes in any order and single quotes`() { + val html = """<link type='application/rss+xml' href='/rss' rel='alternate'>""" + + assertEquals("https://example.com/rss", FeedDiscovery.findFeedUrl(html, "https://example.com")) + } + + @Test + fun `prefers the first feed when a page advertises several`() { + val html = + """<link rel="alternate" type="application/rss+xml" href="/feed" title="Posts">""" + + """<link rel="alternate" type="application/rss+xml" href="/comments/feed" title="Comments">""" + + assertEquals("https://example.com/feed", FeedDiscovery.findFeedUrl(html, "https://example.com/")) + } + + @Test + fun `ignores an alternate link that is not a feed`() { + val html = + """<link rel="alternate" hreflang="fr" href="/fr/">""" + + """<link rel="alternate" type="application/json" href="/feed.json">""" + + assertNull(FeedDiscovery.findFeedUrl(html, "https://example.com/")) + } + + @Test + fun `a page with no feed at all reports nothing rather than guessing a path`() { + assertNull(FeedDiscovery.findFeedUrl("<html><head></head><body>hi</body></html>", "https://example.com/")) + } + + @Test + fun `an http feed link is upgraded, since the app cannot fetch cleartext`() { + val html = """<link rel="alternate" type="application/rss+xml" href="http://example.com/feed">""" + + assertEquals("https://example.com/feed", FeedDiscovery.findFeedUrl(html, "https://example.com/")) + } + + @Test + fun `xml declaration means this was already a feed, so there is nothing to discover`() { + assertNull(FeedDiscovery.findFeedUrl("<?xml version=\"1.0\"?><rss><channel></channel></rss>", "https://x/f")) + } +} diff --git a/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt b/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt index 8995e22..3e2b28f 100644 --- a/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt +++ b/app/src/test/java/org/terst/feedreader/readability/EvalOutcomeTest.kt @@ -1,6 +1,7 @@ package org.terst.feedreader.readability import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse import org.junit.Assert.assertTrue import org.junit.Test @@ -86,4 +87,21 @@ class EvalOutcomeTest { assertTrue(f.userMessage().lowercase().contains("cookie") || f.userMessage().lowercase().contains("consent")) assertTrue(f.userMessage() != ExtractionFailure.NoContent.userMessage()) } + + /** + * A fresh extraction is cleaned before it is accepted, so the widgets never reach the + * screen and never reach `articles.extractedContent` either. + */ + @Test + fun `a successful extraction has its recirculation widgets stripped`() { + val body = "<p>" + "Real article sentence. ".repeat(40) + "</p>" + val widget = "<div data-mrf-recirculation=\"IL_x\"><p><h3><span>Trending</span></h3></p></div>" + val article = ExtractedArticle(title = "T", content = body + widget + body) + + val decision = decideEvalOutcome(article, rawResult = "{}", attempt = 1, maxAttempts = 6) + + val content = ((decision as EvalDecision.Done).result as ExtractionResult.Success).article.content!! + assertFalse("the stray header reached the caller: $content", content.contains("Trending")) + assertTrue(content.contains("Real article sentence.")) + } } diff --git a/app/src/test/java/org/terst/feedreader/readability/RecirculationWidgetsTest.kt b/app/src/test/java/org/terst/feedreader/readability/RecirculationWidgetsTest.kt new file mode 100644 index 0000000..0926b80 --- /dev/null +++ b/app/src/test/java/org/terst/feedreader/readability/RecirculationWidgetsTest.kt @@ -0,0 +1,117 @@ +package org.terst.feedreader.readability + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * Promo blocks a publisher's widget platform injects into the middle of the article body. + * + * Reported live on West Hawaii Today: "articles get a bunch of stray 'Trending' headers". + * Confirmed on 9 of the 12 articles in its feed, two per article. + */ +class RecirculationWidgetsTest { + + /** The exact markup that survives extraction, from a West Hawaii Today article. */ + private val realWidget = + """<div data-mrf-recirculation="IL_gKyXYBmnRnamdDxChJw6lw" data-testid="text-html" """ + + """data-mrf-experience="IL_gKyXYBmnRnamdDxChJw6lw"><p><h3><span>Trending</span></h3></p></div>""" + + @Test + fun `strips a recirculation widget from the middle of an article`() { + val html = "<p>Real sentence one.</p>$realWidget<p>Real sentence two.</p>" + + val cleaned = RecirculationWidgets.strip(html)!! + + assertFalse("the stray header is still there: $cleaned", cleaned.contains("Trending")) + assertTrue(cleaned.contains("Real sentence one.")) + assertTrue(cleaned.contains("Real sentence two.")) + } + + @Test + fun `strips every widget, not just the first`() { + val html = "<p>One.</p>$realWidget<p>Two.</p>$realWidget<p>Three.</p>" + + val cleaned = RecirculationWidgets.strip(html)!! + + assertEquals(0, Regex("Trending").findAll(cleaned).count()) + assertTrue(cleaned.contains("Three.")) + } + + @Test + fun `leaves an article with no widgets exactly as it was`() { + val html = "<div><p>Nothing to do here.</p><h2>A real heading</h2><p>More.</p></div>" + + assertEquals(html, RecirculationWidgets.strip(html)) + } + + @Test + fun `never removes a block long enough to be real writing`() { + val paragraph = "Beaches across the island lost coral cover. ".repeat(20) + val html = """<div data-mrf-recirculation="X"><p>$paragraph</p></div>""" + + assertTrue( + "a block this long is article text whatever attribute it carries", + RecirculationWidgets.strip(html)!!.contains("Beaches across the island") + ) + } + + @Test + fun `handles an unquoted or single-quoted attribute`() { + val html = "<p>a</p><div data-mrf-recirculation='Y'><h3>Trending</h3></div><p>b</p>" + + assertFalse(RecirculationWidgets.strip(html)!!.contains("Trending")) + } + + @Test + fun `survives a nested div inside the widget`() { + val html = "<p>a</p><div data-mrf-recirculation=\"Z\"><div><h3>Trending</h3></div></div><p>b</p>" + + val cleaned = RecirculationWidgets.strip(html)!! + assertFalse(cleaned.contains("Trending")) + assertTrue(cleaned.contains("<p>a</p>")) + assertTrue(cleaned.contains("<p>b</p>")) + } + + @Test + fun `an unclosed widget div does not eat the rest of the article`() { + val html = "<p>Before.</p><div data-mrf-recirculation=\"W\"><h3>Trending</h3><p>After.</p>" + + assertTrue( + "a malformed widget must not take the article with it", + RecirculationWidgets.strip(html)!!.contains("Before.") + ) + } + + @Test + fun `null and blank are left alone`() { + assertEquals(null, RecirculationWidgets.strip(null)) + assertEquals("", RecirculationWidgets.strip("")) + } + + /** + * The whole extracted article, captured from the live page on 2026-10-21 by running the + * vendored Readability.js over it exactly as the app does. + * + * The unit cases above use the widget markup on its own, which proves the matcher but not + * that it meets reality in one piece. This is the article Peter would have been reading. + */ + @Test + fun `the real extracted article loses both widgets and keeps its text`() { + val extracted = checkNotNull( + javaClass.getResourceAsStream("/west-hawaii-today-extracted.html") + ).bufferedReader().use { it.readText() } + assertEquals("fixture should carry the reported bug", 2, Regex("Trending").findAll(extracted).count()) + + val cleaned = RecirculationWidgets.strip(extracted)!! + + assertEquals(0, Regex("Trending").findAll(cleaned).count()) + assertTrue(cleaned.contains("Leavitt officially stepped down as White House press secretary")) + assertTrue(cleaned.contains("The New York Times")) + assertTrue( + "barely any of the article should have gone with the widgets", + cleaned.length > extracted.length - 600 + ) + } +} diff --git a/app/src/test/java/org/terst/feedreader/ui/YouTubeLinksTest.kt b/app/src/test/java/org/terst/feedreader/ui/YouTubeLinksTest.kt new file mode 100644 index 0000000..3afba16 --- /dev/null +++ b/app/src/test/java/org/terst/feedreader/ui/YouTubeLinksTest.kt @@ -0,0 +1,139 @@ +package org.terst.feedreader.ui + +import org.junit.Assert.assertEquals +import org.junit.Assert.assertFalse +import org.junit.Assert.assertNull +import org.junit.Assert.assertTrue +import org.junit.Test + +/** + * The YouTube placeholder: recognising one, and building a page its embed will actually play in. + * + * Both halves come from the same report: "YouTube play is misconfigured, long click on YouTube + * placeholder should open the URL directly." + */ +class YouTubeLinksTest { + + /** + * What a long-press on the reader's video placeholder actually reports. + * + * Not the link's href, which is what the first version of this assumed and got wrong on the + * device: the placeholder is `<a href="...watch?v=ID"><img src="...i.ytimg.com/vi/ID/..."></a>`, + * and for an image inside a link WebView's hit test reports SRC_IMAGE_ANCHOR_TYPE with the + * *image's* URL in `extra`, not the anchor's. Long-pressing the thumbnail produced the + * ordinary link menu showing `https://i.ytimg.com/vi/dQw4w9WgXcQ/hq720.jpg`. + * + * The thumbnail URL carries the video id, so it is resolved back to the video. + */ + @Test + fun `a long-press on the thumbnail resolves to the video, which is what WebView reports`() { + assertEquals( + "https://www.youtube.com/watch?v=dQw4w9WgXcQ", + youTubeTargetForLongPress("https://i.ytimg.com/vi/dQw4w9WgXcQ/hq720.jpg") + ) + } + + @Test + fun `any thumbnail size resolves, since the reader has used more than one`() { + assertEquals( + "https://www.youtube.com/watch?v=dQw4w9WgXcQ", + youTubeTargetForLongPress("https://i.ytimg.com/vi/dQw4w9WgXcQ/maxresdefault.jpg") + ) + assertEquals( + "https://www.youtube.com/watch?v=dQw4w9WgXcQ", + youTubeTargetForLongPress("https://i9.ytimg.com/vi_webp/dQw4w9WgXcQ/hq720.webp") + ) + } + + @Test + fun `a watch link resolves to itself, for wherever one does come through`() { + assertEquals( + "https://www.youtube.com/watch?v=dQw4w9WgXcQ", + youTubeTargetForLongPress("https://www.youtube.com/watch?v=dQw4w9WgXcQ") + ) + } + + @Test + fun `a short youtu-be link resolves to the video`() { + assertEquals( + "https://www.youtube.com/watch?v=dQw4w9WgXcQ", + youTubeTargetForLongPress("https://youtu.be/dQw4w9WgXcQ") + ) + } + + @Test + fun `an ordinary article link is not a video`() { + assertNull(youTubeTargetForLongPress("https://www.nytimes.com/2026/10/20/us/politics/x.html")) + } + + @Test + fun `an ordinary article image is not a video`() { + assertNull(youTubeTargetForLongPress("https://www.westhawaiitoday.com/wp-content/uploads/reef.jpg")) + } + + @Test + fun `a lookalike host is not YouTube`() { + assertNull(youTubeTargetForLongPress("https://notyoutube.com/watch?v=dQw4w9WgXcQ")) + assertNull(youTubeTargetForLongPress("https://youtube.com.evil.example/watch?v=dQw4w9WgXcQ")) + assertNull(youTubeTargetForLongPress("https://i.ytimg.com.evil.example/vi/dQw4w9WgXcQ/hq720.jpg")) + } + + @Test + fun `a YouTube link that is not one video stays on the ordinary menu`() { + assertNull(youTubeTargetForLongPress("https://www.youtube.com/@someone")) + assertNull(youTubeTargetForLongPress("https://www.youtube.com/playlist?list=PL123")) + } + + @Test + fun `rubbish is not a video`() { + assertNull(youTubeTargetForLongPress("")) + assertNull(youTubeTargetForLongPress("not a url at all")) + } + + // --- the embed page ------------------------------------------------------------------ + + @Test + fun `the embed page puts the player in an iframe, which is the whole point`() { + val html = youTubeEmbedPageHtml("dQw4w9WgXcQ") + + assertTrue("loading the embed top-level is what YouTube rejects: $html", html.contains("<iframe")) + assertTrue(html.contains("https://www.youtube-nocookie.com/embed/dQw4w9WgXcQ")) + } + + @Test + fun `the embed page keeps autoplay and inline playback`() { + val html = youTubeEmbedPageHtml("dQw4w9WgXcQ") + + assertTrue(html.contains("autoplay=1")) + assertTrue(html.contains("playsinline=1")) + assertTrue("without this the iframe is not allowed to autoplay", html.contains("allow=\"autoplay")) + } + + @Test + fun `the base URL is a real https origin, since that is what the embed requires`() { + assertTrue(YOUTUBE_EMBED_BASE_URL.startsWith("https://")) + } + + @Test + fun `the base URL is not YouTube's own origin`() { + assertFalse( + "the embed has to be a guest on somebody else's page, not its own", + YOUTUBE_EMBED_BASE_URL.contains("youtube") + ) + } + + @Test + fun `a video id that could break out of the attribute is scrubbed`() { + val html = youTubeEmbedPageHtml("""a"><script>alert(1)</script>""") + + assertFalse("markup from the id reached the page: $html", html.contains("<script")) + assertFalse(html.contains("alert(1)")) + // What is left is only ever id characters, so the src attribute still closes where it should. + assertTrue(html.contains("embed/ascriptalert1script?")) + } + + @Test + fun `the watch link for a video is the one a phone can hand to the YouTube app`() { + assertEquals("https://www.youtube.com/watch?v=dQw4w9WgXcQ", youTubeWatchUrl("dQw4w9WgXcQ")) + } +} diff --git a/app/src/test/resources/west-hawaii-today-extracted.html b/app/src/test/resources/west-hawaii-today-extracted.html new file mode 100644 index 0000000..5c52608 --- /dev/null +++ b/app/src/test/resources/west-hawaii-today-extracted.html @@ -0,0 +1,24 @@ +<div id="readability-page-1" class="page"><div id="article_info"> + + + + + + <p>Published 12:05 am Wednesday, October 21, 2026</p> + + <div> + <p> + <strong>By Maggie Haberman and Jonathan Swan NYTimes News Service</strong> + </p> + + </div><!-- END BYLINE --> + </div><div id="article_content"><p>WASHINGTON — In late August, during one of Karoline Leavitt’s final meetings with President Donald Trump in the Oval Office, he pitched her on an idea in front of several senior aides. He told her to move reporters out of the James S. Brady Press Briefing Room, which would effectively remove news organizations from the White House. It would be, he told her, her “last legacy.”</p> +<p>But the proposal never moved forward while she was there. The president had put the request to her several times over months, according to three people with knowledge of the discussions who were not authorized to speak publicly. Leavitt officially stepped down as White House press secretary on Aug. 27.</p> +<p>Trump has told aides he would prefer to bring back the indoor pool President Franklin D. Roosevelt added in 1933 as therapy for his polio-afflicted legs; President Richard Nixon later covered it over to make room for the news media.</p> +<div data-mrf-recirculation="IL_gKyXYBmnRnamdDxChJw6lw" data-testid="text-html" data-mrf-experience="IL_gKyXYBmnRnamdDxChJw6lw"><p><h3><span>Trending</span></h3></p></div><p>While moving the media out of the White House may be a relatively small debate considering Trump’s attacks on journalists more broadly, it would be highly symbolic. The proximity of the media to the seat of U.S. executive power has long been an acknowledgment of the importance of a free press in a constitutional democracy.</p> +<p>Trump has brought up the press-eviction idea before, dating to his first term, and he has never quite dropped it.</p> +<p>As recently as Sept. 27, Trump shared a pair of images on Truth Social showing the briefing room as it is now, captioned “Fake News,” alongside another that shows the space reimagined as a gilded swimming pool, captioned “FDR Pool.”</p> +<p>The Wall Street Journal reported last month that Trump had repeatedly discussed moving the press corps out of the White House, and that his advisers had so far either offered pushback or waited for him to move on.</p> +<p>What has changed since Trump’s first term or even earlier this term is the staff around him. The president’s core team has shrunk from the already-tight unit that he brought into the White House last year. During earlier discussions, most of Trump’s senior aides agreed it served him better to keep the media nearby. But Trump has grown more enraged about his press coverage, and other advisers of growing influence have encouraged him to take drastic measures to punish journalists.</p> +<p><ins>This article originally appeared in <a href="https://www.nytimes.com/2026/10/20/us/politics/trump-white-house-media.html" data-mrf-link="https://www.nytimes.com/2026/10/20/us/politics/trump-white-house-media.html" cmp-ltrk="Article Links" cmp-ltrk-idx="0" mrfobservableid="0c7048ed-f27d-4067-a945-5b92fb782ba2">The New York Times</a>. </ins></p> +<div data-mrf-recirculation="IL_p3LFgBtiQmCCCy-bXT17QA" data-testid="text-html" data-mrf-experience="IL_p3LFgBtiQmCCCy-bXT17QA"><p><h3><span>Trending</span></h3></p></div><p><em>© 2026 The New York Times Company</em></p></div></div>
\ No newline at end of file |