From 41593a0e9d153d7fd049d9915b3e9080fec47b7d Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 25 Jul 2026 12:44:56 +0200 Subject: [PATCH 1/7] fix(calendars): make the Settings toggle the one visibility model (#75) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 16 +++ .../jeanlucmakiola/calendula/CalendulaApp.kt | 17 +++ .../data/calendar/CalendarDataSource.kt | 31 +++++ .../calendula/data/calendar/CalendarMapper.kt | 5 + .../data/calendar/CalendarRepository.kt | 9 ++ .../data/calendar/CalendarRepositoryImpl.kt | 56 +++++--- .../calendar/CalendarVisibilityMigration.kt | 85 ++++++++++++ .../calendula/data/calendar/Projections.kt | 2 + .../calendula/data/prefs/CalendarPrefs.kt | 46 ++++--- .../data/reminders/EventReminderReceiver.kt | 41 +----- .../data/reminders/ReminderNotifier.kt | 7 - .../data/reminders/SuppressedReminderStore.kt | 130 ------------------ .../domain/CalendarVisibilityPlan.kt | 45 ++++++ .../jeanlucmakiola/calendula/domain/Models.kt | 15 ++ .../calendula/ui/calendars/CalendarsScreen.kt | 41 +++--- .../ui/calendars/CalendarsViewModel.kt | 76 ++-------- .../calendula/ui/edit/EventEditViewModel.kt | 20 ++- .../calendula/ui/filter/FilterViewModel.kt | 11 +- .../calendula/ui/imports/ImportViewModel.kt | 16 +-- .../ui/permission/PermissionViewModel.kt | 12 +- app/src/main/res/values/strings.xml | 4 +- .../data/calendar/CalendarMapperTest.kt | 14 ++ .../calendar/CalendarRepositoryImplTest.kt | 81 ++++++++--- .../data/calendar/FakeCalendarDataSource.kt | 13 ++ .../calendula/data/prefs/CalendarPrefsTest.kt | 55 +++++--- .../data/reminders/PostableAlertsTest.kt | 71 ---------- .../reminders/SuppressedReminderStoreTest.kt | 129 ----------------- .../domain/CalendarVisibilityPlanTest.kt | 96 +++++++++++++ .../ui/edit/EventEditViewModelTest.kt | 22 ++- docs/ARCHITECTURE.md | 9 ++ 30 files changed, 619 insertions(+), 556 deletions(-) create mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityMigration.kt delete mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStore.kt create mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlan.kt delete mode 100644 app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/PostableAlertsTest.kt delete mode 100644 app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStoreTest.kt create mode 100644 app/src/test/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlanTest.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index c16f6ae..a0f8157 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed +- Reminders now arrive for every calendar you have switched on. A calendar that + was hidden at system level — switched off in another calendar app, or never + switched on after being added — still showed its events and listed their + reminders in Calendula, but never notified: Android only schedules reminder + alarms for calendars marked visible, and Calendula kept its own separate + on/off list that had no say in it. There is now one switch: **Settings → + Calendars** turns a calendar on or off for the whole device, so what you see + and what reminds you can no longer disagree. Your current selection is carried + over on first launch, and calendars whose events aren't stored on this device + are left as they are ([#75]). + + The drawer's filter is unchanged and still app-only: hiding a calendar there + tidies your view without silencing its reminders. + ## [2.16.0] — 2026-07-24 ### Added @@ -1112,3 +1127,4 @@ automatically, with zero telemetry and no internet permission. [#42]: https://codeberg.org/jlmakiola/calendula/issues/42 [#44]: https://codeberg.org/jlmakiola/calendula/issues/44 [#70]: https://codeberg.org/jlmakiola/calendula/issues/70 +[#75]: https://codeberg.org/jlmakiola/calendula/issues/75 diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt b/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt index 87fd0a2..d9044c5 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt @@ -4,6 +4,7 @@ import android.app.Application import dagger.hilt.android.EntryPointAccessors import dagger.hilt.android.HiltAndroidApp import de.jeanlucmakiola.calendula.data.backup.BackupScheduler +import de.jeanlucmakiola.calendula.data.calendar.CalendarVisibilityMigration import de.jeanlucmakiola.calendula.data.backup.BackupWorker import de.jeanlucmakiola.calendula.data.contacts.SpecialDatesScheduler import de.jeanlucmakiola.calendula.data.contacts.SpecialDatesSyncWorker @@ -39,6 +40,22 @@ class CalendulaApp : Application() { ) reconcileAutoBackup() reconcileSpecialDates() + migrateCalendarVisibility() + } + + /** + * Fold the retired app-local "disabled calendars" set into the system's + * `Calendars.VISIBLE` once (#75). A no-op after it has run, and on a fresh + * install; a launch without the calendar permissions leaves it pending, and + * granting them on the permission screen runs it there instead. + */ + private fun migrateCalendarVisibility() { + val deps = EntryPointAccessors.fromApplication( + this, CalendarVisibilityMigration.Deps::class.java, + ) + CoroutineScope(SupervisorJob() + Dispatchers.Default).launch { + deps.calendarVisibilityMigration().runIfNeeded() + } } /** diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt index ac0b56e..77fe9b8 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt @@ -110,6 +110,16 @@ interface CalendarDataSource { /** Permanently delete a local calendar the app owns, with all its events. */ fun deleteCalendar(id: Long) + /** + * Show or hide the calendar device-wide by writing `Calendars.VISIBLE` — the + * app's one visibility model (#75). `VISIBLE` also gates the provider's own + * reminder scheduling, so switching a calendar off here is what actually + * stops its notifications; switching it on is what brings them back. + * Writable by a plain app (one of the three columns the platform documents + * as such) and device-local — no sync adapter pushes it anywhere. + */ + fun setCalendarVisible(id: Long, visible: Boolean) + /** * Create a local calendar tagged as the special-dates mirror for [type] * (a marker in `CAL_SYNC2`); returns its `Calendars._ID`. Otherwise a normal @@ -373,6 +383,27 @@ class AndroidCalendarDataSource @Inject constructor( if (deleted == 0) throw WriteFailedException("delete calendar id=$id") } + /** + * Addressed by appended id on the plain (non-sync-adapter) Calendars URI, + * one calendar per call. Both parts are load-bearing: + * `CalendarProvider2.updateInTransaction` short-circuits to a raw database + * update unless the selection is `_id=…`, skipping the dirty marking *and* + * the `checkNextAlarm()` reschedule — i.e. an `_id IN (…)` batch would write + * the flag but never re-arm the reminder alarms this write exists to + * trigger. The sync-adapter URI is avoided so the write also applies to + * synced calendars, which is where the bug bites. + */ + override fun setCalendarVisible(id: Long, visible: Boolean) { + val values = ContentValues().apply { + put(CalendarContract.Calendars.VISIBLE, if (visible) 1 else 0) + } + val rows = resolver.update( + ContentUris.withAppendedId(CalendarContract.Calendars.CONTENT_URI, id), + values, null, null, + ) + if (rows == 0) throw WriteFailedException("set calendar visibility id=$id") + } + override fun createManagedCalendar(displayName: String, color: Int, type: SpecialDateType): Long { val name = displayName.trim().ifEmpty { Fallbacks.UNNAMED_CALENDAR } val values = ContentValues().apply { diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarMapper.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarMapper.kt index c4d214a..af2ac96 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarMapper.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarMapper.kt @@ -31,5 +31,10 @@ internal fun ColumnReader.toCalendarSource(): CalendarSource { isManaged = isLocal && getString(CalendarProjection.IDX_MANAGED_MARKER) ?.startsWith(CalendarProjection.MANAGED_MARKER_PREFIX) == true, + // A provider that leaves the column NULL is treated as syncing — the + // harmless default, since this flag only ever holds the one-shot + // visibility migration back from switching a calendar on. + syncsEvents = isNull(CalendarProjection.IDX_SYNC_EVENTS) || + getInt(CalendarProjection.IDX_SYNC_EVENTS) != 0, ) } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt index c006159..1dcd32a 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt @@ -38,6 +38,15 @@ interface CalendarRepository { /** Permanently delete a local calendar the app owns, with all its events. */ suspend fun deleteCalendar(id: Long) + /** + * Show or hide [ids] device-wide (`Calendars.VISIBLE`), which is also what + * turns the provider's reminder scheduling for them on or off — see + * [CalendarDataSource.setCalendarVisible]. Each calendar is written on its + * own, in order; a failure part-way leaves the earlier writes standing (the + * observer reports whatever actually landed). + */ + suspend fun setCalendarsVisible(ids: Collection, visible: Boolean) + /** * Every event of the writable local calendars, ready to serialise into a * whole-calendar `.ics` backup (see [CalendarDataSource.exportableEvents]). diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt index f9528d2..059d473 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt @@ -57,42 +57,59 @@ class CalendarRepositoryImpl @Inject constructor( .reQuery { dataSource.calendars() } .flowOn(io) - // Instances are filtered by the app-side hidden ∪ disabled calendar sets - // (M3): an event is dropped whenever the user has hidden *or* disabled its - // calendar. Re-runs when the provider ticks *or* either set changes — - // toggling a calendar in the filter sheet or the calendar manager updates - // every view immediately. [calendars] stays unfiltered so those screens can - // list and re-enable hidden/disabled calendars. + // Instances are filtered by the system's per-calendar VISIBLE flag ∪ the + // app-side hidden set: an event is dropped when the user switched its + // calendar off in Settings → Calendars (which also stops the provider + // scheduling its reminders) *or* hid it in the filter sheet. Re-runs when + // the provider ticks — writing VISIBLE notifies, so switching a calendar + // updates every view — or when the hidden set changes. [calendars] stays + // unfiltered so those screens can list and re-enable invisible calendars. override fun instances(range: ClosedRange): Flow> = combine( ticks .onStart { emit(Unit) } .reQuery { - dataSource.instances( - beginMillis = range.start.toEpochMillis(), - endMillis = range.endInclusive.toEpochMillis(), + // Both reads in one pass, so a list of instances is never + // filtered against a visibility snapshot from another tick. + QueriedInstances( + instances = dataSource.instances( + beginMillis = range.start.toEpochMillis(), + endMillis = range.endInclusive.toEpochMillis(), + ), + invisibleCalendarIds = invisibleCalendarIds(), ) }, prefs.hiddenCalendarIds, - prefs.disabledCalendarIds, - ) { instances, hidden, disabled -> - val excluded = hidden + disabled - if (excluded.isEmpty()) instances - else instances.filterNot { it.calendarId in excluded } + ) { queried, hidden -> + val excluded = hidden + queried.invisibleCalendarIds + if (excluded.isEmpty()) queried.instances + else queried.instances.filterNot { it.calendarId in excluded } } - // hidden and disabled both derive from one DataStore, so toggling - // either makes both re-emit and combine briefly surfaces the same - // list twice — collapse the duplicate so views don't re-render for it. + // Any DataStore edit re-emits the hidden set even when it is + // unchanged (e.g. writing the last-used calendar), which would + // re-surface an identical list — collapse those so views don't + // re-render for them. .distinctUntilChanged() .flowOn(io) + /** One instances query plus the visibility it must be filtered against. */ + private data class QueriedInstances( + val instances: List, + val invisibleCalendarIds: Set, + ) + + /** Calendars switched off at system level — hidden, and never reminded about. */ + private fun invisibleCalendarIds(): Set = dataSource.calendars() + .filterNot { it.isVisibleInSystem } + .mapTo(mutableSetOf()) { it.id } + override suspend fun eventDetail(eventId: Long): EventDetail = withContext(io) { dataSource.eventDetail(eventId) ?: throw NoSuchEventException(eventId) } override suspend fun searchEvents(query: String): List = withContext(io) { if (query.isBlank()) return@withContext emptyList() - val excluded = prefs.hiddenCalendarIds.first() + prefs.disabledCalendarIds.first() + val excluded = prefs.hiddenCalendarIds.first() + invisibleCalendarIds() dataSource.searchEvents(query) .let { if (excluded.isEmpty()) it else it.filterNot { e -> e.calendarId in excluded } } } @@ -118,6 +135,9 @@ class CalendarRepositoryImpl @Inject constructor( override suspend fun deleteCalendar(id: Long) = withContext(io) { dataSource.deleteCalendar(id) } + override suspend fun setCalendarsVisible(ids: Collection, visible: Boolean) = + withContext(io) { ids.forEach { dataSource.setCalendarVisible(it, visible) } } + override suspend fun exportEvents(calendarIds: Set?) = withContext(io) { dataSource.exportableEvents(calendarIds) } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityMigration.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityMigration.kt new file mode 100644 index 0000000..9d7a656 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityMigration.kt @@ -0,0 +1,85 @@ +package de.jeanlucmakiola.calendula.data.calendar + +import android.Manifest +import android.content.Context +import android.content.pm.PackageManager +import android.util.Log +import androidx.core.content.ContextCompat +import dagger.hilt.EntryPoint +import dagger.hilt.InstallIn +import dagger.hilt.android.qualifiers.ApplicationContext +import dagger.hilt.components.SingletonComponent +import de.jeanlucmakiola.calendula.data.di.IoDispatcher +import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs +import de.jeanlucmakiola.calendula.domain.calendarVisibilityPlan +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withContext +import kotlin.coroutines.cancellation.CancellationException +import javax.inject.Inject +import javax.inject.Singleton + +/** + * One-shot reconciliation of the two visibility models Calendula used to run in + * parallel (#75): the app-local "disabled calendars" set is folded into the + * system's `Calendars.VISIBLE`, which from now on is the only one. See + * [calendarVisibilityPlan] for the rules — the app's state wins, so what the + * user sees today is exactly what they keep seeing after the upgrade. + * + * Runs at most once. Both permissions are required (the flag is a provider + * write), so a run that finds them missing changes nothing and leaves the guard + * unset — the next launch, or the moment the permission screen grants them, + * tries again. Only once the writes land is the guard set and the retired key + * dropped. + */ +@Singleton +class CalendarVisibilityMigration @Inject constructor( + @ApplicationContext private val context: Context, + private val dataSource: CalendarDataSource, + private val prefs: CalendarPrefs, + @IoDispatcher private val io: CoroutineDispatcher, +) { + + suspend fun runIfNeeded() = withContext(io) { + if (prefs.visibilityMigrationDone.first()) return@withContext + if (!hasCalendarPermissions()) return@withContext + try { + val plan = calendarVisibilityPlan( + calendars = dataSource.calendars(), + disabledCalendarIds = prefs.legacyDisabledCalendarIds(), + ) + // One calendar per write: the provider skips its reminder-alarm + // reschedule for anything but a single-id update (see + // [CalendarDataSource.setCalendarVisible]). + plan.hide.forEach { dataSource.setCalendarVisible(it, false) } + plan.show.forEach { dataSource.setCalendarVisible(it, true) } + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + // A part-applied plan is safe to redo: every write is idempotent and + // the source set is still on disk, so leave the guard unset and try + // again next launch rather than stranding the user half-migrated. + Log.w(TAG, "Calendar visibility migration failed; will retry", e) + return@withContext + } + prefs.setVisibilityMigrationDone() + prefs.clearLegacyDisabledCalendarIds() + } + + private fun hasCalendarPermissions(): Boolean = + listOf(Manifest.permission.READ_CALENDAR, Manifest.permission.WRITE_CALENDAR) + .all { + ContextCompat.checkSelfPermission(context, it) == PackageManager.PERMISSION_GRANTED + } + + /** Lets non-injectable entry points (the Application) reach the migration. */ + @EntryPoint + @InstallIn(SingletonComponent::class) + interface Deps { + fun calendarVisibilityMigration(): CalendarVisibilityMigration + } + + private companion object { + const val TAG = "CalendarVisibilityMigration" + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt index 774e19b..21eb4e7 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt @@ -19,6 +19,7 @@ internal object CalendarProjection { // uses to recognise its own managed calendars, independent of any // stored preference id (which a backup restore / data wipe can lose). MANAGED_MARKER_COLUMN, + CalendarContract.Calendars.SYNC_EVENTS, ) const val DESCRIPTION_COLUMN: String = CalendarContract.Calendars.CAL_SYNC1 @@ -36,6 +37,7 @@ internal object CalendarProjection { const val IDX_ACCESS_LEVEL = 6 const val IDX_DESCRIPTION = 7 const val IDX_MANAGED_MARKER = 8 + const val IDX_SYNC_EVENTS = 9 } internal object InstanceProjection { diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt index bc01927..8c85d8a 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt @@ -2,17 +2,21 @@ package de.jeanlucmakiola.calendula.data.prefs import androidx.datastore.core.DataStore import androidx.datastore.preferences.core.Preferences +import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.stringPreferencesKey import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map import javax.inject.Inject import javax.inject.Singleton /** - * App-side preference for "calendars the user has hidden in this app", - * separate from the system's per-calendar VISIBLE flag. + * App-side preference for "calendars the user has hidden in this app" — the + * drawer's filter sheet, a purely in-app declutter. It deliberately does *not* + * suppress reminders; switching a calendar off entirely is the system's + * `Calendars.VISIBLE` flag, written straight to the provider (#75). * * Persisted as a comma-separated string of Long ids; non-numeric tokens are * silently dropped (defensive — see CalendarPrefsTest). @@ -40,28 +44,35 @@ class CalendarPrefs @Inject constructor( } /** - * App-side preference for "calendars the user has disabled in this app" — a - * heavier level than [hiddenCalendarIds]. A disabled calendar is removed from - * every surface (drawer filter, event-form picker, import picker) and its - * events never appear; it stays listed only in Settings → Calendars so it can - * be re-enabled. Stored exactly like the hidden set; never touches the - * system's VISIBLE/SYNC_EVENTS flags, so other calendar apps are unaffected. + * The retired app-local "disabled calendars" set, readable only so the + * one-shot visibility migration can reconcile it into `Calendars.VISIBLE` + * (see CalendarVisibilityMigration). Nothing else may consult it — it is a + * second visibility model, which is exactly what #75 removed. Dropped by + * [clearLegacyDisabledCalendarIds] once the migration has run. */ - val disabledCalendarIds: Flow> = store.data.map { prefs -> + suspend fun legacyDisabledCalendarIds(): Set = store.data.first().let { prefs -> prefs[DISABLED_IDS_KEY].orEmpty() .split(',') .mapNotNull { it.trim().toLongOrNull() } .toSet() } - suspend fun setDisabledCalendarIds(ids: Set) { - store.edit { prefs -> - if (ids.isEmpty()) { - prefs.remove(DISABLED_IDS_KEY) - } else { - prefs[DISABLED_IDS_KEY] = ids.sorted().joinToString(",") - } - } + suspend fun clearLegacyDisabledCalendarIds() { + store.edit { prefs -> prefs.remove(DISABLED_IDS_KEY) } + } + + /** + * Whether the one-shot reconciliation of the retired disabled set into + * `Calendars.VISIBLE` has run. Set only once it actually wrote (it needs + * `WRITE_CALENDAR`), so a run skipped for a missing permission is retried on + * the next launch. + */ + val visibilityMigrationDone: Flow = store.data.map { prefs -> + prefs[VISIBILITY_MIGRATION_KEY] == true + } + + suspend fun setVisibilityMigrationDone() { + store.edit { prefs -> prefs[VISIBILITY_MIGRATION_KEY] = true } } /** @@ -79,6 +90,7 @@ class CalendarPrefs @Inject constructor( companion object { internal val HIDDEN_IDS_KEY = stringPreferencesKey("hidden_calendar_ids") internal val DISABLED_IDS_KEY = stringPreferencesKey("disabled_calendar_ids") + internal val VISIBILITY_MIGRATION_KEY = booleanPreferencesKey("visibility_migration_done") internal val LAST_USED_CALENDAR_KEY = longPreferencesKey("last_used_calendar_id") } } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt index 22fc5cf..a83cc52 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt @@ -8,7 +8,6 @@ import android.content.pm.PackageManager import android.provider.CalendarContract import androidx.core.content.ContextCompat import dagger.hilt.android.AndroidEntryPoint -import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Dispatchers @@ -17,29 +16,6 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.launch import javax.inject.Inject -/** - * True when [this] alert belongs to a calendar the user disabled in-app, so its - * reminder must be suppressed (mirroring the event filtering in - * CalendarRepositoryImpl). Alerts whose calendar is unknown (id 0L — e.g. a - * pre-upgrade snooze PendingIntent minted before EXTRA_CALENDAR_ID existed) are - * never treated as disabled. This is the one predicate the disabled-calendar - * gate is built from: [postableAlerts] here and the choke point in - * [ReminderNotifier.post] both use it. - */ -internal fun ReminderAlert.isForDisabledCalendar(disabledCalendarIds: Set): Boolean = - calendarId != 0L && calendarId in disabledCalendarIds - -/** - * The due alerts that should actually surface as notifications: everything - * except alerts whose calendar the user has disabled in-app. The caller still - * marks the full due set fired, so suppressed alerts are not re-broadcast by the - * provider. - */ -internal fun postableAlerts( - due: List, - disabledCalendarIds: Set, -): List = due.filterNot { it.isForDisabledCalendar(disabledCalendarIds) } - /** * Becomes the app that turns the calendar provider's reminder alarms into * visible notifications (the Etar model — the provider broadcasts @@ -49,6 +25,10 @@ internal fun postableAlerts( * we query every still-scheduled, due `CalendarAlerts` row ourselves, post * them, and mark them fired. Posting happens before marking — a crash in * between re-posts silently (same tag) rather than losing the reminder. + * + * There is no per-calendar filtering here: a calendar switched off in + * Settings → Calendars has `Calendars.VISIBLE = 0`, and the provider creates no + * alert rows for it in the first place (#75). */ @AndroidEntryPoint class EventReminderReceiver : BroadcastReceiver() { @@ -56,8 +36,6 @@ class EventReminderReceiver : BroadcastReceiver() { @Inject lateinit var alertStore: ReminderAlertStore @Inject lateinit var notifier: ReminderNotifier @Inject lateinit var settingsPrefs: SettingsPrefs - @Inject lateinit var calendarPrefs: CalendarPrefs - @Inject lateinit var suppressedStore: SuppressedReminderStore override fun onReceive(context: Context, intent: Intent) { if (intent.action != CalendarContract.ACTION_EVENT_REMINDER) return @@ -72,17 +50,8 @@ class EventReminderReceiver : BroadcastReceiver() { if (settingsPrefs.remindersEnabled.first()) { val now = System.currentTimeMillis() val due = alertStore.dueAlerts(now) - val disabled = calendarPrefs.disabledCalendarIds.first() - val postable = postableAlerts(due, disabled) - // Suppress reminders for disabled calendars, but still mark - // every due alert fired so the provider stops re-broadcasting - // the suppressed ones. Stash those suppressed alerts so - // re-enabling their calendar can recover them (they would - // otherwise stay STATE_FIRED forever with no re-scan). - postable.forEach { notifier.post(it) } + due.forEach { notifier.post(it) } alertStore.markFired(due.map { it.alertId }, now) - suppressedStore.stash(due - postable.toSet(), now) - suppressedStore.purgeExpired(now) } } finally { pendingResult.finish() diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt index 73a4656..6c62972 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt @@ -14,7 +14,6 @@ import androidx.core.content.ContextCompat import dagger.hilt.android.qualifiers.ApplicationContext import de.jeanlucmakiola.calendula.MainActivity import de.jeanlucmakiola.calendula.R -import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs import de.jeanlucmakiola.calendula.data.prefs.is24Hour import de.jeanlucmakiola.calendula.data.prefs.resolveFirstDay @@ -38,7 +37,6 @@ import javax.inject.Singleton class ReminderNotifier @Inject constructor( @ApplicationContext private val context: Context, private val settingsPrefs: SettingsPrefs, - private val calendarPrefs: CalendarPrefs, ) { /** False when the user declined `POST_NOTIFICATIONS` or muted the app. */ @@ -50,11 +48,6 @@ class ReminderNotifier @Inject constructor( } suspend fun post(alert: ReminderAlert) { - // The single choke point for the disabled-calendar gate: it covers both - // the provider broadcast (EventReminderReceiver) and a snoozed re-show - // (ReminderActionReceiver), so a calendar disabled after a snooze no - // longer notifies — without either receiver duplicating the check. - if (alert.isForDisabledCalendar(calendarPrefs.disabledCalendarIds.first())) return ensureChannel() val title = alert.title.ifBlank { context.getString(R.string.event_untitled) } val is24Hour = settingsPrefs.timeFormat.first() diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStore.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStore.kt deleted file mode 100644 index eb5b6e0..0000000 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStore.kt +++ /dev/null @@ -1,130 +0,0 @@ -package de.jeanlucmakiola.calendula.data.reminders - -import androidx.datastore.core.DataStore -import androidx.datastore.preferences.core.MutablePreferences -import androidx.datastore.preferences.core.Preferences -import androidx.datastore.preferences.core.edit -import androidx.datastore.preferences.core.stringSetPreferencesKey -import java.util.Base64 -import javax.inject.Inject -import javax.inject.Singleton - -/** - * Still relevant while the event has not ended: a reminder for an event that is - * already over is pointless to re-surface. Falls back to the begin time when the - * end is unknown (0L). Used both to decide what to re-post and to purge the stash. - */ -internal fun ReminderAlert.isRelevantAt(nowMillis: Long): Boolean = - (endMillis.takeIf { it > 0L } ?: beginMillis) >= nowMillis - -/** - * Local stash of reminder alerts that fired while their calendar was disabled - * in-app. [EventReminderReceiver] marks every due alert `STATE_FIRED` regardless - * (so the provider stops re-broadcasting the suppressed ones), which would - * otherwise lose those reminders forever — there is no re-scan. Stashing lets - * [de.jeanlucmakiola.calendula.ui.calendars.CalendarsViewModel] re-post them if - * the user re-enables the calendar before the event is over. - * - * Persisted in the shared preferences DataStore as a set of self-describing - * strings (one per alert); the stash never reaches a screen, so there is no - * domain model. Entries whose event has already ended are dropped on the next - * stash/recover/purge, so the stash only ever holds a handful of pending alerts. - */ -@Singleton -class SuppressedReminderStore @Inject constructor( - private val store: DataStore, -) { - - /** Add [alerts] to the stash, replacing any existing entry with the same id. */ - suspend fun stash(alerts: List, nowMillis: Long) { - if (alerts.isEmpty()) return - store.edit { prefs -> - val byId = decodeAll(prefs).associateByTo(mutableMapOf()) { it.alertId } - alerts.forEach { byId[it.alertId] = it } - prefs.putStash(byId.values.filter { it.isRelevantAt(nowMillis) }) - } - } - - /** - * Remove and return the still-relevant stashed alerts belonging to any of - * [calendarIds]; drops expired entries for every calendar in passing. - */ - suspend fun recoverFor(calendarIds: Set, nowMillis: Long): List { - val recovered = mutableListOf() - store.edit { prefs -> - val kept = decodeAll(prefs).filter { alert -> - when { - !alert.isRelevantAt(nowMillis) -> false // expired: drop - alert.calendarId in calendarIds -> { recovered += alert; false } - else -> true - } - } - prefs.putStash(kept) - } - return recovered - } - - /** Drop entries whose event has already ended — cheap opportunistic cleanup. */ - suspend fun purgeExpired(nowMillis: Long) { - store.edit { prefs -> - prefs.putStash(decodeAll(prefs).filter { it.isRelevantAt(nowMillis) }) - } - } - - private fun decodeAll(prefs: Preferences): List = - prefs[KEY].orEmpty().mapNotNull { decodeStashEntry(it) } - - private fun MutablePreferences.putStash(alerts: List) { - val encoded = alerts.map { encodeStashEntry(it) }.toSet() - if (encoded.isEmpty()) remove(KEY) else set(KEY, encoded) - } - - private companion object { - val KEY = stringSetPreferencesKey("suppressed_reminders") - } -} - -// One stash entry as a delimited string. The '|' separator is safe because every -// free-text field is Base64-encoded first (that alphabet never contains '|'), and -// a null location is stored as a distinct sentinel that Base64 also never yields. -private const val FIELD_SEP = "|" -private const val NULL_LOCATION = "-" - -internal fun encodeStashEntry(alert: ReminderAlert): String = listOf( - alert.alertId.toString(), - alert.eventId.toString(), - alert.calendarId.toString(), - alert.beginMillis.toString(), - alert.endMillis.toString(), - if (alert.isAllDay) "1" else "0", - alert.title.toBase64(), - alert.location?.toBase64() ?: NULL_LOCATION, -).joinToString(FIELD_SEP) - -/** Reverse of [encodeStashEntry]; returns null for a malformed entry (dropped). */ -internal fun decodeStashEntry(raw: String): ReminderAlert? { - val parts = raw.split(FIELD_SEP) - if (parts.size != 8) return null - return try { - ReminderAlert( - alertId = parts[0].toLong(), - eventId = parts[1].toLong(), - calendarId = parts[2].toLong(), - beginMillis = parts[3].toLong(), - endMillis = parts[4].toLong(), - title = parts[6].fromBase64(), - location = parts[7].takeIf { it != NULL_LOCATION }?.fromBase64(), - isAllDay = parts[5] == "1", - ) - } catch (e: NumberFormatException) { - null - } catch (e: IllegalArgumentException) { // bad Base64 - null - } -} - -private fun String.toBase64(): String = - Base64.getEncoder().encodeToString(toByteArray(Charsets.UTF_8)) - -private fun String.fromBase64(): String = - String(Base64.getDecoder().decode(this), Charsets.UTF_8) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlan.kt b/app/src/main/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlan.kt new file mode 100644 index 0000000..e3e0ca6 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlan.kt @@ -0,0 +1,45 @@ +package de.jeanlucmakiola.calendula.domain + +/** + * The `Calendars.VISIBLE` writes that bring the system flag in line with the + * app's retired disabled set. Empty when the two already agree. + */ +data class CalendarVisibilityPlan( + val show: Set = emptySet(), + val hide: Set = emptySet(), +) { + val isEmpty: Boolean get() = show.isEmpty() && hide.isEmpty() +} + +/** + * Reconcile the two visibility models Calendula used to keep in parallel (#75), + * with the app's state winning: + * + * - enabled in-app but hidden at system level → show it, *unless* the calendar + * doesn't sync its events to the device ([CalendarSource.syncsEvents]). Those + * hold no local events, so switching them on would produce nothing but an + * "enabled yet permanently empty" row; left off, they read honestly as off. + * - disabled in-app → hide it at system level, which is what stops the provider + * scheduling its reminders. + * - anything already in agreement is left untouched, so the migration writes as + * little as possible. + * + * App-state-wins is also what keeps the upgrade invisible: reconciling the other + * way would make events the user sees today vanish. + */ +fun calendarVisibilityPlan( + calendars: List, + disabledCalendarIds: Set, +): CalendarVisibilityPlan { + val show = mutableSetOf() + val hide = mutableSetOf() + for (calendar in calendars) { + val disabledInApp = calendar.id in disabledCalendarIds + when { + disabledInApp && calendar.isVisibleInSystem -> hide += calendar.id + !disabledInApp && !calendar.isVisibleInSystem && calendar.syncsEvents -> + show += calendar.id + } + } + return CalendarVisibilityPlan(show = show, hide = hide) +} diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt b/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt index aaf4fb5..3d6fd02 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt @@ -8,6 +8,13 @@ data class CalendarSource( val accountName: String, val accountType: String, val color: Int, + /** + * The system's per-calendar `Calendars.VISIBLE` flag — the single visibility + * model: it decides both what Calendula shows and whether the provider + * schedules this calendar's reminder alarms at all (#75). Settings → + * Calendars writes it; the drawer's filter sheet is a separate, purely + * in-app declutter that leaves reminders alone. + */ val isVisibleInSystem: Boolean, /** * Whether events in this calendar can be created/edited/deleted @@ -34,6 +41,14 @@ data class CalendarSource( * even after a backup restore clears the app's stored ids. */ val isManaged: Boolean = false, + /** + * Whether the provider keeps this calendar's events on the device + * (`Calendars.SYNC_EVENTS`). Independent of [isVisibleInSystem]: a calendar + * with `syncsEvents = false` holds no local events at all, so making it + * visible cannot produce a single one — which is why the one-shot visibility + * migration leaves those calendars switched off. + */ + val syncsEvents: Boolean = true, ) data class EventInstance( diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt index 1c99d16..ade9210 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt @@ -139,7 +139,6 @@ fun CalendarsScreen( viewModel: CalendarsViewModel = hiltViewModel(), ) { val calendars by viewModel.calendars.collectAsStateWithLifecycle() - val disabledIds by viewModel.disabledCalendarIds.collectAsStateWithLifecycle() val error by viewModel.error.collectAsStateWithLifecycle() val backupResult by viewModel.backupResult.collectAsStateWithLifecycle() val autoBackup by viewModel.autoBackup.collectAsStateWithLifecycle() @@ -177,7 +176,6 @@ fun CalendarsScreen( CalendarsList( local = calendars.filter { it.isLocal }, synced = calendars.filterNot { it.isLocal }, - disabledIds = disabledIds, error = error, onConsumeError = viewModel::consumeError, backupResult = backupResult, @@ -191,8 +189,8 @@ fun CalendarsScreen( onBack = onBack, onAdd = { editorSession++; editorId = NEW_CALENDAR_ID }, onEdit = { calendar -> editorSession++; editorId = calendar.id }, - onSetDisabled = viewModel::setDisabled, - onSetAccountDisabled = viewModel::setAccountDisabled, + onSetVisible = viewModel::setCalendarVisible, + onSetAccountVisible = viewModel::setAccountVisible, ) } } @@ -201,7 +199,6 @@ fun CalendarsScreen( private fun CalendarsList( local: List, synced: List, - disabledIds: Set, error: Boolean, onConsumeError: () -> Unit, backupResult: BackupResult?, @@ -215,8 +212,8 @@ private fun CalendarsList( onBack: () -> Unit, onAdd: () -> Unit, onEdit: (CalendarSource) -> Unit, - onSetDisabled: (Long, Boolean) -> Unit, - onSetAccountDisabled: (Collection, Boolean) -> Unit, + onSetVisible: (Long, Boolean) -> Unit, + onSetAccountVisible: (Collection, Boolean) -> Unit, ) { val context = LocalContext.current val snackbarHostState = remember { SnackbarHostState() } @@ -286,7 +283,7 @@ private fun CalendarsList( // Local (device-only) calendars — one collapsible group. The header's // "+" adds a calendar; the switch enables/disables them all at once; // tapping a calendar row opens its editor. - val localDisabled = local.isNotEmpty() && local.all { it.id in disabledIds } + val localDisabled = local.isNotEmpty() && local.none { it.isVisibleInSystem } CalendarGroup( title = stringResource(R.string.calendars_local_header), expanded = localExpanded, @@ -298,14 +295,14 @@ private fun CalendarsList( onManage = onAdd, onToggleExpand = { localExpanded = !localExpanded }, showToggleAll = local.isNotEmpty(), - allEnabled = local.none { it.id in disabledIds }, - onToggleAll = { enabled -> onSetAccountDisabled(local.map { it.id }, !enabled) }, + allEnabled = local.all { it.isVisibleInSystem }, + onToggleAll = { enabled -> onSetAccountVisible(local.map { it.id }, enabled) }, ) { if (local.isEmpty()) { HintText(stringResource(R.string.calendars_local_empty)) } else { local.forEachIndexed { index, calendar -> - val disabled = calendar.id in disabledIds + val disabled = !calendar.isVisibleInSystem GroupedRow( title = calendar.displayName, summary = calendar.description, @@ -317,7 +314,7 @@ private fun CalendarsList( EnableSwitch( calendarName = calendar.displayName, enabled = !disabled, - onToggle = { enabled -> onSetDisabled(calendar.id, !enabled) }, + onToggle = { enabled -> onSetVisible(calendar.id, enabled) }, ) }, onClick = { onEdit(calendar) }, @@ -415,7 +412,7 @@ private fun CalendarsList( .forEach { (account, cals) -> val expanded = account !in collapsedAccounts val accountType = cals.first().accountType - val accountDisabled = cals.all { it.id in disabledIds } + val accountDisabled = cals.none { it.isVisibleInSystem } Spacer(Modifier.height(16.dp)) CalendarGroup( title = account, @@ -436,11 +433,11 @@ private fun CalendarsList( } }, showToggleAll = true, - allEnabled = cals.none { it.id in disabledIds }, - onToggleAll = { enabled -> onSetAccountDisabled(cals.map { it.id }, !enabled) }, + allEnabled = cals.all { it.isVisibleInSystem }, + onToggleAll = { enabled -> onSetAccountVisible(cals.map { it.id }, enabled) }, ) { cals.forEachIndexed { index, calendar -> - val disabled = calendar.id in disabledIds + val disabled = !calendar.isVisibleInSystem GroupedRow( title = calendar.displayName, position = if (index == cals.lastIndex) Position.Bottom else Position.Middle, @@ -451,7 +448,7 @@ private fun CalendarsList( EnableSwitch( calendarName = calendar.displayName, enabled = !disabled, - onToggle = { enabled -> onSetDisabled(calendar.id, !enabled) }, + onToggle = { enabled -> onSetVisible(calendar.id, enabled) }, ) }, ) @@ -698,10 +695,12 @@ private fun CalendarEditor( } /** - * The per-row enable/disable control. Checked = the calendar is shown in the - * app; unchecking disables it (events, filters and pickers all drop it) without - * touching any provider data. Carries its own content description so the toggle - * is self-describing to screen readers even on a dimmed row. + * The per-row on/off control, writing the system's `Calendars.VISIBLE`: checked + * = the calendar is shown, unchecked = it drops out of every surface (events, + * filters, pickers) and the provider stops scheduling its reminders. The flag is + * device-local — nothing is deleted and nothing is synced anywhere. Carries its + * own content description so the toggle is self-describing to screen readers + * even on a dimmed row. */ @Composable private fun EnableSwitch( diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt index 8439616..78423c0 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt @@ -12,10 +12,7 @@ import de.jeanlucmakiola.calendula.data.calendar.CalendarRepository import de.jeanlucmakiola.calendula.data.di.IoDispatcher import de.jeanlucmakiola.calendula.data.ics.IcsExporter import de.jeanlucmakiola.calendula.data.prefs.BackupStatus -import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs -import de.jeanlucmakiola.calendula.data.reminders.ReminderNotifier -import de.jeanlucmakiola.calendula.data.reminders.SuppressedReminderStore import de.jeanlucmakiola.calendula.domain.CalendarSource import de.jeanlucmakiola.calendula.domain.ics.IcsWriter import kotlinx.coroutines.CoroutineDispatcher @@ -45,10 +42,7 @@ class CalendarsViewModel @Inject constructor( @ApplicationContext private val context: Context, private val repository: CalendarRepository, private val icsExporter: IcsExporter, - private val prefs: CalendarPrefs, private val settingsPrefs: SettingsPrefs, - private val suppressedStore: SuppressedReminderStore, - private val notifier: ReminderNotifier, @IoDispatcher private val io: CoroutineDispatcher, ) : ViewModel() { @@ -62,20 +56,6 @@ class CalendarsViewModel @Inject constructor( initialValue = emptyList(), ) - /** - * Calendars the user has disabled in the app. This screen is the only - * surface that lists them, so it both reads the set (to dim the rows) and - * toggles it. Every other surface simply excludes these ids. - */ - val disabledCalendarIds: StateFlow> = - prefs.disabledCalendarIds - .flowOn(io) - .stateIn( - scope = viewModelScope, - started = SharingStarted.WhileSubscribed(5_000L), - initialValue = emptySet(), - ) - /** Automatic-backup settings + last-run status, for the Backup section UI. */ val autoBackup: StateFlow = combine( settingsPrefs.autoBackupEnabled, @@ -140,54 +120,24 @@ class CalendarsViewModel @Inject constructor( } /** - * Enable or disable a calendar app-side. Disabling removes it from every - * surface but Settings → Calendars (and hides its events) without touching - * provider data — purely a reversible Calendula-local view choice. + * Switch a calendar on or off. This is the app's one visibility model: it + * writes the system's `Calendars.VISIBLE`, so the calendar disappears from + * every surface *and* the provider stops (or resumes) scheduling its + * reminders. Nothing is patched by hand — the provider notifies and the + * observer re-queries. */ - fun setDisabled(id: Long, disabled: Boolean) { - viewModelScope.launch { - val current = prefs.disabledCalendarIds.first() - val next = if (disabled) current + id else current - id - if (next != current) { - prefs.setDisabledCalendarIds(next) - if (!disabled) recoverReminders(setOf(id)) - } - } + fun setCalendarVisible(id: Long, visible: Boolean) = write { + repository.setCalendarsVisible(listOf(id), visible) } /** - * Enable or disable every calendar of one account in a single write — the - * "toggle all" affordance on an account header. Done as one set update so the - * per-calendar [setDisabled] calls can't race each other. + * Switch every calendar of one account on or off — the "toggle all" + * affordance on an account header. Each row is written on its own (the + * provider only re-arms reminder alarms for a single-id update), in one + * coroutine so the writes can't race each other. */ - fun setAccountDisabled(ids: Collection, disabled: Boolean) { - viewModelScope.launch { - val current = prefs.disabledCalendarIds.first() - val next = if (disabled) current + ids else current - ids.toSet() - if (next != current) { - prefs.setDisabledCalendarIds(next) - if (!disabled) recoverReminders(current intersect ids.toSet()) - } - } - } - - /** - * Re-post the reminders that fired while [reEnabledIds] were disabled and are - * still relevant (event not yet over), then drop them from the stash. Runs - * after the disabled set is written, so the notifier's own disabled gate lets - * them through. Best-effort at re-enable time: it mirrors the receiver gates - * (reminders on + postable), and there is no later re-scan, so alerts left - * unposted because those gates are closed are simply released. - */ - private suspend fun recoverReminders(reEnabledIds: Set) { - if (reEnabledIds.isEmpty()) return - val recovered = suppressedStore.recoverFor(reEnabledIds, System.currentTimeMillis()) - if (recovered.isNotEmpty() && - settingsPrefs.remindersEnabled.first() && - notifier.canPost() - ) { - recovered.forEach { notifier.post(it) } - } + fun setAccountVisible(ids: Collection, visible: Boolean) = write { + repository.setCalendarsVisible(ids, visible) } // --- Automatic backup (issue #8) ------------------------------------ diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt index 45ae36f..457b998 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt @@ -177,18 +177,16 @@ class EventEditViewModel @Inject constructor( repository.calendars().catch { emit(emptyList()) } /** - * Writable calendars — the only valid event targets. Disabled calendars are - * excluded, so you can't create into a calendar you've removed from the app; - * a last-used preselect landing on a now-disabled calendar falls back to the - * first remaining writable one (handled by [resolvedCalendarId] and [state]). - * Managed special-dates calendars are excluded too: their events are owned by - * the contact sync, which would delete any user event created there. + * Writable calendars — the only valid event targets. Calendars switched off + * in Settings → Calendars are excluded, so you can't create into one you've + * turned off; a last-used preselect landing on a now-off calendar falls back + * to the first remaining writable one (handled by [resolvedCalendarId] and + * [state]). Managed special-dates calendars are excluded too: their events + * are owned by the contact sync, which would delete any user event created + * there. */ - private val writableCalendars: Flow> = combine( - allCalendars, - prefs.disabledCalendarIds, - ) { calendars, disabled -> - calendars.filter { it.canModifyContents && it.id !in disabled && !it.isManaged } + private val writableCalendars: Flow> = allCalendars.map { calendars -> + calendars.filter { it.canModifyContents && it.isVisibleInSystem && !it.isManaged } } /** The target calendar id, resolved exactly as the form shows it. */ diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/FilterViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/FilterViewModel.kt index 28dd615..66062b0 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/FilterViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/filter/FilterViewModel.kt @@ -30,12 +30,11 @@ class FilterViewModel @Inject constructor( combine( repository.calendars(), prefs.hiddenCalendarIds, - prefs.disabledCalendarIds, - ) { calendars, hidden, disabled -> - // Disabled calendars are gone from the app entirely — they don't - // belong in the drawer's hide/show list (you can't hide what's - // already disabled). They live only in Settings → Calendars. - val enabled = calendars.filterNot { it.id in disabled } + ) { calendars, hidden -> + // Calendars switched off in Settings → Calendars are off device-wide + // and don't belong in the drawer's hide/show list (you can't hide + // what is already off). They live only in Settings → Calendars. + val enabled = calendars.filter { it.isVisibleInSystem } if (enabled.isEmpty()) { FilterUiState.Failure(FailureReason.NoCalendarsConfigured) } else { diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/imports/ImportViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/imports/ImportViewModel.kt index 8ba9084..d7c536a 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/imports/ImportViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/imports/ImportViewModel.kt @@ -7,7 +7,6 @@ import dagger.hilt.android.lifecycle.HiltViewModel import de.jeanlucmakiola.calendula.data.calendar.CalendarRepository import de.jeanlucmakiola.calendula.data.di.IoDispatcher import de.jeanlucmakiola.calendula.data.ics.IcsImporter -import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs import de.jeanlucmakiola.calendula.domain.CalendarSource import de.jeanlucmakiola.calendula.domain.EventForm import de.jeanlucmakiola.calendula.domain.ics.IcsImportSummary @@ -57,7 +56,6 @@ sealed interface ImportUiState { class ImportViewModel @Inject constructor( private val repository: CalendarRepository, private val importer: IcsImporter, - private val prefs: CalendarPrefs, @IoDispatcher private val io: CoroutineDispatcher, ) : ViewModel() { @@ -87,16 +85,18 @@ class ImportViewModel @Inject constructor( warnings = parsed.warnings, ) else -> { - // A disabled calendar is removed from the app, so it can't be - // an import target — exclude it alongside the read-only ones. - // Managed special-dates calendars are contact-derived and - // editor-locked, so they're not a valid destination either. - val disabled = prefs.disabledCalendarIds.first() + // A calendar switched off in Settings → Calendars is off + // everywhere, so it can't be an import target — exclude it + // alongside the read-only ones. Managed special-dates + // calendars are contact-derived and editor-locked, so + // they're not a valid destination either. ImportUiState.Many( events = parsed.events, warnings = parsed.warnings, calendars = repository.calendars().first() - .filter { it.canModifyContents && !it.isManaged && it.id !in disabled }, + .filter { + it.canModifyContents && !it.isManaged && it.isVisibleInSystem + }, ) } } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt index 05df891..6754834 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt @@ -1,20 +1,30 @@ package de.jeanlucmakiola.calendula.ui.permission import androidx.lifecycle.ViewModel +import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel +import de.jeanlucmakiola.calendula.data.calendar.CalendarVisibilityMigration import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow +import kotlinx.coroutines.launch import javax.inject.Inject @HiltViewModel -class PermissionViewModel @Inject constructor() : ViewModel() { +class PermissionViewModel @Inject constructor( + private val visibilityMigration: CalendarVisibilityMigration, +) : ViewModel() { private val _state = MutableStateFlow(PermissionUiState.Rationale) val state: StateFlow = _state.asStateFlow() fun onGranted() { _state.value = PermissionUiState.Granted + // The one-shot visibility migration needs the calendar permissions, so a + // launch that started without them left it pending (#75) — this is the + // moment it can finally run. A no-op once it has, and on a fresh install + // (nothing was ever disabled) it just stamps its guard. + viewModelScope.launch { visibilityMigration.runIfNeeded() } } fun onDenied() { diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index ef9bfce..96b3f4a 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -473,8 +473,8 @@ Your calendars No local calendars yet. Create one to keep events on this device only. Add calendar - Turn a calendar off to remove it from the app — its events, filters and pickers. Nothing is deleted, and you can turn it back on here anytime. - Show \"%1$s\" in the app + Turn a calendar off to hide it on this device — its events disappear from the app and it stops reminding you. Nothing is deleted, no other device is affected, and you can turn it back on here anytime. + Show \"%1$s\" Synced calendars These come from accounts on your device. Create and edit them in their own app. Manage in app diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarMapperTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarMapperTest.kt index ea02483..1645692 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarMapperTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarMapperTest.kt @@ -15,6 +15,7 @@ class CalendarMapperTest { visible: Int = 1, accessLevel: Int = CalendarContract.Calendars.CAL_ACCESS_OWNER, description: String? = null, + syncEvents: Int? = 1, ): MapColumnReader = MapColumnReader( CalendarProjection.IDX_ID to id, CalendarProjection.IDX_DISPLAY_NAME to displayName, @@ -24,6 +25,7 @@ class CalendarMapperTest { CalendarProjection.IDX_VISIBLE to visible, CalendarProjection.IDX_ACCESS_LEVEL to accessLevel, CalendarProjection.IDX_DESCRIPTION to description, + CalendarProjection.IDX_SYNC_EVENTS to syncEvents, ) @Test @@ -49,6 +51,18 @@ class CalendarMapperTest { ) } + @Test + fun `sync_events 0 marks the calendar as not syncing its events`() { + assertThat(reader(syncEvents = 0).toCalendarSource().syncsEvents).isFalse() + } + + @Test + fun `a NULL sync_events column is treated as syncing`() { + // The harmless default: it only ever holds the visibility migration back + // from switching a calendar on. + assertThat(reader(syncEvents = null).toCalendarSource().syncsEvents).isTrue() + } + @Test fun `null displayName falls back to placeholder`() { val src = reader(displayName = null).toCalendarSource() diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt index 14fa836..6ad5df5 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt @@ -41,8 +41,12 @@ class CalendarRepositoryImplTest { produceFile = { tempDir.resolve("repo_test_prefs.preferences_pb").toFile() }, ) - private fun makeCal(id: Long, name: String = "Cal $id"): CalendarSource = - CalendarSource(id, name, "x@y", "LOCAL", 0xFF112233.toInt(), true) + private fun makeCal( + id: Long, + name: String = "Cal $id", + visible: Boolean = true, + ): CalendarSource = + CalendarSource(id, name, "x@y", "LOCAL", 0xFF112233.toInt(), visible) private fun makeEvent( id: Long, @@ -171,37 +175,40 @@ class CalendarRepositoryImplTest { } @Test - fun `instances drops events whose calendar the user disabled`(@TempDir tempDir: Path) = runTest { - val prefs = newPrefs(tempDir) - prefs.setDisabledCalendarIds(setOf(2L)) + fun `instances drops events whose calendar is hidden at system level`( + @TempDir tempDir: Path, + ) = runTest { val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(makeCal(1L), makeCal(2L, visible = false)) instancesResult = { _, _ -> listOf( - makeEvent(10L, "Enabled", calendarId = 1L), - makeEvent(11L, "Disabled", calendarId = 2L), + makeEvent(10L, "Shown", calendarId = 1L), + makeEvent(11L, "Switched off", calendarId = 2L), ) } } - val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + val repo = CalendarRepositoryImpl(fake, newPrefs(tempDir), newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) val range = Instant.fromEpochMilliseconds(0)..Instant.fromEpochMilliseconds(10_000L) repo.instances(range).test { - assertThat(awaitItem().map { it.title }).containsExactly("Enabled") + assertThat(awaitItem().map { it.title }).containsExactly("Shown") cancelAndIgnoreRemainingEvents() } } @Test - fun `instances applies the union of hidden and disabled sets`(@TempDir tempDir: Path) = runTest { + fun `instances applies the union of hidden and system-invisible calendars`( + @TempDir tempDir: Path, + ) = runTest { val prefs = newPrefs(tempDir) prefs.setHiddenCalendarIds(setOf(2L)) - prefs.setDisabledCalendarIds(setOf(3L)) val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(makeCal(1L), makeCal(2L), makeCal(3L, visible = false)) instancesResult = { _, _ -> listOf( makeEvent(10L, "Shown", calendarId = 1L), makeEvent(11L, "Hidden", calendarId = 2L), - makeEvent(12L, "Disabled", calendarId = 3L), + makeEvent(12L, "Switched off", calendarId = 3L), ) } } @@ -215,9 +222,11 @@ class CalendarRepositoryImplTest { } @Test - fun `instances re-emits when the disabled set changes`(@TempDir tempDir: Path) = runTest { - val prefs = newPrefs(tempDir) + fun `instances re-emit after a calendar is switched off in the provider`( + @TempDir tempDir: Path, + ) = runTest { val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(makeCal(1L), makeCal(2L)) instancesResult = { _, _ -> listOf( makeEvent(10L, "A", calendarId = 1L), @@ -225,19 +234,59 @@ class CalendarRepositoryImplTest { ) } } - val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + val repo = CalendarRepositoryImpl(fake, newPrefs(tempDir), newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) val range = Instant.fromEpochMilliseconds(0)..Instant.fromEpochMilliseconds(10_000L) repo.instances(range).test { assertThat(awaitItem().map { it.title }).containsExactly("A", "B").inOrder() - prefs.setDisabledCalendarIds(setOf(2L)) + // The write itself is what the provider notifies about; the observer + // tick is what makes the views re-query. + repo.setCalendarsVisible(listOf(2L), false) + fake.tick() assertThat(awaitItem().map { it.title }).containsExactly("A") cancelAndIgnoreRemainingEvents() } } + @Test + fun `setCalendarsVisible addresses each calendar on its own`( + @TempDir tempDir: Path, + ) = runTest { + // An _id IN (…) batch would skip the provider's own reminder-alarm + // reschedule, so every calendar must be written by appended id. + val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(makeCal(1L), makeCal(2L), makeCal(3L)) + } + val repo = CalendarRepositoryImpl(fake, newPrefs(tempDir), newSettings(tempDir), Dispatchers.Unconfined) + + repo.setCalendarsVisible(listOf(1L, 3L), false) + + assertThat(fake.visibilityWrites).containsExactly(1L to false, 3L to false).inOrder() + } + + @Test + fun `searchEvents drops results from calendars that are off or hidden`( + @TempDir tempDir: Path, + ) = runTest { + val prefs = newPrefs(tempDir) + prefs.setHiddenCalendarIds(setOf(3L)) + val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(makeCal(1L), makeCal(2L, visible = false), makeCal(3L)) + searchResult = { + listOf( + makeEvent(10L, "Shown", calendarId = 1L), + makeEvent(11L, "Switched off", calendarId = 2L), + makeEvent(12L, "Hidden", calendarId = 3L), + ) + } + } + val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), Dispatchers.Unconfined) + + assertThat(repo.searchEvents("e").map { it.title }).containsExactly("Shown") + } + @Test fun `createEvent delegates and returns the new id`(@TempDir tempDir: Path) = runTest { val fake = FakeCalendarDataSource().apply { nextInsertId = 77L } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt index dce1a32..ca2aa79 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt @@ -95,6 +95,19 @@ internal class FakeCalendarDataSource : CalendarDataSource { updatedCalendars += UpdatedCalendar(id, displayName, color, description) } + /** (id, visible) pairs passed to [setCalendarVisible], in call order. */ + val visibilityWrites = mutableListOf>() + + override fun setCalendarVisible(id: Long, visible: Boolean) { + writeError?.let { throw it } + visibilityWrites += id to visible + // Reflect the write so a follow-up [calendars] read sees it, the way the + // provider would once its notification has re-triggered the query. + calendarsResult = calendarsResult.map { + if (it.id == id) it.copy(isVisibleInSystem = visible) else it + } + } + override fun deleteCalendar(id: Long) { writeError?.let { throw it } deletedCalendarIds += id diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefsTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefsTest.kt index a077bae..930af94 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefsTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefsTest.kt @@ -52,32 +52,51 @@ class CalendarPrefsTest { } @Test - fun `disabledCalendarIds defaults to empty when unset`(@TempDir tempDir: Path) = runTest { - val prefs = CalendarPrefs(newDataStore(tempDir)) - assertThat(prefs.disabledCalendarIds.first()).isEmpty() + fun `legacy disabled set reads back what an older version stored`( + @TempDir tempDir: Path, + ) = runTest { + val store = newDataStore(tempDir) + val prefs = CalendarPrefs(store) + store.updateData { p -> + p.toMutablePreferences().apply { this[CalendarPrefs.DISABLED_IDS_KEY] = "2,9" } + } + assertThat(prefs.legacyDisabledCalendarIds()).isEqualTo(setOf(2L, 9L)) } @Test - fun `setDisabledCalendarIds round-trips through DataStore`(@TempDir tempDir: Path) = runTest { + fun `legacy disabled set is empty when nothing was ever stored`( + @TempDir tempDir: Path, + ) = runTest { val prefs = CalendarPrefs(newDataStore(tempDir)) - prefs.setDisabledCalendarIds(setOf(1L, 42L, 7L)) - assertThat(prefs.disabledCalendarIds.first()).isEqualTo(setOf(1L, 42L, 7L)) + assertThat(prefs.legacyDisabledCalendarIds()).isEmpty() } @Test - fun `setting empty disabled set clears storage`(@TempDir tempDir: Path) = runTest { - val prefs = CalendarPrefs(newDataStore(tempDir)) - prefs.setDisabledCalendarIds(setOf(1L)) - prefs.setDisabledCalendarIds(emptySet()) - assertThat(prefs.disabledCalendarIds.first()).isEmpty() - } - - @Test - fun `hidden and disabled sets are stored independently`(@TempDir tempDir: Path) = runTest { - val prefs = CalendarPrefs(newDataStore(tempDir)) + fun `clearing the legacy disabled set leaves the hidden set alone`( + @TempDir tempDir: Path, + ) = runTest { + val store = newDataStore(tempDir) + val prefs = CalendarPrefs(store) prefs.setHiddenCalendarIds(setOf(1L)) - prefs.setDisabledCalendarIds(setOf(2L)) + store.updateData { p -> + p.toMutablePreferences().apply { this[CalendarPrefs.DISABLED_IDS_KEY] = "2" } + } + + prefs.clearLegacyDisabledCalendarIds() + + assertThat(prefs.legacyDisabledCalendarIds()).isEmpty() assertThat(prefs.hiddenCalendarIds.first()).isEqualTo(setOf(1L)) - assertThat(prefs.disabledCalendarIds.first()).isEqualTo(setOf(2L)) + } + + @Test + fun `visibility migration guard defaults to not-done and latches`( + @TempDir tempDir: Path, + ) = runTest { + val prefs = CalendarPrefs(newDataStore(tempDir)) + assertThat(prefs.visibilityMigrationDone.first()).isFalse() + + prefs.setVisibilityMigrationDone() + + assertThat(prefs.visibilityMigrationDone.first()).isTrue() } } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/PostableAlertsTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/PostableAlertsTest.kt deleted file mode 100644 index 5fedc7d..0000000 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/PostableAlertsTest.kt +++ /dev/null @@ -1,71 +0,0 @@ -package de.jeanlucmakiola.calendula.data.reminders - -import com.google.common.truth.Truth.assertThat -import org.junit.jupiter.api.Test - -class PostableAlertsTest { - - private fun alert(alertId: Long, calendarId: Long) = ReminderAlert( - alertId = alertId, - eventId = alertId * 10, - calendarId = calendarId, - beginMillis = 0L, - endMillis = 0L, - title = "Event $alertId", - location = null, - isAllDay = false, - ) - - @Test - fun `keeps alerts when no calendar is disabled`() { - val due = listOf(alert(1, calendarId = 100), alert(2, calendarId = 200)) - - val postable = postableAlerts(due, disabledCalendarIds = emptySet()) - - assertThat(postable).isEqualTo(due) - } - - @Test - fun `drops alerts for a disabled calendar`() { - val keep = alert(1, calendarId = 100) - val drop = alert(2, calendarId = 200) - - val postable = postableAlerts(listOf(keep, drop), disabledCalendarIds = setOf(200)) - - assertThat(postable).containsExactly(keep) - } - - @Test - fun `drops every alert when all their calendars are disabled`() { - val due = listOf(alert(1, calendarId = 100), alert(2, calendarId = 100)) - - val postable = postableAlerts(due, disabledCalendarIds = setOf(100)) - - assertThat(postable).isEmpty() - } - - @Test - fun `keeps multiple alerts from the same enabled calendar`() { - val due = listOf(alert(1, calendarId = 100), alert(2, calendarId = 100)) - - val postable = postableAlerts(due, disabledCalendarIds = setOf(999)) - - assertThat(postable).isEqualTo(due) - } - - @Test - fun `alert with unknown calendar id 0 is never treated as disabled`() { - // Pre-upgrade snooze PendingIntents carry no calendar id (defaults to 0L). - val preUpgrade = alert(1, calendarId = 0L) - - assertThat(preUpgrade.isForDisabledCalendar(disabledCalendarIds = setOf(0L))).isFalse() - assertThat(postableAlerts(listOf(preUpgrade), disabledCalendarIds = setOf(0L))) - .containsExactly(preUpgrade) - } - - @Test - fun `isForDisabledCalendar matches only the disabled ids`() { - assertThat(alert(1, calendarId = 200).isForDisabledCalendar(setOf(200))).isTrue() - assertThat(alert(1, calendarId = 100).isForDisabledCalendar(setOf(200))).isFalse() - } -} diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStoreTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStoreTest.kt deleted file mode 100644 index 74c5eda..0000000 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/SuppressedReminderStoreTest.kt +++ /dev/null @@ -1,129 +0,0 @@ -package de.jeanlucmakiola.calendula.data.reminders - -import androidx.datastore.core.DataStore -import androidx.datastore.preferences.core.PreferenceDataStoreFactory -import androidx.datastore.preferences.core.Preferences -import com.google.common.truth.Truth.assertThat -import kotlinx.coroutines.test.runTest -import org.junit.jupiter.api.Test -import org.junit.jupiter.api.io.TempDir -import java.nio.file.Path - -class SuppressedReminderStoreTest { - - private fun newDataStore(tempDir: Path): DataStore = - PreferenceDataStoreFactory.create( - produceFile = { tempDir.resolve("test_prefs.preferences_pb").toFile() }, - ) - - private fun alert( - alertId: Long, - calendarId: Long, - endMillis: Long = Long.MAX_VALUE, - title: String = "Event $alertId", - location: String? = null, - ) = ReminderAlert( - alertId = alertId, - eventId = alertId * 10, - calendarId = calendarId, - beginMillis = 0L, - endMillis = endMillis, - title = title, - location = location, - isAllDay = false, - ) - - @Test - fun `encode then decode round-trips every field including delimiters`() { - val original = alert( - alertId = 7, - calendarId = 42, - endMillis = 123_456_789L, - // Free-text with the field separator and other awkward characters. - title = "Lunch | with | Alice", - location = "Café, 3rd floor | room B", - ).copy(beginMillis = 100L, isAllDay = true) - - val decoded = decodeStashEntry(encodeStashEntry(original)) - - assertThat(decoded).isEqualTo(original) - } - - @Test - fun `decode returns null for a malformed entry`() { - assertThat(decodeStashEntry("not-a-valid-entry")).isNull() - } - - @Test - fun `null location round-trips`() { - val original = alert(1, calendarId = 1, location = null) - assertThat(decodeStashEntry(encodeStashEntry(original))).isEqualTo(original) - } - - @Test - fun `recoverFor returns and removes only the re-enabled calendars`() = runTest { - val store = SuppressedReminderStore(newDataStore(tempDir)) - val keep = alert(1, calendarId = 100) - val recoverA = alert(2, calendarId = 200) - val recoverB = alert(3, calendarId = 200) - store.stash(listOf(keep, recoverA, recoverB), nowMillis = 0L) - - val recovered = store.recoverFor(setOf(200L), nowMillis = 0L) - - assertThat(recovered).containsExactly(recoverA, recoverB) - // The still-disabled calendar's alert stays stashed; the recovered ones are gone. - assertThat(store.recoverFor(setOf(100L, 200L), nowMillis = 0L)).containsExactly(keep) - } - - @Test - fun `stash drops alerts whose event already ended`() = runTest { - val store = SuppressedReminderStore(newDataStore(tempDir)) - val past = alert(1, calendarId = 100, endMillis = 500L) - val future = alert(2, calendarId = 100, endMillis = 2_000L) - - store.stash(listOf(past, future), nowMillis = 1_000L) - - assertThat(store.recoverFor(setOf(100L), nowMillis = 1_000L)).containsExactly(future) - } - - @Test - fun `purgeExpired removes only entries past their event end`() = runTest { - val store = SuppressedReminderStore(newDataStore(tempDir)) - // Stash both before "now" so neither is dropped on write, then advance time. - store.stash( - listOf( - alert(1, calendarId = 100, endMillis = 500L), - alert(2, calendarId = 100, endMillis = 2_000L), - ), - nowMillis = 0L, - ) - - store.purgeExpired(nowMillis = 1_000L) - - assertThat(store.recoverFor(setOf(100L), nowMillis = 0L).map { it.alertId }) - .containsExactly(2L) - } - - @Test - fun `stash replaces an existing entry with the same alert id`() = runTest { - val store = SuppressedReminderStore(newDataStore(tempDir)) - store.stash(listOf(alert(1, calendarId = 100, title = "old")), nowMillis = 0L) - store.stash(listOf(alert(1, calendarId = 100, title = "new")), nowMillis = 0L) - - val recovered = store.recoverFor(setOf(100L), nowMillis = 0L) - - assertThat(recovered).hasSize(1) - assertThat(recovered.single().title).isEqualTo("new") - } - - @Test - fun `isRelevantAt is true up to the event end and false after`() { - val a = alert(1, calendarId = 1, endMillis = 1_000L) - assertThat(a.isRelevantAt(999L)).isTrue() - assertThat(a.isRelevantAt(1_000L)).isTrue() - assertThat(a.isRelevantAt(1_001L)).isFalse() - } - - @TempDir - lateinit var tempDir: Path -} diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlanTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlanTest.kt new file mode 100644 index 0000000..c6f9a54 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlanTest.kt @@ -0,0 +1,96 @@ +package de.jeanlucmakiola.calendula.domain + +import com.google.common.truth.Truth.assertThat +import org.junit.jupiter.api.Test + +/** + * The one-shot reconciliation of the retired app-local "disabled calendars" set + * into the system's `Calendars.VISIBLE` (#75), across every + * (sync_events, visible) combination. + */ +class CalendarVisibilityPlanTest { + + private fun cal( + id: Long, + visible: Boolean = true, + syncsEvents: Boolean = true, + ): CalendarSource = CalendarSource( + id = id, + displayName = "Cal $id", + accountName = "acc@local", + accountType = "LOCAL", + color = 0, + isVisibleInSystem = visible, + syncsEvents = syncsEvents, + ) + + @Test + fun `a calendar disabled in-app is switched off at system level`() { + val plan = calendarVisibilityPlan(listOf(cal(1L, visible = true)), setOf(1L)) + assertThat(plan.hide).containsExactly(1L) + assertThat(plan.show).isEmpty() + } + + @Test + fun `a calendar enabled in-app but hidden at system level is switched on`() { + val plan = calendarVisibilityPlan(listOf(cal(1L, visible = false)), emptySet()) + assertThat(plan.show).containsExactly(1L) + assertThat(plan.hide).isEmpty() + } + + @Test + fun `a not-synced calendar is never switched on`() { + // sync_events = 0 means the provider holds none of its events locally, + // so making it visible could only produce an enabled-yet-empty row. + val plan = calendarVisibilityPlan( + listOf(cal(1L, visible = false, syncsEvents = false)), + emptySet(), + ) + assertThat(plan.isEmpty).isTrue() + } + + @Test + fun `a not-synced calendar disabled in-app is still switched off`() { + val plan = calendarVisibilityPlan( + listOf(cal(1L, visible = true, syncsEvents = false)), + setOf(1L), + ) + assertThat(plan.hide).containsExactly(1L) + } + + @Test + fun `calendars that already agree are left untouched`() { + val plan = calendarVisibilityPlan( + listOf(cal(1L, visible = true), cal(2L, visible = false)), + disabledCalendarIds = setOf(2L), + ) + assertThat(plan.isEmpty).isTrue() + } + + @Test + fun `a disabled id for a calendar that no longer exists writes nothing`() { + val plan = calendarVisibilityPlan(listOf(cal(1L)), setOf(99L)) + assertThat(plan.isEmpty).isTrue() + } + + @Test + fun `a mixed device is reconciled in both directions at once`() { + val plan = calendarVisibilityPlan( + listOf( + cal(1L, visible = true), // enabled, already visible → untouched + cal(2L, visible = false), // enabled but hidden → show + cal(3L, visible = true), // disabled in-app → hide + cal(4L, visible = false, syncsEvents = false), // not synced → untouched + ), + disabledCalendarIds = setOf(3L), + ) + assertThat(plan.show).containsExactly(2L) + assertThat(plan.hide).containsExactly(3L) + } + + @Test + fun `nothing disabled and everything visible is a no-op`() { + val plan = calendarVisibilityPlan(listOf(cal(1L), cal(2L)), emptySet()) + assertThat(plan.isEmpty).isTrue() + } +} diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModelTest.kt index 51a9819..898399f 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModelTest.kt @@ -45,9 +45,9 @@ class EventEditViewModelTest { private val beginMillis = 1_781_164_800_000L private val endMillis = beginMillis + 3_600_000L - private fun cal(id: Long): CalendarSource = CalendarSource( + private fun cal(id: Long, visible: Boolean = true): CalendarSource = CalendarSource( id = id, displayName = "Cal $id", accountName = "acc@local", accountType = "LOCAL", - color = 0xFF112233.toInt(), isVisibleInSystem = true, canModifyContents = true, + color = 0xFF112233.toInt(), isVisibleInSystem = visible, canModifyContents = true, ) private fun detail(calendarId: Long, rrule: String? = null): EventDetail = EventDetail( @@ -90,6 +90,24 @@ class EventEditViewModelTest { /** Keep [EventEditViewModel.state] hot so it computes while the test drives it. */ private fun CoroutineScope.activate(vm: EventEditViewModel): Job = launch { vm.state.collect {} } + @Test + fun `a calendar switched off in settings is not offered as a target`( + @TempDir tempDir: Path, + ) = runTest(dispatcher) { + val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(cal(1L), cal(2L, visible = false)) + eventDetailResult = { detail(calendarId = 1L) } + } + val vm = viewModel(tempDir, fake) + val job = activate(vm) + + vm.openForEdit(eventId = 42L, beginMillis = beginMillis, endMillis = endMillis) + advanceUntilIdle() + + assertThat(vm.state.value?.calendars?.map { it.id }).containsExactly(1L) + job.cancel() + } + @Test fun `changing the calendar routes the save through a move, not an update`( @TempDir tempDir: Path, diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index a8c3600..302dac8 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -157,6 +157,15 @@ sequenceDiagram Posting happens before marking: a crash in between re-posts silently (same tag + `setOnlyAlertOnce`) rather than losing a reminder. Swiped notifications never return because `FIRED` rows are never re-queried. + +**One visibility model.** The provider only schedules alarms for calendars with +`Calendars.VISIBLE = 1`, so that flag *is* the app's on/off switch: Settings → +Calendars writes it (one calendar per update — `CalendarProvider2` skips its own +`checkNextAlarm()` reschedule for any selection that isn't `_id=`), and every +display predicate reads `CalendarSource.isVisibleInSystem`. There is deliberately +no second, app-local model and therefore nothing to suppress here. 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 scheduling, `BOOT_COMPLETED`, snooze/dismiss actions, battery-exemption prompts. From ef48717e2c89037b880c5fff2eae83552f8aaf9b Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 25 Jul 2026 13:16:55 +0200 Subject: [PATCH 2/7] fix(calendars): hide-only visibility reconcile, and keep it working read-only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- CHANGELOG.md | 13 +- .../jeanlucmakiola/calendula/CalendulaApp.kt | 20 +-- .../data/calendar/CalendarDataSource.kt | 29 +++++ .../data/calendar/CalendarRepository.kt | 5 + .../data/calendar/CalendarRepositoryImpl.kt | 94 +++++++++++--- .../calendar/CalendarVisibilityMigration.kt | 85 ------------- .../calendar/CalendarVisibilityReconciler.kt | 106 ++++++++++++++++ .../calendula/data/prefs/CalendarPrefs.kt | 99 +++++++++------ .../data/reminders/EventReminderReceiver.kt | 4 +- .../data/reminders/ReminderActionReceiver.kt | 4 +- .../data/reminders/ReminderNotifier.kt | 18 +++ .../domain/CalendarVisibilityPlan.kt | 65 ++++++---- .../jeanlucmakiola/calendula/domain/Models.kt | 9 +- .../jeanlucmakiola/calendula/ui/RootScreen.kt | 9 ++ .../ui/calendars/CalendarVisibilityNotice.kt | 62 ++++++++++ .../calendula/ui/calendars/CalendarsScreen.kt | 4 +- .../ui/permission/PermissionViewModel.kt | 13 +- app/src/main/res/values-de/strings.xml | 2 - app/src/main/res/values-es/strings.xml | 2 - app/src/main/res/values-fr/strings.xml | 2 - app/src/main/res/values-it/strings.xml | 2 - app/src/main/res/values-pl/strings.xml | 2 - app/src/main/res/values/strings.xml | 6 +- .../calendar/CalendarRepositoryImplTest.kt | 117 ++++++++++++++++++ .../data/calendar/FakeCalendarDataSource.kt | 17 ++- .../calendula/data/prefs/CalendarPrefsTest.kt | 46 ++++--- .../domain/CalendarVisibilityPlanTest.kt | 102 +++++++++------ docs/ARCHITECTURE.md | 15 ++- 28 files changed, 687 insertions(+), 265 deletions(-) delete mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityMigration.kt create mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityReconciler.kt create mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarVisibilityNotice.kt diff --git a/CHANGELOG.md b/CHANGELOG.md index a0f8157..543bf84 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,9 +15,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 alarms for calendars marked visible, and Calendula kept its own separate on/off list that had no say in it. There is now one switch: **Settings → Calendars** turns a calendar on or off for the whole device, so what you see - and what reminds you can no longer disagree. Your current selection is carried - over on first launch, and calendars whose events aren't stored on this device - are left as they are ([#75]). + and what reminds you can no longer disagree ([#75]). + + Calendars you had switched off in Calendula are switched off here too on first + launch. Calendars that were already off — hidden in another calendar app, or + never switched on after being added — stay off, and Calendula says so once + rather than quietly switching them on for every app on your device; you can + turn any of them back on in Settings → Calendars. + + If you gave Calendula read-only access to your calendars, the switch still + works: your choice is kept in the app until it can be written. The drawer's filter is unchanged and still app-only: hiding a calendar there tidies your view without silencing its reminders. diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt b/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt index d9044c5..d87f65d 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt @@ -4,7 +4,7 @@ import android.app.Application import dagger.hilt.android.EntryPointAccessors import dagger.hilt.android.HiltAndroidApp import de.jeanlucmakiola.calendula.data.backup.BackupScheduler -import de.jeanlucmakiola.calendula.data.calendar.CalendarVisibilityMigration +import de.jeanlucmakiola.calendula.data.calendar.CalendarVisibilityReconciler import de.jeanlucmakiola.calendula.data.backup.BackupWorker import de.jeanlucmakiola.calendula.data.contacts.SpecialDatesScheduler import de.jeanlucmakiola.calendula.data.contacts.SpecialDatesSyncWorker @@ -40,21 +40,23 @@ class CalendulaApp : Application() { ) reconcileAutoBackup() reconcileSpecialDates() - migrateCalendarVisibility() + reconcileCalendarVisibility() } /** - * Fold the retired app-local "disabled calendars" set into the system's - * `Calendars.VISIBLE` once (#75). A no-op after it has run, and on a fresh - * install; a launch without the calendar permissions leaves it pending, and - * granting them on the permission screen runs it there instead. + * Flush any calendar switch-off the app hasn't been allowed to write into + * the system's `Calendars.VISIBLE` yet — including the retired app-local + * "disabled calendars" set the upgrade inherits (#75). A no-op on a fresh + * install and in the steady state; a launch without the calendar permissions + * leaves the set pending, and granting them on the permission screen runs it + * there instead. */ - private fun migrateCalendarVisibility() { + private fun reconcileCalendarVisibility() { val deps = EntryPointAccessors.fromApplication( - this, CalendarVisibilityMigration.Deps::class.java, + this, CalendarVisibilityReconciler.Deps::class.java, ) CoroutineScope(SupervisorJob() + Dispatchers.Default).launch { - deps.calendarVisibilityMigration().runIfNeeded() + deps.calendarVisibilityReconciler().run() } } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt index 77fe9b8..499b956 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt @@ -120,6 +120,22 @@ interface CalendarDataSource { */ fun setCalendarVisible(id: Long, visible: Boolean) + /** + * Whether one calendar is currently switched on at system level, without + * reading every row — for the reminder gate, which sees a calendar id and + * nothing else. Null when the answer can't be had: no row (the calendar was + * deleted) or no read permission. + */ + fun isCalendarVisible(id: Long): Boolean? + + /** + * Whether the app holds `WRITE_CALENDAR`, i.e. may write + * [setCalendarVisible] at all. Read-only users (READ granted, WRITE denied) + * keep their calendar switches app-side instead — see + * [de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs.pendingDisabledCalendarIds]. + */ + fun canWriteCalendars(): Boolean + /** * Create a local calendar tagged as the special-dates mirror for [type] * (a marker in `CAL_SYNC2`); returns its `Calendars._ID`. Otherwise a normal @@ -404,6 +420,19 @@ class AndroidCalendarDataSource @Inject constructor( if (rows == 0) throw WriteFailedException("set calendar visibility id=$id") } + override fun isCalendarVisible(id: Long): Boolean? { + if (!hasCalendarPermission()) return null + return resolver.query( + ContentUris.withAppendedId(CalendarContract.Calendars.CONTENT_URI, id), + arrayOf(CalendarContract.Calendars.VISIBLE), + null, null, null, + )?.use { if (it.moveToFirst()) it.getInt(0) != 0 else null } + } + + override fun canWriteCalendars(): Boolean = + ContextCompat.checkSelfPermission(context, Manifest.permission.WRITE_CALENDAR) == + PackageManager.PERMISSION_GRANTED + override fun createManagedCalendar(displayName: String, color: Int, type: SpecialDateType): Long { val name = displayName.trim().ifEmpty { Fallbacks.UNNAMED_CALENDAR } val values = ContentValues().apply { diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt index 1dcd32a..066d9af 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt @@ -44,6 +44,11 @@ interface CalendarRepository { * [CalendarDataSource.setCalendarVisible]. Each calendar is written on its * own, in order; a failure part-way leaves the earlier writes standing (the * observer reports whatever actually landed). + * + * Without `WRITE_CALENDAR` the choice is kept app-side instead (see + * [de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs.pendingDisabledCalendarIds]), + * where it filters events and reminders just the same until it can be + * written. */ suspend fun setCalendarsVisible(ids: Collection, visible: Boolean) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt index 059d473..0b4dd9d 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt @@ -20,7 +20,10 @@ import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOn import kotlinx.coroutines.flow.onStart +import kotlinx.coroutines.sync.Mutex +import kotlinx.coroutines.sync.withLock import kotlinx.coroutines.withContext +import java.util.concurrent.atomic.AtomicLong import kotlin.time.Instant import javax.inject.Inject import javax.inject.Singleton @@ -47,23 +50,43 @@ class CalendarRepositoryImpl @Inject constructor( extraBufferCapacity = 1, ) + /** + * Bumped on every provider notification, so one tick's calendar read can be + * shared by everything that needs it (see [calendarsSnapshot]). + */ + private val generation = AtomicLong(0L) + init { - dataSource.registerChangeListener { ticks.tryEmit(Unit) } + dataSource.registerChangeListener { + generation.incrementAndGet() + ticks.tryEmit(Unit) + } } + // A switch-off the app hasn't been allowed to write yet is folded into the + // flag itself, so every consumer — the Settings switch, the filter sheet, + // the form and import pickers, the widgets — reads one visibility and can't + // disagree with what the user just tapped. The reconciler reads the data + // source directly, because it needs the provider's own answer. override fun calendars(): Flow> = - ticks - .onStart { emit(Unit) } - .reQuery { dataSource.calendars() } - .flowOn(io) + combine( + ticks.onStart { emit(Unit) }.reQuery { calendarsSnapshot() }, + prefs.pendingDisabledCalendarIds, + ) { calendars, pendingDisabled -> + if (pendingDisabled.isEmpty()) calendars + else calendars.map { + if (it.id in pendingDisabled) it.copy(isVisibleInSystem = false) else it + } + }.flowOn(io) // Instances are filtered by the system's per-calendar VISIBLE flag ∪ the - // app-side hidden set: an event is dropped when the user switched its - // calendar off in Settings → Calendars (which also stops the provider - // scheduling its reminders) *or* hid it in the filter sheet. Re-runs when - // the provider ticks — writing VISIBLE notifies, so switching a calendar - // updates every view — or when the hidden set changes. [calendars] stays - // unfiltered so those screens can list and re-enable invisible calendars. + // switch-offs still waiting to be written to it ∪ the app-side hidden set: + // an event is dropped when the user switched its calendar off in Settings → + // Calendars (which also stops the provider scheduling its reminders) *or* + // hid it in the filter sheet. Re-runs when the provider ticks — writing + // VISIBLE notifies, so switching a calendar updates every view — or when + // either set changes. [calendars] stays unfiltered so those screens can list + // and re-enable invisible calendars. override fun instances(range: ClosedRange): Flow> = combine( ticks @@ -80,8 +103,9 @@ class CalendarRepositoryImpl @Inject constructor( ) }, prefs.hiddenCalendarIds, - ) { queried, hidden -> - val excluded = hidden + queried.invisibleCalendarIds + prefs.pendingDisabledCalendarIds, + ) { queried, hidden, pendingDisabled -> + val excluded = hidden + pendingDisabled + queried.invisibleCalendarIds if (excluded.isEmpty()) queried.instances else queried.instances.filterNot { it.calendarId in excluded } } @@ -99,17 +123,42 @@ class CalendarRepositoryImpl @Inject constructor( ) /** Calendars switched off at system level — hidden, and never reminded about. */ - private fun invisibleCalendarIds(): Set = dataSource.calendars() + private suspend fun invisibleCalendarIds(): Set = calendarsSnapshot() .filterNot { it.isVisibleInSystem } .mapTo(mutableSetOf()) { it.id } + private val calendarsLock = Mutex() + private var cachedGeneration = -1L + private var cachedCalendars: List = emptyList() + + /** + * The calendar list for the current tick, queried once and shared. Every + * open view collects [calendars] *and* filters its instances by visibility, + * which used to cost one full `Calendars` query each per tick. Reusing a + * single read also keeps them consistent: within a tick, what a screen lists + * and what its events are filtered against can't come from two snapshots. + * + * An empty result is never cached — it is what a read without the calendar + * permission returns, and the grant itself doesn't notify the provider. + */ + private suspend fun calendarsSnapshot(): List = calendarsLock.withLock { + val current = generation.get() + if (current != cachedGeneration || cachedCalendars.isEmpty()) { + cachedCalendars = dataSource.calendars() + cachedGeneration = current + } + cachedCalendars + } + override suspend fun eventDetail(eventId: Long): EventDetail = withContext(io) { dataSource.eventDetail(eventId) ?: throw NoSuchEventException(eventId) } override suspend fun searchEvents(query: String): List = withContext(io) { if (query.isBlank()) return@withContext emptyList() - val excluded = prefs.hiddenCalendarIds.first() + invisibleCalendarIds() + val excluded = prefs.hiddenCalendarIds.first() + + prefs.pendingDisabledCalendarIds.first() + + invisibleCalendarIds() dataSource.searchEvents(query) .let { if (excluded.isEmpty()) it else it.filterNot { e -> e.calendarId in excluded } } } @@ -136,7 +185,20 @@ class CalendarRepositoryImpl @Inject constructor( withContext(io) { dataSource.deleteCalendar(id) } override suspend fun setCalendarsVisible(ids: Collection, visible: Boolean) = - withContext(io) { ids.forEach { dataSource.setCalendarVisible(it, visible) } } + withContext(io) { + if (dataSource.canWriteCalendars()) { + ids.forEach { dataSource.setCalendarVisible(it, visible) } + // Nothing of ours is left waiting for the provider once the + // write lands (and switching one back on retires its entry). + prefs.removePendingDisabledCalendarIds(ids) + } else if (visible) { + prefs.removePendingDisabledCalendarIds(ids) + } else { + // Read-only permission: the switch still works, app-side, and + // the reconciler flushes it if WRITE_CALENDAR ever arrives. + prefs.addPendingDisabledCalendarIds(ids) + } + } override suspend fun exportEvents(calendarIds: Set?) = withContext(io) { dataSource.exportableEvents(calendarIds) } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityMigration.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityMigration.kt deleted file mode 100644 index 9d7a656..0000000 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityMigration.kt +++ /dev/null @@ -1,85 +0,0 @@ -package de.jeanlucmakiola.calendula.data.calendar - -import android.Manifest -import android.content.Context -import android.content.pm.PackageManager -import android.util.Log -import androidx.core.content.ContextCompat -import dagger.hilt.EntryPoint -import dagger.hilt.InstallIn -import dagger.hilt.android.qualifiers.ApplicationContext -import dagger.hilt.components.SingletonComponent -import de.jeanlucmakiola.calendula.data.di.IoDispatcher -import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs -import de.jeanlucmakiola.calendula.domain.calendarVisibilityPlan -import kotlinx.coroutines.CoroutineDispatcher -import kotlinx.coroutines.flow.first -import kotlinx.coroutines.withContext -import kotlin.coroutines.cancellation.CancellationException -import javax.inject.Inject -import javax.inject.Singleton - -/** - * One-shot reconciliation of the two visibility models Calendula used to run in - * parallel (#75): the app-local "disabled calendars" set is folded into the - * system's `Calendars.VISIBLE`, which from now on is the only one. See - * [calendarVisibilityPlan] for the rules — the app's state wins, so what the - * user sees today is exactly what they keep seeing after the upgrade. - * - * Runs at most once. Both permissions are required (the flag is a provider - * write), so a run that finds them missing changes nothing and leaves the guard - * unset — the next launch, or the moment the permission screen grants them, - * tries again. Only once the writes land is the guard set and the retired key - * dropped. - */ -@Singleton -class CalendarVisibilityMigration @Inject constructor( - @ApplicationContext private val context: Context, - private val dataSource: CalendarDataSource, - private val prefs: CalendarPrefs, - @IoDispatcher private val io: CoroutineDispatcher, -) { - - suspend fun runIfNeeded() = withContext(io) { - if (prefs.visibilityMigrationDone.first()) return@withContext - if (!hasCalendarPermissions()) return@withContext - try { - val plan = calendarVisibilityPlan( - calendars = dataSource.calendars(), - disabledCalendarIds = prefs.legacyDisabledCalendarIds(), - ) - // One calendar per write: the provider skips its reminder-alarm - // reschedule for anything but a single-id update (see - // [CalendarDataSource.setCalendarVisible]). - plan.hide.forEach { dataSource.setCalendarVisible(it, false) } - plan.show.forEach { dataSource.setCalendarVisible(it, true) } - } catch (e: CancellationException) { - throw e - } catch (e: Exception) { - // A part-applied plan is safe to redo: every write is idempotent and - // the source set is still on disk, so leave the guard unset and try - // again next launch rather than stranding the user half-migrated. - Log.w(TAG, "Calendar visibility migration failed; will retry", e) - return@withContext - } - prefs.setVisibilityMigrationDone() - prefs.clearLegacyDisabledCalendarIds() - } - - private fun hasCalendarPermissions(): Boolean = - listOf(Manifest.permission.READ_CALENDAR, Manifest.permission.WRITE_CALENDAR) - .all { - ContextCompat.checkSelfPermission(context, it) == PackageManager.PERMISSION_GRANTED - } - - /** Lets non-injectable entry points (the Application) reach the migration. */ - @EntryPoint - @InstallIn(SingletonComponent::class) - interface Deps { - fun calendarVisibilityMigration(): CalendarVisibilityMigration - } - - private companion object { - const val TAG = "CalendarVisibilityMigration" - } -} diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityReconciler.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityReconciler.kt new file mode 100644 index 0000000..0784c7b --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityReconciler.kt @@ -0,0 +1,106 @@ +package de.jeanlucmakiola.calendula.data.calendar + +import android.Manifest +import android.content.Context +import android.content.pm.PackageManager +import android.util.Log +import androidx.core.content.ContextCompat +import dagger.hilt.EntryPoint +import dagger.hilt.InstallIn +import dagger.hilt.android.qualifiers.ApplicationContext +import dagger.hilt.components.SingletonComponent +import de.jeanlucmakiola.calendula.data.di.IoDispatcher +import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs +import de.jeanlucmakiola.calendula.domain.CalendarSource +import de.jeanlucmakiola.calendula.domain.calendarVisibilityPlan +import de.jeanlucmakiola.calendula.domain.hasSystemHiddenCalendars +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withContext +import kotlin.coroutines.cancellation.CancellationException +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Keeps the app's pending "switched off" set (see + * [CalendarPrefs.pendingDisabledCalendarIds]) and the system's + * `Calendars.VISIBLE` in step — the fold-in of the retired app-local visibility + * model (#75), and the standing drain for switch-offs made without + * `WRITE_CALENDAR`. + * + * Runs on every launch, and again the moment the permission screen grants the + * calendar permissions. It is a no-op whenever the pending set is empty, which + * is the steady state: each entry is written and dropped individually, so a run + * that dies part-way resumes exactly where it stopped and never re-applies a + * write the user has since undone by hand. + * + * The reconciliation only hides (see [calendarVisibilityPlan]). Calendars hidden + * at system level stay hidden, and the first run that sees one arms the one-time + * notice explaining why Calendula no longer lists their events. + */ +@Singleton +class CalendarVisibilityReconciler @Inject constructor( + @ApplicationContext private val context: Context, + private val dataSource: CalendarDataSource, + private val prefs: CalendarPrefs, + @IoDispatcher private val io: CoroutineDispatcher, +) { + + suspend fun run() = withContext(io) { + // Everything, the DataStore reads included, sits inside the guard: this + // runs in a bare application-scope coroutine with no exception handler, + // so an IOException from a damaged preferences file would otherwise take + // the process down on every launch. + try { + if (!hasPermission(Manifest.permission.READ_CALENDAR)) return@withContext + val pending = prefs.pendingDisabledCalendarIds.first() + val calendars = dataSource.calendars() + armNoticeOnce(calendars, pending) + if (pending.isEmpty() || !hasPermission(Manifest.permission.WRITE_CALENDAR)) { + return@withContext + } + val plan = calendarVisibilityPlan(calendars, pending) + // Already off, or gone from the device — nothing to write, so let + // those ids leave the pending set with the rest. + prefs.removePendingDisabledCalendarIds(plan.settled) + // One calendar per write: the provider skips its reminder-alarm + // reschedule for anything but a single-id update (see + // [CalendarDataSource.setCalendarVisible]). Dropping each id as it + // lands keeps a part-applied run resumable. + for (id in plan.hide) { + dataSource.setCalendarVisible(id, false) + prefs.removePendingDisabledCalendarIds(setOf(id)) + } + } catch (e: CancellationException) { + throw e + } catch (e: Exception) { + Log.w(TAG, "Calendar visibility reconcile failed; will retry", e) + } + } + + /** + * Arm the one-time notice if the device holds a calendar switched off + * outside Calendula. Evaluated once, on the first run that can read the + * calendars at all; the answer — including "nothing to say" — is stored, so + * the notice can't resurface later, when the same state would no longer be + * news to the user. + */ + private suspend fun armNoticeOnce(calendars: List, pending: Set) { + if (prefs.visibilityNoticePending.first() != null) return + prefs.setVisibilityNoticePending(hasSystemHiddenCalendars(calendars, pending)) + } + + private fun hasPermission(permission: String): Boolean = + ContextCompat.checkSelfPermission(context, permission) == PackageManager.PERMISSION_GRANTED + + /** Lets non-injectable entry points (the Application) reach the reconciler. */ + @EntryPoint + @InstallIn(SingletonComponent::class) + interface Deps { + fun calendarVisibilityReconciler(): CalendarVisibilityReconciler + } + + private companion object { + const val TAG = "CalendarVisibility" + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt index 8c85d8a..f96d11f 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt @@ -1,22 +1,24 @@ package de.jeanlucmakiola.calendula.data.prefs import androidx.datastore.core.DataStore +import androidx.datastore.preferences.core.MutablePreferences import androidx.datastore.preferences.core.Preferences import androidx.datastore.preferences.core.booleanPreferencesKey import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.stringPreferencesKey import kotlinx.coroutines.flow.Flow -import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.map import javax.inject.Inject import javax.inject.Singleton /** - * App-side preference for "calendars the user has hidden in this app" — the - * drawer's filter sheet, a purely in-app declutter. It deliberately does *not* - * suppress reminders; switching a calendar off entirely is the system's - * `Calendars.VISIBLE` flag, written straight to the provider (#75). + * App-side calendar preferences. [hiddenCalendarIds] is the drawer's filter + * sheet — a purely in-app declutter that deliberately does *not* suppress + * reminders. Switching a calendar off entirely is the system's + * `Calendars.VISIBLE` flag, written straight to the provider (#75); + * [pendingDisabledCalendarIds] only holds those switch-offs the app has not been + * allowed to write yet. * * Persisted as a comma-separated string of Long ids; non-numeric tokens are * silently dropped (defensive — see CalendarPrefsTest). @@ -27,52 +29,58 @@ class CalendarPrefs @Inject constructor( ) { val hiddenCalendarIds: Flow> = store.data.map { prefs -> - prefs[HIDDEN_IDS_KEY].orEmpty() - .split(',') - .mapNotNull { it.trim().toLongOrNull() } - .toSet() + prefs[HIDDEN_IDS_KEY].parseIds() } suspend fun setHiddenCalendarIds(ids: Set) { + store.edit { prefs -> prefs.writeIds(HIDDEN_IDS_KEY, ids) } + } + + /** + * Calendars switched off in Settings → Calendars that the provider does not + * know about yet. That switch writes the system's `Calendars.VISIBLE` (#75), + * which needs `WRITE_CALENDAR` — a user who granted read-only keeps their + * choice here instead, and so does everyone upgrading from the retired + * app-local model, whose set is read straight back out of the same key. + * + * Honoured as a display and reminder filter for as long as it is non-empty, + * so an un-flushable switch still does what the user asked. Not a second + * visibility model: `CalendarVisibilityReconciler` drains it into the + * provider entry by entry the moment the app may write, and nothing ever + * adds to it while it may. + */ + val pendingDisabledCalendarIds: Flow> = store.data.map { prefs -> + prefs[DISABLED_IDS_KEY].parseIds() + } + + suspend fun addPendingDisabledCalendarIds(ids: Collection) = + editPendingDisabled { it + ids } + + /** + * Drop [ids] from the pending set — one id at a time as the reconciler + * flushes it, so a run that fails part-way never re-applies what already + * landed (and can't undo a switch the user has since flipped by hand). + */ + suspend fun removePendingDisabledCalendarIds(ids: Collection) = + editPendingDisabled { it - ids.toSet() } + + private suspend fun editPendingDisabled(transform: (Set) -> Set) { store.edit { prefs -> - if (ids.isEmpty()) { - prefs.remove(HIDDEN_IDS_KEY) - } else { - prefs[HIDDEN_IDS_KEY] = ids.sorted().joinToString(",") - } + prefs.writeIds(DISABLED_IDS_KEY, transform(prefs[DISABLED_IDS_KEY].parseIds())) } } /** - * The retired app-local "disabled calendars" set, readable only so the - * one-shot visibility migration can reconcile it into `Calendars.VISIBLE` - * (see CalendarVisibilityMigration). Nothing else may consult it — it is a - * second visibility model, which is exactly what #75 removed. Dropped by - * [clearLegacyDisabledCalendarIds] once the migration has run. + * Whether the one-time "visibility follows this device" notice is still + * owed. Null until the reconciler has evaluated it (which needs the calendar + * permission), false once it has been shown or was never needed. */ - suspend fun legacyDisabledCalendarIds(): Set = store.data.first().let { prefs -> - prefs[DISABLED_IDS_KEY].orEmpty() - .split(',') - .mapNotNull { it.trim().toLongOrNull() } - .toSet() + val visibilityNoticePending: Flow = store.data.map { prefs -> + prefs[VISIBILITY_NOTICE_KEY] } - suspend fun clearLegacyDisabledCalendarIds() { - store.edit { prefs -> prefs.remove(DISABLED_IDS_KEY) } - } - - /** - * Whether the one-shot reconciliation of the retired disabled set into - * `Calendars.VISIBLE` has run. Set only once it actually wrote (it needs - * `WRITE_CALENDAR`), so a run skipped for a missing permission is retried on - * the next launch. - */ - val visibilityMigrationDone: Flow = store.data.map { prefs -> - prefs[VISIBILITY_MIGRATION_KEY] == true - } - - suspend fun setVisibilityMigrationDone() { - store.edit { prefs -> prefs[VISIBILITY_MIGRATION_KEY] = true } + suspend fun setVisibilityNoticePending(pending: Boolean) { + store.edit { prefs -> prefs[VISIBILITY_NOTICE_KEY] = pending } } /** @@ -90,7 +98,16 @@ class CalendarPrefs @Inject constructor( companion object { internal val HIDDEN_IDS_KEY = stringPreferencesKey("hidden_calendar_ids") internal val DISABLED_IDS_KEY = stringPreferencesKey("disabled_calendar_ids") - internal val VISIBILITY_MIGRATION_KEY = booleanPreferencesKey("visibility_migration_done") + internal val VISIBILITY_NOTICE_KEY = booleanPreferencesKey("visibility_notice_pending") internal val LAST_USED_CALENDAR_KEY = longPreferencesKey("last_used_calendar_id") } } + +private fun String?.parseIds(): Set = orEmpty() + .split(',') + .mapNotNull { it.trim().toLongOrNull() } + .toSet() + +private fun MutablePreferences.writeIds(key: Preferences.Key, ids: Set) { + if (ids.isEmpty()) remove(key) else set(key, ids.sorted().joinToString(",")) +} diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt index a83cc52..e61dbfb 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt @@ -28,7 +28,9 @@ import javax.inject.Inject * * There is no per-calendar filtering here: a calendar switched off in * Settings → Calendars has `Calendars.VISIBLE = 0`, and the provider creates no - * alert rows for it in the first place (#75). + * alert rows for it in the first place (#75). The one case that flag can't + * cover — a read-only install, which keeps its switches app-side — is gated in + * [ReminderNotifier.post], where the snoozed re-show passes too. */ @AndroidEntryPoint class EventReminderReceiver : BroadcastReceiver() { diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderActionReceiver.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderActionReceiver.kt index 5a25f8c..bbe8ea3 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderActionReceiver.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderActionReceiver.kt @@ -23,7 +23,9 @@ import javax.inject.Inject * - **Snooze** cancels the notification and schedules an exact alarm to re-show * it after the user's snooze delay. * - **Show** (the alarm) re-posts the same notification, so the user can snooze - * or dismiss it again. + * or dismiss it again — unless the calendar was switched off during the + * snooze, which [ReminderNotifier.post] catches (this alarm is ours, so no + * provider alert row stands between it and the notification). */ @AndroidEntryPoint class ReminderActionReceiver : BroadcastReceiver() { diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt index 6c62972..4d89253 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt @@ -14,6 +14,8 @@ import androidx.core.content.ContextCompat import dagger.hilt.android.qualifiers.ApplicationContext import de.jeanlucmakiola.calendula.MainActivity import de.jeanlucmakiola.calendula.R +import de.jeanlucmakiola.calendula.data.calendar.CalendarDataSource +import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs import de.jeanlucmakiola.calendula.data.prefs.is24Hour import de.jeanlucmakiola.calendula.data.prefs.resolveFirstDay @@ -37,6 +39,8 @@ import javax.inject.Singleton class ReminderNotifier @Inject constructor( @ApplicationContext private val context: Context, private val settingsPrefs: SettingsPrefs, + private val calendarPrefs: CalendarPrefs, + private val calendarDataSource: CalendarDataSource, ) { /** False when the user declined `POST_NOTIFICATIONS` or muted the app. */ @@ -47,7 +51,21 @@ class ReminderNotifier @Inject constructor( return granted && NotificationManagerCompat.from(context).areNotificationsEnabled() } + /** + * The single choke point for "this calendar is switched off". The provider + * side needs no help — with `VISIBLE = 0` it creates no alert rows at all — + * but two paths reach [post] without one: a snooze we re-show from our own + * exact alarm, scheduled before the calendar was switched off, and a + * read-only install whose switch lives app-side ([CalendarPrefs]) because it + * may not write the flag. Both are covered here rather than in either + * receiver. + */ + private suspend fun isSilenced(calendarId: Long): Boolean = + calendarId in calendarPrefs.pendingDisabledCalendarIds.first() || + calendarDataSource.isCalendarVisible(calendarId) == false + suspend fun post(alert: ReminderAlert) { + if (isSilenced(alert.calendarId)) return ensureChannel() val title = alert.title.ifBlank { context.getString(R.string.event_untitled) } val is24Hour = settingsPrefs.timeFormat.first() diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlan.kt b/app/src/main/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlan.kt index e3e0ca6..4f57088 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlan.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlan.kt @@ -1,45 +1,58 @@ package de.jeanlucmakiola.calendula.domain /** - * The `Calendars.VISIBLE` writes that bring the system flag in line with the - * app's retired disabled set. Empty when the two already agree. + * The `Calendars.VISIBLE` writes that flush the app's pending "switched off" + * set into the provider, plus the ids that need no write at all. */ data class CalendarVisibilityPlan( - val show: Set = emptySet(), val hide: Set = emptySet(), + val settled: Set = emptySet(), ) { - val isEmpty: Boolean get() = show.isEmpty() && hide.isEmpty() + val isEmpty: Boolean get() = hide.isEmpty() && settled.isEmpty() } /** - * Reconcile the two visibility models Calendula used to keep in parallel (#75), - * with the app's state winning: + * Reconcile [pendingDisabledIds] — calendars switched off in Settings → + * Calendars while the app could not write `Calendars.VISIBLE`, plus whatever + * the retired app-local visibility model left behind (#75) — against the + * calendars actually on the device. * - * - enabled in-app but hidden at system level → show it, *unless* the calendar - * doesn't sync its events to the device ([CalendarSource.syncsEvents]). Those - * hold no local events, so switching them on would produce nothing but an - * "enabled yet permanently empty" row; left off, they read honestly as off. - * - disabled in-app → hide it at system level, which is what stops the provider - * scheduling its reminders. - * - anything already in agreement is left untouched, so the migration writes as - * little as possible. + * The plan only ever *hides*. Switching a calendar off is intent the user + * expressed in Calendula, so carrying it into the provider is fair. The other + * direction is deliberately absent: a calendar hidden at system level was hidden + * somewhere else (another calendar app, the account's own settings), and + * switching it back on would un-hide it there too *and* start firing reminders + * nobody asked for. Calendula follows that flag instead and explains itself once + * (see [hasSystemHiddenCalendars]). * - * App-state-wins is also what keeps the upgrade invisible: reconciling the other - * way would make events the user sees today vanish. + * [CalendarVisibilityPlan.settled] carries the ids that need no write — already + * hidden, or gone from the device. They leave the pending set exactly as a + * successful write would. */ fun calendarVisibilityPlan( calendars: List, - disabledCalendarIds: Set, + pendingDisabledIds: Set, ): CalendarVisibilityPlan { - val show = mutableSetOf() + val byId = calendars.associateBy { it.id } val hide = mutableSetOf() - for (calendar in calendars) { - val disabledInApp = calendar.id in disabledCalendarIds - when { - disabledInApp && calendar.isVisibleInSystem -> hide += calendar.id - !disabledInApp && !calendar.isVisibleInSystem && calendar.syncsEvents -> - show += calendar.id - } + val settled = mutableSetOf() + for (id in pendingDisabledIds) { + val calendar = byId[id] + // No row means the calendar is gone; already invisible means someone + // (us, on an earlier run) got there first. Either way: nothing to write. + if (calendar != null && calendar.isVisibleInSystem) hide += id else settled += id } - return CalendarVisibilityPlan(show = show, hide = hide) + return CalendarVisibilityPlan(hide = hide, settled = settled) } + +/** + * Whether any calendar is switched off at system level without Calendula having + * asked for it. Those calendars showed their events before the app adopted + * `Calendars.VISIBLE` as its one visibility model and no longer do, which is + * what the one-time notice explains — the alternative, switching them on, would + * reach into every other calendar app on the device. + */ +fun hasSystemHiddenCalendars( + calendars: List, + pendingDisabledIds: Set, +): Boolean = calendars.any { !it.isVisibleInSystem && it.id !in pendingDisabledIds } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt b/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt index 3d6fd02..311d590 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt @@ -43,10 +43,11 @@ data class CalendarSource( val isManaged: Boolean = false, /** * Whether the provider keeps this calendar's events on the device - * (`Calendars.SYNC_EVENTS`). Independent of [isVisibleInSystem]: a calendar - * with `syncsEvents = false` holds no local events at all, so making it - * visible cannot produce a single one — which is why the one-shot visibility - * migration leaves those calendars switched off. + * (`Calendars.SYNC_EVENTS`). Independent of [isVisibleInSystem]. For a + * synced account it means the events aren't stored locally at all, so the + * calendar reads as permanently empty; a device-local calendar another app + * created can hold events with the flag off, so it says nothing there. + * Read for the "not synced" row label (#76). */ val syncsEvents: Boolean = true, ) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt index 9dadd0e..aee5ac2 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt @@ -21,6 +21,8 @@ import androidx.lifecycle.Lifecycle import androidx.lifecycle.LifecycleEventObserver import de.jeanlucmakiola.calendula.data.contacts.SpecialDatesScheduler import de.jeanlucmakiola.calendula.data.contacts.hasContactsPermission +import de.jeanlucmakiola.calendula.ui.calendars.CalendarVisibilityNoticeDialog +import de.jeanlucmakiola.calendula.ui.calendars.CalendarVisibilityNoticeViewModel import de.jeanlucmakiola.calendula.ui.permission.PermissionScreen import de.jeanlucmakiola.calendula.ui.permission.ReminderOnboardingScreen import de.jeanlucmakiola.calendula.ui.permission.ReminderOnboardingViewModel @@ -78,6 +80,13 @@ fun RootScreen( // frame instead of flashing the wrong screen. val reminderOnboarding: ReminderOnboardingViewModel = hiltViewModel() val onboardingDone by reminderOnboarding.onboardingDone.collectAsStateWithLifecycle() + // One-time explainer for the switch to the device's own calendar + // visibility (#75); armed by the reconciler, shown over the app. + val visibilityNotice: CalendarVisibilityNoticeViewModel = hiltViewModel() + val noticePending by visibilityNotice.pending.collectAsStateWithLifecycle() + if (onboardingDone == true && noticePending) { + CalendarVisibilityNoticeDialog(onDismiss = visibilityNotice::dismiss) + } Crossfade(targetState = onboardingDone, animationSpec = gateSpec, label = "onboardingGate") { done -> when (done) { true -> CalendarHost( diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarVisibilityNotice.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarVisibilityNotice.kt new file mode 100644 index 0000000..faf6313 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarVisibilityNotice.kt @@ -0,0 +1,62 @@ +package de.jeanlucmakiola.calendula.ui.calendars + +import androidx.compose.material.icons.Icons +import androidx.compose.material.icons.filled.VisibilityOff +import androidx.compose.material3.AlertDialog +import androidx.compose.material3.Icon +import androidx.compose.material3.Text +import androidx.compose.material3.TextButton +import androidx.compose.runtime.Composable +import androidx.compose.ui.res.stringResource +import androidx.lifecycle.ViewModel +import androidx.lifecycle.viewModelScope +import dagger.hilt.android.lifecycle.HiltViewModel +import de.jeanlucmakiola.calendula.R +import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs +import kotlinx.coroutines.flow.SharingStarted +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.stateIn +import kotlinx.coroutines.launch +import javax.inject.Inject + +/** + * The one-time notice that Calendula now follows the device's per-calendar + * visibility (#75). Armed by `CalendarVisibilityReconciler` on the first launch + * that finds a calendar switched off outside the app — those used to show their + * events here and no longer do, and the app deliberately does not switch them + * back on, because that would un-hide them in every other calendar app too. + */ +@HiltViewModel +class CalendarVisibilityNoticeViewModel @Inject constructor( + private val prefs: CalendarPrefs, +) : ViewModel() { + + val pending: StateFlow = prefs.visibilityNoticePending + .map { it == true } + .stateIn( + scope = viewModelScope, + started = SharingStarted.WhileSubscribed(5_000L), + initialValue = false, + ) + + fun dismiss() { + viewModelScope.launch { prefs.setVisibilityNoticePending(false) } + } +} + +/** Plain informational dialog — one acknowledgement, nothing to decide. */ +@Composable +fun CalendarVisibilityNoticeDialog(onDismiss: () -> Unit) { + AlertDialog( + onDismissRequest = onDismiss, + icon = { Icon(Icons.Default.VisibilityOff, contentDescription = null) }, + title = { Text(stringResource(R.string.calendars_visibility_notice_title)) }, + text = { Text(stringResource(R.string.calendars_visibility_notice_message)) }, + confirmButton = { + TextButton(onClick = onDismiss) { + Text(stringResource(R.string.dialog_ok)) + } + }, + ) +} diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt index ade9210..9ed5226 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsScreen.kt @@ -278,7 +278,7 @@ private fun CalendarsList( predictiveBack = true, ) { // What the per-calendar / per-account switches below actually do. - HintText(stringResource(R.string.calendars_disable_hint)) + HintText(stringResource(R.string.calendars_visibility_hint)) // Local (device-only) calendars — one collapsible group. The header's // "+" adds a calendar; the switch enables/disables them all at once; @@ -708,7 +708,7 @@ private fun EnableSwitch( enabled: Boolean, onToggle: (Boolean) -> Unit, ) { - val label = stringResource(R.string.calendars_show_in_app_a11y, calendarName) + val label = stringResource(R.string.calendars_visibility_a11y, calendarName) Switch( checked = enabled, onCheckedChange = onToggle, diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt index 6754834..b12a257 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt @@ -3,7 +3,7 @@ package de.jeanlucmakiola.calendula.ui.permission import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel -import de.jeanlucmakiola.calendula.data.calendar.CalendarVisibilityMigration +import de.jeanlucmakiola.calendula.data.calendar.CalendarVisibilityReconciler import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow @@ -12,7 +12,7 @@ import javax.inject.Inject @HiltViewModel class PermissionViewModel @Inject constructor( - private val visibilityMigration: CalendarVisibilityMigration, + private val visibilityReconciler: CalendarVisibilityReconciler, ) : ViewModel() { private val _state = MutableStateFlow(PermissionUiState.Rationale) @@ -20,11 +20,10 @@ class PermissionViewModel @Inject constructor( fun onGranted() { _state.value = PermissionUiState.Granted - // The one-shot visibility migration needs the calendar permissions, so a - // launch that started without them left it pending (#75) — this is the - // moment it can finally run. A no-op once it has, and on a fresh install - // (nothing was ever disabled) it just stamps its guard. - viewModelScope.launch { visibilityMigration.runIfNeeded() } + // The visibility reconcile needs the calendar permissions, so the launch + // that started without them skipped it (#75) — this is the moment it can + // finally run. A no-op on a fresh install, where nothing is pending. + viewModelScope.launch { visibilityReconciler.run() } } fun onDenied() { diff --git a/app/src/main/res/values-de/strings.xml b/app/src/main/res/values-de/strings.xml index 89b876a..d217d97 100644 --- a/app/src/main/res/values-de/strings.xml +++ b/app/src/main/res/values-de/strings.xml @@ -449,8 +449,6 @@ Dadurch werden der Kalender „%1$s“ und seine Ereignisse gelöscht. Alle von dir hinzugefügten Erinnerungen oder Notizen gehen verloren. Deaktivieren Speichern - Deaktiviere einen Kalender, um ihn aus der App zu entfernen – seine Ereignisse, Filter und Auswahlmöglichkeiten. Es wird nichts gelöscht und du kannst ihn hier jederzeit wieder aktivieren. - „%1$s“ in der App anzeigen Weitere Optionen für %1$s Alle aktivieren Alle deaktivieren diff --git a/app/src/main/res/values-es/strings.xml b/app/src/main/res/values-es/strings.xml index 95f07ab..3e41783 100644 --- a/app/src/main/res/values-es/strings.xml +++ b/app/src/main/res/values-es/strings.xml @@ -333,8 +333,6 @@ Tus calendarios Sin calendarios locales todavia. Crea uno para mantener eventos solo en este dispositivo. Añadir calendario - Desactiva un calendario para removerlo de la aplicación — sus eventos, filtros y selectores. Nada se eliminara, y puedes reactivarlo en cualquier momento. - Mostrar \"%1$s\" en la aplicación Calendarios sincronizados Estos provienen de cuentas en tu dispositivo. Crea o editalos en su propia aplicación. Gestionar en aplicación diff --git a/app/src/main/res/values-fr/strings.xml b/app/src/main/res/values-fr/strings.xml index c242be3..d4e1b7c 100644 --- a/app/src/main/res/values-fr/strings.xml +++ b/app/src/main/res/values-fr/strings.xml @@ -400,8 +400,6 @@ Vos calendriers Pas encore de calendrier local. Créez-en un pour conserver les événements uniquement sur cet appareil. Ajouter un calendrier - Désactivez un calendrier pour le retirer de l’application, ses événements, ses filtres et ses sélecteurs. Rien n’est supprimé et vous pouvez le réactiver à tout moment ici. - Afficher « %1$s » dans l’application Calendriers synchronisés Ils proviennent de comptes sur votre appareil. Créez et modifiez-les dans leur propre application. Gérer dans l’application diff --git a/app/src/main/res/values-it/strings.xml b/app/src/main/res/values-it/strings.xml index 884d476..2c36753 100644 --- a/app/src/main/res/values-it/strings.xml +++ b/app/src/main/res/values-it/strings.xml @@ -317,8 +317,6 @@ Calendari locali Non ci sono ancora calendari locali. Creane uno con eventi salvati solo su questo dispositivo. Aggiungi calendario - Disattiva un calendario per non vederlo più sull\'app. Non viene cancellato nulla, puoi riattivarlo in qualsiasi momento. - Mostra \"%1$s\" nell\'app Calendari sincronizzati Questi calendari vengono dagli account sincronizzati sul tuo dispositivo. Puoi modificarli o crearne di nuovi dall\'app di sincronizzazione. Gestisci in app diff --git a/app/src/main/res/values-pl/strings.xml b/app/src/main/res/values-pl/strings.xml index 57320a3..89b8563 100644 --- a/app/src/main/res/values-pl/strings.xml +++ b/app/src/main/res/values-pl/strings.xml @@ -396,8 +396,6 @@ Twoje kalendarze Brak lokalnych kalendarzy. Utwórz kalendarz, aby zapisywać wydarzenia tylko na tym urządzeniu. Dodaj kalendarz - Wyłącz kalendarz, aby ukryć go w aplikacji — wraz z jego wydarzeniami, filtrami i selektorami. Nic nie zostanie usunięte, a w każdej chwili możesz go tutaj ponownie włączyć. - Pokaż „%1$s” w aplikacji Synchronizowane kalendarze Pochodzą z kont na Twoim urządzeniu. Możesz je tworzyć i edytować w powiązanych z nimi aplikacjach. Zarządzaj w aplikacji diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 96b3f4a..a382653 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -473,8 +473,10 @@ Your calendars No local calendars yet. Create one to keep events on this device only. Add calendar - Turn a calendar off to hide it on this device — its events disappear from the app and it stops reminding you. Nothing is deleted, no other device is affected, and you can turn it back on here anytime. - Show \"%1$s\" + Turn a calendar off to hide it on this device — its events disappear from the app and it stops reminding you. This is the same switch your other calendar apps use, so they hide it too. Nothing is deleted, no other device is affected, and you can turn it back on here anytime. + Show \"%1$s\" + Some calendars are switched off + Calendula now shows the calendars that are switched on for this device, so what you see and what reminds you can no longer disagree. Some of yours are currently off — they were switched off here or in another calendar app. Turn any of them back on in Settings → Calendars. Synced calendars These come from accounts on your device. Create and edit them in their own app. Manage in app diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt index 6ad5df5..324f092 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt @@ -12,6 +12,7 @@ import de.jeanlucmakiola.calendula.domain.EventColorOption import de.jeanlucmakiola.calendula.domain.EventForm import de.jeanlucmakiola.calendula.domain.EventInstance import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.flow.first import kotlinx.datetime.LocalDate import kotlinx.datetime.LocalDateTime import kotlinx.datetime.LocalTime @@ -266,6 +267,122 @@ class CalendarRepositoryImplTest { assertThat(fake.visibilityWrites).containsExactly(1L to false, 3L to false).inOrder() } + @Test + fun `without write permission the switch is kept app-side and still filters`( + @TempDir tempDir: Path, + ) = runTest { + // READ granted, WRITE denied: the provider flag can't be written, so the + // choice is parked in the pending set — and honoured from there, or the + // user's switched-off calendars would come back on upgrade (#75). + val prefs = newPrefs(tempDir) + val fake = FakeCalendarDataSource().apply { + canWrite = false + calendarsResult = listOf(makeCal(1L), makeCal(2L)) + instancesResult = { _, _ -> + listOf( + makeEvent(10L, "A", calendarId = 1L), + makeEvent(11L, "B", calendarId = 2L), + ) + } + } + val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + + val range = Instant.fromEpochMilliseconds(0)..Instant.fromEpochMilliseconds(10_000L) + repo.instances(range).test { + assertThat(awaitItem().map { it.title }).containsExactly("A", "B").inOrder() + + repo.setCalendarsVisible(listOf(2L), false) + + assertThat(awaitItem().map { it.title }).containsExactly("A") + assertThat(fake.visibilityWrites).isEmpty() + assertThat(prefs.pendingDisabledCalendarIds.first()).containsExactly(2L) + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `calendars reports a pending switch-off as off`(@TempDir tempDir: Path) = runTest { + // Otherwise the Settings switch would snap straight back on for a + // read-only install, and the pickers would keep offering the calendar. + val prefs = newPrefs(tempDir) + val fake = FakeCalendarDataSource().apply { + canWrite = false + calendarsResult = listOf(makeCal(1L), makeCal(2L)) + } + val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + + repo.calendars().test { + assertThat(awaitItem().map { it.isVisibleInSystem }).containsExactly(true, true) + + repo.setCalendarsVisible(listOf(2L), false) + + assertThat(awaitItem().single { it.id == 2L }.isVisibleInSystem).isFalse() + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `switching a calendar back on without write permission retires its entry`( + @TempDir tempDir: Path, + ) = runTest { + val prefs = newPrefs(tempDir) + prefs.addPendingDisabledCalendarIds(setOf(2L)) + val fake = FakeCalendarDataSource().apply { + canWrite = false + calendarsResult = listOf(makeCal(1L), makeCal(2L)) + } + val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), Dispatchers.Unconfined) + + repo.setCalendarsVisible(listOf(2L), true) + + assertThat(prefs.pendingDisabledCalendarIds.first()).isEmpty() + } + + @Test + fun `a provider write clears anything still pending for that calendar`( + @TempDir tempDir: Path, + ) = runTest { + val prefs = newPrefs(tempDir) + prefs.addPendingDisabledCalendarIds(setOf(2L)) + val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(makeCal(1L), makeCal(2L)) + } + val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), Dispatchers.Unconfined) + + repo.setCalendarsVisible(listOf(2L), false) + + assertThat(fake.visibilityWrites).containsExactly(2L to false) + assertThat(prefs.pendingDisabledCalendarIds.first()).isEmpty() + } + + @Test + fun `one tick costs one calendar query however many collectors there are`( + @TempDir tempDir: Path, + ) = runTest { + val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(makeCal(1L), makeCal(2L)) + instancesResult = { _, _ -> listOf(makeEvent(10L, "A", calendarId = 1L)) } + } + val repo = CalendarRepositoryImpl(fake, newPrefs(tempDir), newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + + val range = Instant.fromEpochMilliseconds(0)..Instant.fromEpochMilliseconds(10_000L) + repo.calendars().test { + awaitItem() + repo.instances(range).test { + awaitItem() + // Both flows listed/filtered off the same snapshot. + assertThat(fake.calendarQueries).isEqualTo(1) + cancelAndIgnoreRemainingEvents() + } + + // The next tick invalidates it — one fresh read, not one per flow. + fake.tick() + awaitItem() + assertThat(fake.calendarQueries).isEqualTo(2) + cancelAndIgnoreRemainingEvents() + } + } + @Test fun `searchEvents drops results from calendars that are off or hidden`( @TempDir tempDir: Path, diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt index ca2aa79..ccc9b68 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt @@ -61,7 +61,14 @@ internal class FakeCalendarDataSource : CalendarDataSource { private val listeners = mutableListOf<() -> Unit>() - override fun calendars(): List = calendarsResult + /** How often [calendars] was queried — the repository shares one read per tick. */ + var calendarQueries = 0 + private set + + override fun calendars(): List { + calendarQueries++ + return calendarsResult + } override fun instances(beginMillis: Long, endMillis: Long): List = instancesResult(beginMillis, endMillis) override fun searchEvents(query: String): List = searchResult(query) @@ -108,6 +115,14 @@ internal class FakeCalendarDataSource : CalendarDataSource { } } + /** Whether the fake holds `WRITE_CALENDAR`; false models a read-only grant. */ + var canWrite: Boolean = true + + override fun canWriteCalendars(): Boolean = canWrite + + override fun isCalendarVisible(id: Long): Boolean? = + calendarsResult.firstOrNull { it.id == id }?.isVisibleInSystem + override fun deleteCalendar(id: Long) { writeError?.let { throw it } deletedCalendarIds += id diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefsTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefsTest.kt index 930af94..7c1a9e9 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefsTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefsTest.kt @@ -52,51 +52,65 @@ class CalendarPrefsTest { } @Test - fun `legacy disabled set reads back what an older version stored`( + fun `the pending disabled set reads back what an older version stored`( @TempDir tempDir: Path, ) = runTest { + // Same key as the retired app-local "disabled calendars" model: an + // upgrade inherits that set as switch-offs still owed to the provider. val store = newDataStore(tempDir) val prefs = CalendarPrefs(store) store.updateData { p -> p.toMutablePreferences().apply { this[CalendarPrefs.DISABLED_IDS_KEY] = "2,9" } } - assertThat(prefs.legacyDisabledCalendarIds()).isEqualTo(setOf(2L, 9L)) + assertThat(prefs.pendingDisabledCalendarIds.first()).isEqualTo(setOf(2L, 9L)) } @Test - fun `legacy disabled set is empty when nothing was ever stored`( + fun `the pending disabled set is empty when nothing was ever stored`( @TempDir tempDir: Path, ) = runTest { val prefs = CalendarPrefs(newDataStore(tempDir)) - assertThat(prefs.legacyDisabledCalendarIds()).isEmpty() + assertThat(prefs.pendingDisabledCalendarIds.first()).isEmpty() } @Test - fun `clearing the legacy disabled set leaves the hidden set alone`( + fun `pending ids are added and dropped one at a time`(@TempDir tempDir: Path) = runTest { + val prefs = CalendarPrefs(newDataStore(tempDir)) + prefs.addPendingDisabledCalendarIds(listOf(2L, 9L)) + prefs.addPendingDisabledCalendarIds(listOf(4L)) + + prefs.removePendingDisabledCalendarIds(setOf(9L)) + + assertThat(prefs.pendingDisabledCalendarIds.first()).isEqualTo(setOf(2L, 4L)) + } + + @Test + fun `draining the pending set leaves the hidden set alone`( @TempDir tempDir: Path, ) = runTest { - val store = newDataStore(tempDir) - val prefs = CalendarPrefs(store) + val prefs = CalendarPrefs(newDataStore(tempDir)) prefs.setHiddenCalendarIds(setOf(1L)) - store.updateData { p -> - p.toMutablePreferences().apply { this[CalendarPrefs.DISABLED_IDS_KEY] = "2" } - } + prefs.addPendingDisabledCalendarIds(setOf(2L)) - prefs.clearLegacyDisabledCalendarIds() + prefs.removePendingDisabledCalendarIds(setOf(2L)) - assertThat(prefs.legacyDisabledCalendarIds()).isEmpty() + assertThat(prefs.pendingDisabledCalendarIds.first()).isEmpty() assertThat(prefs.hiddenCalendarIds.first()).isEqualTo(setOf(1L)) } @Test - fun `visibility migration guard defaults to not-done and latches`( + fun `the visibility notice is unevaluated until it is written`( @TempDir tempDir: Path, ) = runTest { + // Null is what makes the evaluation one-shot: "no" is stored just as + // firmly as "yes", so the notice can't resurface on a later launch. val prefs = CalendarPrefs(newDataStore(tempDir)) - assertThat(prefs.visibilityMigrationDone.first()).isFalse() + assertThat(prefs.visibilityNoticePending.first()).isNull() - prefs.setVisibilityMigrationDone() + prefs.setVisibilityNoticePending(true) + assertThat(prefs.visibilityNoticePending.first()).isTrue() - assertThat(prefs.visibilityMigrationDone.first()).isTrue() + prefs.setVisibilityNoticePending(false) + assertThat(prefs.visibilityNoticePending.first()).isFalse() } } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlanTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlanTest.kt index c6f9a54..8f01442 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlanTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/domain/CalendarVisibilityPlanTest.kt @@ -4,53 +4,45 @@ import com.google.common.truth.Truth.assertThat import org.junit.jupiter.api.Test /** - * The one-shot reconciliation of the retired app-local "disabled calendars" set - * into the system's `Calendars.VISIBLE` (#75), across every - * (sync_events, visible) combination. + * Draining the app's pending "switched off" set into the system's + * `Calendars.VISIBLE` (#75) — including the set an upgrade inherits from the + * retired app-local visibility model. */ class CalendarVisibilityPlanTest { private fun cal( id: Long, visible: Boolean = true, + local: Boolean = false, syncsEvents: Boolean = true, ): CalendarSource = CalendarSource( id = id, displayName = "Cal $id", accountName = "acc@local", - accountType = "LOCAL", + accountType = if (local) "LOCAL" else "com.google", color = 0, isVisibleInSystem = visible, + isLocal = local, syncsEvents = syncsEvents, ) @Test - fun `a calendar disabled in-app is switched off at system level`() { + fun `a pending calendar is switched off at system level`() { val plan = calendarVisibilityPlan(listOf(cal(1L, visible = true)), setOf(1L)) assertThat(plan.hide).containsExactly(1L) - assertThat(plan.show).isEmpty() + assertThat(plan.settled).isEmpty() } @Test - fun `a calendar enabled in-app but hidden at system level is switched on`() { + fun `a calendar hidden at system level is never switched on`() { + // The plan only hides: switching it on would un-hide the calendar in + // every other calendar app and start firing its reminders. val plan = calendarVisibilityPlan(listOf(cal(1L, visible = false)), emptySet()) - assertThat(plan.show).containsExactly(1L) - assertThat(plan.hide).isEmpty() - } - - @Test - fun `a not-synced calendar is never switched on`() { - // sync_events = 0 means the provider holds none of its events locally, - // so making it visible could only produce an enabled-yet-empty row. - val plan = calendarVisibilityPlan( - listOf(cal(1L, visible = false, syncsEvents = false)), - emptySet(), - ) assertThat(plan.isEmpty).isTrue() } @Test - fun `a not-synced calendar disabled in-app is still switched off`() { + fun `a not-synced calendar is switched off like any other`() { val plan = calendarVisibilityPlan( listOf(cal(1L, visible = true, syncsEvents = false)), setOf(1L), @@ -59,38 +51,74 @@ class CalendarVisibilityPlanTest { } @Test - fun `calendars that already agree are left untouched`() { - val plan = calendarVisibilityPlan( - listOf(cal(1L, visible = true), cal(2L, visible = false)), - disabledCalendarIds = setOf(2L), - ) - assertThat(plan.isEmpty).isTrue() + fun `a pending calendar already switched off is settled without a write`() { + val plan = calendarVisibilityPlan(listOf(cal(1L, visible = false)), setOf(1L)) + assertThat(plan.hide).isEmpty() + assertThat(plan.settled).containsExactly(1L) } @Test - fun `a disabled id for a calendar that no longer exists writes nothing`() { + fun `a pending id for a calendar that no longer exists is settled`() { val plan = calendarVisibilityPlan(listOf(cal(1L)), setOf(99L)) + assertThat(plan.hide).isEmpty() + assertThat(plan.settled).containsExactly(99L) + } + + @Test + fun `an empty pending set writes nothing`() { + val plan = calendarVisibilityPlan(listOf(cal(1L), cal(2L, visible = false)), emptySet()) assertThat(plan.isEmpty).isTrue() } @Test - fun `a mixed device is reconciled in both directions at once`() { + fun `a mixed device splits into writes and settled ids`() { val plan = calendarVisibilityPlan( listOf( - cal(1L, visible = true), // enabled, already visible → untouched - cal(2L, visible = false), // enabled but hidden → show - cal(3L, visible = true), // disabled in-app → hide - cal(4L, visible = false, syncsEvents = false), // not synced → untouched + cal(1L, visible = true), // not pending → untouched + cal(2L, visible = false), // hidden elsewhere → untouched + cal(3L, visible = true), // pending → hide + cal(4L, visible = false), // pending, already off → settled ), - disabledCalendarIds = setOf(3L), + pendingDisabledIds = setOf(3L, 4L, 77L), ) - assertThat(plan.show).containsExactly(2L) assertThat(plan.hide).containsExactly(3L) + assertThat(plan.settled).containsExactly(4L, 77L) } @Test - fun `nothing disabled and everything visible is a no-op`() { - val plan = calendarVisibilityPlan(listOf(cal(1L), cal(2L)), emptySet()) - assertThat(plan.isEmpty).isTrue() + fun `a calendar hidden outside the app arms the notice`() { + assertThat( + hasSystemHiddenCalendars(listOf(cal(1L), cal(2L, visible = false)), emptySet()), + ).isTrue() + } + + @Test + fun `a calendar we are about to hide ourselves does not arm the notice`() { + // It is off because the user switched it off here — nothing to explain. + assertThat( + hasSystemHiddenCalendars(listOf(cal(1L, visible = false)), setOf(1L)), + ).isFalse() + } + + @Test + fun `an all-visible device does not arm the notice`() { + assertThat(hasSystemHiddenCalendars(listOf(cal(1L), cal(2L)), emptySet())).isFalse() + } + + @Test + fun `a device-local calendar is treated like any other`() { + // Its sync_events flag says nothing about whether it holds events, so + // neither the plan nor the notice may reason about it. + val plan = calendarVisibilityPlan( + listOf(cal(1L, visible = true, local = true, syncsEvents = false)), + setOf(1L), + ) + assertThat(plan.hide).containsExactly(1L) + assertThat( + hasSystemHiddenCalendars( + listOf(cal(2L, visible = false, local = true, syncsEvents = false)), + emptySet(), + ), + ).isTrue() } } diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 302dac8..50698ae 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -162,10 +162,17 @@ notifications never return because `FIRED` rows are never re-queried. `Calendars.VISIBLE = 1`, so that flag *is* the app's on/off switch: Settings → Calendars writes it (one calendar per update — `CalendarProvider2` skips its own `checkNextAlarm()` reschedule for any selection that isn't `_id=`), and every -display predicate reads `CalendarSource.isVisibleInSystem`. There is deliberately -no second, app-local model and therefore nothing to suppress here. The drawer's -filter sheet (`CalendarPrefs.hiddenCalendarIds`) is a separate in-app declutter -that never touches reminders. See `docs/design/calendar-visibility-model.md`. +display predicate reads `CalendarSource.isVisibleInSystem`. The reconciliation +runs one way only: a calendar the user switched off in Calendula is switched off +in the provider, never the reverse — un-hiding one would reach into every other +calendar app on the device — and a one-time notice explains the calendars that +were already off. `CalendarPrefs.pendingDisabledCalendarIds` holds the switch-offs +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 +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. From edbeadfa3084b24832228f398a74b9c5d2b77b86 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 25 Jul 2026 21:33:58 +0200 Subject: [PATCH 3/7] fix(reminders): stop marking a silenced reminder handled and losing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "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) --- .../calendula/data/reminders/AlertHandling.kt | 30 +++++++++ .../data/reminders/EventReminderReceiver.kt | 10 ++- .../data/reminders/ReminderNotifier.kt | 11 +++- .../data/reminders/ReminderRecovery.kt | 46 +++++++++++++ .../ui/calendars/CalendarsViewModel.kt | 8 +++ .../data/reminders/AlertHandlingTest.kt | 66 +++++++++++++++++++ 6 files changed, 166 insertions(+), 5 deletions(-) create mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandling.kt create mode 100644 app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderRecovery.kt create mode 100644 app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandlingTest.kt diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandling.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandling.kt new file mode 100644 index 0000000..9d6cf57 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandling.kt @@ -0,0 +1,30 @@ +package de.jeanlucmakiola.calendula.data.reminders + +/** + * Still relevant while the event has not ended: a reminder for an event that is + * already over is pointless to re-surface. Falls back to the begin time when the + * end is unknown (0L). + */ +internal fun ReminderAlert.isRelevantAt(nowMillis: Long): Boolean = + (endMillis.takeIf { it > 0L } ?: beginMillis) >= nowMillis + +/** + * The alerts [EventReminderReceiver] may mark handled (`STATE_FIRED`): the ones + * it posted, plus the ones it silenced whose event is already over. + * + * A silenced alert for an event still ahead is deliberately left + * `STATE_SCHEDULED`. Silencing is not handling — the calendar is switched off in + * Calendula while the provider still holds `VISIBLE = 1` (a read-only install, + * or an upgrade whose flush hasn't landed), so switching it back on before the + * event must still be able to surface the reminder. [ReminderAlertStore.dueAlerts] + * only ever returns scheduled rows, so marking them here would lose them for + * good; leaving them makes the provider's own table the stash + * ([ReminderRecovery]). + */ +internal fun handledAlertIds( + due: List, + postedIds: Set, + nowMillis: Long, +): List = due + .filter { it.alertId in postedIds || !it.isRelevantAt(nowMillis) } + .map { it.alertId } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt index e61dbfb..95067e3 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/EventReminderReceiver.kt @@ -30,7 +30,9 @@ import javax.inject.Inject * Settings → Calendars has `Calendars.VISIBLE = 0`, and the provider creates no * alert rows for it in the first place (#75). The one case that flag can't * cover — a read-only install, which keeps its switches app-side — is gated in - * [ReminderNotifier.post], where the snoozed re-show passes too. + * [ReminderNotifier.post], where the snoozed re-show passes too. What that gate + * silences is *not* marked fired while the event is still ahead, so switching + * the calendar back on can still surface it (see [handledAlertIds]). */ @AndroidEntryPoint class EventReminderReceiver : BroadcastReceiver() { @@ -52,8 +54,10 @@ class EventReminderReceiver : BroadcastReceiver() { if (settingsPrefs.remindersEnabled.first()) { val now = System.currentTimeMillis() val due = alertStore.dueAlerts(now) - due.forEach { notifier.post(it) } - alertStore.markFired(due.map { it.alertId }, now) + val postedIds = due + .filter { notifier.post(it) } + .mapTo(mutableSetOf()) { it.alertId } + alertStore.markFired(handledAlertIds(due, postedIds, now), now) } } finally { pendingResult.finish() diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt index 4d89253..0b16714 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderNotifier.kt @@ -64,8 +64,13 @@ class ReminderNotifier @Inject constructor( calendarId in calendarPrefs.pendingDisabledCalendarIds.first() || calendarDataSource.isCalendarVisible(calendarId) == false - suspend fun post(alert: ReminderAlert) { - if (isSilenced(alert.calendarId)) return + /** + * Post [alert], unless its calendar is switched off. Returns whether the + * notification was put up: a silenced alert must stay unhandled so that + * switching the calendar back on can still surface it (see [handledAlertIds]). + */ + suspend fun post(alert: ReminderAlert): Boolean { + if (isSilenced(alert.calendarId)) return false ensureChannel() val title = alert.title.ifBlank { context.getString(R.string.event_untitled) } val is24Hour = settingsPrefs.timeFormat.first() @@ -117,6 +122,8 @@ class ReminderNotifier @Inject constructor( // POST_NOTIFICATIONS was revoked between canPost() and here. Log.w(TAG, "Could not post reminder for event ${alert.eventId}", e) } + // Handled either way: re-running it would hit the same revoked permission. + return true } /** Remove a posted reminder (snooze re-shows it later; dismiss is final). */ diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderRecovery.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderRecovery.kt new file mode 100644 index 0000000..93bf846 --- /dev/null +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/reminders/ReminderRecovery.kt @@ -0,0 +1,46 @@ +package de.jeanlucmakiola.calendula.data.reminders + +import de.jeanlucmakiola.calendula.data.di.IoDispatcher +import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.withContext +import javax.inject.Inject +import javax.inject.Singleton + +/** + * Re-posts the reminders a switched-off calendar silenced, when it is switched + * back on while they still matter. + * + * Only the app-side switch needs this — a read-only install, or an upgrade the + * reconciler hasn't flushed yet. With `Calendars.VISIBLE = 0` the provider + * deletes the calendar's alert rows itself and re-creates them on the way back; + * app-side the rows stay, still `STATE_SCHEDULED`, because + * [EventReminderReceiver] deliberately leaves the ones it silenced unhandled + * (see [handledAlertIds]). So the provider's own table is the stash, and nothing + * is mirrored locally. + * + * Best effort at switch-on time: it mirrors the receiver's gates (reminders on, + * notifications postable) and there is no later re-scan, so an alert left + * unposted because those are closed is simply released. + */ +@Singleton +class ReminderRecovery @Inject constructor( + private val alertStore: ReminderAlertStore, + private val notifier: ReminderNotifier, + private val settingsPrefs: SettingsPrefs, + @IoDispatcher private val io: CoroutineDispatcher, +) { + + suspend fun rePostFor(calendarIds: Collection) = withContext(io) { + if (calendarIds.isEmpty()) return@withContext + if (!settingsPrefs.remindersEnabled.first() || !notifier.canPost()) return@withContext + val now = System.currentTimeMillis() + val ids = calendarIds.toSet() + val recovered = alertStore.dueAlerts(now) + .filter { it.calendarId in ids && it.isRelevantAt(now) } + if (recovered.isEmpty()) return@withContext + val postedIds = recovered.filter { notifier.post(it) }.map { it.alertId } + alertStore.markFired(postedIds, now) + } +} diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt index 78423c0..1635054 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarsViewModel.kt @@ -13,6 +13,7 @@ import de.jeanlucmakiola.calendula.data.di.IoDispatcher import de.jeanlucmakiola.calendula.data.ics.IcsExporter import de.jeanlucmakiola.calendula.data.prefs.BackupStatus import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs +import de.jeanlucmakiola.calendula.data.reminders.ReminderRecovery import de.jeanlucmakiola.calendula.domain.CalendarSource import de.jeanlucmakiola.calendula.domain.ics.IcsWriter import kotlinx.coroutines.CoroutineDispatcher @@ -43,6 +44,7 @@ class CalendarsViewModel @Inject constructor( private val repository: CalendarRepository, private val icsExporter: IcsExporter, private val settingsPrefs: SettingsPrefs, + private val reminderRecovery: ReminderRecovery, @IoDispatcher private val io: CoroutineDispatcher, ) : ViewModel() { @@ -125,9 +127,14 @@ class CalendarsViewModel @Inject constructor( * every surface *and* the provider stops (or resumes) scheduling its * reminders. Nothing is patched by hand — the provider notifies and the * observer re-queries. + * + * Switching one back on also re-posts the reminders it silenced while it was + * off and that are still relevant — those the app kept app-side because it + * may not write the flag ([ReminderRecovery]). */ fun setCalendarVisible(id: Long, visible: Boolean) = write { repository.setCalendarsVisible(listOf(id), visible) + if (visible) reminderRecovery.rePostFor(listOf(id)) } /** @@ -138,6 +145,7 @@ class CalendarsViewModel @Inject constructor( */ fun setAccountVisible(ids: Collection, visible: Boolean) = write { repository.setCalendarsVisible(ids, visible) + if (visible) reminderRecovery.rePostFor(ids) } // --- Automatic backup (issue #8) ------------------------------------ diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandlingTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandlingTest.kt new file mode 100644 index 0000000..9b377e2 --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/reminders/AlertHandlingTest.kt @@ -0,0 +1,66 @@ +package de.jeanlucmakiola.calendula.data.reminders + +import com.google.common.truth.Truth.assertThat +import org.junit.jupiter.api.Test + +/** + * What the reminder receiver may mark handled. The silenced-but-still-ahead case + * is the one that matters: those rows are the only copy of the reminder (#75). + */ +class AlertHandlingTest { + + private val now = 1_700_000_000_000L + + private fun alert( + id: Long, + calendarId: Long = 1L, + beginMillis: Long = now + 60_000L, + endMillis: Long = now + 3_600_000L, + ) = ReminderAlert( + alertId = id, + eventId = id * 10, + calendarId = calendarId, + beginMillis = beginMillis, + endMillis = endMillis, + title = "E $id", + location = null, + isAllDay = false, + ) + + @Test + fun `posted alerts are handled`() { + val due = listOf(alert(1L), alert(2L)) + + assertThat(handledAlertIds(due, postedIds = setOf(1L, 2L), nowMillis = now)) + .containsExactly(1L, 2L) + } + + @Test + fun `a silenced alert whose event is still ahead stays unhandled`() { + // Switching its calendar back on before the event has to bring it back, + // and dueAlerts only ever returns STATE_SCHEDULED rows. + val due = listOf(alert(1L), alert(2L)) + + assertThat(handledAlertIds(due, postedIds = setOf(1L), nowMillis = now)) + .containsExactly(1L) + } + + @Test + fun `a silenced alert whose event is over is handled`() { + // Nothing left to re-surface, so it must not linger as scheduled. + val over = alert(2L, beginMillis = now - 7_200_000L, endMillis = now - 3_600_000L) + val due = listOf(alert(1L), over) + + assertThat(handledAlertIds(due, postedIds = setOf(1L), nowMillis = now)) + .containsExactly(1L, 2L) + } + + @Test + fun `an unknown end time falls back to the begin time`() { + val started = alert(1L, beginMillis = now - 1L, endMillis = 0L) + val notYet = alert(2L, beginMillis = now + 1L, endMillis = 0L) + + assertThat(handledAlertIds(listOf(started, notYet), postedIds = emptySet(), nowMillis = now)) + .containsExactly(1L) + } +} From bb6e3ad336f7d168d7cd53f4afcc120381728c6e Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 25 Jul 2026 21:34:24 +0200 Subject: [PATCH 4/7] fix(calendars): only tell upgrades about the visibility change, and reconcile on every grant MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../jeanlucmakiola/calendula/CalendulaApp.kt | 6 +-- .../calendar/CalendarVisibilityReconciler.kt | 53 ++++++++++++++----- .../jeanlucmakiola/calendula/ui/RootScreen.kt | 5 ++ .../ui/calendars/CalendarVisibilityNotice.kt | 14 +++++ .../ui/permission/PermissionViewModel.kt | 13 ++--- 5 files changed, 65 insertions(+), 26 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt b/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt index d87f65d..ba988f6 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/CalendulaApp.kt @@ -47,9 +47,9 @@ class CalendulaApp : Application() { * Flush any calendar switch-off the app hasn't been allowed to write into * the system's `Calendars.VISIBLE` yet — including the retired app-local * "disabled calendars" set the upgrade inherits (#75). A no-op on a fresh - * install and in the steady state; a launch without the calendar permissions - * leaves the set pending, and granting them on the permission screen runs it - * there instead. + * install and in the steady state; a launch without the calendar permission + * leaves the set pending, and `RootScreen` runs it again once the app comes + * up holding it — whichever way it was granted. */ private fun reconcileCalendarVisibility() { val deps = EntryPointAccessors.fromApplication( diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityReconciler.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityReconciler.kt index 0784c7b..bca56dd 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityReconciler.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarVisibilityReconciler.kt @@ -11,7 +11,6 @@ import dagger.hilt.android.qualifiers.ApplicationContext import dagger.hilt.components.SingletonComponent import de.jeanlucmakiola.calendula.data.di.IoDispatcher import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs -import de.jeanlucmakiola.calendula.domain.CalendarSource import de.jeanlucmakiola.calendula.domain.calendarVisibilityPlan import de.jeanlucmakiola.calendula.domain.hasSystemHiddenCalendars import kotlinx.coroutines.CoroutineDispatcher @@ -28,15 +27,19 @@ import javax.inject.Singleton * model (#75), and the standing drain for switch-offs made without * `WRITE_CALENDAR`. * - * Runs on every launch, and again the moment the permission screen grants the - * calendar permissions. It is a no-op whenever the pending set is empty, which + * Runs on every launch, and again whenever the app comes up holding the calendar + * permission — a grant made on Android's own app-settings screen never reaches + * the permission screen's callback. It is a no-op whenever the pending set is + * empty and the notice has been settled, which * is the steady state: each entry is written and dropped individually, so a run * that dies part-way resumes exactly where it stopped and never re-applies a * write the user has since undone by hand. * * The reconciliation only hides (see [calendarVisibilityPlan]). Calendars hidden - * at system level stay hidden, and the first run that sees one arms the one-time - * notice explaining why Calendula no longer lists their events. + * at system level stay hidden, and on an *upgraded* install the first run that + * sees one arms the one-time notice explaining why Calendula no longer lists + * their events. A fresh install never had the old behaviour, so it retires that + * notice unshown — every other device ships with something hidden. */ @Singleton class CalendarVisibilityReconciler @Inject constructor( @@ -52,10 +55,19 @@ class CalendarVisibilityReconciler @Inject constructor( // so an IOException from a damaged preferences file would otherwise take // the process down on every launch. try { + // A fresh install has no retired model behind it — nothing to + // migrate, and nothing to explain. Settled ahead of the permission + // gate so an update installed before the first grant can't make a + // first run look like an upgrade afterwards. + if (!isUpgradeInstall()) settleNoticeOnce(pending = false) if (!hasPermission(Manifest.permission.READ_CALENDAR)) return@withContext val pending = prefs.pendingDisabledCalendarIds.first() + val noticeSettled = prefs.visibilityNoticePending.first() != null + // The steady state, and every run after the first: nothing left to + // drain and nothing left to decide, so don't pay for the query. + if (pending.isEmpty() && noticeSettled) return@withContext val calendars = dataSource.calendars() - armNoticeOnce(calendars, pending) + settleNoticeOnce(hasSystemHiddenCalendars(calendars, pending)) if (pending.isEmpty() || !hasPermission(Manifest.permission.WRITE_CALENDAR)) { return@withContext } @@ -79,15 +91,30 @@ class CalendarVisibilityReconciler @Inject constructor( } /** - * Arm the one-time notice if the device holds a calendar switched off - * outside Calendula. Evaluated once, on the first run that can read the - * calendars at all; the answer — including "nothing to say" — is stored, so - * the notice can't resurface later, when the same state would no longer be - * news to the user. + * Settle the one-time notice: [pending] arms it, false retires it unshown. + * Answered once, by whichever run can answer it first; the answer is stored + * either way, so the notice can't resurface later, when the same state would + * no longer be news to the user. */ - private suspend fun armNoticeOnce(calendars: List, pending: Set) { + private suspend fun settleNoticeOnce(pending: Boolean) { if (prefs.visibilityNoticePending.first() != null) return - prefs.setVisibilityNoticePending(hasSystemHiddenCalendars(calendars, pending)) + prefs.setVisibilityNoticePending(pending) + } + + /** + * Whether this install has ever run an earlier version. The notice explains + * a change to behaviour the user has already seen, so a first install has + * nothing to announce — and hidden calendars are the *norm* on a fresh + * device (a second account's, "Holidays in …", a subscribed calendar), which + * would otherwise put a changelog dialog in front of a first-run user. + */ + private fun isUpgradeInstall(): Boolean = try { + @Suppress("DEPRECATION") + val info = context.packageManager.getPackageInfo(context.packageName, 0) + info.lastUpdateTime > info.firstInstallTime + } catch (e: PackageManager.NameNotFoundException) { + Log.w(TAG, "Own package info unavailable; treating as a fresh install", e) + false } private fun hasPermission(permission: String): Boolean = diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt index aee5ac2..33385ff 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/RootScreen.kt @@ -7,6 +7,7 @@ import androidx.compose.material3.ExperimentalMaterial3ExpressiveApi import androidx.compose.material3.MaterialTheme import androidx.compose.runtime.Composable import androidx.compose.runtime.DisposableEffect +import androidx.compose.runtime.LaunchedEffect import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import androidx.compose.runtime.setValue @@ -84,6 +85,10 @@ fun RootScreen( // visibility (#75); armed by the reconciler, shown over the app. val visibilityNotice: CalendarVisibilityNoticeViewModel = hiltViewModel() val noticePending by visibilityNotice.pending.collectAsStateWithLifecycle() + // Runs on entry however the permission was granted — including from + // Android's app-settings screen, which only comes back through the + // ON_RESUME check above. Cheap once there is nothing left to do. + LaunchedEffect(Unit) { visibilityNotice.reconcile() } if (onboardingDone == true && noticePending) { CalendarVisibilityNoticeDialog(onDismiss = visibilityNotice::dismiss) } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarVisibilityNotice.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarVisibilityNotice.kt index faf6313..98c3356 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarVisibilityNotice.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/calendars/CalendarVisibilityNotice.kt @@ -12,6 +12,7 @@ import androidx.lifecycle.ViewModel import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel import de.jeanlucmakiola.calendula.R +import de.jeanlucmakiola.calendula.data.calendar.CalendarVisibilityReconciler import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs import kotlinx.coroutines.flow.SharingStarted import kotlinx.coroutines.flow.StateFlow @@ -30,8 +31,21 @@ import javax.inject.Inject @HiltViewModel class CalendarVisibilityNoticeViewModel @Inject constructor( private val prefs: CalendarPrefs, + private val reconciler: CalendarVisibilityReconciler, ) : ViewModel() { + /** + * Reconcile whenever the app comes up with the calendar permission held. + * The launch itself is covered by `CalendulaApp`, but a permission granted + * on Android's app-settings screen comes back through `RootScreen`'s + * ON_RESUME and never touches the permission screen's callback — so the + * trigger hangs off "we are showing the app", not off one grant route. + * Settled runs cost two DataStore reads and stop there. + */ + fun reconcile() { + viewModelScope.launch { reconciler.run() } + } + val pending: StateFlow = prefs.visibilityNoticePending .map { it == true } .stateIn( diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt index b12a257..6cc03eb 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/permission/PermissionViewModel.kt @@ -1,29 +1,22 @@ package de.jeanlucmakiola.calendula.ui.permission import androidx.lifecycle.ViewModel -import androidx.lifecycle.viewModelScope import dagger.hilt.android.lifecycle.HiltViewModel -import de.jeanlucmakiola.calendula.data.calendar.CalendarVisibilityReconciler import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.asStateFlow -import kotlinx.coroutines.launch import javax.inject.Inject @HiltViewModel -class PermissionViewModel @Inject constructor( - private val visibilityReconciler: CalendarVisibilityReconciler, -) : ViewModel() { +class PermissionViewModel @Inject constructor() : ViewModel() { private val _state = MutableStateFlow(PermissionUiState.Rationale) val state: StateFlow = _state.asStateFlow() + // The visibility reconcile a grant owes (#75) hangs off RootScreen showing + // the app instead: it has to cover the grants made outside it too. fun onGranted() { _state.value = PermissionUiState.Granted - // The visibility reconcile needs the calendar permissions, so the launch - // that started without them skipped it (#75) — this is the moment it can - // finally run. A no-op on a fresh install, where nothing is pending. - viewModelScope.launch { visibilityReconciler.run() } } fun onDenied() { From 4ad805e747a990ca1d8bf6397f498e45cee08db0 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 25 Jul 2026 21:34:24 +0200 Subject: [PATCH 5/7] fix(calendars): don't flash a flushed calendar's events back on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../data/calendar/CalendarRepositoryImpl.kt | 74 +++++++++++++------ .../calendula/data/prefs/CalendarPrefs.kt | 17 +++-- .../calendar/CalendarRepositoryImplTest.kt | 60 +++++++++++++++ 3 files changed, 122 insertions(+), 29 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt index 0b4dd9d..19dcb5a 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImpl.kt @@ -16,9 +16,12 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableSharedFlow import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.distinctUntilChanged +import kotlinx.coroutines.flow.drop import kotlinx.coroutines.flow.first import kotlinx.coroutines.flow.flow import kotlinx.coroutines.flow.flowOn +import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.merge import kotlinx.coroutines.flow.onStart import kotlinx.coroutines.sync.Mutex import kotlinx.coroutines.sync.withLock @@ -63,21 +66,38 @@ class CalendarRepositoryImpl @Inject constructor( } } + /** + * Re-query signal for everything filtered by visibility: the provider's own + * notifications, plus every change to the pending switch-off set (an id + * leaves it as its `VISIBLE` write lands, which changes what is shown). + * [calendarsSnapshot] keeps the two in step. + */ + private fun visibilityTicks(): Flow = merge( + ticks.onStart { emit(Unit) }, + // The current value is already covered by the tick above; only later + // changes re-query (the set is deduped, so an unrelated DataStore write + // doesn't). + prefs.pendingDisabledCalendarIds.drop(1).map {}, + ) + // A switch-off the app hasn't been allowed to write yet is folded into the // flag itself, so every consumer — the Settings switch, the filter sheet, // the form and import pickers, the widgets — reads one visibility and can't // disagree with what the user just tapped. The reconciler reads the data // source directly, because it needs the provider's own answer. override fun calendars(): Flow> = - combine( - ticks.onStart { emit(Unit) }.reQuery { calendarsSnapshot() }, - prefs.pendingDisabledCalendarIds, - ) { calendars, pendingDisabled -> + visibilityTicks().reQuery { + val calendars = calendarsSnapshot() + val pendingDisabled = prefs.pendingDisabledCalendarIds.first() if (pendingDisabled.isEmpty()) calendars else calendars.map { if (it.id in pendingDisabled) it.copy(isVisibleInSystem = false) else it } - }.flowOn(io) + } + // Collapse re-emissions that carry an identical list (see + // [instances]). + .distinctUntilChanged() + .flowOn(io) // Instances are filtered by the system's per-calendar VISIBLE flag ∪ the // switch-offs still waiting to be written to it ∪ the app-side hidden set: @@ -89,23 +109,21 @@ class CalendarRepositoryImpl @Inject constructor( // and re-enable invisible calendars. override fun instances(range: ClosedRange): Flow> = combine( - ticks - .onStart { emit(Unit) } - .reQuery { - // Both reads in one pass, so a list of instances is never - // filtered against a visibility snapshot from another tick. - QueriedInstances( - instances = dataSource.instances( - beginMillis = range.start.toEpochMillis(), - endMillis = range.endInclusive.toEpochMillis(), - ), - invisibleCalendarIds = invisibleCalendarIds(), - ) - }, + visibilityTicks().reQuery { + // All three reads in one pass, so a list of instances is never + // filtered against a visibility snapshot from another tick. + QueriedInstances( + instances = dataSource.instances( + beginMillis = range.start.toEpochMillis(), + endMillis = range.endInclusive.toEpochMillis(), + ), + switchedOffCalendarIds = invisibleCalendarIds() + + prefs.pendingDisabledCalendarIds.first(), + ) + }, prefs.hiddenCalendarIds, - prefs.pendingDisabledCalendarIds, - ) { queried, hidden, pendingDisabled -> - val excluded = hidden + pendingDisabled + queried.invisibleCalendarIds + ) { queried, hidden -> + val excluded = hidden + queried.switchedOffCalendarIds if (excluded.isEmpty()) queried.instances else queried.instances.filterNot { it.calendarId in excluded } } @@ -119,7 +137,7 @@ class CalendarRepositoryImpl @Inject constructor( /** One instances query plus the visibility it must be filtered against. */ private data class QueriedInstances( val instances: List, - val invisibleCalendarIds: Set, + val switchedOffCalendarIds: Set, ) /** Calendars switched off at system level — hidden, and never reminded about. */ @@ -129,6 +147,7 @@ class CalendarRepositoryImpl @Inject constructor( private val calendarsLock = Mutex() private var cachedGeneration = -1L + private var cachedPending: Set? = null private var cachedCalendars: List = emptyList() /** @@ -140,12 +159,21 @@ class CalendarRepositoryImpl @Inject constructor( * * An empty result is never cached — it is what a read without the calendar * permission returns, and the grant itself doesn't notify the provider. + * + * The pending switch-off set keys the cache alongside the tick. An id leaves + * that set the moment its `VISIBLE` write lands, while the observer that + * would invalidate the snapshot is only dispatched through the main looper + * afterwards — so a snapshot taken while the id was still pending, read + * against the set that no longer holds it, would report the calendar as *on* + * again and re-admit exactly the events being hidden. */ private suspend fun calendarsSnapshot(): List = calendarsLock.withLock { val current = generation.get() - if (current != cachedGeneration || cachedCalendars.isEmpty()) { + val pending = prefs.pendingDisabledCalendarIds.first() + if (current != cachedGeneration || pending != cachedPending || cachedCalendars.isEmpty()) { cachedCalendars = dataSource.calendars() cachedGeneration = current + cachedPending = pending } cachedCalendars } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt index f96d11f..e615803 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/prefs/CalendarPrefs.kt @@ -8,6 +8,7 @@ import androidx.datastore.preferences.core.edit import androidx.datastore.preferences.core.longPreferencesKey import androidx.datastore.preferences.core.stringPreferencesKey import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.distinctUntilChanged import kotlinx.coroutines.flow.map import javax.inject.Inject import javax.inject.Singleton @@ -28,9 +29,13 @@ class CalendarPrefs @Inject constructor( private val store: DataStore, ) { - val hiddenCalendarIds: Flow> = store.data.map { prefs -> - prefs[HIDDEN_IDS_KEY].parseIds() - } + // Both id sets are deduped: the store is shared with SettingsPrefs, so every + // unrelated write (a settings toggle, the last-used calendar) re-emits an + // identical set otherwise — and a change to the pending set now costs a + // fresh provider read in CalendarRepositoryImpl. + val hiddenCalendarIds: Flow> = store.data + .map { prefs -> prefs[HIDDEN_IDS_KEY].parseIds() } + .distinctUntilChanged() suspend fun setHiddenCalendarIds(ids: Set) { store.edit { prefs -> prefs.writeIds(HIDDEN_IDS_KEY, ids) } @@ -49,9 +54,9 @@ class CalendarPrefs @Inject constructor( * provider entry by entry the moment the app may write, and nothing ever * adds to it while it may. */ - val pendingDisabledCalendarIds: Flow> = store.data.map { prefs -> - prefs[DISABLED_IDS_KEY].parseIds() - } + val pendingDisabledCalendarIds: Flow> = store.data + .map { prefs -> prefs[DISABLED_IDS_KEY].parseIds() } + .distinctUntilChanged() suspend fun addPendingDisabledCalendarIds(ids: Collection) = editPendingDisabled { it + ids } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt index 324f092..9baa38f 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepositoryImplTest.kt @@ -376,6 +376,8 @@ class CalendarRepositoryImplTest { } // The next tick invalidates it — one fresh read, not one per flow. + // (The list has to change: an identical one is collapsed.) + fake.calendarsResult = listOf(makeCal(1L), makeCal(2L), makeCal(3L)) fake.tick() awaitItem() assertThat(fake.calendarQueries).isEqualTo(2) @@ -383,6 +385,64 @@ class CalendarRepositoryImplTest { } } + @Test + fun `calendars does not re-emit an unchanged list`(@TempDir tempDir: Path) = runTest { + // The store is shared with SettingsPrefs, so an unrelated write would + // otherwise re-run every view's combine for an identical list. + val prefs = newPrefs(tempDir) + val fake = FakeCalendarDataSource().apply { calendarsResult = listOf(makeCal(1L)) } + val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + + repo.calendars().test { + assertThat(awaitItem().map { it.id }).containsExactly(1L) + + prefs.setLastUsedCalendarId(1L) + fake.tick() + + expectNoEvents() + cancelAndIgnoreRemainingEvents() + } + } + + @Test + fun `a flushed switch-off never reads as on again before the provider ticks`( + @TempDir tempDir: Path, + ) = runTest { + // The reconciler's shape: write VISIBLE = 0 straight to the provider, + // then release the id app-side. The provider's notification only arrives + // afterwards (it is dispatched through the main looper), so the release + // must not be read against the snapshot from before the write — that + // would flash exactly the events being hidden back into every view. + val prefs = newPrefs(tempDir) + prefs.addPendingDisabledCalendarIds(setOf(2L)) + val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(makeCal(1L), makeCal(2L)) + instancesResult = { _, _ -> + listOf(makeEvent(10L, "A", calendarId = 1L), makeEvent(11L, "B", calendarId = 2L)) + } + } + val repo = CalendarRepositoryImpl(fake, prefs, newSettings(tempDir), UnconfinedTestDispatcher(testScheduler)) + val range = Instant.fromEpochMilliseconds(0)..Instant.fromEpochMilliseconds(10_000L) + + // Warm the snapshot the way an open view would. + repo.instances(range).test { + assertThat(awaitItem().map { it.title }).containsExactly("A") + cancelAndIgnoreRemainingEvents() + } + + fake.setCalendarVisible(2L, false) // no tick(): the observer hasn't fired yet + prefs.removePendingDisabledCalendarIds(setOf(2L)) + + repo.instances(range).test { + assertThat(awaitItem().map { it.title }).containsExactly("A") + cancelAndIgnoreRemainingEvents() + } + repo.calendars().test { + assertThat(awaitItem().single { it.id == 2L }.isVisibleInSystem).isFalse() + cancelAndIgnoreRemainingEvents() + } + } + @Test fun `searchEvents drops results from calendars that are off or hidden`( @TempDir tempDir: Path, From ce4d6bc4d1d9c54fb00c5620dc1b8f52372a0ddf Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 25 Jul 2026 21:34:24 +0200 Subject: [PATCH 6/7] fix(calendars): keep an event's own calendar when it is switched off MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../calendula/ui/edit/EventEditViewModel.kt | 16 ++++++++++---- .../ui/settings/SettingsViewModel.kt | 9 ++++++-- .../ui/edit/EventEditViewModelTest.kt | 21 +++++++++++++++++++ 3 files changed, 40 insertions(+), 6 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt index 457b998..42cee09 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModel.kt @@ -184,6 +184,9 @@ class EventEditViewModel @Inject constructor( * [state]). Managed special-dates calendars are excluded too: their events * are owned by the contact sync, which would delete any user event created * there. + * + * This is the list of *targets*. An event already living in an excluded + * calendar keeps it — [state] adds it back to the picker. */ private val writableCalendars: Flow> = allCalendars.map { calendars -> calendars.filter { it.canModifyContents && it.isVisibleInSystem && !it.isManaged } @@ -232,11 +235,16 @@ class EventEditViewModel @Inject constructor( // off the calendar's durable marker, not a stored id, so it holds after a // backup restore too. val isManaged = local.editTarget != null && resolvedCalendar?.isManaged == true - // The picker offers writable calendars only; when editing a managed event - // its own (excluded) calendar is added back so the row still names it. + // The picker offers writable calendars only; the event's own calendar is + // added back whenever it isn't among them — a managed special-dates one, + // or one switched off on this device — so the row keeps naming it instead + // of reading as the "no calendar" error, and saving can leave the event + // where it is. A calendar the app may not write to is still no target. + val ownCalendar = resolvedCalendar?.takeIf { own -> + own.canModifyContents && external.writable.none { it.id == own.id } + } val pickerCalendars = - if (isManaged && resolvedCalendar != null) external.writable + resolvedCalendar - else external.writable + if (ownCalendar != null) external.writable + ownCalendar else external.writable // An all-day event is date-anchored, so a zone is meaningless on it — // the field is withheld from both lists rather than shown as a no-op. val offerableFields = EventFormField.entries.toSet() - diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/settings/SettingsViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/settings/SettingsViewModel.kt index a549089..9adcf3a 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/settings/SettingsViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/settings/SettingsViewModel.kt @@ -74,9 +74,14 @@ class SettingsViewModel @Inject constructor( private val dynamicColorAvailable = Build.VERSION.SDK_INT >= Build.VERSION_CODES.S - /** Writable calendars — the only ones that take a per-calendar reminder override. */ + /** + * Writable calendars that are switched on — the only ones that take a + * per-calendar reminder override. A calendar switched off in Settings → + * Calendars is `VISIBLE = 0`, so the provider schedules no alarms for it and + * a default reminder configured there could never fire (#75). + */ private val writableCalendars: Flow> = repository.calendars() - .map { calendars -> calendars.filter { it.canModifyContents } } + .map { calendars -> calendars.filter { it.canModifyContents && it.isVisibleInSystem } } .catch { emit(emptyList()) } val state: StateFlow = diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModelTest.kt index 898399f..184be1b 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/ui/edit/EventEditViewModelTest.kt @@ -108,6 +108,27 @@ class EventEditViewModelTest { job.cancel() } + @Test + fun `editing an event in a switched-off calendar keeps it in the picker`( + @TempDir tempDir: Path, + ) = runTest(dispatcher) { + // Otherwise the calendar row renders as the "no calendar" error and any + // pick routes the save through a move the user never asked for. + val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(cal(1L), cal(2L, visible = false)) + eventDetailResult = { detail(calendarId = 2L) } + } + val vm = viewModel(tempDir, fake) + val job = activate(vm) + + vm.openForEdit(eventId = 42L, beginMillis = beginMillis, endMillis = endMillis) + advanceUntilIdle() + + assertThat(vm.state.value?.calendars?.map { it.id }).containsExactly(1L, 2L) + assertThat(vm.state.value?.form?.calendarId).isEqualTo(2L) + job.cancel() + } + @Test fun `changing the calendar routes the save through a move, not an update`( @TempDir tempDir: Path, From 7aef01d95e4af204ccda033a047765d1c0854dd9 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 25 Jul 2026 21:34:24 +0200 Subject: [PATCH 7/7] docs(architecture): record what the second review pass changed 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) --- docs/ARCHITECTURE.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 50698ae..629f57f 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -171,8 +171,13 @@ 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 -separate in-app declutter that never touches reminders. +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. + Deliberately absent until real devices prove it necessary: own alarm scheduling, `BOOT_COMPLETED`, snooze/dismiss actions, battery-exemption prompts.