Breadway/breadbar

Fork 0

Expose notification settings for the BOS Settings v2 panel #56

New issue

Open

opened 2026-10-02 17:55:15 +08:00 by Breadway · 3 comments

Breadway commented

2026-10-02 17:55:15 +08:00

Owner

The approved BOS Settings v2 proposal needs Do not disturb state, a schedule, per-app notification permissions, sound and lock-screen preview controls.

Current breadbar source exposes history, pill and stage actions on dev.breadway.Bar. Its DND flag is an in-process Rc<Cell> and is toggled inside stage/mod.rs. Settings cannot reliably read or change it, and no persisted schedule or per-app permission configuration is exposed.

Add a typed, versioned interface to read/apply persisted notification preferences. Keep bar controls and external Settings controls in sync, retain critical alert behavior, and expose capability information for unsupported options. Implement schedule and per-app policy in the notification server rather than storing preferences that are never consumed.

Needed by BSET-43. No GUI or desktop testing is authorised for this implementation session.

The approved BOS Settings v2 proposal needs Do not disturb state, a schedule, per-app notification permissions, sound and lock-screen preview controls. Current breadbar source exposes history, pill and stage actions on dev.breadway.Bar. Its DND flag is an in-process Rc<Cell<bool>> and is toggled inside stage/mod.rs. Settings cannot reliably read or change it, and no persisted schedule or per-app permission configuration is exposed. Add a typed, versioned interface to read/apply persisted notification preferences. Keep bar controls and external Settings controls in sync, retain critical alert behavior, and expose capability information for unsupported options. Implement schedule and per-app policy in the notification server rather than storing preferences that are never consumed. Needed by BSET-43. No GUI or desktop testing is authorised for this implementation session.

Breadway referenced this issue from a pull request that will close it,

2026-10-03 16:13:05 +08:00

Add notification preferences for BOS Settings v2 #57

Breadway commented

2026-10-03 16:17:13 +08:00

Author

Owner

Implemented in breadbar PR #57: #57

Versioned persisted API consumed by BOS Settings, manual/overnight DND, per-app all/banners/history/off policy, sound eligibility and lock-time suppression. Stage controls share preferences and indicators refresh across outputs. Companion clock-format and validated font/text preferences are included.

Verification: clippy with warnings denied, 24 headless notification tests, a built test binary, and green Forgejo CI at commit 8263a8f117. No GUI launch or live configuration writes were used. Sound requires canberra-gtk-play and the compositor controls whether popups can appear on a secure lock surface. Ready for user review.

Implemented in breadbar PR #57: https://git.breadway.dev/Breadway/breadbar/pulls/57 Versioned persisted API consumed by BOS Settings, manual/overnight DND, per-app all/banners/history/off policy, sound eligibility and lock-time suppression. Stage controls share preferences and indicators refresh across outputs. Companion clock-format and validated font/text preferences are included. Verification: clippy with warnings denied, 24 headless notification tests, a built test binary, and green Forgejo CI at commit 8263a8f11788fe53282cd45c0529a1264884d418. No GUI launch or live configuration writes were used. Sound requires canberra-gtk-play and the compositor controls whether popups can appear on a secure lock surface. Ready for user review.

Breadway commented

2026-10-13 21:48:30 +08:00

Author

Owner

Independent review of head 8263a8f117. Verdict: changes requested. Not merged.

Reviewed in an isolated worktree at that SHA. Required check check / check (push) is success on this SHA (actions run 185, commit_sha matches, finished 2026-10-03). No prior reviews. Branch protection list is empty. dev-release.yml is main-only and correctly did not run here. A formal REQUEST_CHANGES review was rejected by Forgejo ("reject your own pull is not allowed"), so this comment is the persisted verdict.

The PR is not mergeable against current origin/main (e5f148009a). merge-tree reports one conflict in src/main.rs: main clones notif_store into stage::Shared::new, this branch does not and inserts the preference poll in the same region. src/notifications/mod.rs and src/stage/mod.rs also changed on both sides. Merge main into this branch and resolve that before a new review.

Criteria:

Versioned persisted notification-preference API on dev.breadway.Bar, consumed by BOS Settings.

Provider half is present: GetNotificationPreferences and SetNotificationPreferences on dev.breadway.Bar (src/notifications/mod.rs:267-301), api_version 1, file breadbar/notifications.json, unknown keys kept, writes use write_atomic_backed_up (preferences.rs:88-106).
BOS Settings consumption is unverified. Companion bos-settings PR #46 is not in this diff. No test calls the D-Bus methods, round-trips JSON, or checks the backup. Rejecting a bad write and preserving unknown keys are untested.

Manual and scheduled DND.

effective_dnd covers manual override and overnight/daytime ranges (preferences.rs:63-81). Tests assert the predicate only (preferences.rs:193-218).
Stage toggle writes dnd and clears schedule when turning off (stage/mod.rs:163-174). No test shows a stage toggle and a Settings read seeing the same value.
Invalid schedule times are rejected by save, but a stored invalid time makes effective_dnd return false even while schedule is true. That failure mode is untested.
Equal start and end is treated as off (preferences.rs:73-74) and is undocumented.

Per-app banner, history, and off policy.

presentation() implements all / banners / history / off (preferences.rs:149-166). The unit test checks the flags, not that Notify records history, skips a popup, or drops an Off app.
notify() uses presentation for retain/banner (mod.rs:453-490), but the existing notify tests set persist_history false, so they never load preferences. Policy is not on the user-visible path in tests.

Sounds and lock-time suppression.

Sound is gated by policy All, the sound flag, and suppress-sound (mod.rs:516-526). sound_available reports whether canberra-gtk-play is on PATH. No test asserts a sound process is or is not spawned.
Lock suppression is split. presentation() honors the locked flag, but notify() calls it with locked=false (mod.rs:461-466) and then does a second pgrep/loginctl check (mod.rs:491-514). Critical notifications are suppressed while locked, including when lock_screen is true, because the second check ignores critical. The unit test says the opposite (preferences.rs:190-191). That is a real behavior bug, not just a missing test.

Critical alerts retained.

presentation() lets critical bypass DND but not History or Off. The test covers the predicate.
notify() does not apply that critical exception on the lock path, as above. No notify() test sends a critical notification under DND or lock and checks history plus banner.

Stage DND controls share the same preferences, and all stage indicators refresh together.

set_dnd persists and paint_dnd walks live stage chips and registered marks (stage/mod.rs:163-183). Primary polls every 2s and calls paint_dnd (main.rs:2484-2496).
No test registers two marks or two stages and shows one write updates both.
The 2s poll calls refresh_open_notifications unconditionally. That only repaints the open stage; it does not prove every indicator moved together, and it is untested.

Clock-format and font/text preferences consumed.

clock_24h() reads breadbar/clock.json (clock.rs:59-69) and time() uses it. formatted() uses it only as a full replacement for the theme format (clock.rs:39-44). The visible plain clock calls formatted() (main.rs:3100-3102), so a 12-hour preference discards the theme format instead of changing the hour field. Flip and placeholder clocks call time(), so they do follow clock.json. No clock test exists.
text_css() allowlists font and size and yields when breadbar/style.css is newer (preferences.rs:109-141). Nothing tests the allowlist, the mtime precedence, or that the CSS provider receives it. Wiring is a 2s poll (main.rs:2473-2488) with no assertion.

Also: GetNotificationPreferences inserts every history app at the default policy as a side effect of a read (mod.rs:285-287). A later Set of that payload persists apps the user never chose. Do not do that on read; return known apps separately if Settings needs a list.

Do not merge. Update the branch with current main, fix the lock/critical split and the clock formatted() path, and add tests that drive Notify (history, banner event, sound gate, lock, critical) plus save/load round-trip, unknown-key preservation, and stage indicator refresh. BOS Settings UI consumption can stay unverified on this provider PR if the provider tests above exist.

Independent review of head 8263a8f11788fe53282cd45c0529a1264884d418. Verdict: changes requested. Not merged. Reviewed in an isolated worktree at that SHA. Required check `check / check (push)` is success on this SHA (actions run 185, commit_sha matches, finished 2026-10-03). No prior reviews. Branch protection list is empty. dev-release.yml is main-only and correctly did not run here. A formal REQUEST_CHANGES review was rejected by Forgejo ("reject your own pull is not allowed"), so this comment is the persisted verdict. The PR is not mergeable against current origin/main (e5f148009a938174979e32b18e8214cf652ce560). merge-tree reports one conflict in src/main.rs: main clones notif_store into stage::Shared::new, this branch does not and inserts the preference poll in the same region. src/notifications/mod.rs and src/stage/mod.rs also changed on both sides. Merge main into this branch and resolve that before a new review. Criteria: 1. Versioned persisted notification-preference API on dev.breadway.Bar, consumed by BOS Settings. - Provider half is present: GetNotificationPreferences and SetNotificationPreferences on dev.breadway.Bar (src/notifications/mod.rs:267-301), api_version 1, file breadbar/notifications.json, unknown keys kept, writes use write_atomic_backed_up (preferences.rs:88-106). - BOS Settings consumption is unverified. Companion bos-settings PR #46 is not in this diff. No test calls the D-Bus methods, round-trips JSON, or checks the backup. Rejecting a bad write and preserving unknown keys are untested. 2. Manual and scheduled DND. - effective_dnd covers manual override and overnight/daytime ranges (preferences.rs:63-81). Tests assert the predicate only (preferences.rs:193-218). - Stage toggle writes dnd and clears schedule when turning off (stage/mod.rs:163-174). No test shows a stage toggle and a Settings read seeing the same value. - Invalid schedule times are rejected by save, but a stored invalid time makes effective_dnd return false even while schedule is true. That failure mode is untested. - Equal start and end is treated as off (preferences.rs:73-74) and is undocumented. 3. Per-app banner, history, and off policy. - presentation() implements all / banners / history / off (preferences.rs:149-166). The unit test checks the flags, not that Notify records history, skips a popup, or drops an Off app. - notify() uses presentation for retain/banner (mod.rs:453-490), but the existing notify tests set persist_history false, so they never load preferences. Policy is not on the user-visible path in tests. 4. Sounds and lock-time suppression. - Sound is gated by policy All, the sound flag, and suppress-sound (mod.rs:516-526). sound_available reports whether canberra-gtk-play is on PATH. No test asserts a sound process is or is not spawned. - Lock suppression is split. presentation() honors the locked flag, but notify() calls it with locked=false (mod.rs:461-466) and then does a second pgrep/loginctl check (mod.rs:491-514). Critical notifications are suppressed while locked, including when lock_screen is true, because the second check ignores critical. The unit test says the opposite (preferences.rs:190-191). That is a real behavior bug, not just a missing test. 5. Critical alerts retained. - presentation() lets critical bypass DND but not History or Off. The test covers the predicate. - notify() does not apply that critical exception on the lock path, as above. No notify() test sends a critical notification under DND or lock and checks history plus banner. 6. Stage DND controls share the same preferences, and all stage indicators refresh together. - set_dnd persists and paint_dnd walks live stage chips and registered marks (stage/mod.rs:163-183). Primary polls every 2s and calls paint_dnd (main.rs:2484-2496). - No test registers two marks or two stages and shows one write updates both. - The 2s poll calls refresh_open_notifications unconditionally. That only repaints the open stage; it does not prove every indicator moved together, and it is untested. 7. Clock-format and font/text preferences consumed. - clock_24h() reads breadbar/clock.json (clock.rs:59-69) and time() uses it. formatted() uses it only as a full replacement for the theme format (clock.rs:39-44). The visible plain clock calls formatted() (main.rs:3100-3102), so a 12-hour preference discards the theme format instead of changing the hour field. Flip and placeholder clocks call time(), so they do follow clock.json. No clock test exists. - text_css() allowlists font and size and yields when breadbar/style.css is newer (preferences.rs:109-141). Nothing tests the allowlist, the mtime precedence, or that the CSS provider receives it. Wiring is a 2s poll (main.rs:2473-2488) with no assertion. Also: GetNotificationPreferences inserts every history app at the default policy as a side effect of a read (mod.rs:285-287). A later Set of that payload persists apps the user never chose. Do not do that on read; return known apps separately if Settings needs a list. Do not merge. Update the branch with current main, fix the lock/critical split and the clock formatted() path, and add tests that drive Notify (history, banner event, sound gate, lock, critical) plus save/load round-trip, unknown-key preservation, and stage indicator refresh. BOS Settings UI consumption can stay unverified on this provider PR if the provider tests above exist.

Breadway closed this issue

2026-10-14 09:48:05 +08:00

Breadway commented

2026-10-14 09:56:26 +08:00

Author

Owner

Independent re-review of breadbar PR #57 head 535d746d0e. Verdict: pass. Merged as 3f17c44e4f.

All repair items from the changes_requested review confirmed in an isolated worktree at the exact head: merge conflict resolved (16dac6d keeps notif_store clone plus poll, mergeable true); lock/critical unified under presentation() with paired predicate plus live-path tests; clock formatted() layers over theme format with tests; Notify/persistence/provider round-trip/backup/unknown-keys/sound tests pass; invalid schedule falls back to 22:00-07:00; Get reports known_apps without absorbing history apps; stage DND round trip, shared flag, refresh fan-out, and two-stage refresh verified; text_css allowlist and mtime precedence verified via throwaway scratch probes (scratch restored). Runs: clippy -D warnings clean, cargo test 151 passed 0 failed, required check task 2420/run 194 success on this exact SHA, no outstanding reviews. Pre-merge re-read confirmed head, base (e5f1480), checks, and mergeability unchanged.

BOS Settings UI consumption stays unverified on this provider PR by design; as the provider half this is correct. Full verdict persisted on the PR (comment 4647). Cards stay in review for coordinator verification, not done.

Independent re-review of breadbar PR #57 head 535d746d0e144a8897caa8ea09face7237fd29f1. Verdict: pass. Merged as 3f17c44e4f63be360182875ef1e543ce77338f30. All repair items from the changes_requested review confirmed in an isolated worktree at the exact head: merge conflict resolved (16dac6d keeps notif_store clone plus poll, mergeable true); lock/critical unified under presentation() with paired predicate plus live-path tests; clock formatted() layers over theme format with tests; Notify/persistence/provider round-trip/backup/unknown-keys/sound tests pass; invalid schedule falls back to 22:00-07:00; Get reports known_apps without absorbing history apps; stage DND round trip, shared flag, refresh fan-out, and two-stage refresh verified; text_css allowlist and mtime precedence verified via throwaway scratch probes (scratch restored). Runs: clippy -D warnings clean, cargo test 151 passed 0 failed, required check task 2420/run 194 success on this exact SHA, no outstanding reviews. Pre-merge re-read confirmed head, base (e5f1480), checks, and mergeability unchanged. BOS Settings UI consumption stays unverified on this provider PR by design; as the provider half this is correct. Full verdict persisted on the PR (comment 4647). Cards stay in review for coordinator verification, not done.

Breadway reopened this issue

2026-10-14 09:56:27 +08:00