From 97f650da1458e920daec198a204340354920b5b8 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 25 Jul 2026 12:14:13 +0200 Subject: [PATCH] docs(design): plan the one-visibility-model fix for #75 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reminders never fire for a calendar hidden at system level, and nothing in the app hints at it: the provider only schedules reminder alarms for Calendars.VISIBLE=1, while Calendula filters with its own disabledCalendarIds pref and parses isVisibleInSystem without ever using it. Captures the decision to collapse to one model (the Settings → Calendars toggle becomes Calendars.VISIBLE, the filter sheet stays app-local), the verified provider behaviour behind it — including that VISIBLE is device-local and that a non-`_id=` selection makes the provider skip its own checkNextAlarm() reschedule — the narrowed migration, and what gets deleted. Co-Authored-By: Claude Opus 5 (1M context) --- docs/design/calendar-visibility-model.md | 177 +++++++++++++++++++++++ 1 file changed, 177 insertions(+) create mode 100644 docs/design/calendar-visibility-model.md diff --git a/docs/design/calendar-visibility-model.md b/docs/design/calendar-visibility-model.md new file mode 100644 index 0000000..2d5cf68 --- /dev/null +++ b/docs/design/calendar-visibility-model.md @@ -0,0 +1,177 @@ +# Design: One calendar-visibility model (reminders silently never fire) + +Status: **planned** on `fix/calendar-visibility-model` +Date: 2026-07-25 +Tracking: Codeberg #75. Split out of this work: #76 (read-only / not-synced +communication), #77 (accounts merged by name). + +## The bug + +Reminders never arrive for a calendar that is hidden at system level, and nothing +in the app hints at it. The event shows up, its reminders are listed on the detail +screen, and no notification ever fires. + +The cause is a design defect, not a device quirk: the app has **two** visibility +models. Calendula's own `disabledCalendarIds` decides what it shows, while the +system's `Calendars.VISIBLE` decides whether the provider schedules reminder alarms +at all. The app reads `VISIBLE` into `CalendarSource.isVisibleInSystem` and then +never uses it — `calendars()` queries with a `null` selection. So the two models can +disagree indefinitely, and when they do, reminder delivery dies quietly. + +Not a regression (nothing in `data/reminders/` changed in 2.14/2.15) and not +Android-10-specific — the gate is identical on every supported version. + +## Verified provider behaviour + +Checked against AOSP source and empirically on device; details in the +`calendars-visible-flag-facts` note. + +- AOSP `CalendarAlarmManager.scheduleNextAlarmLocked` selects reminders with + `WHERE Calendars.VISIBLE=1 AND … AND Reminders.METHOD=1`, and its cleanup + selector *deletes* `CalendarAlerts` rows whose event has `visible=0`. Identical in + the android10, android13 and android15 branches. +- `VISIBLE` is one of only three `Calendars` columns Android documents as "writable + by both an app and a sync adapter" (with `CALENDAR_DISPLAY_NAME` and + `SYNC_EVENTS`). Writing it is sanctioned, not a workaround. +- **`VISIBLE` is device-local.** Google's sync adapter neither pushes a local change + upstream nor overwrites one on a later sync — verified by flipping a Google + account's primary calendar, forcing the upload sync (the adapter cleared + `DIRTY`/`MUTATORS`) and observing no change on the web, and by forcing a full sync + carrying `sync_extra_get_settings=true` without the local value being reverted. The + adapter only seeds the initial value at calendar creation. Caveat: a negative + observation, so "no propagation seen where it would have been visible" rather than + proof of impossibility. +- `VISIBLE` and `SYNC_EVENTS` are independent. `visible=1, sync_events=0` is + reachable and persistent: a calendar that looks enabled everywhere and is + permanently empty, because its events are not on the device at all. + +## Decision + +Collapse to one model, the way Etar does it — the Settings → Calendars toggle *is* +`Calendars.VISIBLE`. + +The two existing levels map onto it directly: + +| Level | Before | After | +| --- | --- | --- | +| Filter sheet | `hiddenCalendarIds` (app-local) | unchanged | +| Settings → Calendars | `disabledCalendarIds` (app-local) | `Calendars.VISIBLE` | + +The filter sheet stays a purely in-app declutter toggle and deliberately does **not** +suppress reminders — hiding a calendar from view is not the same as asking not to be +reminded. Settings-off does stop reminders, because the provider then schedules +nothing. + +Rejected alternatives: + +- **Own the reminder delivery (the Fossify model)** — schedule our own alarms so + `VISIBLE` stops mattering. Bigger, and it leaves two visibility models in place. + Kept as a possible later step; see Non-goals. +- **Warn about the mismatch** — a banner papering over two sources of truth rather + than removing one. +- **Have the toggle write `SYNC_EVENTS` too** — turning it off makes the provider + drop the calendar's local events, so an innocuous switch becomes destructive. Never + written. + +No hint text and no confirmation dialog: the toggle now means "show this calendar", +which is what a user assumes, and the change does not leave the device. + +## Work + +### 1. Write path + +- `CalendarDataSource.setCalendarVisible(id: Long, visible: Boolean)` — update + `ContentUris.withAppendedId(Calendars.CONTENT_URI, id)`, *not* the sync-adapter URI + (must work on synced calendars) and *not* a `WHERE _id IN (…)` batch. See the + gotcha below. Mirror in `FakeCalendarDataSource`; repository passthrough on `io`. +- No manual UI patching: the `ContentObserver` re-queries, and the provider re-runs + `checkNextAlarm()` itself when `VISIBLE` changes. The observer-driven invariant + holds for free. + +> **Load-bearing gotcha.** `CalendarProvider2.updateInTransaction` returns early with +> a raw `mDb.update()` unless the selection is `_id=?` or starts with `_id=` +> (`:4281`). That early return skips the dirty marking, the `MUTATORS` stamp **and the +> `checkNextAlarm()` reschedule at `:4314-4322`** — i.e. it would silently skip the +> very reminder rescheduling this fix exists to trigger. Always address one calendar +> by appended id. A batch of per-row `ContentProviderOperation`s is fine (those +> generate `_id=n`, which the provider handles explicitly); a single IN-clause op is +> not. + +### 2. Read path + +- 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). +- `hiddenCalendarIds` keeps working exactly as it does today, unioned as before. + +### 3. Deletions + +With `VISIBLE=0` the provider creates no alert rows, so there is nothing to suppress, +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 + +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) + +Runs once after `WRITE_CALENDAR` is granted, guarded by a DataStore flag, then the +old key is dropped. + +- Enabled in-app **and** `visible=0` **and** `sync_events=1` → write `visible=1`. +- Disabled in-app → write `visible=0`. +- Everything else untouched. + +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. + +### 5. `sync_events = 0` rows + +No toggle at all — flipping it cannot produce an event. The row still shows, labelled +"not synced", sorted to the bottom of its account group regardless of alphabetical +order, and dimmed using the screen's existing `dimmed`/`dimIf` convention. Consult +the `material-3` skill before designing the label. **Tracked in #76**, not here. + +## Non-goals + +The missed-broadcast class of failure: a reminder lost because the provider's +`EVENT_REMINDER` broadcast never arrives (OEM app-sleeping, an OEM-modified provider) +or because a competing calendar app marked the alert `FIRED` first. There is no +rescan, and `dueAlerts` only accepts `STATE_SCHEDULED`, so one missed broadcast loses +that reminder permanently. + +If we take that on later, note that Etar's `AlarmScheduler` self-disables once a real +`EVENT_REMINDER` is ever seen (`PROVIDER_REMINDER_PREF_KEY`) — **that latch must not +be copied.** It assumes the only failure is "broadcast never arrives"; ours includes +"broadcast arrives fine for other calendars, but no alert row was ever created". Any +scheduler of ours must run unconditionally and dedupe by notification identity. + +## Testing + +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 +- `FakeCalendarDataSource` assertions that `setCalendarVisible` addresses a single + calendar by id + +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 +the Settings toggle brings them back. + +## Release + +Behaviour change plus a migration and a device-wide-flag change, so this reads as a +minor (2.17.0) rather than a patch. New strings land in `values/strings.xml` only; +translations come back through Weblate.