From 3f09a66079a2ffdc3eaf3422d1ef1b134d0762fc Mon Sep 17 00:00:00 2001 From: Usama <83345144+usamaiqb@users.noreply.github.com> Date: Tue, 1 Sep 2026 19:12:58 +0500 Subject: [PATCH] feat(reader): search and the table of contents through the page Both worked before the WebView swap and neither survived it: they index a rendering, and the rendering moved into the page. The controls were hidden rather than left to report nothing. They are back, driven by the renderer that already ships find and an outline -- this is wiring, not a rewrite. `__mdrPage` gains `runFind`, `stepFind`, `closeFind` and `revealSlug`, and the page forwards three reports out through `__mdrHost`: the outline, the heading being read, and the match count. `openFind` is deliberately absent -- it re-runs whatever query the page last had, and the app is the one holding the query. Search runs on whichever surface is drawing, and the two are not alike. The page owns its match set, since matches are DOM ranges over a rendering only it has, so a query goes in and an index comes back as an event; raw mode keeps the span pass over text held out here. That fork is decided in one place, `usesPageFind`, off the same predicate the reader uses to pick a surface -- and a test asserts the two agree across the whole input space, because a disagreement is a search running against a surface that is not there. Requests reach the page over a buffered channel rather than as state. "Next match" has no value to hold, and every toggle into raw mode and back destroys the WebView, so a request in flight has to survive that instead of being lost or replayed at every page that follows. The outline came off the format layer entirely. A heading's position is a fact about the rendering, so `DocumentFormat.headings`, the ATX parser and the heading math in `ScrollAnchor` and `ReaderContentView` are deleted, and `HeadingItem` moves beside the WebView host carrying a slug rather than a character offset. The detekt baseline loses three entries and gains none. Two fixes fall out of it. A search with no usable renderer reported matches and highlighted nothing, because the fallback drew the unmarked source; it shares raw mode's branch now, which is the branch the highlighting is written to. And the shared stylesheet is re-vendored: the `::highlight(mr-find-*)` rules were in the desktop chrome stylesheet this host deliberately never loads, so find painted on desktop and was invisible here. --- app/src/main/assets/renderer/MANIFEST.md | 4 +- app/src/main/assets/renderer/android-host.js | 86 +++++- .../renderer/css/document-overrides.css | 20 ++ .../com/markreader/format/DocumentFormat.kt | 8 - .../markreader/format/FormatCapabilities.kt | 9 +- .../java/com/markreader/format/HeadingItem.kt | 13 - .../format/markdown/MarkdownFormat.kt | 58 ---- .../markreader/ui/reader/ReaderContentView.kt | 52 ---- .../com/markreader/ui/reader/ScrollAnchor.kt | 74 ----- .../com/markreader/ui/reader/WebViewReader.kt | 34 ++ .../com/markreader/ui/screens/EditorScreen.kt | 2 - .../markreader/ui/screens/ViewerChoices.kt | 26 +- .../com/markreader/ui/screens/ViewerScreen.kt | 90 +++--- .../markreader/ui/screens/ViewerViewModel.kt | 292 ++++++++++++++---- .../com/markreader/webview/HeadingItem.kt | 19 ++ .../com/markreader/webview/PageCommand.kt | 28 ++ .../markreader/webview/WebViewDocumentHost.kt | 122 ++++++-- .../markreader/format/FormatRegistryTest.kt | 10 - .../format/markdown/MarkdownHeadingsTest.kt | 97 ------ .../format/plaintext/PlainTextFormatTest.kt | 6 - .../markreader/ui/reader/ScrollAnchorTest.kt | 83 ----- .../ui/screens/ViewerChoicesTest.kt | 64 ++++ config/detekt/baseline.xml | 5 +- 23 files changed, 651 insertions(+), 551 deletions(-) delete mode 100644 app/src/main/java/com/markreader/format/HeadingItem.kt create mode 100644 app/src/main/java/com/markreader/webview/HeadingItem.kt create mode 100644 app/src/main/java/com/markreader/webview/PageCommand.kt delete mode 100644 app/src/test/java/com/markreader/format/markdown/MarkdownHeadingsTest.kt diff --git a/app/src/main/assets/renderer/MANIFEST.md b/app/src/main/assets/renderer/MANIFEST.md index 6946a74..f92ba00 100644 --- a/app/src/main/assets/renderer/MANIFEST.md +++ b/app/src/main/assets/renderer/MANIFEST.md @@ -10,8 +10,8 @@ diverges the two apps in the meantime. | | | | --- | --- | | Repo | `markreader-desktop` (`../../rust/markreader-desktop` alongside this one) | -| Commit | `0a9b2b36a0c327ed96c0966afc7517774ceb5647` | -| Subject | `fix(renderer): restore the front matter rules markdown.css lost` | +| Commit | `dacbaf9c1cd9a5b2a6b272f12fae4215c1363adc` | +| Subject | `fix(renderer): paint find matches from the document stylesheet` | | Branch | `refactor/shared-document-renderer` | | Command | `npm run build:esm`, output `out/renderer-esm/` | diff --git a/app/src/main/assets/renderer/android-host.js b/app/src/main/assets/renderer/android-host.js index 121c059..fe4c80c 100644 --- a/app/src/main/assets/renderer/android-host.js +++ b/app/src/main/assets/renderer/android-host.js @@ -8,15 +8,17 @@ * * 1. installs `window.markreader` as a `DocumentHost`, on top of the `@JavascriptInterface` * object Kotlin injects as `__mdrHost`; - * 2. builds the one `DocumentView` this page holds; + * 2. builds the one `DocumentView` this page holds, and forwards the events Compose has to + * draw from — the outline, the active heading, the match count — out through `__mdrHost`; * 3. exposes `window.__mdrPage` for Kotlin to drive through `evaluateJavascript`. * * **The two directions are deliberately separate objects.** `window.markreader` is the * contract in `host.ts` — the narrow surface the shared document module is allowed to assume, * which on Android is `capabilities`, `openExternal`, `openPath` and `readFile`. `__mdrPage` - * is this page's own control surface, which is nobody's contract and can change freely. The - * desktop bridge conflates them because there the shell *is* the page; here the shell is - * Compose, so keeping them apart is what stops app concerns leaking into the host contract. + * is this page's own control surface, which is nobody's contract and can change freely, and the + * event forwarding above is its inbound half and equally ours to change. The desktop bridge + * conflates them because there the shell *is* the page; here the shell is Compose, so keeping + * them apart is what stops app concerns leaking into the host contract. * * `showInFolder` is absent, not stubbed. Android has no file manager to reveal into, and the * contract says a host that cannot do something omits it — `capabilitiesOf()` then reports @@ -51,6 +53,9 @@ const NO_BRIDGE = { rendered: path => console.info('[host] rendered', path), openPath: path => console.info('[host] openPath', path), openExternal: url => console.info('[host] openExternal', url), + outline: json => console.info('[host] outline', json), + activeHeading: slug => console.info('[host] activeHeading', slug), + findState: (index, total) => console.info('[host] findState', index, total), }; const bridge = window.__mdrHost ?? NO_BRIDGE; @@ -149,6 +154,27 @@ view.on('document', doc => { } }); +/* + * The outline and the find state are the two things Compose has to draw that only the page + * knows, so they leave as events rather than being asked for. `document.js` recomputes both on + * every render — a theme switch, a live reload, a document opened by following a link — and + * Kotlin has no way to notice that happened. + * + * The outline crosses as JSON because it is a list. The other two are primitives, which + * `addJavascriptInterface` marshals on its own, so wrapping them in an object would only give + * the Kotlin side something to parse. + * + * The active heading is `undefined` in a document with no headings at all; `''` carries that + * across, because a `@JavascriptInterface` String parameter is a poor place to learn the + * difference between a missing argument and a null one. + * + * The find state's query is dropped: the app is holding the query, and it is only ever told + * the answer to something it asked. What it cannot know is the count. + */ +view.on('headings', headings => bridge.outline(JSON.stringify(headings))); +view.on('activeHeading', slug => bridge.activeHeading(slug ?? '')); +view.on('find', state => bridge.findState(state.index, state.total)); + /** * Writes the three typography custom properties `markdown.css` reads. * @@ -222,6 +248,58 @@ const page = { view.render(); } }, + + /** + * Finds `query` in the rendering, paints every match and reveals the first. + * + * **The match set is the page's, not Kotlin's.** Matches are DOM ranges over the rendered + * document, and this is the only side that has the rendering — so what goes back out is a + * count and an index, through the `find` event above, and the search bar is drawn from that. + * + * Highlighting needs the CSS Custom Highlight API, which `document.js` feature-detects. Below + * that the count and the navigation still work and nothing is painted, which is a poor search + * but not a broken one. + * + * @param json {string} a JSON `{ query }`. + */ + runFind(json) { + view.runFind(String(JSON.parse(json).query ?? '')); + }, + + /** + * Moves to another match, wrapping at either end. + * + * @param json {string} a JSON `{ delta }` — `1` for the next match, `-1` for the previous. + */ + stepFind(json) { + view.stepFind(Number(JSON.parse(json).delta) || 0); + }, + + /** + * Drops the query and its highlights, because the search affordance closed. + * + * `openFind` has no counterpart here on purpose: it re-runs whatever query the *page* was + * last given, and the app is the one holding the query — the search bar's text survives the + * bar closing, so opening it sends `runFind` and there is nothing for a second door to do. + */ + closeFind() { + view.closeFind(); + }, + + /** + * Scrolls a heading into view: the table of contents today, the resume position when that + * moves onto an anchor. + * + * The headroom left above the target is the renderer's (`REVEAL_OFFSET`), which knows nothing + * about the chrome Compose draws over the page — so a reveal that does not also collapse the + * chrome can land the heading behind it. Fixing that properly means teaching the shared + * reveal about the content inset, which is desktop work. + * + * @param json {string} a JSON `{ slug }`, one of the slugs the outline reported. + */ + revealSlug(json) { + view.revealSlug(String(JSON.parse(json).slug ?? '')); + }, }; Object.defineProperty(window, '__mdrPage', { diff --git a/app/src/main/assets/renderer/css/document-overrides.css b/app/src/main/assets/renderer/css/document-overrides.css index d0d53f7..b475cdd 100644 --- a/app/src/main/assets/renderer/css/document-overrides.css +++ b/app/src/main/assets/renderer/css/document-overrides.css @@ -99,6 +99,26 @@ th { border-radius: 3px; } +/* ------------------------------------------------------------------------- find */ + +/* + * The paint for `runFind`. Registering a range with `CSS.highlights.set()` does nothing on its + * own — a custom highlight with no `::highlight()` rule for its name is invisible — so these + * are not decoration, they are the other half of the feature. + * + * They were in `app.css`, which is why find highlighted on desktop and not on Android: the + * ranges are painted by `document.ts`, but the only host loading the chrome stylesheet was the + * desktop one. Same bug as the mermaid and KaTeX rules below, one instance later. + */ +::highlight(mr-find-match) { + background-color: rgba(234, 92, 0, 0.33); +} + +::highlight(mr-find-current) { + background-color: rgba(234, 92, 0, 0.8); + color: #ffffff; +} + /* ---------------------------------------------------------------------- mermaid */ .mermaid-block { diff --git a/app/src/main/java/com/markreader/format/DocumentFormat.kt b/app/src/main/java/com/markreader/format/DocumentFormat.kt index cb1859e..0808338 100644 --- a/app/src/main/java/com/markreader/format/DocumentFormat.kt +++ b/app/src/main/java/com/markreader/format/DocumentFormat.kt @@ -62,14 +62,6 @@ interface DocumentFormat { suspend fun loadEditorPreview(text: String, theme: RenderTheme): ReaderContent? = load(text, theme) - /** - * The document's table of contents. Empty unless [FormatCapabilities.hasHeadings]. - * - * Takes both the source and its rendering because the offsets have to index the text the - * reader actually scrolls, not the markup that produced it. - */ - fun headings(text: String, content: ReaderContent): List = emptyList() - /** * Drop any cached renderer. The reader palette changed, so a renderer built for the old one * would paint the previous theme's colours. diff --git a/app/src/main/java/com/markreader/format/FormatCapabilities.kt b/app/src/main/java/com/markreader/format/FormatCapabilities.kt index fe68bc3..6eadf4a 100644 --- a/app/src/main/java/com/markreader/format/FormatCapabilities.kt +++ b/app/src/main/java/com/markreader/format/FormatCapabilities.kt @@ -11,7 +11,14 @@ package com.markreader.format * The defaults describe the least capable document there is: text that renders as itself. */ data class FormatCapabilities( - /** The document has a heading structure, so a table of contents is meaningful. */ + /** + * The document has a heading structure, so a table of contents is meaningful. + * + * Says only that the *format* has headings, not where they are. The outline itself comes from + * whatever drew the document — for the shared renderer, from the page — because a heading's + * position is a fact about the rendering. Without this a code file, which reaches the renderer + * as one fenced block, would offer a table of contents that is always empty. + */ val hasHeadings: Boolean = false, /** * The rendered form differs from the source, so the raw/rendered toggle does something. diff --git a/app/src/main/java/com/markreader/format/HeadingItem.kt b/app/src/main/java/com/markreader/format/HeadingItem.kt deleted file mode 100644 index 8b6c3fb..0000000 --- a/app/src/main/java/com/markreader/format/HeadingItem.kt +++ /dev/null @@ -1,13 +0,0 @@ -package com.markreader.format - -/** - * One entry in a document's table of contents. - * - * [offset] is a character offset into the *rendered* text, because that is what the reader - * scrolls. Only formats whose [FormatCapabilities.hasHeadings] is true produce these. - */ -data class HeadingItem( - val text: String, - val level: Int, - val offset: Int -) diff --git a/app/src/main/java/com/markreader/format/markdown/MarkdownFormat.kt b/app/src/main/java/com/markreader/format/markdown/MarkdownFormat.kt index 79f17f6..10251b9 100644 --- a/app/src/main/java/com/markreader/format/markdown/MarkdownFormat.kt +++ b/app/src/main/java/com/markreader/format/markdown/MarkdownFormat.kt @@ -6,14 +6,10 @@ import androidx.core.graphics.ColorUtils import com.markreader.R import com.markreader.format.DocumentFormat import com.markreader.format.FormatCapabilities -import com.markreader.format.HeadingItem import com.markreader.format.ReaderContent import com.markreader.format.RenderTheme import kotlinx.coroutines.CancellationException -/** Headings deeper than this are left out of the table of contents; it stops being navigation. */ -private const val MAX_TOC_LEVEL = 3 - /** Markwon's own default: the text colour at ~10% alpha, so inline and block code agree. */ private const val CODE_BACKGROUND_ALPHA = 25 @@ -72,62 +68,8 @@ class MarkdownFormat(private val newRenderer: (isDark: Boolean) -> MarkwonRender null } - /** - * Offsets index the rendering, so a rendering this side cannot see has no offsets to give. - * - * The WebView knows where its headings are and Kotlin does not; a list built from source - * offsets would scroll the reader to the wrong place, or to nowhere. Better an absent table - * of contents than one that lies — **rebuilding it in the page is planned**, with search. - */ - override fun headings(text: String, content: ReaderContent): List = - when (content) { - is ReaderContent.Spans -> parseHeadings(text, content.text.toString()) - is ReaderContent.Markdown, is ReaderContent.Table -> emptyList() - } - override fun invalidateRenderers() = renderers.clear() private fun renderer(isDark: Boolean): MarkwonRenderer = renderers.getOrPut(isDark) { newRenderer(isDark) } } - -/** - * ATX headings, with each offset mapped from the source onto [renderedText]. - * - * The mapping is a forward scan rather than a lookup: heading text can repeat, and a cursor that - * only moves forward keeps the second "Usage" pointing at the second one. Where the text cannot - * be found the source offset is used, which is wrong by however much markup the renderer ate — - * but it is monotonic, so the table of contents still leads down the document. - */ -private fun parseHeadings(markdown: String, renderedText: String?): List { - val headings = mutableListOf() - var sourceOffset = 0 - var renderedCursor = 0 - markdown.lineSequence().forEach { line -> - val lineStart = sourceOffset - sourceOffset += line.length + 1 - val heading = markdownHeading(line) ?: return@forEach - val found = renderedText?.indexOf(heading.text, renderedCursor) ?: -1 - val offset = when { - renderedText.isNullOrEmpty() -> lineStart - found >= 0 -> { - renderedCursor = (found + heading.text.length).coerceAtMost(renderedText.length) - found - } - else -> lineStart.coerceAtMost(renderedText.length - 1) - } - headings.add(heading.copy(offset = offset)) - } - return headings -} - -/** The heading on this line, with [HeadingItem.offset] left for [parseHeadings] to fill in. */ -private fun markdownHeading(line: String): HeadingItem? { - val trimmed = line.trimStart() - if (!trimmed.startsWith("#")) return null - val level = trimmed.takeWhile { it == '#' }.length - if (level !in 1..MAX_TOC_LEVEL) return null - val text = trimmed.drop(level).trim() - if (text.isBlank()) return null - return HeadingItem(text = text, level = level, offset = 0) -} diff --git a/app/src/main/java/com/markreader/ui/reader/ReaderContentView.kt b/app/src/main/java/com/markreader/ui/reader/ReaderContentView.kt index 0fc9a9d..5e71003 100644 --- a/app/src/main/java/com/markreader/ui/reader/ReaderContentView.kt +++ b/app/src/main/java/com/markreader/ui/reader/ReaderContentView.kt @@ -18,9 +18,7 @@ import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.widthIn import androidx.compose.runtime.Composable import androidx.compose.runtime.State -import androidx.compose.runtime.getValue import androidx.compose.runtime.remember -import androidx.compose.runtime.rememberUpdatedState import androidx.compose.runtime.snapshots.Snapshot import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier @@ -30,7 +28,6 @@ import androidx.compose.ui.viewinterop.AndroidView import com.markreader.data.CodeFontPreference import com.markreader.data.ReadingFontPreference import com.markreader.data.TextAlignmentPreference -import com.markreader.format.HeadingItem import com.markreader.format.ReaderContent /** @@ -103,8 +100,6 @@ fun RenderedTextView( onScrollChanged: (scrollY: Int, maxScrollY: Int) -> Unit, onScrollExtentChanged: (maxScrollY: Int) -> Unit, onScrollConsumed: () -> Unit, - headings: List, - onActiveHeadingChanged: (Int) -> Unit, isCodeBlockWrapEnabled: Boolean, selectionHighlightColor: Int, fontSizeSp: Float, @@ -162,9 +157,6 @@ fun RenderedTextView( codeBlockBackgroundColor = codeBlockBackgroundColor ) } - val currentHeadings by rememberUpdatedState(headings) - val onActiveHeadingChangedState by rememberUpdatedState(onActiveHeadingChanged) - Box( modifier = Modifier .fillMaxSize() @@ -211,25 +203,6 @@ fun RenderedTextView( } setOnScrollChangeListener { _, _, scrollY, _, _ -> onScrollChanged(scrollY, computeMaxScrollY(this)) - val list = currentHeadings - if (list.isEmpty()) return@setOnScrollChangeListener - val firstChild = getChildAt(0) ?: return@setOnScrollChangeListener - if (firstChild is LinearLayout) { - onActiveHeadingChangedState( - findActiveHeadingInSplit( - firstChild, controller.splitBoundaries, list, scrollY - ) - ) - } else { - val tv = firstChild as? TextView - ?: return@setOnScrollChangeListener - val layout = tv.layout ?: return@setOnScrollChangeListener - if (layout.text.isEmpty()) return@setOnScrollChangeListener - val y = scrollY + tv.paddingTop - onActiveHeadingChangedState( - findActiveHeadingIndex(layout.lineMetrics(), list, y) - ) - } } // Report scroll extent whenever content lays out so reading // progress is available before the first scroll event. This @@ -611,31 +584,6 @@ fun RenderedTextView( onScrollConsumed() } } - - // Heading tracking - val curChild = scrollView.getChildAt(0) - if (currentHeadings.isNotEmpty()) { - if (curChild is LinearLayout) { - onActiveHeadingChangedState( - findActiveHeadingInSplit( - curChild, controller.splitBoundaries, - currentHeadings, scrollView.scrollY - ) - ) - } else { - val tv = when (curChild) { - is TextView -> curChild - else -> null - } - val layout = tv?.layout - if (layout != null && layout.text.isNotEmpty()) { - val y = scrollView.scrollY + (tv?.paddingTop ?: 0) - onActiveHeadingChangedState( - findActiveHeadingIndex(layout.lineMetrics(), currentHeadings, y) - ) - } - } - } }, onRelease = { scrollView -> controller.extentListener?.let { listener -> diff --git a/app/src/main/java/com/markreader/ui/reader/ScrollAnchor.kt b/app/src/main/java/com/markreader/ui/reader/ScrollAnchor.kt index e5156fc..7b6e790 100644 --- a/app/src/main/java/com/markreader/ui/reader/ScrollAnchor.kt +++ b/app/src/main/java/com/markreader/ui/reader/ScrollAnchor.kt @@ -7,7 +7,6 @@ import android.widget.HorizontalScrollView import android.widget.LinearLayout import android.widget.ScrollView import android.widget.TextView -import com.markreader.format.HeadingItem /** * The part of a laid-out [Layout] the anchor math reads. @@ -129,67 +128,6 @@ internal fun scrollYForOffsetInSplit( internal fun scrollYForOffsetInText(lines: LineMetrics, paddingTop: Int, offset: Int): Int = lines.getLineTop(lines.getLineForOffset(offset)) + paddingTop -/** - * Index of the last heading whose line starts at or above [y], or -1 when the - * viewport sits above the first one. Headings are ordered by offset, so this is - * a binary search. - */ -internal fun findActiveHeadingIndex( - lines: LineMetrics, - headings: List, - y: Int -): Int { - var low = 0 - var high = headings.lastIndex - var result = -1 - val textLength = lines.textLength - while (low <= high) { - val mid = (low + high) ushr 1 - val offset = headings[mid].offset.coerceIn(0, textLength - 1) - val line = lines.getLineForOffset(offset) - val top = lines.getLineTop(line) - if (top <= y) { - result = mid - low = mid + 1 - } else { - high = mid - 1 - } - } - return result -} - -/** - * The same lookup across a split container. Segments are laid out independently, - * so there is no single monotonic line index to binary search — each segment's - * headings are checked against its own layout. - */ -internal fun findActiveHeadingInSplit( - boundaries: List, - childCount: Int, - headings: List, - adjustedY: Int, - placementAt: (Int) -> SegmentPlacement -): Int { - if (headings.isEmpty() || boundaries.isEmpty()) return -1 - var result = -1 - for (i in 0 until childCount.coerceAtMost(boundaries.size)) { - val placement = placementAt(i) - val lines = placement.lines ?: continue - val (segStart, segEnd, _) = boundaries[i] - for (h in headings.indices) { - val offset = headings[h].offset - if (offset < segStart || offset >= segEnd) continue - val localOffset = (offset - segStart).coerceIn(0, lines.textLength - 1) - val line = lines.getLineForOffset(localOffset) - val top = placement.top + lines.getLineTop(line) - if (top <= adjustedY) { - result = h - } - } - } - return result -} - // ---------- View adapters ---------- internal fun segmentPlacement(view: View): SegmentPlacement { @@ -255,18 +193,6 @@ internal fun resolveScrollY( return scrollYForOffsetInText(lines, tv.paddingTop, offset) } -internal fun findActiveHeadingInSplit( - container: LinearLayout, - boundaries: List, - headings: List, - scrollY: Int -): Int = findActiveHeadingInSplit( - boundaries = boundaries, - childCount = container.childCount, - headings = headings, - adjustedY = scrollY + container.paddingTop -) { segmentPlacement(container.getChildAt(it)) } - internal fun computeMaxScrollY(scrollView: ScrollView): Int { val child = scrollView.getChildAt(0) ?: return 0 return (child.height + scrollView.paddingTop + scrollView.paddingBottom - scrollView.height) diff --git a/app/src/main/java/com/markreader/ui/reader/WebViewReader.kt b/app/src/main/java/com/markreader/ui/reader/WebViewReader.kt index 4901b39..b608797 100644 --- a/app/src/main/java/com/markreader/ui/reader/WebViewReader.kt +++ b/app/src/main/java/com/markreader/ui/reader/WebViewReader.kt @@ -21,8 +21,11 @@ import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.unit.Dp import androidx.compose.ui.unit.dp import androidx.compose.ui.viewinterop.AndroidView +import com.markreader.webview.HeadingItem +import com.markreader.webview.PageCommand import com.markreader.webview.RendererSettings import com.markreader.webview.WebViewDocumentHost +import kotlinx.coroutines.flow.Flow /** * The reader surface for [com.markreader.format.ReaderContent.Markdown]: the shared renderer, @@ -39,6 +42,11 @@ import com.markreader.webview.WebViewDocumentHost * WebView makes materially harder than the native container that design was written for — is * planned separately. * + * Search and the table of contents cross here in both directions, because both belong to the + * rendering and the rendering is in the page: requests go in through [commands], and the page + * reports its outline, the heading being read and its match count back out. Compose draws all + * three — the WebView draws no chrome of its own. + * * The host owns a WebView, which outlives its Activity if nobody says otherwise, so it is * destroyed on dispose and the document is dropped from the path space with it. */ @@ -58,6 +66,20 @@ fun WebViewReader( * way. Carries the WebView version when known. */ onRendererUnavailable: (webViewVersion: String?) -> Unit, + /** + * Search and outline requests, from the chrome that draws them. + * + * A flow rather than parameters because these are requests, not state: "next match" has no + * value to hold. Collected for as long as this reader is composed, which is also as long as + * the page it drives exists. + */ + commands: Flow, + /** The document's outline, as the page rendered it. */ + onOutlineChange: (List) -> Unit, + /** The heading being read, by slug, or null in a document with no headings. */ + onActiveHeadingChange: (slug: String?) -> Unit, + /** Which match the page's find is on, and how many there are. An index of -1 means none. */ + onFindStateChange: (index: Int, total: Int) -> Unit, modifier: Modifier = Modifier, /** * Chrome drawn over the reader. Pushed into the page as document padding rather than applied @@ -74,13 +96,21 @@ fun WebViewReader( // length: this is written from a WebView callback and read from another, and a snapshot write // from either would invalidate the composition on every document that renders. val restore = remember { ScrollRestore() } + // The host is remembered, so it would otherwise hold whichever callbacks the first + // composition happened to pass. val unavailable by rememberUpdatedState(onRendererUnavailable) + val outlineChanged by rememberUpdatedState(onOutlineChange) + val activeHeadingChanged by rememberUpdatedState(onActiveHeadingChange) + val findStateChanged by rememberUpdatedState(onFindStateChange) val host = remember { WebViewDocumentHost( context, scope, onDocumentRendered = { restore.run() }, onRendererUnavailable = { unavailable(it) }, + onOutlineChanged = { outlineChanged(it) }, + onActiveHeadingChanged = { activeHeadingChanged(it) }, + onFindStateChanged = { index, total -> findStateChanged(index, total) }, ) } restore.webView = host.webView @@ -112,6 +142,10 @@ fun WebViewReader( host.applyContentInset(contentTopInset.value) } + LaunchedEffect(host, commands) { + commands.collect { command -> host.send(command) } + } + Box( modifier = modifier .fillMaxSize() diff --git a/app/src/main/java/com/markreader/ui/screens/EditorScreen.kt b/app/src/main/java/com/markreader/ui/screens/EditorScreen.kt index fe7a35e..56506d9 100644 --- a/app/src/main/java/com/markreader/ui/screens/EditorScreen.kt +++ b/app/src/main/java/com/markreader/ui/screens/EditorScreen.kt @@ -332,8 +332,6 @@ fun EditorScreen( onScrollChanged = { _, _ -> }, onScrollExtentChanged = {}, onScrollConsumed = {}, - headings = emptyList(), - onActiveHeadingChanged = {}, isCodeBlockWrapEnabled = true, selectionHighlightColor = if (previewIsDark) 0x99FFD54F.toInt() else 0x994285F4.toInt(), fontSizeSp = prefs.fontSizeSp, diff --git a/app/src/main/java/com/markreader/ui/screens/ViewerChoices.kt b/app/src/main/java/com/markreader/ui/screens/ViewerChoices.kt index a5d2b84..d3b26ad 100644 --- a/app/src/main/java/com/markreader/ui/screens/ViewerChoices.kt +++ b/app/src/main/java/com/markreader/ui/screens/ViewerChoices.kt @@ -5,7 +5,7 @@ import com.markreader.R import com.markreader.format.ReaderContent /** - * The three decisions the viewer makes about *what* to show, kept out of the composable so they + * The four decisions the viewer makes about *what* to show, kept out of the composable so they * can be tested. * * They are here rather than inline because one of their inputs — a WebView that cannot run the @@ -19,7 +19,9 @@ import com.markreader.format.ReaderContent * * @param rendererUnavailable the WebView never started the renderer, so a `Markdown` document has * nothing to draw it *with*. Raw mode needs no WebView, which is why the source is the fallback - * rather than a second native renderer kept alive for the purpose. + * rather than a second native renderer kept alive for the purpose. It shares raw mode's branch + * rather than having one of its own: the source is on screen either way, so a search over the + * source has to be able to highlight it either way. */ fun readerContentFor( isRawMode: Boolean, @@ -28,11 +30,27 @@ fun readerContentFor( rendered: ReaderContent?, rendererUnavailable: Boolean, ): ReaderContent = when { - isRawMode -> rawHighlighted ?: ReaderContent.Spans(rawText) - rendererUnavailable -> ReaderContent.Spans(rawText) + isRawMode || rendererUnavailable -> rawHighlighted ?: ReaderContent.Spans(rawText) else -> rendered ?: ReaderContent.Spans(rawText) } +/** + * Whether what is on screen is the page's own rendering. + * + * The one question search and the table of contents both ask, for the same reason: both index a + * rendering, and only one of the two reader surfaces keeps its rendering out here. Where the page + * is drawing, a query goes to the page's find and an outline is available; where a `TextView` is, + * the query is a span pass over text this side holds and there is no outline to offer. + * + * True exactly when [readerContentFor] answers [ReaderContent.Markdown], which the tests assert + * rather than assume — the two are read for different reasons and must not drift. + */ +fun isDrawnByPage( + isRawMode: Boolean, + rendered: ReaderContent?, + rendererUnavailable: Boolean, +): Boolean = !isRawMode && !rendererUnavailable && rendered is ReaderContent.Markdown + /** * What the chrome calls the current view. * diff --git a/app/src/main/java/com/markreader/ui/screens/ViewerScreen.kt b/app/src/main/java/com/markreader/ui/screens/ViewerScreen.kt index 52d51a7..f40674d 100644 --- a/app/src/main/java/com/markreader/ui/screens/ViewerScreen.kt +++ b/app/src/main/java/com/markreader/ui/screens/ViewerScreen.kt @@ -118,12 +118,12 @@ import com.markreader.ui.components.GroupOuterRadius import com.markreader.ui.components.SegmentPosition import com.markreader.ui.components.segmentPositionFor import com.markreader.ui.components.segmentShape -import com.markreader.format.HeadingItem import com.markreader.format.ReaderContent import com.markreader.ui.export.ExportManager import com.markreader.ui.resolve import com.markreader.ui.reader.RenderedTextView import com.markreader.ui.reader.WebViewReader +import com.markreader.webview.HeadingItem import com.markreader.webview.rendererSettingsFor import kotlin.math.roundToInt @@ -246,24 +246,18 @@ fun ViewerScreen( } else { stringResource(R.string.untitled) } - // Set once the WebView reports it cannot run the renderer. Held for the session rather than - // per document: a WebView too old for the renderer will not become new enough for the next - // one, and retrying would cost every document a ten-second wait before falling back. - var rendererUnavailable by rememberSaveable { mutableStateOf(false) } - var rendererWebViewVersion by rememberSaveable { mutableStateOf(null) } - val viewModeLabel = stringResource( viewModeLabelFor( hasRawView = uiState.capabilities.hasRawView, isRawMode = uiState.viewMode == ViewMode.Raw, formatNameRes = uiState.formatNameRes, - rendererUnavailable = rendererUnavailable + rendererUnavailable = uiState.rendererUnavailable ) ) val canToggleViewMode = canToggleViewMode( hasRawView = uiState.capabilities.hasRawView, hasRendered = uiState.rendered != null, - rendererUnavailable = rendererUnavailable + rendererUnavailable = uiState.rendererUnavailable ) val dynamicLightScheme = remember(context, prefs.useDynamicColors) { @@ -362,14 +356,15 @@ fun ViewerScreen( rawText = uiState.rawText, rawHighlighted = uiState.rawHighlighted, rendered = uiState.rendered, - rendererUnavailable = rendererUnavailable + rendererUnavailable = uiState.rendererUnavailable + ) + // Search runs on both surfaces, so nothing gates the search button. The outline only exists + // where the page is drawing: it is a list of slugs, and only the page can act on a slug. + val canShowOutline = uiState.capabilities.hasHeadings && isDrawnByPage( + isRawMode = uiState.viewMode == ViewMode.Raw, + rendered = uiState.rendered, + rendererUnavailable = uiState.rendererUnavailable ) - // Both features exist today and neither survived the swap to the WebView — rebuilding them - // in the page, where the rendering is, is planned. Until then the controls are hidden rather than left - // to report nothing: raw mode still searches, and a table of contents built from source - // offsets would scroll to the wrong place. Deleting this `&&` is most of what re-enables - // them. - val isContentIndexable = readerContent is ReaderContent.Spans Scaffold( // Matches the reading surface. The content now runs full height under the @@ -481,7 +476,7 @@ fun ViewerScreen( } }, navigationIcon = { - if (uiState.capabilities.hasHeadings && isContentIndexable) { + if (canShowOutline) { FilledTonalIconButton( onClick = { haptics.performHapticFeedback(HapticFeedbackType.Confirm) @@ -500,24 +495,22 @@ fun ViewerScreen( } }, actions = { - if (isContentIndexable) { - FilledTonalIconButton( - onClick = { - haptics.performHapticFeedback(HapticFeedbackType.Confirm) - viewModel.onSearchToggled() - }, - colors = IconButtonDefaults.filledTonalIconButtonColors( - containerColor = chromeColors.tonalContainer, - contentColor = chromeColors.content - ) - ) { - Icon( - imageVector = Icons.Rounded.Search, - contentDescription = stringResource(R.string.viewer_search) - ) - } - Spacer(modifier = Modifier.width(8.dp)) + FilledTonalIconButton( + onClick = { + haptics.performHapticFeedback(HapticFeedbackType.Confirm) + viewModel.onSearchToggled() + }, + colors = IconButtonDefaults.filledTonalIconButtonColors( + containerColor = chromeColors.tonalContainer, + contentColor = chromeColors.content + ) + ) { + Icon( + imageVector = Icons.Rounded.Search, + contentDescription = stringResource(R.string.viewer_search) + ) } + Spacer(modifier = Modifier.width(8.dp)) FilledTonalIconButton( onClick = { haptics.performHapticFeedback(HapticFeedbackType.Confirm) @@ -799,10 +792,10 @@ fun ViewerScreen( // container, so it takes the inset itself and the reader starts below // it. Rare enough that losing the overlay in that case is fine. val warningMessage = when { - !rendererUnavailable -> uiState.warningMessage?.resolve() - rendererWebViewVersion != null -> stringResource( + !uiState.rendererUnavailable -> uiState.warningMessage?.resolve() + uiState.rendererWebViewVersion != null -> stringResource( R.string.viewer_warning_webview_unavailable_version, - rendererWebViewVersion.orEmpty() + uiState.rendererWebViewVersion.orEmpty() ) else -> stringResource(R.string.viewer_warning_webview_unavailable) } @@ -830,10 +823,11 @@ fun ViewerScreen( contentTopInset = if (hasWarning) 0.dp else chromeTopInset, savedScrollY = savedScrollY, onScroll = viewModel::onScrollPositionChanged, - onRendererUnavailable = { version -> - rendererWebViewVersion = version - rendererUnavailable = true - } + onRendererUnavailable = viewModel::onRendererUnavailable, + commands = viewModel.pageCommands, + onOutlineChange = viewModel::onOutlineChanged, + onActiveHeadingChange = viewModel::onActiveHeadingChanged, + onFindStateChange = viewModel::onFindStateChanged ) is ReaderContent.Spans, is ReaderContent.Table -> RenderedTextView( @@ -846,8 +840,6 @@ fun ViewerScreen( onScrollChanged = viewModel::onScrollPositionChanged, onScrollExtentChanged = viewModel::onScrollExtentChanged, onScrollConsumed = viewModel::onScrollConsumed, - headings = uiState.headings, - onActiveHeadingChanged = viewModel::onActiveHeadingChanged, isCodeBlockWrapEnabled = isCodeBlockWrapEnabled, selectionHighlightColor = selectionHighlightColor, fontSizeSp = prefs.fontSizeSp, @@ -931,9 +923,13 @@ fun ViewerScreen( ) } else { val tocListState = rememberLazyListState() + // Opens on the section being read, one row up so it has context above it. + val activeHeadingIndex = uiState.headings.indexOfFirst { + it.slug == uiState.activeHeadingSlug + } LaunchedEffect(Unit) { - if (uiState.activeHeadingIndex > 0) { - tocListState.scrollToItem(uiState.activeHeadingIndex - 1) + if (activeHeadingIndex > 0) { + tocListState.scrollToItem(activeHeadingIndex - 1) } } LazyColumn( @@ -945,11 +941,11 @@ fun ViewerScreen( TocHeadingRow( heading = heading, position = segmentPositionFor(index, uiState.headings.size), - isActive = index == uiState.activeHeadingIndex, + isActive = index == activeHeadingIndex, colors = chromeColors, onClick = { haptics.performHapticFeedback(HapticFeedbackType.Confirm) - viewModel.onHeadingSelected(heading.offset) + viewModel.onHeadingSelected(heading) isTocVisible = false } ) diff --git a/app/src/main/java/com/markreader/ui/screens/ViewerViewModel.kt b/app/src/main/java/com/markreader/ui/screens/ViewerViewModel.kt index 5fc0992..390ea04 100644 --- a/app/src/main/java/com/markreader/ui/screens/ViewerViewModel.kt +++ b/app/src/main/java/com/markreader/ui/screens/ViewerViewModel.kt @@ -22,11 +22,12 @@ import com.markreader.file.DocumentTypeResolver import com.markreader.format.DocumentFormat import com.markreader.format.FormatCapabilities import com.markreader.format.FormatRegistry -import com.markreader.format.HeadingItem import com.markreader.format.ReaderContent import com.markreader.format.RenderTheme import com.markreader.ui.UiMessage import com.markreader.ui.reader.SearchHighlightSpan +import com.markreader.webview.HeadingItem +import com.markreader.webview.PageCommand import com.markreader.R import com.markreader.file.DocumentSource import com.markreader.file.FileTooLargeException @@ -36,9 +37,13 @@ import com.markreader.file.tryTakePersistablePermission import kotlinx.coroutines.CancellationException import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job +import kotlinx.coroutines.channels.BufferOverflow +import kotlinx.coroutines.channels.Channel +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.flow.receiveAsFlow import kotlinx.coroutines.launch import kotlinx.coroutines.withContext @@ -59,8 +64,10 @@ data class ViewerUiState( val isEmptyFile: Boolean = false, val needsPermission: Boolean = false, val viewMode: ViewMode = ViewMode.Rendered, + /** The outline, as the page rendered it. Empty on every surface but the page. */ val headings: List = emptyList(), - val activeHeadingIndex: Int = -1, + /** The heading being read, by slug. Null before the page reports one, and in a flat document. */ + val activeHeadingSlug: String? = null, val isSearchActive: Boolean = false, val searchQuery: String = "", val searchMatchCount: Int = 0, @@ -70,7 +77,17 @@ data class ViewerUiState( /** What the chrome may offer, decided by the document's format rather than by its type. */ val capabilities: FormatCapabilities = FormatCapabilities(), /** Shown in place of a view mode where [capabilities] says there is no raw view. */ - @StringRes val formatNameRes: Int = R.string.viewer_subtitle_plain_text + @StringRes val formatNameRes: Int = R.string.viewer_subtitle_plain_text, + /** + * This WebView cannot run the shared renderer, so the source is drawn instead. + * + * Held for the session rather than per document: a WebView too old for the renderer will not + * become new enough for the next one, and retrying would cost every document a ten-second + * wait before falling back. + */ + val rendererUnavailable: Boolean = false, + /** The WebView's version, when it was known, for a warning worth reading. */ + val rendererWebViewVersion: String? = null ) class ViewerViewModel( @@ -106,6 +123,23 @@ class ViewerViewModel( private val _scrollToOffset = MutableStateFlow(null) val scrollToOffset: StateFlow = _scrollToOffset.asStateFlow() + /** + * Search and outline requests bound for the page. + * + * A channel and not a [StateFlow], because these are requests and not state: "next match" has + * no value to hold, and three of them are three requests. It is also why they survive the + * WebView being rebuilt — which every toggle into raw mode and back does — instead of being + * lost, or replayed at whatever page comes next forever. + * + * Drops the oldest when it fills. A backlog that deep means the page is not collecting, and + * in that state the newest request is the only one still worth delivering. + */ + private val pageCommandQueue = Channel( + capacity = PAGE_COMMAND_BUFFER, + onBufferOverflow = BufferOverflow.DROP_OLDEST + ) + val pageCommands: Flow = pageCommandQueue.receiveAsFlow() + private val _scrollY = MutableStateFlow(0) val scrollY: StateFlow = _scrollY.asStateFlow() @@ -148,15 +182,19 @@ class ViewerViewModel( rawHighlighted = null, rendered = null, headings = emptyList(), - activeHeadingIndex = -1, + activeHeadingSlug = null, isSearchActive = false, searchQuery = "", searchMatchCount = 0, searchMatchIndex = 0 ) + // The page's find state outlives the document it was run against, so a new document + // would otherwise open with the previous one's query still painted in it. + send(PageCommand.CloseFind) + loadingJob = viewModelScope.launch { - kotlinx.coroutines.delay(300) + kotlinx.coroutines.delay(LOADING_INDICATOR_DELAY_MS) if (_uiState.value.isLoading) { _uiState.value = _uiState.value.copy(isLoadingVisible = true) } @@ -261,7 +299,6 @@ class ViewerViewModel( return@launch } - val headings = format.headings(markdown, loaded) loadingJob?.cancel() baseRendered = loaded renderCache[RenderCacheKey(documentType, theme)] = loaded @@ -274,8 +311,6 @@ class ViewerViewModel( rendered = loaded, errorMessage = null, warningMessage = null, - headings = headings, - activeHeadingIndex = clampActiveHeadingIndex(_uiState.value.activeHeadingIndex, headings), documentType = documentType, capabilities = format.capabilities, formatNameRes = format.nameRes @@ -296,19 +331,34 @@ class ViewerViewModel( ViewMode.Rendered } _uiState.value = _uiState.value.copy(viewMode = next) - applySearchQuery(_uiState.value.searchQuery) + resyncSearch() } + // ------------------------------------------------------------------------- search + // + // A search runs on whichever surface is drawing, and the two are not alike. Where the page is + // drawing, it owns the match set — matches are DOM ranges over a rendering only it has — so + // the query goes in and a count comes back as an event. Where a TextView is, the matches are + // offsets into a string held here, painted with spans. Everything below is that fork, and + // [usesPageFind] is the only place it is decided. + fun onSearchToggled() { val active = !_uiState.value.isSearchActive + val query = if (active) _uiState.value.searchQuery else "" _uiState.value = _uiState.value.copy( isSearchActive = active, - searchQuery = if (active) _uiState.value.searchQuery else "", + searchQuery = query, searchMatchCount = 0, searchMatchIndex = 0 ) - if (!active) { - searchJob?.cancel() + searchJob?.cancel() + if (active) { + // The bar keeps its text when it closes, so re-opening it re-runs that query rather + // than showing a stale count against a document nothing has searched. + runSearch(query) + } else if (usesPageFind) { + send(PageCommand.CloseFind) + } else { applySearchQuery("") } } @@ -320,40 +370,138 @@ class ViewerViewModel( ) searchJob?.cancel() if (query.isBlank()) { - applySearchQuery(query) + runSearch(query) return } - // Debounced: the highlight pass rebuilds a span per match over the whole - // document, which is too heavy to run per keystroke on large files. + // Debounced on both surfaces, for the same reason on each: the span pass rebuilds a span + // per match over the whole document, and the page's walks its whole DOM for them. searchJob = viewModelScope.launch { - kotlinx.coroutines.delay(200) - applySearchQuery(query) - // Bring the first match into view; without this, a query whose - // matches are all off-screen gives no visible feedback. - _matchOffsets.firstOrNull()?.let { _scrollToOffset.value = it } + kotlinx.coroutines.delay(SEARCH_DEBOUNCE_MS) + runSearch(query) } } - fun onNextMatch() { + fun onNextMatch() = stepMatch(1) + + fun onPreviousMatch() = stepMatch(-1) + + /** The reader picked a heading out of the table of contents. */ + fun onHeadingSelected(heading: HeadingItem) { + send(PageCommand.Reveal(heading.slug)) + } + + /** + * The page's find result: which match is current, and how many there are. An index of -1 + * means there are none, which the search bar shows as its own no-matches state. + * + * Deliberately not filtered against the query in the bar. The page only ever answers + * something this side asked it, and in order, so the last answer is the current one — the + * same guarantee the debounced span pass relies on. + */ + fun onFindStateChanged(index: Int, total: Int) { + _uiState.value = _uiState.value.copy( + searchMatchCount = total, + searchMatchIndex = index.coerceAtLeast(0) + ) + } + + /** Runs [query] on whichever surface is drawing. */ + private fun runSearch(query: String) { + if (usesPageFind) { + // The page reveals the first match itself, and reports the count back. + send(PageCommand.Find(query)) + return + } + applySearchQuery(query) + // Bring the first match into view; without this, a query whose matches are all + // off-screen gives no visible feedback. + _matchOffsets.firstOrNull()?.let { _scrollToOffset.value = it } + } + + /** + * Moves to another match, wrapping at either end. + * + * On the page the index is not ours to compute: it holds the match set, so it steps and says + * where it landed. Out here the offsets are already known and the index is arithmetic. + */ + private fun stepMatch(delta: Int) { + if (usesPageFind) { + send(PageCommand.StepFind(delta)) + return + } val count = _uiState.value.searchMatchCount if (count == 0) return - val next = (_uiState.value.searchMatchIndex + 1) % count + val next = ((_uiState.value.searchMatchIndex + delta) % count + count) % count _uiState.value = _uiState.value.copy(searchMatchIndex = next) _scrollToOffset.value = _matchOffsets.getOrNull(next) applySearchQuery(_uiState.value.searchQuery) } - fun onPreviousMatch() { - val count = _uiState.value.searchMatchCount - if (count == 0) return - val prev = if (_uiState.value.searchMatchIndex - 1 < 0) count - 1 else _uiState.value.searchMatchIndex - 1 - _uiState.value = _uiState.value.copy(searchMatchIndex = prev) - _scrollToOffset.value = _matchOffsets.getOrNull(prev) - applySearchQuery(_uiState.value.searchQuery) + /** + * Re-runs a live search after the surface underneath it changed — a view-mode toggle, or a + * renderer that turned out not to start. + * + * Neither surface can read the other's result, so the one arriving is given the query again + * from scratch. Nothing to do while the bar is closed, because then there is no query. + */ + private fun resyncSearch() { + if (!_uiState.value.isSearchActive) return + val query = _uiState.value.searchQuery + _uiState.value = _uiState.value.copy(searchMatchCount = 0, searchMatchIndex = 0) + if (usesPageFind) { + // Drop the spans the TextView was painting; they index text that is no longer drawn. + applySearchQuery("") + } + // Nothing is sent to close the find on the way out: leaving the page destroys it, and + // whatever WebView replaces it starts with no find at all. + runSearch(query) + } + + /** + * Whether a search goes to the page's own find. + * + * Asked of [readerContentFor]'s own decision, through [isDrawnByPage], so the two cannot + * disagree: a document searched by the page is exactly a document the page is drawing. + */ + private val usesPageFind: Boolean + get() = isDrawnByPage( + isRawMode = _uiState.value.viewMode == ViewMode.Raw, + rendered = baseRendered, + rendererUnavailable = _uiState.value.rendererUnavailable + ) + + private fun send(command: PageCommand) { + pageCommandQueue.trySend(command) + } + + // ------------------------------------------------------------- the page's own reports + + /** The document's outline, as the page rendered it. Re-reported on every render. */ + fun onOutlineChanged(outline: List) { + if (_uiState.value.headings == outline) return + _uiState.value = _uiState.value.copy(headings = outline) } - fun onHeadingSelected(offset: Int) { - _scrollToOffset.value = offset + fun onActiveHeadingChanged(slug: String?) { + if (_uiState.value.activeHeadingSlug == slug) return + _uiState.value = _uiState.value.copy(activeHeadingSlug = slug) + } + + /** + * This WebView never started the renderer, so `Markdown` has nothing to draw it with and the + * source is drawn instead. See [ViewerUiState.rendererUnavailable] for why it is not cleared + * per document. + */ + fun onRendererUnavailable(webViewVersion: String?) { + if (_uiState.value.rendererUnavailable) return + _uiState.value = _uiState.value.copy( + rendererUnavailable = true, + rendererWebViewVersion = webViewVersion, + headings = emptyList(), + activeHeadingSlug = null + ) + // A live search was just left holding a surface that no longer exists. + resyncSearch() } fun onScrollPositionChanged(y: Int, maxY: Int) { @@ -380,13 +528,6 @@ class ViewerViewModel( _scrollToOffset.value = null } - fun onActiveHeadingChanged(index: Int) { - val clamped = clampActiveHeadingIndex(index, _uiState.value.headings) - if (_uiState.value.activeHeadingIndex != clamped) { - _uiState.value = _uiState.value.copy(activeHeadingIndex = clamped) - } - } - fun onReadingSurfaceModeChanged(isDarkSurface: Boolean) { if (renderDarkModeOverride == isDarkSurface) return renderDarkModeOverride = isDarkSurface @@ -543,19 +684,26 @@ class ViewerViewModel( private fun applySearchQuery(query: String) { val currentState = _uiState.value - val activeViewMode = currentState.viewMode - // Search is span-based: it counts matches in a string and paints them with spans. That - // is a requirement of this implementation, not of the reader, so the narrowing lives - // here rather than at the format boundary — a rendered document that is not text has - // no offsets to walk and gets no matches. The rendered view's search is planned to move - // to the WebView's own find-in-page, leaving this path to raw mode. - val renderedSpans = (baseRendered as? ReaderContent.Spans)?.text - val baseText = when (activeViewMode) { - ViewMode.Raw -> currentState.rawText - ViewMode.Rendered -> renderedSpans?.toString().orEmpty() - } + val drawsSource = currentState.viewMode == ViewMode.Raw || currentState.rendererUnavailable + // Search out here is span-based: it counts matches in a string and paints them with + // spans. That is a requirement of this implementation and not of the reader, so the + // narrowing lives here rather than at the format boundary — and a document the page is + // drawing never reaches this function at all, because the page searches it instead. + // + // The text searched is whatever [readerContentFor] says is on screen, asked with no + // highlighting so it answers the unmarked original. Reading `uiState.rendered` instead + // would search the *previous* query's spans. + val drawn = readerContentFor( + isRawMode = currentState.viewMode == ViewMode.Raw, + rawText = currentState.rawText, + rawHighlighted = null, + rendered = baseRendered, + rendererUnavailable = currentState.rendererUnavailable + ) + val baseSpans = (drawn as? ReaderContent.Spans)?.text - if (activeViewMode == ViewMode.Rendered && renderedSpans == null) { + if (baseSpans == null) { + // A rendering that is not text — a table — has no offsets to walk. _matchOffsets = emptyList() _uiState.value = currentState.copy( rawHighlighted = null, @@ -564,6 +712,7 @@ class ViewerViewModel( ) return } + val baseText = baseSpans.toString() if (query.isBlank()) { _matchOffsets = emptyList() @@ -582,10 +731,7 @@ class ViewerViewModel( _matchOffsets = offsets - val highlighted = when (activeViewMode) { - ViewMode.Rendered -> SpannableString(renderedSpans) - ViewMode.Raw -> SpannableString(baseText) - } + val highlighted = SpannableString(baseSpans) val prefs = _uiState.value.userPreferences val isDark = renderDarkModeOverride ?: isDarkTheme(prefs, systemDarkTheme) val activeTheme = if (isDark) prefs.readerDarkTheme else prefs.readerLightTheme @@ -643,32 +789,44 @@ class ViewerViewModel( ) } - _uiState.value = when (activeViewMode) { - ViewMode.Rendered -> _uiState.value.copy( - rendered = ReaderContent.Spans(highlighted), - rawHighlighted = null, + // Where the source is on screen the highlighting has to arrive as `rawHighlighted`, + // because that is the field [readerContentFor] draws from in both of the cases that put + // it there: raw mode, and a renderer that could not start. + _uiState.value = if (drawsSource) { + _uiState.value.copy( + rendered = baseRendered, + rawHighlighted = ReaderContent.Spans(highlighted), searchMatchCount = offsets.size, searchMatchIndex = if (offsets.isEmpty()) 0 else activeIndex ) - ViewMode.Raw -> _uiState.value.copy( - rendered = baseRendered, - rawHighlighted = ReaderContent.Spans(highlighted), + } else { + _uiState.value.copy( + rendered = ReaderContent.Spans(highlighted), + rawHighlighted = null, searchMatchCount = offsets.size, searchMatchIndex = if (offsets.isEmpty()) 0 else activeIndex ) } } - private fun clampActiveHeadingIndex(current: Int, headings: List): Int { - if (headings.isEmpty()) return -1 - if (current < 0) return -1 - return current.coerceAtMost(headings.lastIndex) - } - companion object { /** Coalesces the burst of preference emissions a single theme switch produces. */ private const val REBUILD_DEBOUNCE_MS = 50L + /** How long a query is left alone before it is run. Neither surface searches cheaply. */ + private const val SEARCH_DEBOUNCE_MS = 200L + + /** A read faster than this shows no spinner at all, rather than one that flashes. */ + private const val LOADING_INDICATOR_DELAY_MS = 300L + + /** + * How many page requests are held while no page is collecting them. + * + * Deep enough for the burst a view-mode toggle produces while the WebView is being + * rebuilt, and shallow enough that a backlog beyond it is a page that is not coming back. + */ + private const val PAGE_COMMAND_BUFFER = 16 + private fun isDarkTheme(prefs: UserPreferences, isSystemDarkTheme: Boolean): Boolean { return when (prefs.appThemeMode) { AppThemeModePreference.System -> isSystemDarkTheme diff --git a/app/src/main/java/com/markreader/webview/HeadingItem.kt b/app/src/main/java/com/markreader/webview/HeadingItem.kt new file mode 100644 index 0000000..a6f70be --- /dev/null +++ b/app/src/main/java/com/markreader/webview/HeadingItem.kt @@ -0,0 +1,19 @@ +package com.markreader.webview + +/** + * One entry in the outline the page reports. + * + * **Located by [slug], not by an offset**, and that is why this lives beside the WebView host + * rather than in `format/`. A heading's position is a fact about the rendering, and the rendering + * is inside the page — so the page names each heading with the id it gave that heading's element, + * and the only thing that can act on the name is the page. Nothing on this side can turn a slug + * into a scroll position, and nothing needs to: it hands the slug back through `revealSlug`. + * + * It used to carry a character offset into rendered text, which is what a `TextView` scrolls. + * That died with the native rendered path. + */ +data class HeadingItem( + val text: String, + val level: Int, + val slug: String +) diff --git a/app/src/main/java/com/markreader/webview/PageCommand.kt b/app/src/main/java/com/markreader/webview/PageCommand.kt new file mode 100644 index 0000000..34267d1 --- /dev/null +++ b/app/src/main/java/com/markreader/webview/PageCommand.kt @@ -0,0 +1,28 @@ +package com.markreader.webview + +/** + * Something the app asks the page to do about search or the outline. + * + * These are the only *imperative* things the reader sends into the page. Everything else it + * pushes — the document, the settings, the content inset — is state, and state is applied by + * naming its new value. "Move to the next match" is not a value: the match set only exists in the + * page, so stepping through it has to be a request, and the result comes back as an event. + * + * Delivered over a buffered channel rather than as `State`, so a request that arrives while the + * WebView is being rebuilt — every toggle into raw mode and back destroys it — is held and + * delivered to the page that replaces it, instead of being lost or replayed forever. + */ +sealed interface PageCommand { + + /** Find [query], paint every match, reveal the first. An empty query clears the find. */ + data class Find(val query: String) : PageCommand + + /** Move to another match, wrapping at either end. [delta] is 1 for next, -1 for previous. */ + data class StepFind(val delta: Int) : PageCommand + + /** The search affordance closed; drop the query and its highlights. */ + data object CloseFind : PageCommand + + /** Scroll a heading into view, named by one of the slugs the outline reported. */ + data class Reveal(val slug: String) : PageCommand +} diff --git a/app/src/main/java/com/markreader/webview/WebViewDocumentHost.kt b/app/src/main/java/com/markreader/webview/WebViewDocumentHost.kt index 05959fb..b4fee5d 100644 --- a/app/src/main/java/com/markreader/webview/WebViewDocumentHost.kt +++ b/app/src/main/java/com/markreader/webview/WebViewDocumentHost.kt @@ -24,6 +24,7 @@ import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.launch import kotlinx.coroutines.withContext import kotlinx.coroutines.withTimeoutOrNull +import org.json.JSONArray import org.json.JSONObject /** @@ -42,11 +43,15 @@ import org.json.JSONObject * `readFile` is a plain `fetch` instead of a second bridge, and [SafPathSpace] is the only thing * that has to know a path means a SAF document. * - * **What crosses into Kotlin is four methods and nothing else.** No file reading, no settings, no - * window control: `ready`, `rendered`, `openPath`, `openExternal`. `openPath` deliberately does - * not read the file on the page's side even though `readFile` could, because SAF permission, the - * encoding decision, the recents list and the resume position all live out here — a document the - * page opened by itself would be invisible to all four. + * **What crosses into Kotlin is seven methods and nothing else.** No file reading, no settings, no + * window control. Four are lifecycle and navigation — `ready`, `rendered`, `openPath`, + * `openExternal` — and three report what only the page can know, because it is the side holding + * the rendering: the document's outline, which heading is being read, and how many matches a + * search has. Compose draws its table of contents and its search bar from those three. + * + * `openPath` deliberately does not read the file on the page's side even though `readFile` could, + * because SAF permission, the encoding decision, the recents list and the resume position all + * live out here — a document the page opened by itself would be invisible to all four. * * ### The security half * @@ -92,6 +97,15 @@ class WebViewDocumentHost( * WebView's version name when it is known, for a message worth reading. */ private val onRendererUnavailable: (webViewVersion: String?) -> Unit = {}, + /** The document's outline, as the page rendered it. Re-reported on every render. */ + private val onOutlineChanged: (outline: List) -> Unit = {}, + /** The heading being read, by slug, or null in a document with no headings. */ + private val onActiveHeadingChanged: (slug: String?) -> Unit = {}, + /** + * The result of the last find: which match is current, and how many there are. [index] is -1 + * when there are none. + */ + private val onFindStateChanged: (index: Int, total: Int) -> Unit = { _, _ -> }, ) { private val space = SafPathSpace(context.contentResolver) @@ -145,19 +159,17 @@ class WebViewDocumentHost( // ------------------------------------------------------------------ the path space - /** - * Takes a picked folder into the virtual path space and returns its root, so a document in it - * can resolve its sibling images. Naming a document inside it is `"$root/$relativePath"` — - * the same arithmetic the renderer does, and all the contract permits anyone to do. - * - * The URI permission is the picker's to take; this does not. - */ - fun adoptTree(treeUri: Uri): String = space.adoptTree(treeUri) - /** * Takes a single document — from a file manager, or the share sheet — into the space and * returns its complete virtual path. Its relative references will 404, because a single-document * grant is all the app has permission to read. + * + * Single documents are all this host adopts, because they are all the reader opens. Folders + * are already `SafPathSpace`'s to adopt, and a delegation for them belongs here on the day + * something calls it — a document naming a sibling is `"$root/$relativePath"`, the same + * arithmetic the renderer does. + * + * The URI permission is the picker's to take; this does not. */ fun adoptDocument(documentUri: Uri, displayName: String): String = space.adoptDocument(documentUri, displayName) @@ -248,6 +260,32 @@ class WebViewDocumentHost( } } + // --------------------------------------------------------------- search and outline + + /** + * Asks the page for something about search or the outline. + * + * One method rather than four, because these are the requests that cannot be expressed as + * state — see [PageCommand]. Nothing comes back from any of them directly: the page owns the + * match set, since matches are DOM ranges over a rendering only it has, so what it has to say + * arrives through [onFindStateChanged] and [onOutlineChanged] instead. Those also fire again + * whenever the document is re-rendered underneath a live query. + * + * A [PageCommand.Reveal] naming nothing is ignored by the page, which is what acting on a + * stale outline looks like. + */ + fun send(command: PageCommand) { + scope.launch(Dispatchers.Main) { + pageReady.await() + when (command) { + is PageCommand.Find -> call("runFind", JSONObject().put("query", command.query)) + is PageCommand.StepFind -> call("stepFind", JSONObject().put("delta", command.delta)) + PageCommand.CloseFind -> call("closeFind") + is PageCommand.Reveal -> call("revealSlug", JSONObject().put("slug", command.slug)) + } + } + } + /** Applies reader settings. Safe before the first document; the page re-renders if it needs to. */ fun applySettings(settings: RendererSettings) { scope.launch(Dispatchers.Main) { @@ -269,14 +307,12 @@ class WebViewDocumentHost( * crosses as a *string* that the page then parses. Interpolating the JSON as an expression * would make every document's text a script fragment in this line. */ - private fun call(method: String, payload: JSONObject) { + private fun call(method: String, payload: JSONObject? = null) { if (destroyed) { return } - webView.evaluateJavascript( - "window.__mdrPage.$method(${JSONObject.quote(payload.toString())})", - null, - ) + val argument = payload?.let { JSONObject.quote(it.toString()) } ?: "" + webView.evaluateJavascript("window.__mdrPage.$method($argument)", null) } // -------------------------------------------------------------------- the WebView @@ -415,6 +451,27 @@ class WebViewDocumentHost( @JavascriptInterface fun openExternal(url: String) = fromHostPage { openExternalUrl(context, url) } + /** + * The document's outline, as JSON — the one report here that is a list rather than a + * value, and the only reason this bridge parses anything. + * + * A malformed entry is skipped rather than failing the whole outline: a table of contents + * missing one row is still navigation, and there is no user-facing way to say more. + */ + @JavascriptInterface + fun outline(json: String) = fromHostPage { onOutlineChanged(parseOutline(json)) } + + /** Empty means the document has no headings at all; see `android-host.js`. */ + @JavascriptInterface + fun activeHeading(slug: String) = fromHostPage { + onActiveHeadingChanged(slug.takeIf { it.isNotEmpty() }) + } + + @JavascriptInterface + fun findState(index: Int, total: Int) = fromHostPage { + onFindStateChanged(index, total) + } + /** * Hops to the main thread, and refuses to act on anything but the host page. * @@ -435,6 +492,33 @@ class WebViewDocumentHost( } private companion object { + + /** + * The outline, out of the array `android-host.js` stringifies. + * + * Only the three fields the table of contents draws are read. The page also reports the + * source line each heading came from, which nothing here has a use for yet — the resume + * position is planned to lean on the slug, not the line. + */ + fun parseOutline(json: String): List { + val array = runCatching { JSONArray(json) }.getOrNull() ?: return emptyList() + return (0 until array.length()).mapNotNull { headingAt(array, it) } + } + + /** Null for an entry the table of contents could not act on, which is one with no slug. */ + private fun headingAt(array: JSONArray, index: Int): HeadingItem? { + val entry = array.optJSONObject(index) ?: return null + val slug = entry.optString("slug") + if (slug.isEmpty()) { + return null + } + return HeadingItem( + text = entry.optString("text"), + level = entry.optInt("level", 1), + slug = slug, + ) + } + const val TAG = "MarkReaderWebView" /** [WebViewAssetLoader]'s default domain. Its `https` origin is the page's own. */ diff --git a/app/src/test/java/com/markreader/format/FormatRegistryTest.kt b/app/src/test/java/com/markreader/format/FormatRegistryTest.kt index 30e72e8..ab25e72 100644 --- a/app/src/test/java/com/markreader/format/FormatRegistryTest.kt +++ b/app/src/test/java/com/markreader/format/FormatRegistryTest.kt @@ -81,16 +81,6 @@ class FormatRegistryTest { assertNull(registry.formatFor(DocumentType.Binary)) } - @Test - fun `only formats with headings produce any`() { - val content = ReaderContent.Table(emptyList()) - assertEquals(emptyList(), PlainTextFormat.headings("# Heading", content)) - assertEquals( - emptyList(), - registry.formatFor(DocumentType.SourceCode("kotlin"))!!.headings("# Heading", content) - ) - } - @Test fun `invalidation reaches every format the registry has handed out`() { val recording = RecordingFormat() diff --git a/app/src/test/java/com/markreader/format/markdown/MarkdownHeadingsTest.kt b/app/src/test/java/com/markreader/format/markdown/MarkdownHeadingsTest.kt deleted file mode 100644 index 55289bd..0000000 --- a/app/src/test/java/com/markreader/format/markdown/MarkdownHeadingsTest.kt +++ /dev/null @@ -1,97 +0,0 @@ -package com.markreader.format.markdown - -import com.markreader.format.HeadingItem -import com.markreader.format.ReaderContent -import com.markreader.ui.reader.FakeSpanned -import org.junit.Assert.assertEquals -import org.junit.Test - -/** - * The table of contents, exercised through the format rather than the parser, because the offset - * mapping is the part that goes wrong and it depends on the rendering being passed in. - * - * Offsets index the *rendered* text, since that is what the reader scrolls. Markwon eats the - * `#` markers, so a source offset would be wrong by the accumulated width of every marker above - * it — which is why this mapping exists at all. - * - * All of which applies to the surfaces the app renders itself. A document drawn by the shared - * renderer has its rendering inside the page, and gets no headings from here at all. - */ -class MarkdownHeadingsTest { - - private val format = MarkdownFormat { error("no renderer is needed for these assertions") } - - /** - * Defaults the rendering to the source, for the cases that are about *which* lines are - * headings rather than where they land. Only [ReaderContent.Spans] carries offsets at all, - * so there is nothing else these could be passed. - */ - private fun headings(markdown: String, rendered: String = markdown): List = - format.headings(markdown, ReaderContent.Spans(FakeSpanned(rendered))) - - @Test - fun `levels one to three are navigation, deeper ones are not`() { - val result = headings("# One\n## Two\n### Three\n#### Four\n##### Five") - - assertEquals(listOf("One", "Two", "Three"), result.map { it.text }) - assertEquals(listOf(1, 2, 3), result.map { it.level }) - } - - @Test - fun `a heading with no text is not a heading`() { - assertEquals(emptyList(), headings("#\n## \nbody")) - } - - @Test - fun `leading whitespace does not hide a heading`() { - assertEquals(listOf("Indented"), headings(" ## Indented").map { it.text }) - } - - @Test - fun `a hash inside a line is not a heading`() { - assertEquals(emptyList(), headings("see issue #42 for detail")) - } - - @Test - fun `offsets point into the rendered text, not the source`() { - val result = headings("# Title\n\nBody text.\n\n## Section\n", "Title\n\nBody text.\n\nSection\n") - - assertEquals(listOf(0, 19), result.map { it.offset }) - } - - /** - * The mapping is a forward scan for exactly this: a cursor that only moves forward keeps the - * second "Usage" pointing at the second one rather than back at the first. - */ - @Test - fun `repeated heading text resolves to successive occurrences`() { - val result = headings("## Usage\ntext\n## Usage\n", "Usage\ntext\nUsage\n") - - assertEquals(listOf(0, 11), result.map { it.offset }) - } - - @Test - fun `a heading absent from the rendering falls back to a clamped source offset`() { - // Markwon dropped the second heading entirely; the first still maps. The fallback is the - // source offset (8) clamped into the rendering, which is shorter — scrolling past the end - // is not a thing the reader can do, so the last character is as far as it goes. - val result = headings("# Kept\n\n## Gone\n", "Kept\n\n") - - assertEquals(listOf(0, 5), result.map { it.offset }) - } - - /** - * The WebView path. Offsets index a rendering that lives in the page, so this side has none - * to give — and a table of contents built from source offsets would scroll to the wrong - * place. Absent beats wrong; rebuilding it where the rendering actually is, is planned. - */ - @Test - fun `a document rendered in the page has no headings to offer out here`() { - val source = "# One\nbody\n## Two\n" - - assertEquals( - emptyList(), - format.headings(source, ReaderContent.Markdown(source)) - ) - } -} diff --git a/app/src/test/java/com/markreader/format/plaintext/PlainTextFormatTest.kt b/app/src/test/java/com/markreader/format/plaintext/PlainTextFormatTest.kt index ae4f0af..1ef2aad 100644 --- a/app/src/test/java/com/markreader/format/plaintext/PlainTextFormatTest.kt +++ b/app/src/test/java/com/markreader/format/plaintext/PlainTextFormatTest.kt @@ -5,7 +5,6 @@ import kotlinx.coroutines.runBlocking import org.junit.Assert.assertFalse import org.junit.Assert.assertNull import org.junit.Assert.assertSame -import org.junit.Assert.assertTrue import org.junit.Test /** @@ -48,9 +47,4 @@ class PlainTextFormatTest { assertFalse(capabilities.hasFormattingToolbar) assertFalse(capabilities.isMonospaced) } - - @Test - fun `headings are not offered, whatever the document says`() { - assertTrue(PlainTextFormat.headings("# Not a heading here", load("# Not a heading here")).isEmpty()) - } } diff --git a/app/src/test/java/com/markreader/ui/reader/ScrollAnchorTest.kt b/app/src/test/java/com/markreader/ui/reader/ScrollAnchorTest.kt index da0bb48..81c0d98 100644 --- a/app/src/test/java/com/markreader/ui/reader/ScrollAnchorTest.kt +++ b/app/src/test/java/com/markreader/ui/reader/ScrollAnchorTest.kt @@ -1,6 +1,5 @@ package com.markreader.ui.reader -import com.markreader.format.HeadingItem import org.junit.Assert.assertEquals import org.junit.Assert.assertNull import org.junit.Test @@ -220,86 +219,4 @@ class ScrollAnchorTest { // 400 - 16 = 384, which falls in line 19, starting at offset 190. assertEquals(190, anchorOffsetInText(FakeLines(300), scrollY = 400, paddingTop = 16)) } - - // ---------- active heading ---------- - - private val headings = listOf( - HeadingItem("Intro", 1, 0), - HeadingItem("Middle", 2, 35), - HeadingItem("End", 2, 72) - ) - - @Test - fun `no heading is active above the first one`() { - assertEquals(-1, findActiveHeadingIndex(FakeLines(100), headings, y = -1)) - } - - @Test - fun `the heading exactly at the viewport top is active`() { - // Offsets 0, 35, 72 sit on lines 0, 3 and 7 — tops 0, 60 and 140. - assertEquals(0, findActiveHeadingIndex(FakeLines(100), headings, y = 0)) - assertEquals(0, findActiveHeadingIndex(FakeLines(100), headings, y = 59)) - assertEquals(1, findActiveHeadingIndex(FakeLines(100), headings, y = 60)) - assertEquals(1, findActiveHeadingIndex(FakeLines(100), headings, y = 139)) - assertEquals(2, findActiveHeadingIndex(FakeLines(100), headings, y = 140)) - assertEquals(2, findActiveHeadingIndex(FakeLines(100), headings, y = 10_000)) - } - - @Test - fun `a document with no headings has no active one`() { - assertEquals(-1, findActiveHeadingIndex(FakeLines(100), emptyList(), y = 500)) - } - - @Test - fun `a heading offset past the end of the text is clamped`() { - val stale = listOf(HeadingItem("Stale", 1, 5_000)) - - // Clamped to offset 99 — line 9, top 180. - assertEquals(-1, findActiveHeadingIndex(FakeLines(100), stale, y = 179)) - assertEquals(0, findActiveHeadingIndex(FakeLines(100), stale, y = 180)) - } - - @Test - fun `the split lookup picks the last heading above the viewport`() { - val splitHeadings = listOf( - HeadingItem("First", 1, 5), - HeadingItem("Second", 2, 160), - HeadingItem("Third", 2, 250) - ) - - val active = findActiveHeadingInSplit( - boundaries = boundaries, - childCount = 3, - headings = splitHeadings, - adjustedY = 320, - placementAt = placements( - measured(0, 200, 100), - table(200, 100), - measured(300, 300, 150) - ) - ) - - // "Second" is 10 into the third segment: line 1, top 300 + 20 = 320. - // "Third" is at line 10, top 500, still below the viewport. - assertEquals(1, active) - } - - @Test - fun `segments without a layout are skipped rather than guessed at`() { - val splitHeadings = listOf(HeadingItem("Only", 1, 160)) - - val active = findActiveHeadingInSplit( - boundaries = boundaries, - childCount = 3, - headings = splitHeadings, - adjustedY = 10_000, - placementAt = placements( - measured(0, 200, 100), - table(200, 100), - unmeasured(300, 300) - ) - ) - - assertEquals(-1, active) - } } diff --git a/app/src/test/java/com/markreader/ui/screens/ViewerChoicesTest.kt b/app/src/test/java/com/markreader/ui/screens/ViewerChoicesTest.kt index f201b77..6a31910 100644 --- a/app/src/test/java/com/markreader/ui/screens/ViewerChoicesTest.kt +++ b/app/src/test/java/com/markreader/ui/screens/ViewerChoicesTest.kt @@ -47,6 +47,22 @@ class ViewerChoicesTest { assertEquals(ReaderContent.Spans("# source"), content) } + @Test + fun `the fallback keeps its search highlighting, as raw mode does`() { + // The source is on screen either way, so a search over the source has to be able to + // paint it either way. It used to draw the unmarked text and report matches nobody + // could see. + val content = readerContentFor( + isRawMode = false, + rawText = "# source", + rawHighlighted = highlighted, + rendered = rendered, + rendererUnavailable = true + ) + + assertSame(highlighted, content) + } + @Test fun `raw mode keeps its search highlighting, renderer or not`() { for (unavailable in listOf(false, true)) { @@ -75,6 +91,54 @@ class ViewerChoicesTest { assertEquals(ReaderContent.Spans("# source"), content) } + // --------------------------------------------------------------- which surface is drawing + + @Test + fun `only a rendered Markdown document with a working renderer is drawn by the page`() { + assertTrue( + isDrawnByPage(isRawMode = false, rendered = rendered, rendererUnavailable = false) + ) + assertFalse( + isDrawnByPage(isRawMode = true, rendered = rendered, rendererUnavailable = false) + ) + assertFalse( + isDrawnByPage(isRawMode = false, rendered = rendered, rendererUnavailable = true) + ) + // A format that produced text, or nothing at all, is drawn by a TextView. + assertFalse( + isDrawnByPage(isRawMode = false, rendered = highlighted, rendererUnavailable = false) + ) + assertFalse( + isDrawnByPage(isRawMode = false, rendered = null, rendererUnavailable = false) + ) + } + + @Test + fun `the surface test agrees with what is drawn, in every combination`() { + // The two are read for different reasons — one routes a search, the other picks a view — + // and a disagreement between them is a search running against a surface that is not + // there. Asserted rather than assumed, across the whole input space. + for (isRawMode in listOf(false, true)) { + for (unavailable in listOf(false, true)) { + for (produced in listOf(rendered, highlighted, null)) { + val content = readerContentFor( + isRawMode = isRawMode, + rawText = "# source", + rawHighlighted = null, + rendered = produced, + rendererUnavailable = unavailable + ) + + assertEquals( + "isRawMode=$isRawMode unavailable=$unavailable rendered=$produced", + content is ReaderContent.Markdown, + isDrawnByPage(isRawMode, produced, unavailable) + ) + } + } + } + } + // ------------------------------------------------------------------- what it is called @Test diff --git a/config/detekt/baseline.xml b/config/detekt/baseline.xml index 22c1c92..859f3bd 100644 --- a/config/detekt/baseline.xml +++ b/config/detekt/baseline.xml @@ -8,7 +8,7 @@ CyclomaticComplexMethod:EditorScreen.kt$@OptIn(ExperimentalMaterial3Api::class, ExperimentalLayoutApi::class) @Composable fun EditorScreen( uriString: String?, onNavigateBack: () -> Unit, onFileSaved: () -> Unit, modifier: Modifier = Modifier, viewModel: EditorViewModel = viewModel( factory = EditorViewModel.factory( LocalContext.current.applicationContext as Application, uriString ) ) ) CyclomaticComplexMethod:Prism4jThemes.kt$Prism4jThemeOneDark$private fun colorFor(type: String): Int? CyclomaticComplexMethod:Prism4jThemes.kt$Prism4jThemeOneLight$private fun colorFor(type: String): Int? - CyclomaticComplexMethod:ReaderContentView.kt$@Composable fun RenderedTextView( content: ReaderContent, textColor: Int, padding: PaddingValues, /** * Height of the chrome the reader draws underneath, added to the content's own top * padding *inside* the ScrollView. Kept out of [padding] deliberately: [padding] insets * the viewport, which resizes the scroll container and moves `maxScrollY`, and that is * the feedback loop this avoids. */ contentTopInset: Dp = 0.dp, // Held as State, not Int: the live scroll position changes every frame while // scrolling, and taking it as a value would recompose this composable — and // re-run the AndroidView update block — on every one of those frames. It is // only ever read to restore a position, so it is read without snapshot // observation below. savedScrollY: State<Int>, scrollToOffset: Int?, onScrollChanged: (scrollY: Int, maxScrollY: Int) -> Unit, onScrollExtentChanged: (maxScrollY: Int) -> Unit, onScrollConsumed: () -> Unit, headings: List<HeadingItem>, onActiveHeadingChanged: (Int) -> Unit, isCodeBlockWrapEnabled: Boolean, selectionHighlightColor: Int, fontSizeSp: Float, lineHeight: Float, readingFont: ReadingFontPreference, codeFont: CodeFontPreference = CodeFontPreference.JetBrainsMono, isMonospaced: Boolean = false, textAlignment: TextAlignmentPreference, codeBlockBackgroundColor: Int ) + CyclomaticComplexMethod:ReaderContentView.kt$@Composable fun RenderedTextView( content: ReaderContent, textColor: Int, padding: PaddingValues, /** * Height of the chrome the reader draws underneath, added to the content's own top * padding *inside* the ScrollView. Kept out of [padding] deliberately: [padding] insets * the viewport, which resizes the scroll container and moves `maxScrollY`, and that is * the feedback loop this avoids. */ contentTopInset: Dp = 0.dp, // Held as State, not Int: the live scroll position changes every frame while // scrolling, and taking it as a value would recompose this composable — and // re-run the AndroidView update block — on every one of those frames. It is // only ever read to restore a position, so it is read without snapshot // observation below. savedScrollY: State<Int>, scrollToOffset: Int?, onScrollChanged: (scrollY: Int, maxScrollY: Int) -> Unit, onScrollExtentChanged: (maxScrollY: Int) -> Unit, onScrollConsumed: () -> Unit, isCodeBlockWrapEnabled: Boolean, selectionHighlightColor: Int, fontSizeSp: Float, lineHeight: Float, readingFont: ReadingFontPreference, codeFont: CodeFontPreference = CodeFontPreference.JetBrainsMono, isMonospaced: Boolean = false, textAlignment: TextAlignmentPreference, codeBlockBackgroundColor: Int ) CyclomaticComplexMethod:TableRenderer.kt$internal fun buildTableLayout( text: Spanned, context: Context, textColor: Int, fontSizeSp: Float, lineHeight: Float, readingFont: ReadingFontPreference, selectionHighlightColor: Int, density: Float ): TableLayout? CyclomaticComplexMethod:ViewerScreen.kt$@OptIn(ExperimentalMaterial3Api::class) @Composable fun ViewerScreen( onOpenSettings: () -> Unit, onOpenEditor: (String) -> Unit = {}, uriString: String?, fileSaved: Boolean = false, onFileSavedConsumed: () -> Unit = {} ) CyclomaticComplexMethod:ViewerTheme.kt$fun resolveReaderColors( readerLightTheme: ReaderThemePreference, readerDarkTheme: ReaderThemePreference, dynamicLightScheme: ColorScheme?, dynamicDarkScheme: ColorScheme? ): Pair<ViewerColors, ViewerColors> @@ -35,8 +35,6 @@ MagicNumber:TableRenderer.kt$75 MagicNumber:TableRenderer.kt$8 MagicNumber:ViewerViewModel.kt$ViewerViewModel$1024 - MagicNumber:ViewerViewModel.kt$ViewerViewModel$200 - MagicNumber:ViewerViewModel.kt$ViewerViewModel$300 MagicNumber:ViewerViewModel.kt$ViewerViewModel$400 MatchingDeclarationName:ExpressiveList.kt$SegmentPosition MatchingDeclarationName:ViewerTheme.kt$ViewerColors @@ -52,7 +50,6 @@ ModifierMissing:ViewerScreen.kt$ViewerScreen NestedBlockDepth:TableRenderer.kt$internal fun buildTableLayout( text: Spanned, context: Context, textColor: Int, fontSizeSp: Float, lineHeight: Float, readingFont: ReadingFontPreference, selectionHighlightColor: Int, density: Float ): TableLayout? ParameterNaming:EditorScreen.kt$onFileSaved - ParameterNaming:ReaderContentView.kt$onActiveHeadingChanged ParameterNaming:ReaderContentView.kt$onScrollChanged ParameterNaming:ReaderContentView.kt$onScrollConsumed ParameterNaming:ReaderContentView.kt$onScrollExtentChanged