diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index d61a51e..4df52d5 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -171,7 +171,10 @@ the app has not been allowed to write yet (read-only permission grant, or a pre-permission launch); `CalendarVisibilityReconciler` drains it entry by entry, and until it does, the repository and `ReminderNotifier.post` honour it. That gate also covers a snooze re-shown from our own alarm after its calendar was -switched off. The drawer's filter sheet (`CalendarPrefs.hiddenCalendarIds`) is a +switched off. Silencing is not handling: an alert the gate drops keeps its +`SCHEDULED` state while its event is still ahead (`handledAlertIds`), so +switching the calendar back on re-posts it (`ReminderRecovery`) instead of +losing it — the provider's own table is the stash. The drawer's filter sheet (`CalendarPrefs.hiddenCalendarIds`) is a separate in-app declutter that never touches reminders. See `docs/design/calendar-visibility-model.md`. Deliberately absent until real devices prove it necessary: own alarm diff --git a/docs/design/calendar-visibility-model.md b/docs/design/calendar-visibility-model.md index ee64c07..72503d9 100644 --- a/docs/design/calendar-visibility-model.md +++ b/docs/design/calendar-visibility-model.md @@ -3,15 +3,17 @@ Status: **implemented** on `fix/calendar-visibility-model` (on-device review owed) Date: 2026-07-25, revised 2026-07-25 after code review -Built as specified, then revised in two places the review found (see §4, §6): -the reconciliation no longer switches calendars *on*, and a calendar switched -off without `WRITE_CALENDAR` is kept app-side instead of being lost. Other -notes: `CalendarSource` gained a `syncsEvents` flag (read for #76's "not synced" -label); the reconcile runs from `CalendulaApp.onCreate` *and* from -`PermissionViewModel.onGranted`, so a launch that starts without the calendar -permissions still catches up the moment they are granted; and the Settings hint -string was reworded rather than dropped — it described the retired app-only -behaviour, so leaving it would have been the one piece of false text on screen. +Built as specified, then revised in two places the first review found (see §4, +§6): the reconciliation no longer switches calendars *on*, and a calendar +switched off without `WRITE_CALENDAR` is kept app-side instead of being lost. +A second review pass found four more, all folded in below: the notice was armed +on fresh installs too (§4), a silenced reminder could be marked handled and lost +(§3), an event living in a switched-off calendar lost its calendar in the edit +form (§2), and the reconcile only ran for one of the two ways a permission can +be granted (§4). Other notes: `CalendarSource` gained a `syncsEvents` flag (read +for #76's "not synced" label); and the Settings hint string was reworded rather +than dropped — it described the retired app-only behaviour, so leaving it would +have been the one piece of false text on screen. Tracking: Codeberg #75. Split out of this work: #76 (read-only / not-synced communication), #77 (accounts merged by name). @@ -112,8 +114,21 @@ which is what a user assumes, and the change does not leave the device. - Settings → Calendars row state derives from `isVisibleInSystem`. - Swap the display predicate from `id in disabledIds` to `!isVisibleInSystem` in `CalendarRepositoryImpl.instances`/`searchEvents`, `EventEditViewModel` (form - picker), `ImportViewModel` (import picker). + picker), `ImportViewModel` (import picker), `SettingsViewModel` (the + per-calendar reminder overrides — a default reminder on a switched-off calendar + could never fire). - `hiddenCalendarIds` keeps working exactly as it does today, unioned as before. +- Those lists are the *targets* a user may pick. An event that already lives in an + excluded calendar keeps it: `EventEditViewModel` adds the event's own calendar + back to the picker when it isn't among them (review — otherwise editing an event + in a calendar switched off on this device renders the row as the "no calendar" + error, with an enabled picker that turns any pick into a calendar *move*). +- The repository caches one `Calendars` read per provider tick, and that cache is + keyed on the pending set as well as the tick (review). An id leaves the pending + set the instant its `VISIBLE` write lands, but the `ContentObserver` that + invalidates the snapshot is dispatched through the main looper — reading the + new set against the old snapshot re-admits exactly the events being hidden + until it arrives. ### 3. Deletions @@ -128,14 +143,30 @@ provider alert row behind them — a snooze re-shown from our own exact alarm, scheduled before the calendar was switched off, and a read-only install whose switch lives in the pending set. One choke point covers both receivers. +**Silencing is not handling** (second review). "`VISIBLE = 0` ⇒ no alert rows" +holds only where the flag was actually written; where the switch lives in the +pending set the provider keeps creating and broadcasting rows. Marking those +`STATE_FIRED` with the rest loses them for good — `dueAlerts` only ever returns +`STATE_SCHEDULED`. So the receiver marks what it *posted*, plus what it silenced +for an event already over (`handledAlertIds`); a silenced alert for an event +still ahead stays scheduled, which makes the provider's own table the stash the +deleted `SuppressedReminderStore` used to be. `ReminderRecovery` re-posts them +when the calendar is switched back on, so recovery doesn't wait for the next +unrelated broadcast. + Net code reduction. Update the now-false KDoc on `CalendarPrefs` claiming the toggle "never touches the system's VISIBLE/SYNC_EVENTS flags, so other calendar apps are unaffected". ### 4. Reconciliation (hide-only, standing) -`CalendarVisibilityReconciler` runs on every launch and again when the permission -screen grants the permissions. It drains +`CalendarVisibilityReconciler` runs on every launch, and again whenever +`RootScreen` comes up holding the calendar permission. That second trigger sat on +`PermissionViewModel.onGranted` at first, which only fires for the in-app request +— a permission granted on Android's own app-settings screen comes back through +`RootScreen`'s `ON_RESUME` and would have left the drain unrun for the session +(review). Settled runs cost two DataStore reads and return before the query. It +drains `CalendarPrefs.pendingDisabledCalendarIds` — the retired `disabledCalendarIds` key, re-read under a new name — into the provider: @@ -167,6 +198,15 @@ reconciler arms a one-time dialog (`CalendarVisibilityNoticeDialog`) explaining that visibility now follows the device and where to change it. The answer — including "nothing to say" — is stored, so it can never resurface later. +**Upgrades only** (second review). The notice explains a change to behaviour the +user has seen before, and a device holding something hidden at system level is +the *norm* on a fresh install — a second account's calendars, "Holidays in …", a +subscribed calendar. Arming on that state alone put a changelog dialog in front +of first-run users. `firstInstallTime != lastUpdateTime` is the gate; a fresh +install retires the notice unshown, ahead of the permission check, so an app +update installed before the first grant can't make it look like an upgrade +afterwards. + ### 4b. No `WRITE_CALENDAR` Only `READ_CALENDAR` gates the app (`RootScreen`), and `PermissionScreen` says as @@ -177,7 +217,8 @@ that set filters `instances`/`searchEvents` and gates `ReminderNotifier.post` exactly as `VISIBLE` would. If `WRITE_CALENDAR` ever arrives, the reconciler flushes it and the app-side copy disappears. This is the one place a second visibility model still exists, and it exists only where the first one is -unwritable. +unwritable — and the one place the provider keeps creating alert rows for a +calendar the user switched off, which is why §3's silencing must stay reversible. ### 5. `sync_events = 0` rows @@ -209,6 +250,10 @@ All JVM-testable: - the display-predicate swap, and the pending set filtering alongside it - the read-only path: no provider write, the choice parked in the pending set, events filtered from it +- what a due alert may be marked as handled (`handledAlertIds`), including the + silenced-but-still-ahead row that must stay scheduled +- the flush race: writing `VISIBLE` and releasing the pending id without a + provider tick in between must not re-admit the calendar's events - `FakeCalendarDataSource` assertions that `setCalendarVisible` addresses a single calendar by id - one `Calendars` query per provider tick, however many flows are collecting