| Age | Commit message (Collapse) | Author |
|---|
|
Reported against lcamtuf's Substack ("we should render tex",
blog.coredump.cx/p/yours-truly): reader mode showed raw
"\(p = \text{...}\)" where equations should be.
What the page actually ships is the TeX SOURCE, as text, inside a
"latex-rendered" div -- Substack renders it in the browser with its own
script, which never runs here. Readability keeps that text verbatim (it
strips the class, not the text), so nothing was being discarded during
extraction and no amount of extraction-side repair would have helped:
there was simply never any rendered markup to keep.
So render it during extraction, where JavaScript is already running for
Readability's sake. KaTeX (vendored in assets, never fetched -- this
runs against untrusted third-party pages) converts each expression to
MathML before Readability sees the document, and the reader WebView
renders that natively. The reader keeps JavaScript OFF, as it must, and
needs no stylesheet, webfont or script of KaTeX's; the only reader-side
change is containing a wide equation so it scrolls within its own box
instead of dragging the whole article sideways.
Two details worth knowing:
- KaTeX refuses {align} outside display mode, and Substack wraps display
maths in INLINE delimiters (a block-styled span does the centring), so
a mode that renders nothing is retried in the other one. An expression
neither mode can render is left as source -- no worse than before.
- The reported article came with "extractedContent cached: true", so the
raw source was already on disk and every open read it back. Recognised
and dropped on read (same mechanism as the cached consent notice),
which re-extracts the article the next time it is opened.
APK grows by ~90KB: the vendored katex.min.js (267KB) compresses to 91KB
in the APK. No new permissions, no schema change, no new JS-enabled
surface.
Verified on the emulator against the live article: stale raw-TeX cache
discarded, article re-extracted, all four equations rendered, and a wide
align block scrolls inside itself.
|
|
Reported live: reader mode on a New Republic feature showed nothing but
the cookie disclaimer, and had cached it.
Reproduced outside the app by running ReaderExtractor's own injected
script (Readability.js and all) against the real page. With the article
body present, extraction is perfect. With the body absent from the DOM at
the moment the script runs, Readability's first pass -- which discards
"unlikely candidates", and the Didomi banner's class is
didomi-notice-banner -- comes back under its own 500-character threshold,
so it retries with that stripping switched off. The banner's single
paragraph is 510 characters, i.e. just over the threshold, so it comes
back as the article with the right title attached.
Nothing on the Kotlin side looked at that. Any non-blank content counted
as ExtractionResult.Success, was written to articles.extractedContent,
and was never reconsidered -- so one bad load became a permanent cookie
disclaimer, which is the "extractedContent cached: true" in the report.
ConsentNotice recognises such a result: a hard plain-text length cap
first (nothing long is ever rejected, however much consent boilerplate it
quotes), then either a known CMP container id -- Readability strips
classes but keeps ids -- or two or more distinct consent phrases.
The accept/retry/give-up decision moves out of the WebView callback into
decideEvalOutcome so it can be tested without a device. A consent notice
is now "not ready yet", which routes it into the retry the extractor
already does for an empty result; if it survives all six attempts that is
what the page serves, and it fails as ExtractionFailure.ConsentInterstitial
rather than being accepted.
A fixed extractor cannot repair what is already on disk, so
loadCachedExtraction drops a cached notice on read and re-extracts, in
both ReaderScreen and LinkReaderScreen. No schema change: the repair
happens when the article is next opened. Reader mode is still never
turned on heuristically -- replacing a discarded notice does not flip it.
The bug report now also carries the last extraction error, without which
a report about bad reader content cannot say whether extraction failed.
Tests: 59 -> 78.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018AwnALZL6bD4VBtATDVv3B
|
|
Pinching in the reader used to scale the whole WebView like a photo and snap
back to the real font size on release. It now drives WebSettings.textZoom as
the fingers move, so the text itself resizes and reflows during the gesture
with nothing to snap back from; images and margins stay put. The reload at the
new saved size is what returns textZoom to 100%, so the two never overlap --
and loading reader HTML now goes through ReaderWebView.loadReaderHtml, which
does both together, so a future load site can't forget the reset.
Reader view for a tapped link now honors the saved font size and the same
pinch gesture, instead of always rendering at the 18px default.
A zoomed image can be panned again. The viewer computed its pan bounds from
values captured when the dialog first composed -- an empty viewport around a
not-yet-decoded image -- because a pointerInput(Unit) block keeps the lambda it
was created with. Those bounds allowed exactly one offset, 0, so zooming worked
and dragging did nothing. The whole pinch/pan step is now one pure function the
gesture callback calls with values it reads fresh each time.
|
|
Bug report: extraction failed on one 1500days.com article while others worked.
It is not that article. Extraction was racing the page load, and the report
could say nothing more because the app discarded the reason.
Two separate defects.
1. The race. onPageFinished fires before the page has finished loading —
observed at progress=25 on this article — and the extraction script was
injected there. Sometimes the DOM is far enough along; sometimes
evaluateJavascript hands back a bare "null" because the document is still
being replaced. Measured against the reported URL: 4 successes in 6
attempts before, 10 in 10 after.
An unusable result is no longer final. A null or empty-content result is
retried (6 attempts, 500ms apart, well inside the existing 25s timeout); a
Readability error still fails immediately, since retrying will not change
what the parser thinks of the markup.
2. The silence. ReaderScreen did `extracted?.content?.let { }`, so every
failure — dead host, HTTP error, timeout, parser throw — arrived as the same
null and was dropped. The reader just kept showing raw feed content. The
only way a failure ever surfaced was the user noticing.
extractResult() now returns a typed ExtractionFailure carrying the reason;
ReaderScreen logs the diagnostic and shows the user a short explanation.
Also replaced a dead error callback. The only override was the four-argument
onReceivedError, which Android stopped calling at API 23; minSdk is 26, so it
never fired on any supported device. A failed load therefore invoked nothing
and hung for the full 25 seconds before surfacing as an indistinguishable null.
Now overrides the request/error and onReceivedHttpError forms Android actually
calls, both filtered to the main frame so a 404'd tracker cannot abort an
article that renders fine.
The instrumented test does NOT reproduce the race and says so in its own
docstring: a MockWebServer page finishes loading before onPageFinished fires,
and slow subresources do not help because WebView waits for them. It is kept as
a reliability guard over repeated back-to-back extractions. The race was
verified by hand against the live URL.
47 unit tests, 1 instrumented test, both green.
|
|
Article images could be looked at but never enlarged -- a photo or a
chart in a feed was stuck at whatever size the reader column gave it.
Tapping one now opens a full-screen viewer: pinch to zoom (up to 6x),
double-tap to toggle between fitted and 3x, drag to pan while zoomed,
and dismiss with back, the close button, or a swipe down.
Detecting the tap is the whole difficulty. The reader is a WebView, and
its JavaScript is off deliberately (untrusted article content), so no
in-page script can notice a tap on an <img> and report it out. WebView's
own built-in zoom isn't the answer either: pinching in the reader is
already taken, on purpose, by the reader-font-size gesture, and page
zoom scales the article's whole layout rather than giving an image the
screen. So wrapReaderHtml now wraps every image that isn't already
inside a link in a link to a scheme nothing can navigate to
(feedreader-image:<url>), which WebView hands to shouldOverrideUrlLoading
-- the interception point that already routes ordinary links and YouTube
thumbnails. The new onImageTap callback opens ImageZoomDialog instead of
following it.
Images that already sit inside a link are left alone: ReaderExtractor's
synthesized YouTube thumbnail is exactly that shape, and re-wrapping it
would swallow the tap that plays the video. Those stay zoomable from the
other side -- a link pointing straight at an image file is also treated
as a zoom target, which additionally fixes WordPress's default "link to
media file" markup sending a tap off to LinkReaderScreen to run
Readability over a JPEG. The long-press-link handler strips the scheme
back off, so the actions menu still offers the real image URL.
The viewer is Compose + Coil (AsyncImage), matching what the article
list's thumbnails already use, so a just-scrolled-past image usually
comes straight from Coil's cache; zoom is a graphicsLayer transform, not
a re-decode. Panning is bounded by where the image actually is rather
than by the viewport -- ContentScale.Fit letterboxes anything that isn't
the screen's aspect ratio, and bounding by the viewport would let a wide
photo be dragged up into its own black bar. Coil reports the intrinsic
size once decoded; until then the viewport is the fallback bound.
TDD: the HTML rewriting (linkifyImagesForZoom), the tap-target decision
(zoomableImageTarget), and the zoom/pan math (imageZoomScale,
imageZoomDoubleTapScale, clampImagePanOffset, fittedImageSize) were all
written test-first against stubs, red on assertion mismatches, green on
the real implementations -- 26 new cases in ImageZoomTest, including
three over wrapReaderHtml itself to pin the ordering that keeps the
YouTube thumbnail's link intact. Gesture wiring and the dialog itself
are Compose UI, untestable here for the same reason the rest of ui/ is.
Coverage is unchanged: ui/ is excluded from the Jacoco metric by design.
|
|
Pinching in ReaderScreen's article WebView now adjusts the same
persisted reader font size the Settings slider controls, instead of
doing nothing (WebView's own page zoom was never enabled, so a pinch
previously had no effect at all).
A ScaleGestureDetector is fed through setOnTouchListener, which always
returns false so it never consumes the touch -- WebView's existing
one-finger scrolling, link taps and long-press-link handling are
unaffected, since ScaleGestureDetector only fires once a second pointer
is down. During the gesture the WebView is scaled live via View.scaleX/
scaleY for immediate visual feedback; at gesture end the scale resets
to 1x and the computed, clamped font size is persisted, which drives
ReaderScreen's existing content-reload-on-fontSizePx-change path to
re-render the actual HTML at the new size.
TDD: pinchZoomFontSizePx (the pure scale-factor -> clamped-px
calculation) was written test-first, red on a TODO() stub, green on the
real implementation -- covered by SettingsTest.
newReaderWebView grew from 6 to 8 parameters to carry the new gesture
hooks, which pushed it over detekt's LongParameterList threshold;
bundled all of its callbacks into a ReaderWebViewCallbacks data class
rather than baselining past it, updating both call sites (ReaderScreen,
LinkReaderScreen). ReaderContent gained no new parameter -- it now
independently remembers its own Settings instance to persist the
pinch-committed size, matching how it already remembers its own db and
scope rather than threading them from ReaderScreen.
LinkReaderScreen's WebView is untouched: it doesn't read the reader
font size setting at all today, so wiring pinch-zoom there would be a
separate, unrelated fix.
|
|
The data model and FilterRuleEngine already supported a per-feed scope
(FilterRuleEntity.feedId), but Settings' "Add rule" form never let a
user set it -- only a hardcoded migration had ever created one. Add a
FeedScopePicker so a rule can target "All feeds" (unchanged default)
or one specific feed, and show each existing rule's scope in the list.
Added the first real unit tests in this repo (FilterRuleEngineTest),
covering global vs. feed-scoped matching and mute-wins-over-highlight
priority -- the exact behavior this UI gap left unexercised outside
one migration-seeded row. First real jacoco coverage number: 2.31%.
While verifying, found the pre-commit hook's coverage step was
silently a no-op: it pointed at app/build/reports/jacoco/report.xml,
but the jacoco setup that landed here uses the standard Gradle
JacocoReport task, which writes to
reports/jacoco/jacocoTestReport/jacocoTestReport.xml instead. Fixed
the path so the gate actually reads real numbers now.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017GMEkHeqKz6FLkmizowBTK
|
