Add notification preferences for BOS Settings v2 #57

Merged

Breadway merged 5 commits from feature/notification-preferences into main 2026-10-14 09:48:05 +08:00

Conversation 5 Commits 5 Files changed 7 +1145 -21

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

Owner

Copy link

Closes #56

Adds a persisted, versioned notification preference API on dev.breadway.Bar for the BOS Settings v2 controls. The server consumes manual and scheduled DND, per-app banner/history/off policy, sounds and lock-time suppression. Critical alert behavior is retained. Stage DND controls use the same preferences and all stage indicators refresh together.

Also consumes the Settings clock-format and validated font/text preferences. Later bar-specific CSS edits retain precedence. Unknown preference keys are preserved and writes keep a backup.

Verification: clippy with warnings denied, 24 headless notification tests, and a test binary built using the documented dev dependency overrides. Cargo.lock retains tagged dependencies. Updated with the latest origin/main.

UI testing remains reserved for the user. No app launch, desktop manipulation, live configuration writes or service starts were used for verification. Sound requires canberra-gtk-play; secure lock surfaces determine whether notification windows can appear while locked.

Companion Settings changes: bos-settings PR #46. API details: docs/notification-preferences.md.

Closes #56 Adds a persisted, versioned notification preference API on dev.breadway.Bar for the BOS Settings v2 controls. The server consumes manual and scheduled DND, per-app banner/history/off policy, sounds and lock-time suppression. Critical alert behavior is retained. Stage DND controls use the same preferences and all stage indicators refresh together. Also consumes the Settings clock-format and validated font/text preferences. Later bar-specific CSS edits retain precedence. Unknown preference keys are preserved and writes keep a backup. Verification: clippy with warnings denied, 24 headless notification tests, and a test binary built using the documented dev dependency overrides. Cargo.lock retains tagged dependencies. Updated with the latest origin/main. UI testing remains reserved for the user. No app launch, desktop manipulation, live configuration writes or service starts were used for verification. Sound requires canberra-gtk-play; secure lock surfaces determine whether notification windows can appear while locked. Companion Settings changes: bos-settings PR #46. API details: docs/notification-preferences.md.

Breadway added 2 commits 2026-10-03 16:13:05 +08:00

Add persisted notification preferences for BOS Settings 485b771886

Merge remote-tracking branch 'origin/main' into feature/notification-preferences 8263a8f117

Breadway referenced this pull request 2026-10-03 16:15:13 +08:00

Expose notification settings for the BOS Settings v2 panel #56

Breadway referenced this pull request 2026-10-03 16:17:13 +08:00

Expose notification settings for the BOS Settings v2 panel #56

Breadway referenced this pull request from Breadway/bos-settings 2026-10-03 16:19:13 +08:00

Implement BOS Settings v2 controls and proposal gap fixes #46

Breadway commented 2026-10-13 21:48:25 +08:00

Author

Owner

Copy link

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:

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.

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 commented 2026-10-13 21:48:30 +08:00

Author

Owner

Copy link

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:

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.

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 added 2 commits 2026-10-13 22:07:40 +08:00

chore: merge origin/main into feature/notification-preferences ... 16dac6dba2

Keep the notif_store clone for stage Shared (needed by the RAM panel
store) alongside the notification-preference poll.

fix: resolve BAR-56 review findings on notification preferences ... 1542b545c1

Lock and critical split: presentation() is the single source of truth
and critical alerts now keep history plus banner under DND and on a
locked session; notify() routes through one deliver() core with the live
lock state instead of a second divergent check.

Read path: GetNotificationPreferences reports history apps as known_apps
instead of absorbing them into persisted policy.

Clock: formatted() layers the 12/24h setting over the theme format so the
plain clock keeps the theme date half.

Schedule: invalid stored times fall back to 22:00-07:00 with a log line
instead of silently disabling the schedule; start == end stays off and is
now documented.

Tests: Notify policy routing with and without persistence, critical under
DND and lock pinned in both the predicate and the live path, provider
Set/Get round trip with backup and unknown-key tolerance, sound gate and
command construction without audio, stage DND round trip plus two-mark
fan-out, and clock layering. The 2s poll now repaints every live stage.

Breadway added 1 commit 2026-10-14 09:10:44 +08:00

style: format notification preference extras for rustfmt 535d746d0e

Breadway commented 2026-10-14 09:12:01 +08:00

Author

Owner

Copy link

Review rework follow-up (BAR-56). Branch now at 535d746, fast-forward of 1542b54 plus a rustfmt-only style commit. Merge of origin/main already in (16dac6d, keeps notif_store clone plus preference poll).

Mapping to review items:

1. Merge conflict: resolved in 16dac6d, src/main.rs keeps both.
2. Lock/critical split: presentation() is the single source of truth (preferences.rs:183-201); notify() routes through one deliver() core with live lock state (mod.rs:579-646). Agreement pinned by paired tests in preferences.rs and mod.rs (critical under DND plus lock keeps history plus banner).
3. Clock path: formatted() layers clock.json over the theme format via apply_hour_preference (clock.rs:43-84); plain clock keeps theme date half. Covered by hour_preference_layers_over_theme_format and formatted_keeps_theme_format_under_clock_json.
4. Notify/persistence tests: notify_routes_history_banner_and_off_through_preferences, notify_without_persistence_ignores_stored_preferences, provider_set_then_get_round_trip (round trip, backup, unknown-key tolerance, bad-write rejection), sound_gate_and_command_construction, stage DND round trip plus shared-flag and refresh tests; 2s poll now calls refresh_all_notifications (main.rs).
5. Invalid schedule: falls back to 22:00-07:00 with a log line (preferences.rs:68-84), tested; Get no longer absorbs history apps, they are reported as known_apps (mod.rs:346-358).

Checks on this head: cargo clippy --all-targets -- -D warnings passes, cargo build passes, cargo test passes (151 passed, 0 failed), cargo test --locked resolves. Changed-file rustfmt is clean; repo-wide cargo fmt --check still flags pre-existing files untouched by this change (bluetooth, control, slots, stats, tray, workspaces, panel, theme_widgets, widgets, popup, pure). Docker-based ci/build.sh could not run here (no docker daemon), so check.yml was mirrored with direct cargo. BOS Settings UI consumption stays unverified on this provider PR.

Review rework follow-up (BAR-56). Branch now at 535d746, fast-forward of 1542b54 plus a rustfmt-only style commit. Merge of origin/main already in (16dac6d, keeps notif_store clone plus preference poll). Mapping to review items: 1. Merge conflict: resolved in 16dac6d, src/main.rs keeps both. 2. Lock/critical split: presentation() is the single source of truth (preferences.rs:183-201); notify() routes through one deliver() core with live lock state (mod.rs:579-646). Agreement pinned by paired tests in preferences.rs and mod.rs (critical under DND plus lock keeps history plus banner). 3. Clock path: formatted() layers clock.json over the theme format via apply_hour_preference (clock.rs:43-84); plain clock keeps theme date half. Covered by hour_preference_layers_over_theme_format and formatted_keeps_theme_format_under_clock_json. 4. Notify/persistence tests: notify_routes_history_banner_and_off_through_preferences, notify_without_persistence_ignores_stored_preferences, provider_set_then_get_round_trip (round trip, backup, unknown-key tolerance, bad-write rejection), sound_gate_and_command_construction, stage DND round trip plus shared-flag and refresh tests; 2s poll now calls refresh_all_notifications (main.rs). 5. Invalid schedule: falls back to 22:00-07:00 with a log line (preferences.rs:68-84), tested; Get no longer absorbs history apps, they are reported as known_apps (mod.rs:346-358). Checks on this head: cargo clippy --all-targets -- -D warnings passes, cargo build passes, cargo test passes (151 passed, 0 failed), cargo test --locked resolves. Changed-file rustfmt is clean; repo-wide cargo fmt --check still flags pre-existing files untouched by this change (bluetooth, control, slots, stats, tray, workspaces, panel, theme_widgets, widgets, popup, pure). Docker-based ci/build.sh could not run here (no docker daemon), so check.yml was mirrored with direct cargo. BOS Settings UI consumption stays unverified on this provider PR.

Breadway merged commit 3f17c44e4f into main 2026-10-14 09:48:05 +08:00

Breadway referenced this pull request from a commit 2026-10-14 09:48:07 +08:00

Merge pull request 'Add notification preferences for BOS Settings v2' (#57) from feature/notification-preferences into main

Breadway commented 2026-10-14 09:53:20 +08:00

Author

Owner

Copy link

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

Reviewed the full diff vs base e5f148009a in an isolated worktree at the exact head. Head matches the expected SHA; base main is unchanged since the previous review. Required check check / check (push) is success on this exact SHA (task 2420, run 194, finished 2026-10-14). No outstanding formal reviews. Branch was mergeable with no conflicts.

Repair-item confirmation against the changes_requested review at 8263a8f (comment 4566) and the repair note (comment 4624):

1. Merge conflict: resolved. The branch contains merge 16dac6d of origin/main; src/main.rs keeps both the notif_store clone into stage::Shared::new and the new preference poll block. PR reports mergeable true.
2. Lock/critical split: fixed. presentation() in src/notifications/preferences.rs:183-201 is the single source of truth (critical bypasses DND and lock for history plus banner; History/Off policy still wins). notify() routes through one deliver() core with the live session_locked() result in src/notifications/mod.rs:579-646. Agreement is pinned by paired tests: critical_under_dnd_and_lock_keeps_history_plus_banner (predicate) and notify_critical_keeps_history_plus_banner_under_dnd_and_lock (live path). Both pass.
3. Clock path: fixed. formatted() layers breadbar/clock.json over the theme format via apply_hour_preference in src/bar/clock.rs:43-84; only hour fields change so the theme keeps its date half. Covered by hour_preference_layers_over_theme_format and formatted_keeps_theme_format_under_clock_json. Both pass.
4. Notify/persistence tests: present and passing. notify_routes_history_banner_and_off_through_preferences drives Notify with persist on (history records, History emits HistoryChanged without popup, Off drops silently, default banners); notify_without_persistence_ignores_stored_preferences; provider_set_then_get_round_trip (round trip, backup on second write, unknown-key preservation, bad-write rejection without touching the file, known_apps reported without absorbing history apps into policy); sound_gate_and_command_construction (gate plus exact canberra-gtk-play -i message construction, no audio played).
5. Invalid schedule: falls back to 22:00-07:00 with a log line (preferences.rs:68-84), pinned by invalid_stored_schedule_falls_back_to_default_instead_of_off. Get no longer absorbs history apps; they are reported as known_apps (mod.rs:346-358), pinned by the round-trip test.
6. Stage sharing: set_dnd persists and paint_dnd fans out to live chips and registered marks; stage_dnd_toggle_round_trips_through_settings_store, shared_clones_share_one_dnd_flag, refresh_without_stages_is_safe, and paint_dnd_updates_every_registered_mark all pass. The 2s poll now calls refresh_all_notifications (main.rs) and HistoryChanged refreshes visible history (popup.rs).
7. Font/text: text_css() allowlists font and size and yields when breadbar/style.css is newer. No committed test covers it, so I verified it independently with throwaway probe tests in the scratch worktree only (scratch file restored afterwards): allowlist fallbacks, no markup injection, mtime precedence, and empty default all behave as documented. Get purity (no history-app injection) likewise probed and confirmed.

Independent runs in the scratch worktree at this head: cargo clippy --all-targets -- -D warnings clean; cargo test 151 passed 0 failed; cargo fmt --check flags only pre-existing files plus one cosmetic nit in new test code at mod.rs:1348 (not CI-enforced; check.yml runs clippy plus test only).

BOS Settings UI consumption stays unverified on this provider PR by design. As the provider half this is correct: versioned Get/Set on dev.breadway.Bar with api_version 1, effective_dnd, sound_available, known_apps, unknown-key tolerance, backup writes, and server-side consumption of every stored field.

Merged under standing breadbar dev-release authorization; no tags touched.

Independent re-review of head 535d746d0e144a8897caa8ea09face7237fd29f1. Verdict: pass. Merged as 3f17c44e4f63be360182875ef1e543ce77338f30. Reviewed the full diff vs base e5f148009a938174979e32b18e8214cf652ce560 in an isolated worktree at the exact head. Head matches the expected SHA; base main is unchanged since the previous review. Required check `check / check (push)` is success on this exact SHA (task 2420, run 194, finished 2026-10-14). No outstanding formal reviews. Branch was mergeable with no conflicts. Repair-item confirmation against the changes_requested review at 8263a8f (comment 4566) and the repair note (comment 4624): 1. Merge conflict: resolved. The branch contains merge 16dac6d of origin/main; src/main.rs keeps both the notif_store clone into stage::Shared::new and the new preference poll block. PR reports mergeable true. 2. Lock/critical split: fixed. presentation() in src/notifications/preferences.rs:183-201 is the single source of truth (critical bypasses DND and lock for history plus banner; History/Off policy still wins). notify() routes through one deliver() core with the live session_locked() result in src/notifications/mod.rs:579-646. Agreement is pinned by paired tests: critical_under_dnd_and_lock_keeps_history_plus_banner (predicate) and notify_critical_keeps_history_plus_banner_under_dnd_and_lock (live path). Both pass. 3. Clock path: fixed. formatted() layers breadbar/clock.json over the theme format via apply_hour_preference in src/bar/clock.rs:43-84; only hour fields change so the theme keeps its date half. Covered by hour_preference_layers_over_theme_format and formatted_keeps_theme_format_under_clock_json. Both pass. 4. Notify/persistence tests: present and passing. notify_routes_history_banner_and_off_through_preferences drives Notify with persist on (history records, History emits HistoryChanged without popup, Off drops silently, default banners); notify_without_persistence_ignores_stored_preferences; provider_set_then_get_round_trip (round trip, backup on second write, unknown-key preservation, bad-write rejection without touching the file, known_apps reported without absorbing history apps into policy); sound_gate_and_command_construction (gate plus exact canberra-gtk-play -i message construction, no audio played). 5. Invalid schedule: falls back to 22:00-07:00 with a log line (preferences.rs:68-84), pinned by invalid_stored_schedule_falls_back_to_default_instead_of_off. Get no longer absorbs history apps; they are reported as known_apps (mod.rs:346-358), pinned by the round-trip test. 6. Stage sharing: set_dnd persists and paint_dnd fans out to live chips and registered marks; stage_dnd_toggle_round_trips_through_settings_store, shared_clones_share_one_dnd_flag, refresh_without_stages_is_safe, and paint_dnd_updates_every_registered_mark all pass. The 2s poll now calls refresh_all_notifications (main.rs) and HistoryChanged refreshes visible history (popup.rs). 7. Font/text: text_css() allowlists font and size and yields when breadbar/style.css is newer. No committed test covers it, so I verified it independently with throwaway probe tests in the scratch worktree only (scratch file restored afterwards): allowlist fallbacks, no markup injection, mtime precedence, and empty default all behave as documented. Get purity (no history-app injection) likewise probed and confirmed. Independent runs in the scratch worktree at this head: cargo clippy --all-targets -- -D warnings clean; cargo test 151 passed 0 failed; cargo fmt --check flags only pre-existing files plus one cosmetic nit in new test code at mod.rs:1348 (not CI-enforced; check.yml runs clippy plus test only). BOS Settings UI consumption stays unverified on this provider PR by design. As the provider half this is correct: versioned Get/Set on dev.breadway.Bar with api_version 1, effective_dnd, sound_available, known_apps, unknown-key tolerance, backup writes, and server-side consumption of every stored field. Merged under standing breadbar dev-release authorization; no tags touched.

Breadway referenced this pull request 2026-10-14 09:56:26 +08:00

Expose notification settings for the BOS Settings v2 panel #56

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

Author

Owner

Copy link

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.

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels   

bug

  

ci

  

enhancement

  

feature

  

privacy

  

security

  

theme-system

No labels

bug

ci

enhancement

feature

privacy

security

theme-system

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

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

Breadway/breadbar!57

No description provided.