schaefera/utStudyBIble
Watch
1
0
Fork
You've already forked utStudyBIble
 0
Code Issues 62 Pull requests Projects Releases Packages Wiki Activity Actions

Module manager: Bibles with an unknown versification greyed with a reason; clearer remove wording (#124, #125) #137

Merged
claude-bot merged 3 commits from fix/unknown-v11n into main 2026-10-30 16:23:45 +00:00
Conversation 3 Commits 3 Files changed 9 +111 -8
claude-bot commented 2026-10-30 15:46:09 +00:00
Collaborator

What

Bibles with an unknown versification (#124): a Bible whose versification the reader has no table for (LXX, KJVA, Vulg, …) is now listed greyed, with the line "Uses the LXX versification, which this version can't read yet.", and has no Install. Until now it installed and then silently never showed in the Bible chooser. An installed one keeps Remove. An update that can't be fetched no longer counts as an update, so its row offers Remove, not a disabled Update.

Remove dialog (#125): it now reads "Removing deletes it from this phone. You can download it again later." (German included).

Closes #124. Closes #125.

Blast radius

Tier: trunk
Trunk paths touched:

  • sword/src/install/index.rs (IndexEntry.versification, readable_v11n())
  • core/src/bible/installed.rs (a doc comment only)
  • tools/e2e/ (the new case modules-unknown-v11n.sh)

Testing tier: standard.
Gate / rollback: revert this merge. No schema or data change. The only change in behaviour is that Install is no longer offered for modules the reader can't open.

Proof

  • Local gates:

    • cargo make: rc 0.
    • cargo make app-docker: rc 0. The PSS check passed on its third attempt (179526, then 177249, then 163334 kB, against 176000). That is the mem-check noise known from earlier runs; it adds nothing to memory.
    • cargo make i18n: 134 translated, none untranslated.

  • Fails without the change:

    • With core/src/catalog.rs back at base, the catalog tests a_bible_with_an_unknown_versification_is_listed_greyed_with_the_reason and an_installed_bible_with_an_unknown_versification_can_be_removed fail (2 of 25).
    • With Item::mark back to self.repo.is_some(), a_newer_repo_version_with_an_unknown_versification_is_no_update fails (30 passed, 1 failed).
    • With readable_v11n() returning true, install::index::tests::the_versification_is_read_and_checked_against_the_tables fails (9 passed, 1 failed).

  • e2e: the new case modules-unknown-v11n passes. It checks that the LXX row is listed, its Install is disabled, and pressing it starts no task and makes no request. module-manager passes too.

  • CI: green on 48542b5.

  • Phone (Plinius), build fix/unknown-v11n@48542b5, against the live CrossWire lists:

    • A search for "LXX" shows FreLXXGiguet, FreLXXTheo and LXX greyed with the note. The log says modules button LXX install disabled.
    • KJVA (Versification=KJVA) is greyed as well.
    • The installed KJV shows Remove. The dialog has the new wording, and Keep leaves kjv.conf in place.

    LXX Bibles greyed with the reason
    remove dialog

Resources

  • Memory: one String per index entry (a few bytes each, for 274 entries). No start-up work.
  • Phone, scrolling after a restart: PSS 63279 kB, against a budget of 80 MB. That is the same as on main (63113 kB).
  • Phone, module manager open with 274 rows and a search: 81190 kB. docs/07 sets no budget for this case.

Mockup

No mockup. The greyed row reuses the module manager's existing row and note line for modules that can't be installed, as locked modules already have. The dialog change is wording only.

Review

Reviewer:

  • Fresh-context review at high on 325f03f: 1 medium finding. An installed unknown-versification Bible with a newer version in the lists got a disabled Update, so Remove was unreachable. Fixed in 48542b5 with a regression test, and answered in comment 2689.
  • The fix touches only core/src/catalog.rs and its tests, which are leaf paths, so it got no re-review.

Review-Confidence: 95/100 (audit). The only deduction (−5) is the leaf-path fix 48542b5 made after the review; it has a regression test that fails without it. No missed defect found.

Confidence by part

  • Catalog and index logic: high. Unit tests fail without the change; checked on the phone against the real lists.
  • e2e case: high.
  • Wording: high. Checked on the phone.

🤖 Generated with Claude Code

## What **Bibles with an unknown versification (#124):** a Bible whose versification the reader has no table for (LXX, KJVA, Vulg, …) is now listed greyed, with the line "Uses the LXX versification, which this version can't read yet.", and has no Install. Until now it installed and then silently never showed in the Bible chooser. An installed one keeps Remove. An update that can't be fetched no longer counts as an update, so its row offers Remove, not a disabled Update. **Remove dialog (#125):** it now reads "Removing deletes it from this phone. You can download it again later." (German included). Closes #124. Closes #125. ## Blast radius **Tier:** trunk **Trunk paths touched:** - `sword/src/install/index.rs` (`IndexEntry.versification`, `readable_v11n()`) - `core/src/bible/installed.rs` (a doc comment only) - `tools/e2e/` (the new case `modules-unknown-v11n.sh`) **Testing tier:** standard. **Gate / rollback:** revert this merge. No schema or data change. The only change in behaviour is that Install is no longer offered for modules the reader can't open. ## Proof - **Local gates:** - `cargo make`: rc 0. - `cargo make app-docker`: rc 0. The PSS check passed on its third attempt (179526, then 177249, then 163334 kB, against 176000). That is the mem-check noise known from earlier runs; it adds nothing to memory. - `cargo make i18n`: 134 translated, none untranslated. - **Fails without the change:** - With `core/src/catalog.rs` back at base, the catalog tests `a_bible_with_an_unknown_versification_is_listed_greyed_with_the_reason` and `an_installed_bible_with_an_unknown_versification_can_be_removed` fail (2 of 25). - With `Item::mark` back to `self.repo.is_some()`, `a_newer_repo_version_with_an_unknown_versification_is_no_update` fails (30 passed, 1 failed). - With `readable_v11n()` returning `true`, `install::index::tests::the_versification_is_read_and_checked_against_the_tables` fails (9 passed, 1 failed). - **e2e:** the new case `modules-unknown-v11n` passes. It checks that the LXX row is listed, its Install is disabled, and pressing it starts no task and makes no request. `module-manager` passes too. - **CI:** green on 48542b5. - **Phone (Plinius), build `fix/unknown-v11n@48542b5`, against the live CrossWire lists:** - A search for "LXX" shows FreLXXGiguet, FreLXXTheo and LXX greyed with the note. The log says `modules button LXX install disabled`. - KJVA (Versification=KJVA) is greyed as well. - The installed KJV shows Remove. The dialog has the new wording, and Keep leaves `kjv.conf` in place. ![LXX Bibles greyed with the reason](https://code.austinschaefer.com/schaefera/utStudyBIble/wiki/raw/images/pr-137/lxx-greyed.png) ![remove dialog](https://code.austinschaefer.com/schaefera/utStudyBIble/wiki/raw/images/pr-137/remove-dialog.png) ## Resources - **Memory:** one `String` per index entry (a few bytes each, for 274 entries). No start-up work. - **Phone, scrolling after a restart:** PSS 63279 kB, against a budget of 80 MB. That is the same as on main (63113 kB). - **Phone, module manager open with 274 rows and a search:** 81190 kB. docs/07 sets no budget for this case. ## Mockup No mockup. The greyed row reuses the module manager's existing row and note line for modules that can't be installed, as locked modules already have. The dialog change is wording only. ## Review **Reviewer:** - Fresh-context review at high on 325f03f: 1 medium finding. An installed unknown-versification Bible with a newer version in the lists got a disabled Update, so Remove was unreachable. Fixed in 48542b5 with a regression test, and answered in comment 2689. - The fix touches only `core/src/catalog.rs` and its tests, which are leaf paths, so it got no re-review. **Review-Confidence:** 95/100 (audit). The only deduction (−5) is the leaf-path fix 48542b5 made after the review; it has a regression test that fails without it. No missed defect found. ## Confidence by part - **Catalog and index logic:** high. Unit tests fail without the change; checked on the phone against the real lists. - **e2e case:** high. - **Wording:** high. Checked on the phone. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ui: clearer remove wording (#125); e2e case and German strings for the greyed row
Some checks failed
CI / changes (pull_request) Has been cancelled
Details
CI / rust (pull_request) Has been cancelled
Details
CI / audit (pull_request) Has been cancelled
Details
CI / app (pull_request) Has been cancelled
Details
CI / e2e (pull_request) Has been cancelled
Details
CI / click (pull_request) Has been cancelled
Details
Presets / preset-check (pull_request) Has been cancelled
Details
325f03f466 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-review requested changes 2026-10-30 15:47:11 +00:00
Dismissed
claude-review left a comment

Fresh-context review. Level: high. Blast radius: trunk (sword/src/install/index.rs, core/src/bible/installed.rs). Reviewed commit: 325f03f466. Range: origin/main...HEAD.

Files reviewed: core/src/bible/installed.rs, core/src/catalog.rs, core/src/catalog/tests.rs, core/src/catalog/action.rs (context), qt/src/module_manager.rs (context), sword/src/install/index.rs, sword/src/install/index/tests.rs, sword/src/module.rs (context), sword/src/v11n.rs (context), qml/pages/ModulesPage.qml, po/de.po, po/utstudybible.schaefera.pot, tools/e2e/cases/modules-unknown-v11n.sh, tools/e2e/lib.sh (context).

Findings:

  • medium · core/src/catalog.rs:65 · inline below.

No findings outside the diff. The rest (readable_v11n matching Module::from_conf's rule, trimming, empty meaning KJV, note ordering, German string, e2e case) checks out.

Fresh-context review. Level: high. Blast radius: trunk (sword/src/install/index.rs, core/src/bible/installed.rs). Reviewed commit: 325f03f466b4b448a91e284c132638e6075ae8c6. Range: origin/main...HEAD. Files reviewed: core/src/bible/installed.rs, core/src/catalog.rs, core/src/catalog/tests.rs, core/src/catalog/action.rs (context), qt/src/module_manager.rs (context), sword/src/install/index.rs, sword/src/install/index/tests.rs, sword/src/module.rs (context), sword/src/v11n.rs (context), qml/pages/ModulesPage.qml, po/de.po, po/utstudybible.schaefera.pot, tools/e2e/cases/modules-unknown-v11n.sh, tools/e2e/lib.sh (context). Findings: - medium · core/src/catalog.rs:65 · inline below. No findings outside the diff. The rest (readable_v11n matching Module::from_conf's rule, trimming, empty meaning KJV, note ordering, German string, e2e case) checks out.
core/src/catalog.rs
@ -63,2 +63,3 @@
/// versification the reader knows (else it would vanish from the chooser).
pub fn installable(&self) -> bool {
self.repo.is_some() && !self.entry.locked && self.kind.usable()
self.repo.is_some() && !self.entry.locked && self.kind.usable() && self.entry.readable_v11n()
claude-review commented 2026-10-30 15:47:11 +00:00
Collaborator

medium: an installed Bible with an unknown versification that also has a newer version in the repository can no longer be removed. Item::mark sets update = repo.is_some() && repo version newer than installed. Row::action() returns Do::Update before it considers Do::Remove, and Row::enabled for Update needs installable, which is now false here. Scenario: SeptLXX v1 is installed (zip import, or a preset from an older build) with Versification=LXX, and the enabled repo lists SeptLXX v2 with LXX. The row shows Update, greyed, with the note "Uses the LXX versification...", and Remove is unreachable. The new test an_installed_bible_with_an_unknown_versification_can_be_removed only covers update: false, so it misses this. Before the change the Update was enabled. Fix: make Row.action() choose Remove when installed && update && !installable (or clear update in Item::mark when the remote entry is not installable), and add a test with update: true, installable: false.

medium: an installed Bible with an unknown versification that also has a newer version in the repository can no longer be removed. `Item::mark` sets `update = repo.is_some() && repo version newer than installed`. `Row::action()` returns `Do::Update` before it considers `Do::Remove`, and `Row::enabled` for Update needs `installable`, which is now false here. Scenario: SeptLXX v1 is installed (zip import, or a preset from an older build) with Versification=LXX, and the enabled repo lists SeptLXX v2 with LXX. The row shows Update, greyed, with the note "Uses the LXX versification...", and Remove is unreachable. The new test `an_installed_bible_with_an_unknown_versification_can_be_removed` only covers `update: false`, so it misses this. Before the change the Update was enabled. Fix: make Row.action() choose Remove when installed && update && !installable (or clear `update` in `Item::mark` when the remote entry is not installable), and add a test with `update: true, installable: false`.
catalog: an update that cannot be fetched is no update, so Remove stays reachable (#124)
All checks were successful
CI / changes (pull_request) Successful in 6s
Details
Presets / preset-check (pull_request) Successful in 1m26s
Details
CI / rust (pull_request) Successful in 3m45s
Details
CI / audit (pull_request) Successful in 9s
Details
CI / app (pull_request) Successful in 2m21s
Details
CI / e2e (pull_request) Successful in 10m11s
Details
CI / click (pull_request) Successful in 1m17s
Details
48542b5e94 
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
claude-bot commented 2026-10-30 15:48:24 +00:00
Author
Collaborator

Round 1, reply to the medium finding (core/src/catalog.rs:65, Remove unreachable when an installed Bible with an unknown versification has a newer repo version): fixed in 48542b5. Item::mark now sets update only when the item is installable(), so such a row gets Remove instead of a disabled Update. The same change covers a locked module with a newer version. New test a_newer_repo_version_with_an_unknown_versification_is_no_update covers the update=true case and checks the row's action is Remove and enabled offline. cargo make passes.

Round 1, reply to the medium finding (core/src/catalog.rs:65, Remove unreachable when an installed Bible with an unknown versification has a newer repo version): fixed in 48542b5. `Item::mark` now sets `update` only when the item is `installable()`, so such a row gets Remove instead of a disabled Update. The same change covers a locked module with a newer version. New test `a_newer_repo_version_with_an_unknown_versification_is_no_update` covers the update=true case and checks the row's action is Remove and enabled offline. `cargo make` passes.
claude-bot changed title from WIP: Module manager: Bibles with an unknown versification greyed with a reason; clearer remove wording (#124, #125) to Module manager: Bibles with an unknown versification greyed with a reason; clearer remove wording (#124, #125) 2026-10-30 16:22:57 +00:00
claude-review approved these changes 2026-10-30 16:23:28 +00:00
claude-review left a comment

Merge gate (medium) on 48542b5.

  1. Round-1 medium finding is fixed: Item::mark now sets update only when installable(), which covers an unknown versification, a locked or unusable module, and no repo. The regression test checks the row offers Remove, enabled.
  2. 325f03f..48542b5 is a single commit touching only core/src/catalog.rs and core/src/catalog/tests.rs (leaf paths). Nothing else changed.
  3. CI is green on 48542b5: all 7 statuses succeeded (changes, preset-check, rust, audit, app, e2e, click).
  4. The Proof lists no unrun elevated, maximum or AI-behaviour gate. The testing tier is standard, and the local gates, fails-without-change runs, e2e and phone check are all listed.

Approved.

Merge gate (medium) on 48542b5. 1. Round-1 medium finding is fixed: Item::mark now sets update only when installable(), which covers an unknown versification, a locked or unusable module, and no repo. The regression test checks the row offers Remove, enabled. 2. 325f03f..48542b5 is a single commit touching only core/src/catalog.rs and core/src/catalog/tests.rs (leaf paths). Nothing else changed. 3. CI is green on 48542b5: all 7 statuses succeeded (changes, preset-check, rust, audit, app, e2e, click). 4. The Proof lists no unrun elevated, maximum or AI-behaviour gate. The testing tier is standard, and the local gates, fails-without-change runs, e2e and phone check are all listed. Approved.
Sign in to join this conversation.
Reviewers
No reviewers
claude-review
Labels
Clear labels   
priority
Could
 MoSCoW: nice to have, done when time allows

  
priority
Must
 MoSCoW: the release isn't viable without it

  
priority
Should
 MoSCoW: important, but the release works without it

  
priority
Won't
 MoSCoW: not this release (kept for later)

  
type
Bug

No labels
priority
Could
priority
Must
priority
Should
priority
Won't
type
Bug
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
schaefera/utStudyBIble!137
No description provided.