Running text: paragraphs of verses with inline numbers (#12 part 3c) #115
No reviewers
Labels
No milestone
No project
No assignees
3 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set
Reference
schaefera/utStudyBIble!115
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/running-text"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Part of #12 (part 3c of the split in comment 2328).
What
A new Display setting, Running text (off by default), shows each paragraph as flowing text with small raised verse numbers inline, instead of one row per verse.
<div type="x-p">inside its pre-verse headings.Blast radius
Tier: trunk
Trunk paths touched:
sword/src/filter/osis.rs,core/src/bible*(newblock.rs,chapter.rs,reader.rs),core/src/render.rs,core/src/worker.rs,core/src/worker/session/jump.rs,qt/src/reading_model.rs,qml/pages/ReadingPane.qml,tools/e2e/cases/running-text.shTesting tier: standard (logic and UI).
Gate / rollback: the
text.runningsetting defaults to off. With it off, the pane is verse mode as before: the same StyledText delegate, now inside a Loader. To undo fully, revert this merge; the setting key is then ignored. There is no schema change.Proof
CI on
fb44892: changes, rust, audit, app, e2e and click.CI on
f8ba0c6caught a broken E2E hook:red-letterssaw 0 red verses with red on, because the delegate became a Loader and the list's children losttext.fb44892gives the rowtext: model.text. Locally,E2E_CASES="red-letters red-letters-kill switch-bible-red-letters running-text"passes 4/4: 21 verses red with it on, 0 off. Thef8ba0c6run (job 2018) is the run without the fix.cargo makepasses locally with SWORD_TEST_PATH set: fmt, clippy, tests with coverage, release check, deny and audit.cargo make app-dockerpasses. Mem-check PSS: 168028 kB normal, 169541 kB slow, against the 176000 baseline (verse mode).E2E_CASES="running-text display-settings display-spacing restore" cargo make e2e-docker: 4 passed. Therunning-textcase checks:running 22, verse 22).Fails without the change: each new test fails under a deliberate bug and passes once restored.
plain_length_drops_tags_and_counts_entities_oncelen_utf16()→ 1a_verse_block_is_the_verserunning_text_joins_verses_with_the_start_of_eachnumbers_are_inline_and_counteda_verse_without_text_starts_where_the_next_doesat + 1verse_mode_is_a_block_per_verse!runninginvertedrunning_text_breaks_at_paragraph_startspara_startinvertedrunning_text_without_paragraph_marks_is_one_blocki > fromguard droppedrunning_text_and_its_numbers_change_the_layoutlayouts_colour_the_numbers_for_the_windowrunning_text_loads_the_chapters_again_at_the_top_verseparagraph_starts_come_before_the_first_text(real BSB 13:18 markup)x-pdiv not matchedmarks_paragraph_starts(BSB Matt 13 has 18 and 24)[1])running-textrunning 28.6 verse 22, case failedPhone (arm64 click, ute2e, comments 2376, 2381 and 2394). The owner passed the checklist on
e0a9032: feel, flick, tap and looks. I re-checkedc06603aand3835f97myself:Memory: verse mode is unchanged (mem-check above). Running mode on the phone peaks at 70926 kB PSS against 70272 kB in verse mode, after 20 flings each in the same session (+0.65 MB, a TextEdit per paragraph). No new start-up work: blocks are built only in running mode, from rows already loaded.
Review
Reviewer:
Fresh-context review at high on
199e13d(review 317): 1 medium, 1 low.textreadback always differed, so the guard never applied. Fixed in6375bad; the ruler was later removed in70255da.6375bad.Re-review at medium on
6375bad(318): no findings.Medium review of 199e13d..3835f97 (319), covering the phone fixes: 2 low.
f8ba0c6.Delta review of f8ba0c6..fb44892 (the E2E hook fix): review 320, no findings.
Nothing is deferred or left open.
Review-Confidence: 75/100. A trunk change of 768 lines was reviewed at high and re-reviewed three times, with CI green and fails-without-change evidence for every new test. Review 319 missed one regression: the Loader dropped the rows'
textfor the red-letters E2E hook. CI caught it andfb44892fixed it. So trust the E2E and phone proofs as much as the review.Confidence by part
block.rs,chapter.rs): high. Unit tests are mutation-checked, and the phone checks of Go to, Back and restore pass.osis.rs): high for KJV and BSB, the tested modules. Other modules with unusual markup may still differ.<sup><font size="-1">): medium. It was checked on CI's Qt and on the phone in two fonts and one text size. Other text sizes were only measured in CI at 20 px.🤖 Generated with Claude Code
Fresh-context review, level: high (trunk). Reviewed commit:
199e13ddc1. Range: origin/main...HEAD.Files reviewed: core/src/bible.rs, core/src/bible/block.rs, core/src/bible/chapter.rs, core/src/bible/reader.rs, core/src/render.rs, core/src/settings.rs, core/src/worker.rs, core/src/worker/session.rs (context), core/src/worker/session/jump.rs, qt/src/reading_model.rs, qml/pages/ReadingPane.qml, qml/pages/DisplaySettings.qml, tools/e2e/cases/running-text.sh, tools/e2e/lib.sh (helpers exist), docs/03-architecture.md, docs/04-features.md, po/de.po, po/utstudybible.schaefera.pot.
Checked and found sound: block/verse offset arithmetic (plain_len vs Qt positionAt for tags, entities, UTF-16, collapsed ASCII whitespace from sword::Run::collapse), Loaded row to ordinal to (row, offset) mapping, Span row counts for slide/drop, event ordering of Setting then Loaded on relayout (worker handles commands in order, ids stay above
shown), Rerendered same-rows check, settings order and the start[7] test.Findings outside the diff: none.
Inline: 1 medium, 1 low.
@ -23,2 +23,3 @@console.info("utsb: display gap " + Theme.where(gap.sliderItem, gap.sliderItem.width - 8))console.info("utsb: display red " + Theme.where(red.switchItem) + (red.enabled ? " on" : " off"))console.info("utsb: display running " + Theme.where(running))console.info("utsb: display red "+ Theme.where(red.switchItem) + (red.enabled ? " on" : " off"))low: unrelated edit to the red log line (the space before
+was dropped:"utsb: display red "+ Theme.where). qmlformat in theappjob may flag this; restore the original line.@ -349,0 +391,4 @@ruler.width = widthruler.font.pixelSize = font.pixelSizeruler.font.family = font.familyif (ruler.text !== model.text)medium:
ruler.text !== model.textis always true. The ruler istextFormat: TextEdit.RichText, and in Qt 5TextEdit.textreturnsdocument->toHtml()(a full<!DOCTYPE ...><html>document), never the string that was assigned. So the guard never skips and everyruled()call re-parses the whole paragraph and lays it out again.ruled()runs once perposAt, twice perverseSpan(bothlineTopcalls) and again inplaceTop, so each 100 ms report intopVerse(posAt + verseSpan) does about three full HTML parses and layouts of the row. A module with no paragraph marks is one block per chapter (Ps 119, about 176 verses), so on the low-memory phones this targets, scrolling running text re-parses and re-lays out a chapter-sized document about 30 times a second. Cache the last assigned string in a plain property (property string rulerText) and compare against that, or set the text once per delegate (ontextchange) instead of inruled().Round 1 review, both findings fixed:
textback as HTML. The ruler now keeps anassignedstring property and the guard compares against it, so the paragraph is parsed once per row change, not on every ruled() call.+): restored.app-docker green (PSS 163991 kB normal / 166594 kB slow, budget 176000 kB); e2e running-text and display-settings pass.
Re-review, level: medium (trunk). Reviewed commit:
6375bad33f. Range: 199e13ddc1fd2f4afe7a4be2fd808ec713fbb616..HEAD.Files reviewed: qml/pages/ReadingPane.qml, qml/pages/DisplaySettings.qml (plus the ruler's other uses in ReadingPane.qml).
The range holds two small QML hunks. The
ruler.assignedproperty is the only writer path toruler.text. It is updated together withtext, so the comparison no longer depends on the HTML readback. The DisplaySettings change is a spacing fix in a log string.Findings: none (no defects with a concrete failure scenario).
Phone check (installed 14:29, build
feat/running-text@6375bad33)The app starts cleanly on the phone (journal: window loaded after 350 ms, restored BSB Matt 13, no QML errors). Hold the footer to confirm the build line. Running text is under ⋮ › Settings › Display › Running text.
Reply here, or tell me in the session, with what passed and anything that looks off.
🤖 Generated with Claude Code
Phone check progress (owner, 2026-10-29):
6375bad: inline verse numbers were full size → fixed in70255da(RichText TextEdit rows,<sup>numbers).70255dashows tight line spacing and raised numbers touching the line above; a fix is in progress.🤖 Generated with Claude Code
Phone check of
c06603a+3835f97(arm64 click, Volla), done with ute2e (taps, shots, journal). The owner had already passed the checklist one0a9032(feel, flick, tap).c06603a,<sup><font size="-1">). Full-resolution crops in Sans (ASV Ps 119:139) and Serif (same verse) show the number under a "g" descender of the line above, clear of it in both fonts. Line pitch is logged asrunning 24 verse 24in Sans and23/23in Serif.3835f97). BSB Matt 13 in running mode was 3 rows for the whole window. BSB marks paragraphs as<div type="x-p" sID=…/>inside the pre-verse block after its headings, and the filter skipped that whole block and only knewdiv type="paragraph". Now Matt 13 starts paragraphs at 1, 3, 5, 7–11, 14, 16, 18, 20, 22–24, 27–29, … (the module's own marks), and the window has 73 rows. KJV is unchanged (Gen 1: 1, 6, 9, 14, 24, 26, 29).filter::osis::paragraph_starts_come_before_the_first_text(the real BSB 13:18 markup) andreader::marks_paragraph_starts(BSB Matt 13 has 18 and 24). Both fail without each half of the fix: without the pre-verse check, osis.rs:370; without thex-pdiv match, osis.rs:375.cargo makepasses.</w><transChange>, so this is not caused by this PR. Filed as #119.The phone is back to Sans and verse mode.
🤖 Generated with Claude Code
Review level: medium (re-review of fixes since
199e13d)Commit:
3835f97b5fRange: 199e13d..HEAD
Files reviewed:
(also read for context: core/src/bible/chapter.rs, tools/e2e/lib.sh, tools/golden/fetch.sh)
Findings: no high or medium defects found.
<sup>and a styled<span>", but block.rs emits<sup><font size="-1" color><b>; no<span>exists, so the comment misleads the next reader (no runtime effect).v > 0) fails as a flake rather than a real regression. The failure scenario is timing-dependent and I could not confirm it.Checked and fine: plain-text positions for verseStart/verseEnd are unchanged by the
<sup>markup and the wrapping<div>, sinceatcounts only the digits and the no-break space. The x-psIDdetection inside Skip::Preverse is gated on !seen_text, and theeIDmilestone is correctly not a paragraph start. The Loader-based delegate keeps the verseSpan/posAt/paragraphs API that placeTop and topVerse use.🤖 Generated with Claude Code
Replies to review 319 (medium,
3835f97, no high or medium findings):<span>: fixed inf8ba0c6. The comment now names<sup><font size="-1"><b>. It's a comment change only.pitchProbeis a plainTextdeclared before the rows. Itsheightis computed synchronously from its font and lineHeight, with no layout pass.TextEdit'slineCountandcontentHeightare computed synchronously whentextis assigned, which happens beforeComponent.onCompleted.c06603a(22/22), and on the phone in Sans (24/24) and Serif (23/23). A zero would fail the case loudly instead of passing it silently.🤖 Generated with Claude Code
Level: medium (delta review). Commit:
fb44892742. Range: f8ba0c6..HEAD. Blast radius: trunk. Testing tier: standard.Files reviewed: qml/pages/ReadingPane.qml (+2 lines), plus its consumers: the redCount Timer (ReadingPane.qml:88-99), tools/e2e/cases/red-letters.sh, and the other delegate users (itemAtIndex at lines 306 and 337).
Checks:
items[i].textonverses.contentItem.children. It now finds the delegate'sreadonly property string text: model.text. Children that are not delegates (no stringtext) are skipped by thetypeofguard, so they are not counted.textproperty on the delegate root does not shadow anything. Every use of the model role inside the delegate is the explicitmodel.text(lines 428-429 and 463). The othertext:lines are assignments on child items (the number Text, the ornament and the break lines), and those resolve to their own properties. Nothing in the delegate reads a baretext.paragraphs,textTopand similar), so they are unaffected.Findings: none (no defects with a concrete failure scenario).
🤖 Generated with Claude Code
WIP: Running text: paragraphs of verses with inline numbers (#12 part 3c)to Running text: paragraphs of verses with inline numbers (#12 part 3c)claude-review merge gate, medium effort, head
fb44892. Checked: (1) review 317's medium (ruler.text always true) was fixed in6375badand the ruler later removed; its low was fixed; 319's lows are fixed (f8ba0c6) or refuted (comment 2398). (2) Read both commits after3835f97:f8ba0c6is a doc-comment-only change in render.rs;fb44892addsreadonly property string text: model.textto the row delegate in ReadingPane.qml, restoring the red-count E2E hook behind the Loader. No findings. (3) Commit status onfb44892is success (CI changes, rust, audit, app and the rest). (4) The PR Proof lists no unrun gate the user must decide on; standard tier, so no elevated gate. Supersedes review 317.