Verse sheet with TSK cross-references, Install TSK when missing (#13, 6d) #130
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!130
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/13-6d-tsk"
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 6d of #13.
What
Blast radius
Tier: trunk
Trunk paths touched:
core/src/bible*,core/src/worker*,qt/(reading_model.rs),qml/pages/ReadingPane.qml,qml/Main.qml,.forgejo/workflows/ci.yml(the e2e job also fetches TSK),tools/e2e/Testing tier: standard (also: CI config, the e2e job's module fetch and cache key only)
Gate / rollback: no flag. The sheet is new UI, and without TSK installed nothing is read. To undo it, revert this merge. There is no migration and no schema change.
Proof
cargo makepassed (fmt, clippy, tests with the golden data, deny, audit).cargo make app-dockerpassed. mem-check peak PSS was 165156 kB normal and 169483 kB slow (budget 176000), with text on screen after 320 ms.0268678: changes, rust, audit, app, e2e and click passed.Screenshots are in the PR comments: the phone sheet, the copy tooltip, the sheet without TSK, Show all, and a text tap.
Review
Reviewer: fresh-context reviews 339 (high), 340 and 341, then 342 (the delta). 343 (medium) covered the text-tap commit and 344 was its delta. The findings and answers are in the PR threads. 343's low finding is fixed in
22a91cd, and 344's low finding in0268678, after the delta review. Review 340's low 2 was declined (comment 2561). None are left open.Review-Confidence: 95/100. The audit's one deduction (−5) is the fix after the delta review,
0268678(2 lines of QML, its cases pass); it found no missed defect.Confidence by part
linkAt, running text'scharAt): medium. The e2e cases cover a mid-line tap in both layouts; the wrapped-line-end case was fixed by reading only.🤖 Generated with Claude Code
Review level: high (trunk). Commit
fc0fb8f3f2. Range: origin/main...HEAD.Files reviewed: .forgejo/workflows/ci.yml, core/src/bible.rs, bible/{block,note,reader,refs,tsk,verse_sheet}.rs, bible/reader/{sheet,verse}.rs, bible/refs/{scrip,sweep}.rs, catalog/{kind,tests}.rs, worker.rs, worker/session.rs, worker/session/jump.rs, qt/src/reading_model.rs, qml/Main.qml, qml/controls/{RefRows.qml,qmldir}, qml/pages/{NoteSheet,ReadingPane,VerseSheet}.qml, qml/qml.qrc, tools/e2e/cases/{verse-sheet,verse-sheet-no-tsk}.sh, po/*.
No high or medium defects found.
Findings (low):
session.tskis opened once and never dropped or re-checked. If the user removes TSK in the module manager, the open handle keeps answering: the sheet still lists references and never shows "Needs the TSK module" until the app restarts. (If the handles are per-read, it shows "None for this verse" instead.) Reopen when the module's conf is gone, or droptskon a module change.sheet_refreads and decodes up to 5 verses for every TSK reference of the tapped verse, on the worker, before the sheet opens. Verses with 100+ references block the worker (and so reading and flush commands) and produce large JSON, although only 5 rows show. Resolve the text lazily for the firstcaprefs, or on "Show all".same_versefinds no match in TSK's versification,refsreturns empty, so the sheet says "None for this verse" with TSK installed. That is not the same as TSK having no entry; the wording may mislead for Bibles with a different versification.Testing tier: the maximum-tier targeted check (CI e2e on the final commit running verse-sheet and verse-sheet-no-tsk as PASS, not SKIP) could not be confirmed. The latest run for this head, run 239, was still "waiting" when I reviewed. Confirm it before merge. The e2e cases pass SKIP only via exit 77 when a module is missing, which is why the TSK fetch and cache-key change in ci.yml matter, and that change looks correct.
Answers to review 339 (high,
fc0fb8f; no high or medium):8009389. Each verse sheet now checksTsk::installed(is the conf still inmods.d?). A removed TSK is closed, so the sheet offers Install TSK again. A reinstall opens it again. The worker test now removestsk.confand expects"tsk":false; that assertion fails without the fix. Separately,an_entrys_refs_start_from_its_chapter…used chapter 1, so it passed even with the chapter argument ignored. It now uses Genesis 2 and fails when the chapter is dropped.same_versefinds no KJV verse, TSK really has no entry for that verse, so "None for this verse" is true. The open Bible's other verses are mapped and listed.🤖 Generated with Claude Code
Review level: medium (re-review of fixes). Commit
80093891b3. Range fc0fb8f..HEAD (8009389). Blast radius: trunk.Files: core/src/bible/tsk.rs, core/src/worker.rs, core/src/worker/session/jump.rs.
No high or medium findings. The fix follows the module manager's order:
package::removedeletes the conf first, soTsk::installedturns false as soon as a removal starts.Session::versethen drops the handle. The new worker test covers removal, and the entry test now uses chapter 2, so it checks the entry's chapter rather than a chapter-1 coincidence.Findings:
sword::module::listparses every conf in mods.d on each verse tap, and it fails as a whole if one conf is unreadable. A conf that vanishes mid-listing, for example a different module being removed or installed at that moment, makesinstalledfalse. The open TSK is closed for that tap and reopened on the next, which costs one extra index load. This is transient and cheap, so no change is required.Delta review, level medium, blast radius trunk. Commit
2c01617, range 8009389..HEAD.Files: core/src/bible/tsk.rs, core/src/worker/session/jump.rs. I also read sword::module::list, Conf::get and Module::open.
No defects found.
Checked:
Not covered, not a defect: an update that changes the data without changing Version keeps the stale index. This is an accepted limit of using Version as the signal.
Phone check and screenshots
On the phone (feat/13-6d-tsk@2c016172f, then
c9a637f14):verse-copychecks it: a long press shows "Copy this verse" and copies nothing, and a tap copies Genesis 1:1.VerseSheet: Binding loop detected for property "height"at start-up, already on2c01617. Fixed in15674b3: BottomSheet keeps its fitted height from the layouts' change handlers. The e2e app logs of verse-sheet, verse-sheet-no-tsk, verse-copy and note-sheet now have no binding loop.Review 340, low 2 (
sword::module::listis parsed on each verse tap): kept. It reads the smallmods.dconfs once per tap, after a user action, and it is what lets a removed or updated TSK be noticed without a restart. Low 1 was fixed in2c01617.Gates on
15674b3: app-docker passed (peak PSS 169051 kB, budget 176000; text on screen 316 ms). The sheet e2e cases pass locally, and CI is running.Delta review (medium)
Reviewed commit:
15674b3754Range: 2c016172ff935f1e101cd4a44c8e91f9cf32e77f..HEAD
Files reviewed: qml/controls/BottomSheet.qml, qml/pages/VerseSheet.qml, qml/qml.qrc, tools/e2e/cases/verse-copy.sh, core/src/bible/verse_sheet.rs (doc comment), qml/theme/Theme.qml (where helper, for context), tools/e2e/lib.sh (helpers, for context), NOTICES.md, po/de.po, po/utstudybible.schaefera.pot, assets/icons/copy.svg, qml/icons/copy.png (not inspected in detail).
Findings: none with a concrete failure scenario. The BottomSheet
fitproperty, updated from the layouts' implicitHeight change handlers, removes the binding loop. The e2e log patterns do not collide: 'verse copy ', 'verse copy tip ' and 'verse copied ' are read in a safe order. The long press does not emit clicked, so it does not copy.Level: medium (delta review). Commit
7fb9a930. Range 15674b3..7fb9a93.Files: core/src/bible/reader/verse.rs, core/src/bible/verse_sheet.rs, qml/pages/ReadingPane.qml, qml/pages/VerseSheet.qml, qt/src/reading_model.rs, tools/e2e/cases/verse-text-tap.sh.
No high or medium findings. One low finding is inline. Checked: link vs. text routing, MouseArea z-order against the number area, Flickable interaction, verse_at bounds, JSON escaping of text.
@ -503,0 +560,4 @@if (link !== "")pane.noteTapped(link)elsepane.textTapped(reading.verseAt(model.index, parent.positionAt(mouse.x, mouse.y)))low · running text passes the raw
positionAt(x, y)toverseAt, but the existingposAthelper (line ~447) usespositionAt(...) - 1to get the core's plain-text positions. If that offset applies here too, a tap on the first character of a verse can resolve to the previous verse (and the last character of a verse to the next one). The effect is limited to one character at verse boundaries, and the e2e case taps mid-page so it would not catch it. Consider reusing the same-1(or a shared helper) for consistency.Review 343 had one low finding: in running text, the raw
positionAtwent toverseAt. It is fixed in22a91cd.charAt(x, y)now steps back one place when the cursor place it gets lies to the right of the tap. That happens on a character's right half, so a tap on a verse's last character no longer opens the next verse. A verse's first character is its inline number, which is a link, so it goes throughlinkAteither way. The cases verse-text-tap, note-sheet and running-text pass.I also checked it on the phone at
7fb9a93. A tap on John 11:4's text opens its sheet with the verse under the title. A swipe over the text scrolls without opening a sheet.Level: medium (trunk, standard). Commit
22a91cddc8. Range 7fb9a930..22a91cdd. Files: qml/pages/ReadingPane.qml (read with qt/src/reading_model.rs verse_at and core/src/bible/block.rs ordinal_at).The fix is correct for the reported case: a tap on the right half of a character on the same line. It is also correct for the first character of a line (left half gives rect.x <= x, so p stays), for a wrapped line whose last visible character is followed by a hanging space (the offset after it is still on that line), and for LTR text. RTL would invert the rect.x comparison, but the modules are LTR, so it is not a defect here.
parent.charAtresolves, because the MouseArea's parent is the TextEdit that defines it.One low-severity finding is on the diff line. Nothing is outside the diff. I did not run it on a device.
@ -503,0 +557,4 @@// verse's last character).function charAt(x, y) {const p = positionAt(x, y)return p > 0 && positionToRectangle(p).x > x ? p - 1 : plow · A tap in the ragged-right gap past the end of a wrapped line. positionAt returns the line's end offset, which is the first offset of the next line. positionToRectangle(p).x is then the next line's left edge, which is not greater than x, so charAt returns p, the next line's first character. If a verse starts at that line start (a verse ending with its trailing space at the wrap), the tap opens the next verse. This is the same class of bug the commit fixes. The gap is much wider than a character's right half. Comparing the line as well fixes it:
r = positionToRectangle(p); return p > 0 && (r.y > y || r.x > x) ? p - 1 : p. I did not verify this on a device.Review 344 had one low finding: a tap in the gap past the end of a wrapped line.
positionAtreturns the next line's start there, so the tap could open the next verse. Fixed in0268678, following the review's suggestion.charAtnow also steps back when the cursor rectangle is on a lower line than the tap (r.y > y). The cases verse-text-tap, note-sheet and running-text pass. This was the delta round, so no further review round follows; the fix goes to the audit together with its finding.WIP: Verse sheet with TSK cross-references, Install TSK when missing (#13, 6d)to Verse sheet with TSK cross-references, Install TSK when missing (#13, 6d)Merge gate (medium), head
0268678.