docs: fold the second review pass into the visibility design
Records what changed and why: silencing is not handling (the reminder rows that stay scheduled and the recovery that re-posts them), the notice being an upgrade-only story, the reconcile trigger covering a grant made outside the app, the event's own calendar surviving in the form picker, and the snapshot cache being keyed on the pending set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user