Quick settings: night mode, text size, serif and verse numbers (#12 part 1) #36
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!36
Loading…
Reference in a new issue
No description provided.
Delete branch "phase1/quick-settings"
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 1 of #12. Parts 2 (F3 red letters) and 3 (F4 running text, sliders) follow, so this does not close it.
What
An Aa button at the right of the reading footer opens a bottom sheet with four controls: night mode, text size (80–200 %), a Sans or Serif face, and verse numbers. Each change shows at once, is saved at once by the core, and the verse at the top stays in place. At start the core sends every setting, saved or default, before the text, so night mode doesn't flash white. All sizes and colours now come from one
Themesingleton (qml/theme/).Blast radius
Tier: trunk
Trunk paths touched:
core/src/worker.rs,core/src/worker/session.rs,core/src/worker/session/jump.rs,qt/src/reading_model.rs,qml/pages/ReadingPane.qml,qml/Main.qml,tools/mem-check/run.shTesting tier: standard
Gate / rollback: The defaults are the old look (day, 100 %, sans, verse numbers on), so nothing changes until a setting is touched. To undo, revert this merge. The settings are rows in the existing settings table, with no migration; an older build ignores them.
Resources (docs/07): The sheet is in a
Loaderand is compiled on first use, not at start. At start there are 5 setting events (there was 1), before the Bible loads. mem-check peak PSS with the sheet open is 143.5 MB (normal) and 147.0 MB (slow), against the 165 MB baseline; with the picker it is 143.6 MB. Text appears after 316 ms in the slow profile. On the phone the window loaded after 306 ms.Proof
SWORD_TEST_PATH=~/.cache/utsb-sword cargo makeexits 0: utsb-core 108 passed, plus fmt, clippy, deny and audit.cargo make app-dockerexits 0: qmllint, the -Werror build, clang-tidy, the German catalogue (all 29 strings translated) and mem-check in both profiles. The new e2e (taps on the Aa sheet) prints:mem-check: Aa sheet: night on, text at 200 % (top verse 88 to 301 px), top 'BSB 27197 0.432' before and 'BSB 27197 0.432' aftermem-check: after kill -9: night at line 7, size at 8, text at 9(the settings come back before the text)a_number_is_clamped_and_else_its_default. A wrong flag default failsa_flag_is_one_or_zero_and_else_its_default. Settings sent after the text failsettings_are_saved_at_once_and_come_back_at_start.FAIL: a bigger text size moved the verse at the top(27197 0.432 → 27194 0.727). The verse font pinned to 18 px givesFAIL: ... didn't make the top verse taller ('88 88').palette.windowunbound givesFAIL: night mode didn't change the window colour.e6f8a69: rust, audit, app, click.e6f8a69is installed on the Plinius. Launched from adb, the journal showsphase1/quick-settings@e6f8a694c,window loaded after 306 ms,restored KJV 173 0.809,showing Gen 7, and no QML warnings. Nothing has been tried by touch yet. On the phone, try:Review
Reviewer: Three fresh-context
code-reviewrounds: high one9469f7, medium ondb13a9d, and a medium delta one6f8a69. There were 5 findings: 1 medium and 4 low.Review-Confidence: 85/100. The serif and verse-number toggles are not exercised by any automated or phone check yet; everything else has proof.
Confidence by part
settings.rs, sent before the text): high. Unit and worker tests, all shown to fail without the change.🤖 Generated with Claude Code
Review level: high (trunk). Commit:
e9469f7ec9. Range: origin/main...HEAD.No high or medium findings. Low findings are inline.
Files reviewed: core/src/lib.rs, core/src/settings.rs, core/src/worker.rs, core/src/worker/session.rs, core/src/worker/session/jump.rs, qt/src/reading_model.rs, qml/Main.qml, qml/pages/ReadingPane.qml, qml/pages/QuickSettings.qml, qml/pages/PickerPage.qml, qml/pages/SettingsPage.qml, qml/qml.qrc, qml/theme/Theme.qml, qml/theme/qmldir, po/de.po, po/utstudybible.schaefera.pot (po skimmed only).
@ -0,0 +58,4 @@// Each step is shown and saved at once.onMoved: if (value !== sheet.reading.textScale) sheet.reading.setTextScale(value)}Label {low: the slider shows the core's echoed value. If the worker lags during a fast drag, the handle can snap back to the previous step until the echo arrives. Each step also costs one DB write on the worker. Fix if it shows on the phone: save on release (onPressedChanged) and preview through a local value.
@ -0,0 +22,4 @@// The reading text, and the lines above each chapter.readonly property int textSize: Math.round(18 * textScale / 100)readonly property int ornamentSize: Math.round(22 * textScale / 100)readonly property int bookSize: Math.round(26 * textScale / 100)low: "Liberation Serif" has no fallback. On a device without it, Qt substitutes silently and the Serif button does nothing visible. A family list or a check against Qt.fontFamilies() would grey the button out, as the module-data rule does.
Answers to review 231 (two lows, no code change):
reading.textScalechanges, and each echo carries the value that was just set. The one possible case is a late echo of an earlier step during a very fast drag. That moves the handle back one step for a frame, and the next echo puts it right. A step is one small SQLite upsert on the worker, about 13 steps across the whole range, well inside the "save within 1 s" rule. Keeping the core as the single source of truth is worth that.fc-match "Liberation Serif"on the phone (Plinius, UT 24.04) givesLiberationSerif-Regular.ttf. Other Linux phones come later, and their packaging will declare the font dependency. So this does not fail on any platform we ship to now.Level: medium (trunk: tools/mem-check/, qml/pages/ReadingPane.qml). Commit:
db13a9dbd5. Range: e9469f7ec97400a40f9f232cd4ec4055329a6071..HEAD (one commit).Files reviewed:
Findings: see inline comments (1 medium, 2 low). Nothing outside the diff lines. Not run: the script itself (needs Xvfb, the built app and module data); the tap coordinates were not checked against a rendered layout.
@ -392,0 +412,4 @@tap 520 940tap 270 751waitlines "$log" 'utsb: night true' 1tap 495 824low: the taps (270,751 / 495,824) depend on the sheet's implicit height and on the slider's right end sitting at or beyond x=495. If the right 'A' label or the margin starts before 495, the tap may land on the label and no 'utsb: text scale 200' arrives (a clean timeout), but a smaller sheet (e.g. a font or margin change) would instead hit the wrong control and fail with the generic 'no ... after 8 s'. Consider tapping via the objectNames or logging each control's centre.
@ -392,0 +421,4 @@sleep 2.5resized=$(lastsaved "$log")echo "mem-check: Aa sheet: night on, text at 200 %, top '$before' before and '$resized' after"if ! samespot "$before" "$resized" 0.05; thenmedium: the size check can pass when the size change does nothing. If Theme.textScale is never applied (the keepTop callback broken, or the Theme binding dropped), verse heights don't change, the top verse and fraction stay the same, samespot passes, and the only other assertion is the 'utsb: text scale 200' log line. That line is printed from the property bound to the core, not from anything the Theme or the layout did. The same goes for 'utsb: night true': it proves the core echoed the value, not that the colours changed. Assert that something visible changed, e.g. log Theme.textScale / Theme.night (or a delegate's height) in the same handler after the assignment, or check that the verse count in view dropped.
@ -392,0 +429,4 @@samplekill -9 "$pid"wait "$pid" 2>/dev/null || truelog=$work/app4.loglow: the restarted app runs as "$app", not "$launch "$app"", so in the SLOW=1 run it is no longer pinned to the two CPUs. The post-kill ordering check (night and size before the text) is then made on a faster profile than the one it is meant to cover, hiding a race that only shows when the core is slow. The app3 restart above does the same, so this extends an existing gap.
Answers to review 232, fixed in
e6f8a69:pane.palette.window, and mem-check requires#121212.top verse 88 to 301 px.FAIL: text at 200 % didn't make the top verse taller ('88 88'). Unbindingpalette.windowin Main.qml givesFAIL: night mode didn't change the window colour(window #ffffff). Making keepTop a no-op (checked ondb13a9d) givesFAIL: a bigger text size moved the verse at the top(BSB 27197 0.432→BSB 27194 0.727).waitlinestimeout prints the whole app log, and that log shows which line never came. Positions found through objectName would need a test hook in the app.$launch: fixed. It now starts with$launch, so the slow run stays pinned to two CPUs. app3 is code that was there before this PR.Delta review, level medium, trunk blast radius. Commit
e6f8a694c3, range db13a9dbd554d9d620df43d134715fabf2b5c8a9..HEAD (one commit).No findings. The changes I checked:
$launch "$app"for app4 matches the other launches. taskset execs, so $pid is still the app.Files reviewed:
WIP: Quick settings: night mode, text size, serif and verse numbers (#12 part 1)to Quick settings: night mode, text size, serif and verse numbers (#12 part 1)Merge gate, trunk, standard testing tier. Head
e6f8a694c3. Approval supersedes review 232.e6f8a69. The e2e now logs the palette window colour (#121212 is checked) and the top verse height (it must grow more than 1.5x), so it reads what the pane shows. The low about the restart is fixed too ($launch "$app").e6f8a69): no findings.