Compare commits

..

7 Commits

Author SHA1 Message Date
7aef01d95e docs(architecture): record what the second review pass changed
All checks were successful
Translations / check (pull_request) Successful in 5s
CI / ci (pull_request) Successful in 10m57s
The visibility section claimed the reminder side needed no per-calendar
handling. It does on the one path where VISIBLE was never written: an alert the
notifier silences keeps its SCHEDULED state while its event is still ahead, and
switching the calendar back on re-posts it, so the provider's own table is the
stash the deleted SuppressedReminderStore used to be.

Also rewraps the paragraph and separates it from the one that follows, which it
had been running into since the section was added.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-25 21:38:34 +02:00
ce4d6bc4d1 fix(calendars): keep an event's own calendar when it is switched off
Excluding switched-off calendars from the event form's picker is right for
*targets*, but it also dropped the calendar an event already lives in. Editing
an event of a writable calendar switched off on this device (from a widget, a
deep link, another app's ACTION_EDIT) rendered the calendar row as the red "no
calendar" error, with the picker still enabled — so any pick turned the save
into a calendar move nobody asked for. The event's own calendar is added back
whenever it isn't among the targets, the way the managed special-dates case
already did; a calendar the app may not write to is still no target.

Settings → Notifications had missed the same predicate swap: it kept offering
per-calendar reminder overrides for switched-off calendars, where the provider
schedules no alarms and the setting could never fire.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-25 21:34:24 +02:00
4ad805e747 fix(calendars): don't flash a flushed calendar's events back on
The reconciler writes VISIBLE=0 and then releases the id from the pending set,
but the ContentObserver that invalidates the repository's cached calendar
snapshot is dispatched through the main looper and arrives later. Until it did,
the released set was read against the snapshot from before the write: the
calendar reported as on again and instances re-admitted exactly the events the
migration was hiding — on a cold start, for as long as the busy main thread took.

The snapshot cache is keyed on the pending set as well as the tick now, so any
read that sees a changed set re-queries the provider; a change to the set also
re-runs the flows, and the pending ids are read in the same pass as the
calendars rather than combined in from a live flow. Both id sets are deduped at
the prefs seam (the store is shared with SettingsPrefs, so every unrelated write
re-emitted them) and calendars() collapses identical lists, which keeps those
re-queries as rare as they should be.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-25 21:34:24 +02:00
bb6e3ad336 fix(calendars): only tell upgrades about the visibility change, and reconcile on every grant
Two holes in the reconciler, both found by review.

The one-time notice armed on any device holding a calendar at VISIBLE=0, which
is the norm on a fresh install: a second account's calendars, "Holidays in X", a
subscribed calendar. A brand-new user got a changelog dialog about a migration
they never experienced, right after onboarding. It is gated on
firstInstallTime != lastUpdateTime now, and a fresh install retires the notice
unshown ahead of the permission check — so an update installed before the first
grant can't make it look like an upgrade afterwards.

The catch-up run hung off PermissionViewModel.onGranted, which only fires for the
in-app request. Granting from Android's app-settings screen comes back through
RootScreen's ON_RESUME, so an upgrading user who took that route kept their
inherited switch-offs unflushed — their events filtered app-side while the
provider went on scheduling the reminders they asked to stop. The trigger sits on
RootScreen showing the app instead, which covers both routes. To keep that cheap,
a settled run now returns after two DataStore reads instead of querying every
calendar first.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-25 21:34:24 +02:00
edbeadfa30 fix(reminders): stop marking a silenced reminder handled and losing it
"With VISIBLE=0 the provider creates no alert rows" justified deleting the
suppression stash, but it only holds where the flag was actually written. Where
the switch lives in pendingDisabledCalendarIds — a read-only install, or an
upgrade whose flush hasn't landed — the provider still holds VISIBLE=1 and keeps
creating and broadcasting rows. The receiver silenced those in
ReminderNotifier.post and then marked the whole due batch STATE_FIRED, and
dueAlerts only ever returns STATE_SCHEDULED, so switching the calendar back on
before the event could no longer surface the reminder: it was gone.

post() now reports whether it put a notification up, and 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 SuppressedReminderStore used to be — no local
mirror, no serialization. ReminderRecovery re-posts those rows when the calendar
is switched back on, so recovery doesn't wait for the next unrelated broadcast.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-25 21:33:58 +02:00
ef48717e2c 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>
2026-07-25 13:16:55 +02:00
41593a0e9d fix(calendars): make the Settings toggle the one visibility model (#75)
Reminders never fired for a calendar hidden at system level and nothing hinted
at it: the provider only schedules reminder alarms for Calendars.VISIBLE=1,
while Calendula filtered with its own disabledCalendarIds pref and parsed
isVisibleInSystem without ever using it — two models that could disagree
indefinitely.

Settings → Calendars now writes Calendars.VISIBLE, one calendar per update
(CalendarProvider2 skips its own checkNextAlarm() reschedule for any selection
that isn't _id=), and every display predicate reads isVisibleInSystem. The
drawer's filter sheet stays a purely in-app declutter and still leaves reminders
alone.

With VISIBLE=0 the provider creates no alert rows, so there is nothing left to
suppress: the disabled-calendar gates, SuppressedReminderStore and the re-enable
recovery are gone. A one-shot migration reconciles the retired set with the app's
state winning — enabled in-app and syncing gets shown, disabled gets hidden,
everything else untouched — so the upgrade changes nothing the user sees.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-25 12:44:56 +02:00
2 changed files with 4 additions and 272 deletions

View File

@@ -174,9 +174,10 @@ gate also covers a snooze re-shown from our own alarm after its calendar was
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`.
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.
Deliberately absent until real devices prove it necessary: own alarm
scheduling, `BOOT_COMPLETED`, snooze/dismiss actions, battery-exemption
prompts.

View File

@@ -1,269 +0,0 @@
# Design: One calendar-visibility model (reminders silently never fire)
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 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).
## 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), `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
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`)
- `SuppressedReminderStore` and `CalendarsViewModel.recoverReminders`
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.
**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 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:
- 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**.
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.
**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
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 — 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
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:
- 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
- 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
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.