fix(calendars): hide-only visibility reconcile, and keep it working read-only
Code review of the one-visibility-model fix (#75) found the reconciliation reaching further than it should and the read-only case falling through it. The migration switched calendars *on* to keep the upgrade invisible, but "not disabled in Calendula" is the default for every calendar, including ones the user deliberately hid in Google Calendar, Etar or DAVx5 — those would reappear there and start firing reminders from a switch the user never touched. Its sync_events guard didn't hold either: an ACCOUNT_TYPE_LOCAL calendar another app created can sit at sync_events=0 while holding real device-local events. The reconcile now only hides, and a one-time notice explains that visibility follows the device and where to change it, instead of quietly rewriting other apps' state. Only READ_CALENDAR gates the app, so a read-only install could not write the flag at all: every calendar it had switched off came back with its events and its reminders, and the switch couldn't undo it. Those switch-offs are kept app-side now (the retired disabled-set key, re-read under a new name), folded into the visibility every consumer reads, and drained into the provider entry by entry once WRITE_CALENDAR arrives — which also makes a part-applied run resumable without re-applying a switch the user has since flipped by hand. Also: restore the ReminderNotifier.post gate, the one path a snooze re-shown from our own alarm passes; move the whole reconcile inside its try/catch, so a damaged preferences file can't crash the process at launch; share one Calendars query per provider tick across the flows that need it; and give the reworded Settings hint new keys, so five locales stop rendering the retired app-only wording. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -1,13 +1,15 @@
|
||||
# Design: One calendar-visibility model (reminders silently never fire)
|
||||
|
||||
Status: **implemented** on `fix/calendar-visibility-model` (on-device review owed)
|
||||
Date: 2026-07-25
|
||||
Date: 2026-07-25, revised 2026-07-25 after code review
|
||||
|
||||
Built as specified, with three notes: `CalendarSource` gained a `syncsEvents`
|
||||
flag (the migration's `sync_events = 1` condition needs it, and #76 will label
|
||||
those rows); the migration runs from `CalendulaApp.onCreate` *and* from
|
||||
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 migrates the moment they are granted; and the Settings hint
|
||||
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.
|
||||
Tracking: Codeberg #75. Split out of this work: #76 (read-only / not-synced
|
||||
@@ -119,29 +121,63 @@ With `VISIBLE=0` the provider creates no alert rows, so there is nothing to supp
|
||||
stash or recover. Remove:
|
||||
|
||||
- `postableAlerts` / `isForDisabledCalendar` (`EventReminderReceiver.kt:29-41`)
|
||||
- the disabled gate in `ReminderNotifier.post`
|
||||
- `SuppressedReminderStore` and `CalendarsViewModel.recoverReminders`
|
||||
- `CalendarPrefs.disabledCalendarIds` / `setDisabledCalendarIds` and its key
|
||||
|
||||
The gate in `ReminderNotifier.post` stays (review): two paths reach it without a
|
||||
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.
|
||||
|
||||
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. Migration (one-shot, app state wins)
|
||||
### 4. Reconciliation (hide-only, standing)
|
||||
|
||||
Runs once after `WRITE_CALENDAR` is granted, guarded by a DataStore flag, then the
|
||||
old key is dropped.
|
||||
`CalendarVisibilityReconciler` runs on every launch and again when the permission
|
||||
screen grants the permissions. It drains
|
||||
`CalendarPrefs.pendingDisabledCalendarIds` — the retired `disabledCalendarIds`
|
||||
key, re-read under a new name — into the provider:
|
||||
|
||||
- Enabled in-app **and** `visible=0` **and** `sync_events=1` → write `visible=1`.
|
||||
- Disabled in-app → write `visible=0`.
|
||||
- Everything else untouched.
|
||||
- Pending **and** `visible=1` → write `visible=0`, then drop the id.
|
||||
- Pending but already `visible=0`, or gone from the device → drop the id.
|
||||
- Everything else untouched. Nothing is ever switched **on**.
|
||||
|
||||
The `sync_events=1` condition matters: a `sync_events=0` calendar has no events on
|
||||
the device, so flipping it visible cannot produce anything and would only create the
|
||||
"enabled but permanently empty" state. Leaving those at `visible=0` costs the user
|
||||
nothing and shows them honestly as off. This direction (app state wins) also
|
||||
preserves exactly what existing users see today — reconciling the other way would
|
||||
make their events vanish on upgrade.
|
||||
Each id leaves the set as its own write lands, so a run that dies part-way
|
||||
resumes where it stopped and can never re-apply a hide the user has since undone
|
||||
by hand. The set empties itself; no separate "migration done" flag exists.
|
||||
|
||||
**Hide-only was the review's correction.** The first cut also switched calendars
|
||||
on (enabled in-app + `visible=0` + `sync_events=1` → `visible=1`) to keep the
|
||||
upgrade invisible. Two problems, both real:
|
||||
|
||||
- `VISIBLE` is device-wide. "Not disabled in Calendula" is the default for every
|
||||
calendar, including ones the user deliberately hid in Google Calendar, Etar or
|
||||
DAVx5 — those would pop back there *and* start firing reminders nobody asked
|
||||
for, from a switch the user never touched.
|
||||
- The `sync_events=1` guard doesn't hold. It is sound for sync-adapter accounts,
|
||||
but an `ACCOUNT_TYPE_LOCAL` calendar another app created can sit at
|
||||
`sync_events=0` while holding real device-local events, so the guard would have
|
||||
hidden events it claimed could not exist.
|
||||
|
||||
The cost is that events from a system-hidden calendar stop appearing in
|
||||
Calendula. That is confined to this app, visible and reversible in Settings →
|
||||
Calendars, and announced: on the first run that finds such a calendar, the
|
||||
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.
|
||||
|
||||
### 4b. No `WRITE_CALENDAR`
|
||||
|
||||
Only `READ_CALENDAR` gates the app (`RootScreen`), and `PermissionScreen` says as
|
||||
much: declining write keeps Calendula usable read-only. Those users can't have
|
||||
the flag written for them, so the switch keeps working app-side —
|
||||
`CalendarRepositoryImpl.setCalendarsVisible` writes the pending set instead — and
|
||||
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.
|
||||
|
||||
### 5. `sync_events = 0` rows
|
||||
|
||||
@@ -168,11 +204,14 @@ scheduler of ours must run unconditionally and dedupe by notification identity.
|
||||
|
||||
All JVM-testable:
|
||||
|
||||
- migration reconciliation as a pure function over `(calendars, disabledIds)`,
|
||||
covering each of the four `(sync_events, visible)` combinations
|
||||
- the display-predicate swap
|
||||
- the reconciliation as a pure function over `(calendars, pendingIds)`: hide,
|
||||
settled (already off / gone), never a switch-on, and the notice predicate
|
||||
- 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
|
||||
- `FakeCalendarDataSource` assertions that `setCalendarVisible` addresses a single
|
||||
calendar by id
|
||||
- one `Calendars` query per provider tick, however many flows are collecting
|
||||
|
||||
On-device review before release (UI-touching). Local repro for the #75 shape: force a
|
||||
test calendar to `visible=0` via the provider, confirm its reminders stop and that
|
||||
|
||||
Reference in New Issue
Block a user