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

Sheets keep the system theme after a live light/dark switch (#13) #139

Merged
claude-bot merged 2 commits from fix/sheet-theme into main 2026-10-30 17:39:36 +00:00
Conversation 3 Commits 2 Files changed 6 +56
claude-bot commented 2026-10-30 17:04:53 +00:00
Collaborator

What

On Ubuntu Touch, a pop-up missed a live switch between the light and dark system themes: rows it created afterwards came out in the old theme. You could see this in "Show all" of a verse's cross-references (owner's remark on #13). The new PopupTheme (qml/platform/ut/) sets the theme of the item a pop-up belongs to on the pop-up whenever that theme changes. It is used by the bottom sheets (the verse and note sheets) and the Bible chooser.

Refs #13 (one of the owner's two remarks; the other, note placement, waits on a decision).

Blast radius

Tier: leaf
Trunk paths touched: none. The change is in qml/controls/BottomSheet.qml, qml/pages/BibleChooser.qml, qml/platform/* and qml/qml.qrc.
Testing tier: standard.
Gate / rollback: revert this merge. No data or behaviour change beyond the colours.

Proof

  • Local gates:
    • cargo make app-docker: done. The -Werror build, clang-tidy and translations passed. Mem-check peak PSS was 162652 kB on the normal profile and 171765 kB on the slow one (budget 176000).
    • E2E_CASES="note-sheet verse-sheet verse-sheet-scroll switch-bible" cargo make e2e-docker: 4 passed.
    • The desktop has no Suru style, so ut/PopupTheme.qml doesn't load there and the bug can't show. CI can't cover it.
  • CI: green on f97630b (changes, rust, audit, app, e2e, click).
  • Phone (Plinius), build fix/sheet-theme:

    • Before (main): start the app in light mode, open a verse, switch the system to dark, then tap "Show all". The rows are light while the rest of the app is dark.

      before

    • After (7ccd9b1): the same steps, checked by the owner. The rows are dark. The log shows popup theme 0, then 1 at the switch. Then, on f97630b, the owner switched the theme with the sheet closed, both light→dark and dark→light, and "Show all" matched the theme each time.

      after

    • On load there are no QML warnings. The first try, a plain Binding, logged "Property 'theme' does not exist", so the theme is set from script.

Resources

One hidden item per pop-up (three), with no start-up work and no measurable memory.

Mockup

No mockup: the fix brings back the colours the theme already has.

Review

  • Fresh-context review (medium) on 7ccd9b1, review 364: three low findings, answered in comment 2711.
    • A closed pop-up kept a stale theme: fixed in f97630b and checked on the phone in both directions.
    • Bible chooser rows built before the theme was applied: covered by the same fix.
    • No e2e case: CI can't run one, because it has no Suru style.
  • Review-Confidence: not audited (leaf).

Confidence by part

  • Sheets after a live switch: high. Checked on the phone in both directions, with the sheet open and closed.
  • Bible chooser: medium. It uses the same mechanism but wasn't looked at separately after a switch.

🤖 Generated with Claude Code

## What On Ubuntu Touch, a pop-up missed a live switch between the light and dark system themes: rows it created afterwards came out in the old theme. You could see this in "Show all" of a verse's cross-references (owner's remark on #13). The new `PopupTheme` (`qml/platform/ut/`) sets the theme of the item a pop-up belongs to on the pop-up whenever that theme changes. It is used by the bottom sheets (the verse and note sheets) and the Bible chooser. Refs #13 (one of the owner's two remarks; the other, note placement, waits on a decision). ## Blast radius **Tier:** leaf **Trunk paths touched:** none. The change is in `qml/controls/BottomSheet.qml`, `qml/pages/BibleChooser.qml`, `qml/platform/*` and `qml/qml.qrc`. **Testing tier:** standard. **Gate / rollback:** revert this merge. No data or behaviour change beyond the colours. ## Proof - **Local gates:** - `cargo make app-docker`: done. The -Werror build, clang-tidy and translations passed. Mem-check peak PSS was 162652 kB on the normal profile and 171765 kB on the slow one (budget 176000). - `E2E_CASES="note-sheet verse-sheet verse-sheet-scroll switch-bible" cargo make e2e-docker`: 4 passed. - The desktop has no Suru style, so `ut/PopupTheme.qml` doesn't load there and the bug can't show. CI can't cover it. - **CI:** green on f97630b (changes, rust, audit, app, e2e, click). - **Phone (Plinius), build `fix/sheet-theme`:** - **Before (main):** start the app in light mode, open a verse, switch the system to dark, then tap "Show all". The rows are light while the rest of the app is dark. ![before](https://code.austinschaefer.com/schaefera/utStudyBIble/wiki/raw/images/pr-139/before-show-all.png) - **After (7ccd9b1):** the same steps, checked by the owner. The rows are dark. The log shows `popup theme 0`, then `1` at the switch. Then, on f97630b, the owner switched the theme with the sheet closed, both light→dark and dark→light, and "Show all" matched the theme each time. ![after](https://code.austinschaefer.com/schaefera/utStudyBIble/wiki/raw/images/pr-139/after-show-all.png) - On load there are no QML warnings. The first try, a plain `Binding`, logged "Property 'theme' does not exist", so the theme is set from script. ## Resources One hidden item per pop-up (three), with no start-up work and no measurable memory. ## Mockup No mockup: the fix brings back the colours the theme already has. ## Review - **Fresh-context review (medium) on 7ccd9b1, review 364:** three low findings, answered in comment 2711. - A closed pop-up kept a stale theme: fixed in f97630b and checked on the phone in both directions. - Bible chooser rows built before the theme was applied: covered by the same fix. - No e2e case: CI can't run one, because it has no Suru style. - **Review-Confidence:** not audited (leaf). ## Confidence by part - **Sheets after a live switch:** high. Checked on the phone in both directions, with the sheet open and closed. - **Bible chooser:** medium. It uses the same mechanism but wasn't looked at separately after a switch. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
ui: sheets keep the window's theme after a live switch, so Show all rows are dark (#13)
Some checks failed
CI / changes (pull_request) Successful in 10s
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
7ccd9b1a17 
On Ubuntu Touch a popup's Suru style missed a live light/dark switch:
rows it created afterwards (Show all of the cross-references) came out
light. PopupTheme (platform/ut) sets the window's theme on the bottom
sheets and the Bible chooser whenever it changes.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-review reviewed 2026-10-30 17:05:46 +00:00
claude-review left a comment

Level: medium. Blast radius: leaf. Commit 7ccd9b1. Range: origin/main...HEAD.
Files: qml/controls/BottomSheet.qml, qml/pages/BibleChooser.qml, qml/platform/PopupTheme.qml, qml/platform/ut/PopupTheme.qml, qml/platform/qmldir, qml/qml.qrc (also read: platform/SystemColours.qml, platform/ut/SystemColours.qml).

No high or medium findings. The wrapper follows the SystemColours pattern. The desktop load failure is handled, because item is never set. The sheet id exists in BibleChooser. The qmldir and qrc entries are present.

Findings outside the diff:

  • low · qml/platform/ut/PopupTheme.qml:7 · The theme fallback depends on the item's window. A closed popup's content is not in a window, so theme reads the item's own Suru.theme (probably Light), not the window's. apply() runs only when theme changes. If the popup keeps an explicit theme from an earlier open and the item's pre-open value equals the window's new theme, no change fires and the stale explicit theme stays. I could not verify this without Suru on the desktop. Worth one phone check: dark, open and close, switch to light, reopen the Bible chooser, then repeat in the other direction.
  • low · qml/pages/BibleChooser.qml:26 · The rows are rebuilt in onAboutToShow. If that fires before the content item is attached to the window, the rows are made before apply() runs. The "reopens in the new theme" claim then relies on Suru re-theming existing children, which the comments say it does for live switches. Needs the same phone check.
  • low · tools/e2e · The utsb: popup theme N log hook has no case that consumes it, so nothing guards the regression in CI. A switch-theme, reopen, Show all case would cover it.
Level: medium. Blast radius: leaf. Commit 7ccd9b1. Range: origin/main...HEAD. Files: qml/controls/BottomSheet.qml, qml/pages/BibleChooser.qml, qml/platform/PopupTheme.qml, qml/platform/ut/PopupTheme.qml, qml/platform/qmldir, qml/qml.qrc (also read: platform/SystemColours.qml, platform/ut/SystemColours.qml). No high or medium findings. The wrapper follows the SystemColours pattern. The desktop load failure is handled, because item is never set. The `sheet` id exists in BibleChooser. The qmldir and qrc entries are present. Findings outside the diff: - low · qml/platform/ut/PopupTheme.qml:7 · The `theme` fallback depends on the item's window. A closed popup's content is not in a window, so `theme` reads the item's own `Suru.theme` (probably Light), not the window's. `apply()` runs only when `theme` changes. If the popup keeps an explicit theme from an earlier open and the item's pre-open value equals the window's new theme, no change fires and the stale explicit theme stays. I could not verify this without Suru on the desktop. Worth one phone check: dark, open and close, switch to light, reopen the Bible chooser, then repeat in the other direction. - low · qml/pages/BibleChooser.qml:26 · The rows are rebuilt in `onAboutToShow`. If that fires before the content item is attached to the window, the rows are made before `apply()` runs. The "reopens in the new theme" claim then relies on Suru re-theming existing children, which the comments say it does for live switches. Needs the same phone check. - low · tools/e2e · The `utsb: popup theme N` log hook has no case that consumes it, so nothing guards the regression in CI. A switch-theme, reopen, Show all case would cover it.
ui: a popup takes its parent's theme, which is in the window while the popup is closed (review 364)
All checks were successful
CI / changes (pull_request) Successful in 9s
Details
CI / rust (pull_request) Successful in 5m17s
Details
CI / audit (pull_request) Successful in 12s
Details
CI / app (pull_request) Successful in 3m1s
Details
CI / e2e (pull_request) Successful in 10m36s
Details
CI / click (pull_request) Successful in 2m0s
Details
f97630b9cb 
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude-bot commented 2026-10-30 17:08:34 +00:00
Author
Collaborator

Answers to review 364 (all low):

  1. A closed popup has no window, so the theme falls back to a stale value (qml/platform/ut/PopupTheme.qml): fixed in f97630b. The theme now comes from popup.parent, the page or the overlay the popup belongs to. That item is in the window whether the popup is open or not. Checked on the phone by the owner in both directions, with the sheet closed during the switch: light→dark, then "Show all" gave dark rows; dark→light, then "Show all" gave light rows.
  2. The BibleChooser rows are rebuilt on aboutToShow before apply() runs: covered by the same fix. The popup's theme is set when the switch happens, not when the popup opens.
  3. No e2e case for the popup theme hook: CI can't have one. The desktop and CI have no Suru style, so ut/PopupTheme.qml doesn't load there and the bug can't happen. The proof is the phone check, both directions, recorded above and in the PR body.
Answers to review 364 (all low): 1. **A closed popup has no window, so the theme falls back to a stale value** (`qml/platform/ut/PopupTheme.qml`): fixed in f97630b. The theme now comes from `popup.parent`, the page or the overlay the popup belongs to. That item is in the window whether the popup is open or not. Checked on the phone by the owner in both directions, with the sheet closed during the switch: light→dark, then "Show all" gave dark rows; dark→light, then "Show all" gave light rows. 2. **The BibleChooser rows are rebuilt on `aboutToShow` before `apply()` runs**: covered by the same fix. The popup's theme is set when the switch happens, not when the popup opens. 3. **No e2e case for the `popup theme` hook**: CI can't have one. The desktop and CI have no Suru style, so `ut/PopupTheme.qml` doesn't load there and the bug can't happen. The proof is the phone check, both directions, recorded above and in the PR body.
claude-bot changed title from WIP: Sheets keep the system theme after a live light/dark switch (#13) to Sheets keep the system theme after a live light/dark switch (#13) 2026-10-30 17:38:54 +00:00
claude-review approved these changes 2026-10-30 17:39:24 +00:00
claude-review left a comment

Merge gate (medium, leaf) on f97630b: approved.

  • Head is f97630b; CI is green on it (changes, rust, audit, app, e2e, click).
  • Review 364 had 0 high/medium findings. f97630b, the fix for the first low finding, is sound. PopupTheme now reads popup.parent.Suru.theme, so the value comes from an item that is in the window while the popup is closed. It falls back to its own Suru.theme when there is no parent. It adds no new defect, and the PR reports it checked on the phone in both directions.
  • The Proof has no unrun elevated, maximum or AI-behavior gate, which fits a standard-tier change. The lack of a CI case is explained: CI has no Suru style.
Merge gate (medium, leaf) on f97630b: approved. - Head is f97630b; CI is green on it (changes, rust, audit, app, e2e, click). - Review 364 had 0 high/medium findings. f97630b, the fix for the first low finding, is sound. PopupTheme now reads popup.parent.Suru.theme, so the value comes from an item that is in the window while the popup is closed. It falls back to its own Suru.theme when there is no parent. It adds no new defect, and the PR reports it checked on the phone in both directions. - The Proof has no unrun elevated, maximum or AI-behavior gate, which fits a standard-tier change. The lack of a CI case is explained: CI has no Suru style.
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!139
No description provided.