Display: superscript verse numbers, line spacing and verse gap sliders (#12 part 3a) #113
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!113
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/verse-look"
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 3a of the split in comment 2328).
What
Verse numbers are now small, bold, muted superscripts. StyledText has no
<sup>, and its<font size>ignores the Text's pixel size, so each number is its own small Text at the verse's top left. The body keeps room for it with the number at full size, drawn transparent.Settings → Display gets two sliders, line spacing (
text.line_spacing, 100–200 %) and space between verses (text.verse_gap, 0–32 px at text scale 100). They go in a newqml/controls/LabelledSlider, which saves on release or on a key or screen-reader step. Each change keeps the top verse in place. Still one row per verse; running text comes in 3b/3c.Blast radius
Tier: trunk
Trunk paths touched:
qt/src/reading_model.rs,qml/pages/ReadingPane.qml,core/src/worker.rs,tools/e2e/lib.sh,tools/e2e/cases/Testing tier: standard (logic and settings). UI is also listed, and it needs a phone try (docs/09).
Gate / rollback: revert this merge. The two settings are plain key/value rows with defaults, so there's no migration. An old build ignores the rows.
Proof
3f19492, then on4295de8and4e07d2a:cargo makepassed (fmt, clippy, tests: core has 235, deny, audit).cargo make i18n: up to date, and de.po is filled.cargo make app-dockerpassed (QML format/lint, -Werror build, clang-tidy, PSS).4e07d2a: red-letters, red-letters-kill, switch-bible-red-letters, display-settings, display-spacing and restore all passed (6 of 6). display-settings-kill passed on4295de8.lineHeightbinding removed from the delegate,display-spacingfails: "line spacing at 200 % didn't make the top verse taller ('22 22')".VERSE_GAP's default and range on purpose makesthe_spacings_are_clamped_and_start_as_the_text_wasfail.4e07d2a: 163,774 kB normal and 162,965 kB slow, against the 176,000 kB budget.4295de8: 159,850 to 165,244 kB. Both are within the run-to-run noise in docs/07.4295de8, where the numbers were tiny and on the baseline. That led to4e07d2a, whose numbers are raised and readable (light theme, sans).feat/verse-look@4e07d2ab7is installed: window loaded after 360 ms, no QML warnings.display-spacing-04-reading-loose(line spacing 200 %, gap 32) shows the number in line with the first line's text.4e07d2a: changes, rust, audit, app, e2e and click all passed.Review
Reviewer:
code-reviewat high (review on3f19492): 1 low, keyboard and screen-reader slider steps didn't save. Fixed in4295de8.4e07d2a(review 313): 3 lows, answered in comment 2340. No high or medium findings, so no re-review.Review-Confidence: 80/100 (audit on
4e07d2a, CI green).4295de8fix wasn't reviewed after it was made (leaf,qml/controls).No missed defect. One cosmetic note: fast keyboard steps can briefly snap the slider back to the last saved value.
Confidence by part
4295de8): medium. No test drives it.🤖 Generated with Claude Code
Fresh-context review. Level: high. Blast radius: trunk.
Reviewed commit:
3f194925c3Range: origin/main...HEAD
Files reviewed: core/src/render.rs, core/src/settings.rs, core/src/worker.rs, core/src/worker/session/jump.rs (set_setting path), docs/04-features.md, po/de.po, po/utstudybible.schaefera.pot, qml/controls/LabelledSlider.qml, qml/controls/qmldir, qml/pages/DisplaySettings.qml, qml/pages/ReadingPane.qml, qml/platform/SystemColours.qml, qml/qml.qrc, qml/theme/Theme.qml, qt/src/reading_model.rs, tools/e2e/cases/display-settings-kill.sh, tools/e2e/cases/display-spacing.sh, tools/e2e/lib.sh
Findings outside the diff: none.
Checked and found sound: new settings are in ALL (start order, tests updated), clamped by the same ranges as the slider (100-200 step 10, 0-32 step 2), and layout-only keys do not trigger a re-render (render.apply ignores them, asserted in render.rs and worker.rs tests). The glue only translates. Theme/pane binding order survives the initial 0 defaults the same way text scale does. keepTop wraps both layout changes. The red-letter counter now skips the number's own font tag (number colour is the muted hex, never the red hex). i18n entries are present in the pot and de.po. The kill case reads a fresh log per start (e2e_start truncates), so the "before the text" ordering check is valid.
Findings (low only): see the inline comment.
@ -0,0 +41,4 @@snapMode: Slider.SnapAlwaysvalue: root.valueAccessible.name: root.titleonPressedChanged: if (!pressed && value !== root.value) root.committed(value)low · A change that does not go through a press is shown but never committed. Keyboard Left/Right on a focused Slider and the screen reader's increase/decrease action (Accessible.name is set here, so the slider is exposed) change
valuewithoutpressedtoggling. Scenario: focus the slider, press Right: the label reads 110 %, butcommittednever fires, so the setting is neither saved nor applied, and the label is out of step with the text. Use the Slider'smovedsignal for non-drag changes (commit onmovedwhen notpressed, and on release otherwise).Round 1 answer: the one low finding is fixed in
4295de8.@ -0,0 +41,4 @@Layout.fillWidth: truesnapMode: Slider.SnapAlwaysvalue: root.valueAccessible.name: root.titleReply to the low finding (keyboard/screen-reader steps never committed): fixed in
4295de8. The Slider now also hasonMoved: if (!pressed && value !== root.value) root.committed(value), so a Left/Right key or an accessibility increase/decrease commits at once, while a drag still commits once on release (moved fires during a drag, but pressed is true then). app-docker passes (PSS 165 MB of 176 MB).Level: medium (trunk: qml/pages/ReadingPane.qml). Commit
4e07d2ab79. Range 4295de8..4e07d2a (one commit).Files: qml/pages/ReadingPane.qml, docs/04-features.md.
No high or medium findings. keepTop/topVerse/restore read item.height of the delegate Text; the number is a child Text and the transparent digits keep the body's first-line height, so heights are unchanged. The red-on-screen hook counts only contentItem children (delegates), so dropping the prefix strip is correct, and the transparent font tag does not match the red regex. tools/e2e/cases/red-letters.sh only reads the logged count.
Lows (not on diff lines):
Build
feat/verse-look@4e07d2ab7is on Plinius and running. Window loaded after 360 ms, and the journal has no QML warnings.One change since the review. On the phone, the first build drew verse numbers tiny and on the baseline, because StyledText's
<font size>ignores our pixel size. In4e07d2athe number is its own small Text, raised at the top left of the verse. A delta review of that commit is running.On the phone, try:
🤖 Generated with Claude Code
Answers to review 313. All three findings are low, so there's no re-review.
4e07d2awas 163,774 kB (normal profile) and 162,965 kB (slow). On4295de8it was 159,850 to 165,244 kB. Both are within the noise of the 176,000 kB budget. The PR body has these numbers.target/shots/display-spacing-04-reading-loose.png(line spacing 200 %, gap 32) shows the number in line with the first line's text, because Qt adds the extra leading below each line. The owner also checks this on the phone (comment 2339).🤖 Generated with Claude Code
Phone checks all clear
WIP: Display: superscript verse numbers, line spacing and verse gap sliders (#12 part 3a)to Display: superscript verse numbers, line spacing and verse gap sliders (#12 part 3a)The owner reports that every step of the phone checklist (comment 2339) passed on build
feat/verse-look@4e07d2ab7: "All checks passed on the phone, go ahead".🤖 Generated with Claude Code
Merge gate on
4e07d2ab79(trunk). Checked: reviews 311 and 313 leave no high or medium finding (the one low in 311 is fixed in4295de8, the 3 lows in 313 are answered in comment 2340). No commits after4e07d2a(origin/feat/verse-look is at4e07d2a).4295de8is a 2-line change in LabelledSlider (onMoved commits only when not pressed and the value differs), so no double commit while dragging and no failing path. CI on head is green: changes, rust, audit, app, e2e and click. The audit gave Review-Confidence 80 with no missed defect, and the owner confirmed the phone checklist (comments 2339 and 2343), so no elevated or maximum gate is left unrun.