fix(settings): make the reminder picker's empty state safe and explicit

Three defects in the v2.13.0 multi-select reminder picker:

- Unchecking the last time on a per-calendar picker silently persisted
  an explicit no-reminder override; an accidental toggle-undo wiped the
  calendar's default. Empty-by-unchecking now reverts to 'Use default'
  (Inherit); deliberate no-reminder is its own exclusive 'None' row
  (reusing reminder_none) on both pickers, so the empty state is
  visible and reachable instead of implicit.

- A custom (non-preset) lead time's row vanished the moment it was
  unchecked, stranding the hand-entered value. Custom values seen this
  session keep their row (unchecked) until the picker closes.

- The optimistic selection seeded once from a possibly-not-yet-loaded
  settings state (initialValue emptyList behind a CalendarProvider-
  gated combine), so a quick first toggle after process-death restore
  overwrote the stored default. The local state now re-syncs from the
  incoming selection until the user first interacts.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
2026-07-02 23:01:41 +02:00
parent 037d05b171
commit 5c513a1b19
2 changed files with 108 additions and 30 deletions

View File

@@ -24,6 +24,7 @@ import androidx.compose.material3.Surface
import androidx.compose.material3.Text import androidx.compose.material3.Text
import android.view.WindowManager import android.view.WindowManager
import androidx.compose.runtime.Composable import androidx.compose.runtime.Composable
import androidx.compose.runtime.LaunchedEffect
import androidx.compose.runtime.SideEffect import androidx.compose.runtime.SideEffect
import androidx.compose.runtime.getValue import androidx.compose.runtime.getValue
import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.mutableStateOf
@@ -116,12 +117,16 @@ fun <T> OptionPicker(
* Reminder-default picker, full-screen and **multi-select**: each [presets] * Reminder-default picker, full-screen and **multi-select**: each [presets]
* lead time (plus any chosen custom value) is a checkbox row that toggles * lead time (plus any chosen custom value) is a checkbox row that toggles
* independently, so a default can carry several reminders ("1 week before" *and* * independently, so a default can carry several reminders ("1 week before" *and*
* "on the day"). A per-calendar picker ([allowInherit]) gains an exclusive "Use * "on the day"). Two exclusive checkmark rows sit above the list: an explicit
* default reminder" row above the list; choosing it defers to the global default * "No reminder" (both pickers, so the empty state stays visible and reachable)
* and clears the checked times. With nothing checked and inherit off, the choice * and, for a per-calendar picker ([allowInherit]), "Use default reminder", which
* is [CalendarReminderOverride.None] (an explicit no-reminder). A "Custom" row * defers to the global default. Clearing the last checked time reverts to "Use
* expands an inline number field plus a unit selector to add an arbitrary lead * default" on a per-calendar picker (so an accidental toggle-undo can't silently
* time to the set. Changes apply live via [onSelect]; the user leaves via back. * wipe the calendar's default) and to explicit [CalendarReminderOverride.None]
* on the global default (where empty legitimately means no reminder). A "Custom"
* row expands an inline number field plus a unit selector to add an arbitrary
* lead time to the set. Changes apply live via [onSelect]; the user leaves via
* back.
*/ */
@Composable @Composable
fun ReminderDefaultPicker( fun ReminderDefaultPicker(
@@ -132,48 +137,54 @@ fun ReminderDefaultPicker(
onSelect: (CalendarReminderOverride) -> Unit, onSelect: (CalendarReminderOverride) -> Unit,
onDismiss: () -> Unit, onDismiss: () -> Unit,
) { ) {
// Optimistic local state: the chosen override is authoritative while the // Optimistic local state: once the user edits, the chosen override is
// picker is open, so quick successive toggles compose on each other instead // authoritative while the picker is open, so quick successive toggles compose
// of racing the round-trip through the settings flow (which would drop a // on each other instead of racing the round-trip through the settings flow
// toggle made before the previous write echoes back). Seeded once on open; // (which would drop a toggle made before the previous write echoes back).
// the picker leaves composition on dismiss, so reopening re-reads [selected].
var current by remember { mutableStateOf(selected) } var current by remember { mutableStateOf(selected) }
// Whether the user has toggled anything yet. Until then, [current] keeps
// mirroring [selected]: a picker that first composed against the settings
// flow's initialValue (empty — reachable after process-death restore straight
// onto the Notifications page, before the CalendarProvider-gated combine
// resolves) then adopts the real stored default instead of persisting empty
// on the first tap. After the first edit, local state is authoritative.
var userEdited by remember { mutableStateOf(false) }
LaunchedEffect(selected) {
if (!userEdited) current = selected
}
val inherits = current is CalendarReminderOverride.Inherit val inherits = current is CalendarReminderOverride.Inherit
val isNone = current is CalendarReminderOverride.None
val selectedMinutes = (current as? CalendarReminderOverride.Minutes)?.minutes.orEmpty() val selectedMinutes = (current as? CalendarReminderOverride.Minutes)?.minutes.orEmpty()
// Presets plus any selected custom (non-preset) values, each as its own row. // Custom (non-preset) lead times seen this session, so unchecking one keeps
val rows = presets + selectedMinutes.filter { it !in presets }.sorted() // its row (unchecked) until the picker closes instead of vanishing mid-tap,
// which would strand a hand-entered value with no way to re-check it.
val seenCustom = remember { mutableSetOf<Int>() }
seenCustom += selectedMinutes.filter { it !in presets }
// Presets plus every custom value seen this session, each as its own row.
val rows = presets + seenCustom.sorted()
var customExpanded by rememberSaveable { mutableStateOf(false) } var customExpanded by rememberSaveable { mutableStateOf(false) }
var amountText by rememberSaveable { mutableStateOf("") } var amountText by rememberSaveable { mutableStateOf("") }
var unit by rememberSaveable { mutableStateOf(ReminderUnit.Minutes) } var unit by rememberSaveable { mutableStateOf(ReminderUnit.Minutes) }
fun apply(override: CalendarReminderOverride) { fun apply(override: CalendarReminderOverride) {
userEdited = true
current = override current = override
onSelect(override) onSelect(override)
} }
// Emit the override for a new set of minutes: empty collapses to None, which fun emit(minutes: List<Int>) = apply(reminderOverrideForMinutes(minutes, allowInherit))
// (with inherit unchosen) reads as an explicit "no reminder".
fun emit(minutes: List<Int>) {
val norm = minutes.distinct().sorted()
apply(
if (norm.isEmpty()) {
CalendarReminderOverride.None
} else {
CalendarReminderOverride.Minutes(norm)
},
)
}
fun toggle(minute: Int) = fun toggle(minute: Int) =
emit(if (minute in selectedMinutes) selectedMinutes - minute else selectedMinutes + minute) emit(if (minute in selectedMinutes) selectedMinutes - minute else selectedMinutes + minute)
FullScreenPicker(title = title, onDismiss = onDismiss) { FullScreenPicker(title = title, onDismiss = onDismiss) {
// Exclusive "use default" choice (per-calendar only): its own group above // Exclusive choices in their own group above the multi-select list, since
// the multi-select list, since inheriting is mutually exclusive with // each is mutually exclusive with picking specific times: "use default"
// picking specific times. // (per-calendar only) and an explicit "no reminder" (both pickers, so the
// empty state stays visible and deliberately reachable).
if (allowInherit) { if (allowInherit) {
GroupedRow( GroupedRow(
title = stringResource(R.string.reminder_use_default), title = stringResource(R.string.reminder_use_default),
position = Position.Alone, position = Position.Top,
selected = inherits, selected = inherits,
trailing = if (inherits) { trailing = if (inherits) {
{ SelectedCheck() } { SelectedCheck() }
@@ -182,8 +193,19 @@ fun ReminderDefaultPicker(
}, },
onClick = { apply(CalendarReminderOverride.Inherit) }, onClick = { apply(CalendarReminderOverride.Inherit) },
) )
Spacer(Modifier.height(24.dp))
} }
GroupedRow(
title = stringResource(R.string.reminder_none),
position = if (allowInherit) Position.Bottom else Position.Alone,
selected = isNone,
trailing = if (isNone) {
{ SelectedCheck() }
} else {
null
},
onClick = { apply(CalendarReminderOverride.None) },
)
Spacer(Modifier.height(24.dp))
val rowCount = rows.size + 1 // + the custom row val rowCount = rows.size + 1 // + the custom row
rows.forEachIndexed { index, minute -> rows.forEachIndexed { index, minute ->
val checked = minute in selectedMinutes val checked = minute in selectedMinutes
@@ -222,6 +244,27 @@ fun ReminderDefaultPicker(
} }
} }
/**
* The override a lead-time set maps to when emitted from [ReminderDefaultPicker].
* A non-empty set is [CalendarReminderOverride.Minutes]; an empty set (the last
* time was unchecked) reverts to [CalendarReminderOverride.Inherit] on a
* per-calendar picker ([allowInherit]) so an accidental toggle-undo can't
* silently wipe the calendar's default, and to explicit
* [CalendarReminderOverride.None] on the global default (where empty legitimately
* means no reminder). Pure so it can be unit-tested.
*/
internal fun reminderOverrideForMinutes(
minutes: List<Int>,
allowInherit: Boolean,
): CalendarReminderOverride {
val norm = minutes.distinct().sorted()
return when {
norm.isNotEmpty() -> CalendarReminderOverride.Minutes(norm)
allowInherit -> CalendarReminderOverride.Inherit
else -> CalendarReminderOverride.None
}
}
/** /**
* The expanded "Custom" lead-time editor: a tonal card connected to the Custom * The expanded "Custom" lead-time editor: a tonal card connected to the Custom
* row above it (matching the grouped-row system, so the two read as one * row above it (matching the grouped-row system, so the two read as one

View File

@@ -0,0 +1,35 @@
package de.jeanlucmakiola.calendula.ui.common
import com.google.common.truth.Truth.assertThat
import de.jeanlucmakiola.calendula.data.prefs.CalendarReminderOverride
import org.junit.jupiter.api.Test
/**
* The pure emit rule behind [ReminderDefaultPicker]: a non-empty selection is
* [CalendarReminderOverride.Minutes] (normalised), while clearing the last time
* reverts to [CalendarReminderOverride.Inherit] on a per-calendar picker (so an
* accidental toggle-undo can't silently wipe the calendar's default) and to
* explicit [CalendarReminderOverride.None] on the global default.
*/
class ReminderOverrideForMinutesTest {
@Test
fun `empty selection on a per-calendar picker reverts to inherit`() {
assertThat(reminderOverrideForMinutes(emptyList(), allowInherit = true))
.isEqualTo(CalendarReminderOverride.Inherit)
}
@Test
fun `empty selection on the global default is explicit none`() {
assertThat(reminderOverrideForMinutes(emptyList(), allowInherit = false))
.isEqualTo(CalendarReminderOverride.None)
}
@Test
fun `a non-empty selection is normalised minutes regardless of scope`() {
assertThat(reminderOverrideForMinutes(listOf(30, 10, 10, 60), allowInherit = true))
.isEqualTo(CalendarReminderOverride.Minutes(listOf(10, 30, 60)))
assertThat(reminderOverrideForMinutes(listOf(30, 10, 10, 60), allowInherit = false))
.isEqualTo(CalendarReminderOverride.Minutes(listOf(10, 30, 60)))
}
}