Bible chooser and Go to redesign (#148 step 3) #153
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!153
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/redesign-chooser"
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?
What
Step 3 of #148: boards E4 and E6. The Bible chooser becomes a bottom sheet of rows with a round badge, description, language and a lapis ✓ on the open Bible. Go to gets testament tabs that open on the one being read, plain grid cells with the current book and chapter in lapis, and six numbers to a row. Clearing a search returns to the book being read. New controls:
GridCell,LineTab.Differs from the boards: the three steps stay (books, chapters, verses); the typed field stays behind its setting with no magnifier; the badge shows the module name, as the conf's
Abbreviationis often a long title; rows may wrap to two lines and carry no feature text (that data isn't exposed); no footer line about the verse staying (owner's call). A search matching both testaments shows only the open tab's matches; the tab flips by itself only when it has none.Blast radius
Tier: trunk
Trunk paths touched:
core/src/bible/chapter.rs,core/src/bible/reader.rs(a chapter says its place),qt/src/reading_model.rs,qt/src/picker_model.rs(currentBook,currentChapter,BookList.testament),tools/e2e/Testing tier: standard
Gate / rollback: no flag; revert this merge. No schema or stored data changes.
Budgets: peak PSS 172125 kB normal, 179522 kB slow profile (budget 185000 kB). The chooser's rows are built when its sheet opens (a Repeater, about ten items a row, a handful of Bibles); nothing is added at start-up beyond two integer properties.
Proof
18dd2b7(changes, rust, audit, app, e2e, click). Locally at13a1b50:cargo make,cargo make app-docker(text on screen 318/425 ms against 3000 ms).picker-current: opens on the testament being read with the book marked, a search in the other testament clears back to it, the Old Testament tab marks nothing, the chapter is marked.picker-back,switch-bible-back,typed-referencepass with the new grid.reads_kjvfails with the chapter's place broken inreader.rs; the search-clearing step ofpicker-currentfailed before thePickerPagefix (no 'utsb: picker tabs 2 ' (x4) after 10 s).18dd2b7, the second was confirmed. The ✓ in a row's accessible name has not been tried with a screen reader.Review
Reviewer: fresh-context
claude-review, high at13a1b50(397), medium for 13a1b50..18dd2b7 (399). No high or medium findings. Fixed: the open Bible in the accessible name, the racy negative check. Answered on the PR: the eager Repeater, the literal ✓, the search across testaments.Review-Confidence: 75/100. The audit deducted for the search across testaments being accepted rather than fixed or deferred (-10), and for no proof exercising the row's accessible name (-15). It found no missed defect.
Confidence by part
place,currentBook,currentChapter): high, traced by review and audit and covered by tests.🤖 Generated with Claude Code
Review level: high (blast radius trunk, testing tier standard). Reviewed commit:
13a1b50f8e. Range: origin/main (420093b, up to date, no commits behind)...HEAD.Result: no high or medium findings. Core
Chapter.place, thecurrentBook/currentChapterproperties (set at both reset and scroll paths, -1/0 when empty),BookList.testament, the book/chapter numbering shared withjump, the 5->6 column change ine2e_pick_numbers, and the newpicker-currentcase all trace through consistently. Nothing blocks.Low findings (inline where the line is in the diff):
picker currentlines right after thepicker tabs 1line is seen; the hook logs tabs before the current loop, so the count can be read before a wrongly marked line is written, and the assertion can pass falsely.Files reviewed:
core/src/bible/chapter.rs
core/src/bible/reader.rs
core/src/bible/picker.rs
core/src/bible/place.rs
qt/src/picker_model.rs
qt/src/reading_model.rs
qml/controls/GridCell.qml
qml/controls/LineTab.qml
qml/controls/BottomSheet.qml
qml/controls/RowButton.qml
qml/controls/qmldir
qml/pages/BibleChooser.qml
qml/pages/PickerPage.qml
qml/pages/ReadingPane.qml
qml/Main.qml
qml/qml.qrc
qml/theme/Theme.qml
qml/platform/SystemColours.qml
tools/e2e/cases/picker-current.sh
tools/e2e/cases/switch-bible-back.sh
tools/e2e/lib.sh
docs/10-design.md
po/de.po
po/lectern.aps.pot
CLAUDE.md
@ -116,0 +81,4 @@rightPadding: Theme.insettopPadding: 8bottomPadding: 8Accessible.name: description !== "" ? description : namelow: the accessible name drops the open/not-open state (the check is Accessible.ignored). Add the state, e.g.
Accessible.checked/checkable, or put it in the name.@ -91,1 +128,4 @@inputMethodHints: Qt.ImhNoPredictiveText | Qt.ImhNoAutoUppercase// Narrowed to nothing here while the other testament has some.onTextChanged: {if (books.names(page.testament, text).length === 0 && books.names(3 - page.testament, text).length > 0)low: a query matching in both testaments shows only the current tab's matches, with nothing saying the other tab has more. Consider flipping/hinting, or accept (Enter still jumps).
@ -0,0 +15,4 @@current=$(e2e_count 'utsb: picker current ')e2e_tap $(e2e_last 'utsb: picker tabs ' | awk '{ print $2, $3 }')e2e_wait 'utsb: picker tabs 1 ' $((seen + 1))[ "$(e2e_count 'utsb: picker current ')" = "$current" ] || e2e_fail "the Old Testament marked a current book"low: racy negative check.
picker tabs 1is logged before thepicker currentloop, so this count can be taken before a wrong mark is logged; wait a beat (or wait for a later marker line) before comparing.Answers to review 397. The fourth finding (outside the diff): the eager Repeater is deliberate. It is what lets the rows scroll in BottomSheet's column, the sheet is loaded only when opened, and a phone holds a handful of Bibles at about ten items a row. mem-check's peak PSS is 172–180 MB against the 185 MB budget.
@ -116,0 +81,4 @@rightPadding: Theme.insettopPadding: 8bottomPadding: 8Accessible.name: (current ? "✓ " : "") + (description !== "" ? description : name)Fixed in
0a7e221: the name starts with ✓ for the open Bible, as the old row's did.@ -92,0 +130,4 @@// Narrowed to nothing here while the other testament has some.onTextChanged: {if (books.names(page.testament, text).length === 0 && books.names(3 - page.testament, text).length > 0)page.testament = 3 - page.testamentAccepted as is: both tabs stay in view while typing, one tap shows the other testament's matches, and Enter still jumps to what the text names. The tab only flips on its own when the shown one has no match.
@ -0,0 +14,4 @@seen=$(e2e_count 'utsb: picker tabs 1 ')current=$(e2e_count 'utsb: picker current ')e2e_tap $(e2e_last 'utsb: picker tabs ' | awk '{ print $2, $3 }')e2e_wait 'utsb: picker tabs 1 ' $((seen + 1))Fixed in
0a7e221:picker tabsis now logged after the marks, so a case that has seen it has seen them too; no wait needed.Review level: medium (trunk: tools/e2e/). Commit:
18dd2b7125. Range: 13a1b50..HEAD.No high or medium findings. One low finding, inline. Nothing outside the diff.
Files reviewed: docs/10-design.md, po/de.po, po/lectern.aps.pot, qml/pages/BibleChooser.qml, qml/pages/PickerPage.qml, tools/e2e/cases/picker-current.sh, tools/e2e/lib.sh (context).
@ -116,0 +81,4 @@rightPadding: Theme.insettopPadding: 8bottomPadding: 8Accessible.name: (current ? "✓ " : "") + (description !== "" ? description : name)low: the literal "✓ " is not translated and a screen reader may read it as "check mark" or skip it. Where it is skipped, the open Bible has no non-visual marker. Prefer Accessible.checked or a qsTr'd word.
Answer to review 399.
@ -116,0 +81,4 @@rightPadding: Theme.insettopPadding: 8bottomPadding: 8Accessible.name: (current ? "✓ " : "") + (description !== "" ? description : name)Kept: the row before this PR named the open Bible with the same literal ✓, so this restores existing behaviour rather than adding a gap, and a symbol needs no translation. Whether the app's accessible names should use checked states is a wider question than this view.
WIP: Bible chooser and Go to redesign (#148 step 3)to Bible chooser and Go to redesign (#148 step 3)Merge gate at
18dd2b7. Checked: reviews 397 (high,13a1b50) and 399 (medium, 13a1b50..18dd2b7) have no high or medium findings, so none is left open. The branch has exactly three commits over main (13a1b50,0a7e221,18dd2b7), and 399 covers up to head, so no commit is unreviewed. The low findings were fixed in0a7e221or answered in 398 and 400. CI is green on head: changes, rust, audit, app, e2e and click all succeeded. The PR body's Proof lists no unrun elevated, maximum or AI-behaviour gate (tier standard). The one untested item, the accessible name with a screen reader, is declared as low confidence. Review-Confidence is 75, with its two deductions disclosed. Not merging.