Type: Crimson Pro, pinned chapter heading, floating footer pill (#95) #144
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!144
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/lectern-type-footer"
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?
Closes #75. Part 3 of #95.
What
Blast radius
Tier: trunk
Trunk paths touched:
qml/pages/ReadingPane.qml,core/src/bible/{chapter,reader}.rs,core/src/worker.rs,qt/src/reading_model.rs,tools/e2e/Testing tier: standard (also: ship on sight: styling, docs)
Gate / rollback: no flag; revert this merge. The Serif setting (Display) switches the new default face off per user. No schema change.
Proof
4c19391green (run 287): rust (fmt, clippy, tests + coverage, deny, audit), app (C++/QML lint, -Werror build, clang-tidy, PSS mem-check), e2e (all cases, 13 min), click.heads_chapters_in_the_module_languagewith the old rule (book name on chapter 1 only) restored → FAILED at reader.rs:249,book: ""vs"1. Mose"(Kapitel 2). The settings and worker tests expecttext.serif=1and fail on the old default0.d2f50de; CI's mem-check green on4c19391). Fonts add about 240 kB to the click.pinned-heading(inline heading, pinned mid-chapter, push-off by the next chapter);light-ground,import-url(newe2e_page_menu: the page bar's ⋮ stays at the edge),language-switchadapted.4c19391(cargo make shots, 3 cases passed), below.efb08eb, comment 2788: heading pinned, a tap on it opens nothing, it follows chapters, pill legible.4c19391, comment 2799: journalfeat/lectern-type-footer@4c193915b; Crimson Pro by default; arrow Back; reference centred ("Ps 25" centre at 542 of 1080 px, #75); Back from Ruth 2 returns toBSB 14780 0.920(Ps 25). Owner (comment 2801): dark mode and bottom-edge swipe (scrolls the text, not caught by the pill) work; reading feel signed off.Review
Reviewer: fresh-context review at high (374) + mockup check (375): 6 findings, 5 fixed in
d2f50de, 1 declined (text 18 vs artboard 20 px: existing default with the size slider, a new default reflows saved places; owner kept 18 for now). Re-review 376 (medium): clean. Delta 377 (efb08eb, CI fix): clean. Delta 378 (4c19391, owner's changes): 1 low, refuted (comment 2796). Deferred by the owner: nicer sans #145, search in the pill #138. Left open: none.Review-Confidence: 100/100, every trunk file reviewed at the right depth through the final commit, each finding fixed or refuted, and fails-without-change, CI and the phone check on
4c19391all hold (first audit 85, for the then-unchecked swipe and dark mode on the phone).Confidence by part
efb08eb, but scroll-restore with the heading offset is the part to read.Mockup deviations (artboard: board C, target/mockup/Lapis.dc.html):
Screenshots
Desktop (Xvfb, Default style,
4c19391; the ✕ is the Xvfb mouse cursor).Phone (
4c19391, Suru, Paper), see comment 2799:🤖 Generated with Claude Code
Fresh-context review (posted by claude-review)
Level: high (blast radius: trunk)
Reviewed commit:
2832e3eba9Range: origin/main...HEAD (base
b258394)Files reviewed: NOTICES.md, core/src/bible/chapter.rs, core/src/bible/reader.rs, docs/06-sources.md, docs/10-design.md, qml/fonts/OFL.txt (fonts themselves not opened), qml/pages/DisplaySettings.qml, qml/pages/ReadingPane.qml, qml/qml.qrc, qml/theme/Theme.qml, qt/src/reading_model.rs, tools/e2e/lib.sh, tools/e2e/cases/{back-after-kill,install-queue-cancel,light-ground,note-sheet,picker-back,pinned-heading,switch-bible-back,verse-sheet}.sh, plus callers of Heading.book and of the old footer (no remaining users of
pane.footer; e2e coordinates for the pill, menu, Back and the installs strip check out against the 52 px / 20 px / 24 px geometry).Findings outside the diff: none.
Findings inline:
The Rust side (Heading.book now a String on every chapter, Span::of, tests) and the placeTop/topVerse/restore offset by Theme.headingHeight are consistent with each other and with the docs.
@ -658,0 +713,4 @@// list has placed the verses below it. A fixed height, the same// for every heading, also keeps the verses' offset below the// pinned one (Theme.headingHeight) a constant.section.delegate: Rectangle {medium: this Rectangle is the pinned heading, drawn over the verses, but it has no MouseArea (the pill got one for exactly this reason). A Rectangle without mouse handling is not a hit target, so a tap on the pinned heading falls through to whatever verse text or verse-number area (min Theme.touch square) sits under it, and the hidden verse's sheet opens (reading.showVerse). Scenario: read mid-chapter, tap the chapter name at the top; the VerseSheet of a verse the reader cannot see opens. Fix: add
MouseArea { anchors.fill: parent }inside this delegate (it does not steal flicks; it sets no preventStealing) so the heading's ground swallows taps like the pill's.Mockup check
Compared: artboard
target/mockup/Lapis.dc.html(board C "Ink and lapis": screens 6 John 3 on paper, 7 on white, and the pill) againsttarget/shots/pinned-heading-04-heading-inline.png,-05-heading-pinned.pngand-06-heading-pinned-next.png(dark only; there are no light shots of this case intarget/shots/, so paper and white were checked from the QML colour roles, not pixels) and the code inqml/pages/ReadingPane.qmlandqml/theme/Theme.qml.The PR body lists the deviations ("full Chapter 3 line, SmallCaps 17 px book, text kept at 18 px, no search icon, chapter number always Crimson Pro, fixed heading size, rule width") but gives no reason for any of them. docs/09 asks for a reason each, so the ones below are findings, not accepted deviations.
Medium
qml/pages/ReadingPane.qml~line 745 (book name). Artboard: "JOHN" in all caps, Ubuntu sans, 12 px, letter-spacing .18em,#6B6458. Shot: "John" inFont.SmallCapsat 17 px with 2.5 px spacing. It renders as a large J with tiny "OHN", which reads as a different treatment, not the artboard's quiet label. Qt hasFont.AllUppercase, so Qt's rich-text subset is no reason. Use uppercase at about 12 px, or give the reason.qml/pages/ReadingPane.qml~line 760 (chapter line). Artboard: just the number ("3", "23"), 34 px, weight 600, lapis, with the book name above. Shot: the words "Chapter 3" at 30 px bold. The word and the size are listed but not explained. The localised "Kapitel 3" is a plausible reason, but then the PR must say so (the core still has the number it could pass).qml/pages/ReadingPane.qml~lines 266-320 (floating pill labels). Artboard: "John 3:16" at 15 px weight 500 in full text colour, a 1 px divider, then "KJV" bold, 13 px, letter-spaced, in lapis. Shot (05): the pill is there and shaped right, but "Back", "John 3", "KJV" and the menu glyph are almost invisible dark grey on#211F24(very low contrast). KJV is not lapis and there is no divider. TheToolButtons were givenbackground: nullbut their labels do not usecolours.textColour/colours.accent. This is a legibility bug in dark, not a theme-colour choice.Low
#8FB0F0-like) is a same-role dark variant of#1E4FA8: fine.Not checked: paper and white pixel shots (not in
target/shots/); runcargo make shotsforpinned-headingandlight-groundin the light theme and attach them.Author answers to review round 1 (reviews 374 and 375), fixed in
d2f50de:Checks: app-docker passes (PSS 170888 kB normal, 169157 kB slow, budget 176000); e2e pinned-heading, light-ground and picker-back: 3 passed.
Review level: medium. Blast radius: trunk. Reviewed commit:
d2f50de5e4. Range: 2832e3eba977de75dbb35d962b8893b1f3b58f26..HEAD (one commit,d2f50de).Files in range: qml/pages/ReadingPane.qml (trunk), qml/theme/Theme.qml.
I read the hunks in context, along with the heading format from core (
Heading.chapter,names.chapter) and the colour tokens (colours.hairline,accentandtextColourexist). No defects found.Not checked: CI results and the shots, since I review commits only.
Delta review, level medium, commit
efb08ebdd0, range d2f50de..efb08eb.Files: tools/e2e/lib.sh, tools/e2e/cases/import-url.sh.
No defects found. e2e_page_menu taps (e2e_w-24, e2e_h-20) = (516, 940). PageBar's ⋮ ToolButton is Theme.touch (48 px) wide, last in a RowLayout filling the bottom-edge ToolBar, so it spans x 492..540 and 940 lies inside the bar. The "utsb: menu import x y" log comes from PageBar's onOpened, which e2e_tap_until waits for. The only caller of the old e2e_menu with a page-bar entry was import-url.sh, now switched; e2e_menu (modules/settings) remains reading-pane-only and unchanged. e2e_tap_menu is unaffected.
Findings outside the diff: none.
Phone check (
efb08eb, Paper, Suru style)Build on the phone confirmed by the journal:
feat/lectern-type-footer@efb08ebdd built 11:27 PM installed 11:28 PM.Checked by me with ute2e:
Checked by the owner:
For the owner (feel and system settings I don't change):
Delta review, level medium
Commit:
4c193915bdRange: efb08eb..4c19391
Files reviewed:
Findings outside the diff:
ReadingModelRustderivesDefault, soserifstarts false while the core default is now "1". ReadingPane.qml:214 copies it toTheme.serifon completion, so at start-up the text can first lay out in the system sans and then switch to Crimson Pro when the settings arrive (a relayout and a font flash). If the settings never arrive, it stays sans. Initialiseserif: truein the model's default, asTheme.serifwas.No high or medium defects found.
Author answer to the delta review 378 (efb08eb..4c19391):
serifstarts false, so the text could flash from sans to serif at start-up): refuted.Initialize(reading_model.rs:330–337) runs every setting insettings::ALLthroughsetting()when the object is created, before QML reads a property.SERIF(default"1") is inALL(core/src/settings.rs:71), andsetting()setsrust.serif(line 518). So the model starts with serif on; the derivedfalseis never seen. No change.Phone re-check (
4c19391, Suru, Paper)Installed
4c19391(cargo make click+clickable install, click-review pass). Checked by me with ute2e:utsb: build 0.0.1 feat/lectern-type-footer@4c193915b built 10/31/26 9:59 AM installed 9:59 AM; window loaded after 431 ms.back to BSB 14780 0.920, showing Ps 25 at the same place.For the owner (feel and system settings I don't change):
Owner's phone items on
4c19391(from the session, 2026-10-31): "The swipe up from the bottom just moves text right? If so, that and dark mode are working. The reading experience seems to be as good as ever".WIP: Type: Crimson Pro, pinned chapter heading, floating footer pill (#95)to Type: Crimson Pro, pinned chapter heading, floating footer pill (#95)Merge gate (claude-review) on
4c19391.Checked:
4c19391. There are no commits after the last reviewed one (git log 4c19391..origin/feat/lectern-type-footeris empty).4c19391(changes, rust, audit, app, e2e, click).Nothing left open.