docs(design): plan the one-visibility-model fix for #75
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) <noreply@anthropic.com>
This commit is contained in:
177
docs/design/calendar-visibility-model.md
Normal file
177
docs/design/calendar-visibility-model.md
Normal file
@@ -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.
|
||||
Reference in New Issue
Block a user