Modules redesign (#148 step 4) #154
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!154
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/redesign-modules"
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
The module manager in the Lapis style (board E8): rows with a thin progress bar, installed modules first, filter chips that open a choice sheet, and a sheet per module with its details and actions.
The Remove and Import dialogs are restyled through a new
PageDialog, which lays itself out because Suru broke the stock layout on the phone.Differences from E8: a sheet instead of a detail page (the chevron or any row opens it, a disabled Install too); Remove on update rows is in the sheet; Cancel stays in the row's 72 px bar; the note stays in the row. E8 doesn't draw the ⋮ menu (Import, Refresh), which is unchanged here and gets its own PR next.
Blast radius
Tier: trunk
Trunk paths touched:
qt/src/module_manager.rs,tools/e2e/lib.sh,tools/e2e/cases/module-manager.sh,tools/e2e/cases/typed-reference.shTesting tier: standard (also: ship on sight, for the styling)
Gate / rollback: revert this merge. No schema or stored data changes.
Budgets (docs/07): no measurable change.
app-dockerpassed at 173.6/176.4 MB (normal/slow). A/B over six rounds, base vs branch, sorted MB: normal 186 186 187 195 197 199 vs 182 182 184 197 198 200; slow 181 182 189 196 199 201 vs 185 189 195 197 197 202. The swing is the same binary's noise (#156).Proof
7b319ff(run 321):changes,rust,audit,app,e2eandclickall green.033b7b5, one case each:scroll-chapters, first attempt: a step of 18 verses against a limit of 15. Ten local runs passed with steps of at most 9, and it passed in the two later CI runs. Load on the runner is the likely cause, not proven: #157 is open and this could fail again.typed-reference, re-run: a close tap straight after a line logged when the page is made, so it could be dropped during the transition. Fixed in7b319ff. Not reproduced locally (8 of 8 passed before the fix, 5 of 5 after), so the cause comes from the failed log and the code; the fix passed its one CI run.picker-currentalso fails intermittently, on base too (#155). It passed in all these CI runs.core/src/catalogon purpose: no installed-first sort fails the first new test; no bidi isolate fails the second;if falsefor the update branch fails the second.module-manager,import-url,modules-offline,install-queue-cancelpass locally and in CI.ute2escreenshots: page, sheet, Import and Remove dialogs. The Remove dialog's buttons overlapped its text there, fixed in033b7b5and checked again. Owner's phone check: "Works, go for it". That check was on033b7b5;7b319ffchanges only an e2e case.Review
Reviewer: fresh-context review at high on
0f73646: 3 low findings, 3 fixed (aa951f5), 0 declined, 0 deferred. Re-review at medium of0f73646..7b319ff: no findings, by reading the code only.Review-Confidence: 100/100, every changed trunk file was reviewed, all findings are fixed, CI is green on
7b319ffand the new tests fail without the change. It scores the review, not the code.Confidence by part
core/src/catalog(sort, meta line): high, covered by tests shown to fail without the change.PageDialog: medium. Its layout works around Suru's behaviour, checked on the phone for the two dialogs that use it; the next dialog built on it should be checked there too.typed-referencefix: medium. The cause is inferred, not reproduced.🤖 Generated with Claude Code
Review by claude-review. Level: high. Blast radius: trunk. Commit
0f73646. Range: origin/main...HEAD.Files reviewed: core/src/catalog.rs, core/src/catalog/browser.rs, core/src/catalog/kind.rs, core/src/catalog/tests.rs, docs/10-design.md, po/de.po, po/lectern.aps.pot, qml/controls/BottomSheet.qml, ChoiceSheet.qml, FilterChip.qml, PageDialog.qml, TaskRow.qml, ThinProgress.qml, qmldir, qml/pages/ModuleSheet.qml, ModulesPage.qml, qml/qml.qrc, qt/src/module_manager.rs, tools/e2e/cases/module-manager.sh, tools/e2e/lib.sh (plus the e2e cases that call e2e_modules_action, Browser/Kind/action.rs, Installer fraction accessors).
No high or medium findings. The catalog tests pass (33 passed). Low findings, all on the diff, are inline below.
Findings outside the diff lines:
@ -84,1 +84,3 @@/// The line under the name: language, version and size on disk./// The row's line under the title: name, kind, then language and size/// on disk, or the version an update brings.pub fn meta(&self) -> String {low · When a module has no description, the row title falls back to the module name, and
meta()also starts with the name. The row then reads 'KJV' over 'KJV · Bible · ...'. Imported zips often have an empty Description. Leave the name out of the meta line when the title already is the name.low · For a 'remove' row,
row.doButtonis the chevron Label, and a Label is always enabled. The 'modules button NAME remove enabled/disabled' hook therefore no longer reflectsmodules.actionEnabled. Usemodules.actionEnabled(row.index, row.phase, page.online)here so the hook keeps its meaning.Answers to the review of
0f73646. All three low findings are fixed inaa951f5.Row with no screen reader name (
ModulesPage.qml, in the review body): fixed. The row's ItemDelegate now hastext: row.title, which isn't drawn because the content is ours; it names the row for a screen reader.The fixes change no trunk code (
qml/pages/,core/src/catalog*), so no re-review is due.033b7b5is a later fix found on the phone:PageDialog(leaf) lays out its content, buttons and paddings itself.@ -85,0 +84,4 @@/// The row's line under the title: name (unless the title is the name,/// for a module with no description), kind, then language and size on/// disk, or the version an update brings.pub fn meta(&self) -> String {Fixed in
aa951f5: with no description the title is the name, someta()leaves the name out. The new assertion is inthe_row_says_name_kind_language_and_size_or_the_update.@ -96,3 +98,3 @@const state = row.phase === "failed" ? "failed" : row.busy ? "busy" : row.installed ? "installed" : "available"lines.push(row.name + " " + state + (row.update ? " update" : "") + " @ " + Theme.where(row.doButton))buttons.push(row.name + " " + row.todo + (row.doButton.enabled ? " enabled" : " disabled"))buttons.push(row.name + " " + row.todo + (modules.actionEnabled(row.index, row.phase, page.online) ? " enabled" : " disabled"))Fixed in
aa951f5: the hook readsmodules.actionEnabled(row.index, row.phase, page.online), somodules-offline.sh's check can fail again.Re-review, level medium. Commit
7b319ffb1d. Range 0f73646..HEAD.Files reviewed: core/src/catalog.rs, core/src/catalog/tests.rs, qml/controls/PageDialog.qml, qml/pages/ModulesPage.qml, tools/e2e/cases/typed-reference.sh
No high or medium findings, and nothing outside the diff.
WIP: Modules redesign (#148 step 4)to Modules redesign (#148 step 4)Merge gate: approved on
7b319ff.aa951f5.7b319ff, the commit review 404 covered. No commits after the last review round.