fix(edit): correct the recurrence picker's read-out and end handling

Review follow-ups to the picker redesign, plus the bugs it exposed.

The live read-out now renders customResult itself — the exact string OK
would save — instead of rebuilding a parallel rule with `interval ?: 1`
and `end ?: Never` fallbacks, which let it confidently describe a rule
that differed from the selected controls whenever a field was invalid.

Shrink the invalid space behind it: a blank amount field reads as its
visible placeholder (1 / 10) rather than as an error, and backing out of
the date picker falls back to "never" instead of stranding a dateless
"on a date". Only an out-of-range 0 remains invalid, and that now says so
rather than greying out OK with no cause.

UNTIL displayed the day after the one picked for zones behind UTC:
toRRule deliberately writes the end of the chosen *local* day expressed
in UTC (the provider applies UNTIL coarsely), so the read side must
convert back before taking the date. Fixes the detail screen too, and
untilLocalDate is extracted so it can be tested.

Also: hoist a remember() out of a conditional (a slot that appears and
disappears breaks positional memoisation), match GroupedSurface's 22dp
corners instead of a drifted local 20dp copy, move the cards onto
GroupedSurface, drop the segmented row's icon slot so longer unit labels
fit, and reserve two lines so the stack stops shifting as the phrase
grows with each weekday.

Extract SettingsPrefs.firstDayOfWeek(scope), replacing three copies that
had drifted onto different initialValues.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
2026-07-20 15:20:15 +02:00
parent 722fc87259
commit 98d76deee5
8 changed files with 205 additions and 86 deletions

View File

@@ -23,8 +23,12 @@ import de.jeanlucmakiola.calendula.ui.common.IMPLEMENTED_VIEWS
import de.jeanlucmakiola.calendula.ui.common.QuickSwitchConfig
import de.jeanlucmakiola.calendula.ui.theme.FONT_SYSTEM_TOKEN
import java.time.ZoneId
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.SharingStarted
import kotlinx.coroutines.flow.StateFlow
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.stateIn
import kotlinx.datetime.DayOfWeek
import java.time.temporal.WeekFields
import java.util.Locale
@@ -82,6 +86,24 @@ fun WeekStartPref.resolveFirstDay(locale: Locale): DayOfWeek = when (this) {
WeekStartPref.Auto -> DayOfWeek(WeekFields.of(locale).firstDayOfWeek.value)
}
/**
* The resolved first day of the week as a hot [StateFlow] — the one shape every
* screen that orders or lays out weekdays should use (the month grid, the agenda
* "this week" range, the recurrence weekday toggles). Sharing it keeps the
* initial-frame value identical everywhere; hand-rolled copies had drifted onto
* different `initialValue`s, so two surfaces could disagree for a frame.
*/
fun SettingsPrefs.firstDayOfWeek(scope: CoroutineScope): StateFlow<DayOfWeek> =
weekStart
.map { it.resolveFirstDay(Locale.getDefault()) }
.stateIn(
scope = scope,
started = SharingStarted.WhileSubscribed(5_000L),
// Seed with the locale convention rather than a hardcoded weekday, so
// the first frame is already right for everyone still on Auto.
initialValue = WeekStartPref.Auto.resolveFirstDay(Locale.getDefault()),
)
/**
* Display settings (M4) persisted app-side: theme override, Material You
* dynamic colour, and week start. Language is handled separately through

View File

@@ -7,7 +7,7 @@ import de.jeanlucmakiola.calendula.data.calendar.CalendarRepository
import de.jeanlucmakiola.calendula.data.di.IoDispatcher
import de.jeanlucmakiola.calendula.data.prefs.PastEventDisplay
import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs
import de.jeanlucmakiola.calendula.data.prefs.resolveFirstDay
import de.jeanlucmakiola.calendula.data.prefs.firstDayOfWeek
import de.jeanlucmakiola.calendula.domain.CalendarSource
import de.jeanlucmakiola.calendula.domain.EventInstance
import de.jeanlucmakiola.calendula.domain.FailureReason
@@ -20,13 +20,11 @@ import kotlinx.coroutines.flow.catch
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.flatMapLatest
import kotlinx.coroutines.flow.flowOn
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.stateIn
import kotlinx.datetime.DateTimeUnit
import kotlinx.datetime.DayOfWeek
import kotlinx.datetime.LocalDate
import kotlinx.datetime.TimeZone
import java.util.Locale
import kotlinx.datetime.atStartOfDayIn
import kotlinx.datetime.atTime
import kotlinx.datetime.plus
@@ -51,8 +49,7 @@ class AgendaViewModel @Inject constructor(
) { range, showBar -> AgendaSettings(range, showBar) }
// First day of the week, for the calendar-aligned "this week" range.
private val weekStartDay = settingsPrefs.weekStart
.map { it.resolveFirstDay(Locale.getDefault()) }
private val weekStartDay = settingsPrefs.firstDayOfWeek(viewModelScope)
/**
* How to treat events that already ended today (show / dim / hide). A display

View File

@@ -9,6 +9,10 @@ import androidx.compose.ui.text.buildAnnotatedString
import androidx.compose.ui.text.font.FontStyle
import de.jeanlucmakiola.calendula.R
import java.time.DayOfWeek
import java.time.LocalDate
import java.time.LocalDateTime
import java.time.ZoneId
import java.time.ZoneOffset
import java.time.format.DateTimeFormatter
import java.time.format.FormatStyle
import java.time.format.TextStyle as JavaTextStyle
@@ -111,16 +115,36 @@ private fun rruleDayName(token: String, locale: Locale): String? {
/** Parse an RRULE UNTIL value ("20261231" or "20261231T235959Z") to a localized date. */
private fun parseUntilDate(raw: String, locale: Locale): String? {
val digits = raw.takeWhile { it.isDigit() }
if (digits.length < 8) return null
return try {
val date = java.time.LocalDate.of(
val date = untilLocalDate(raw) ?: return null
return DateTimeFormatter.ofLocalizedDate(FormatStyle.MEDIUM).withLocale(locale).format(date)
}
private val UNTIL_UTC_FORMAT: DateTimeFormatter =
DateTimeFormatter.ofPattern("yyyyMMdd'T'HHmmss'Z'", Locale.ROOT)
/**
* The calendar day an RRULE UNTIL value denotes, *in the device's zone*.
*
* The UTC form ("20261231T225959Z") must be converted back before its date is
* read: `SimpleRecurrence.toRRule` writes the end of the chosen **local** day
* expressed in UTC, so for zones behind UTC that instant already falls on the
* following UTC date. Taking the leading digits straight off would then show
* the day after the one the user picked. Date-only and floating forms are
* already local and pass through unchanged.
*/
internal fun untilLocalDate(raw: String, zone: ZoneId = ZoneId.systemDefault()): LocalDate? {
val value = raw.trim()
return runCatching {
LocalDateTime.parse(value, UNTIL_UTC_FORMAT)
.atOffset(ZoneOffset.UTC)
.atZoneSameInstant(zone)
.toLocalDate()
}.recoverCatching {
val digits = value.takeWhile { it.isDigit() }
LocalDate.of(
digits.substring(0, 4).toInt(),
digits.substring(4, 6).toInt(),
digits.substring(6, 8).toInt(),
)
DateTimeFormatter.ofLocalizedDate(FormatStyle.MEDIUM).withLocale(locale).format(date)
} catch (e: Exception) {
null
}
}.getOrNull()
}

View File

@@ -99,7 +99,6 @@ import androidx.compose.ui.text.style.TextOverflow
import androidx.compose.ui.text.input.ImeAction
import androidx.compose.ui.text.input.KeyboardCapitalization
import androidx.compose.ui.text.input.KeyboardType
import androidx.compose.ui.unit.Dp
import androidx.compose.ui.unit.dp
import androidx.core.content.ContextCompat
import androidx.hilt.navigation.compose.hiltViewModel
@@ -140,11 +139,13 @@ import de.jeanlucmakiola.calendula.ui.common.eventFormFieldIcon
import de.jeanlucmakiola.calendula.ui.common.eventFormFieldLabel
import de.jeanlucmakiola.floret.components.FullScreenPicker
import de.jeanlucmakiola.floret.components.GroupedRow
import de.jeanlucmakiola.floret.components.GroupedSurface
import de.jeanlucmakiola.calendula.ui.common.MILLIS_PER_DAY
import de.jeanlucmakiola.floret.components.InlineTextField
import de.jeanlucmakiola.floret.components.OptionCard
import de.jeanlucmakiola.floret.components.OptionPicker
import de.jeanlucmakiola.floret.components.Position
import de.jeanlucmakiola.floret.components.groupedShape as floretGroupedShape
import de.jeanlucmakiola.floret.components.positionOf
import de.jeanlucmakiola.calendula.ui.common.REMINDER_PRESETS
import de.jeanlucmakiola.calendula.ui.common.TimePickerAlert
@@ -1328,8 +1329,21 @@ private fun RecurrencePickerDialog(
val locale = currentLocale()
val untilDate = untilIso?.let { runCatching { LocalDate.parse(it) }.getOrNull() }
val interval = intervalText.toIntOrNull()?.takeIf { it in 1..999 }
val count = countText.toIntOrNull()?.takeIf { it in 1..999 }
// remember() must not sit behind the ?. — a slot that appears and disappears
// as a date is picked breaks Compose's positional memoisation. Format
// unconditionally and let the null fall through to a null summary.
val untilSummary = remember(untilDate, locale) {
untilDate?.let {
DateTimeFormatter.ofLocalizedDate(FormatStyle.MEDIUM)
.withLocale(locale).format(it.toJavaLocalDate())
}
}
// A blank amount field shows its placeholder ("1" / "10") greyed out, so read
// blank as that default rather than as an error — otherwise simply clearing
// the field silently greys out OK. Only a real out-of-range value (0) is
// invalid, and that's the one case the read-out below reports.
val interval = if (intervalText.isBlank()) 1 else intervalText.toIntOrNull()?.takeIf { it in 1..999 }
val count = if (countText.isBlank()) 10 else countText.toIntOrNull()?.takeIf { it in 1..999 }
val customEnd: RecurrenceEnd? = when (endMode) {
RecurrenceEndMode.Never -> RecurrenceEnd.Never
RecurrenceEndMode.Until -> untilDate?.let { RecurrenceEnd.Until(it) }
@@ -1387,22 +1401,25 @@ private fun RecurrencePickerDialog(
// a parent arrangement would leave a residual gap that snaps shut.
Column(modifier = Modifier.fillMaxWidth()) {
// A live, human-readable read-out of the rule being built, so the
// effect of the controls below is never a guess.
// effect of the controls below is never a guess. It renders
// *[customResult] itself* — the exact string OK would save — so it
// can never describe a different rule than the one that lands. When
// the form can't produce a rule, it says why instead of falling
// back to defaults and quietly disagreeing with the controls.
// minLines reserves the second line up front: without it the whole
// stack below shifts a line as the phrase grows with each weekday.
Text(
text = recurrenceText(
SimpleRecurrence(
freq = freq,
interval = interval ?: 1,
end = customEnd ?: RecurrenceEnd.Never,
byDays = if (freq == RecurrenceFreq.Weekly) {
daysMask.toDaySet()
} else {
emptySet()
},
).toRRule(),
locale,
),
text = customResult?.let { recurrenceText(it, locale) }
?: AnnotatedString(
stringResource(R.string.event_edit_recurrence_incomplete),
),
style = MaterialTheme.typography.titleMedium,
color = if (customResult != null) {
MaterialTheme.colorScheme.onSurface
} else {
MaterialTheme.colorScheme.error
},
minLines = 2,
modifier = Modifier
.fillMaxWidth()
.padding(horizontal = 16.dp),
@@ -1410,11 +1427,9 @@ private fun RecurrencePickerDialog(
// How often: an interval amount plus a frequency segmented row —
// all four units on show, no unit hidden behind a dropdown.
Surface(
color = MaterialTheme.colorScheme.surfaceContainerHigh,
shape = groupedShape(Position.Alone),
GroupedSurface(
position = Position.Alone,
modifier = Modifier
.fillMaxWidth()
.padding(top = 16.dp)
.padding(horizontal = 16.dp),
) {
@@ -1442,7 +1457,18 @@ private fun RecurrencePickerDialog(
shape = SegmentedButtonDefaults.itemShape(
index, RecurrenceFreq.entries.size,
),
label = { Text(stringResource(recurrenceUnitLabel(entry))) },
// Drop the default check-icon slot: four segments
// share one row, and its reserved width is what
// pushes longer labels ("Monate", "Wochen") into
// an ellipsis on a compact screen. Selection is
// already carried by the segment's fill.
icon = {},
label = {
Text(
text = stringResource(recurrenceUnitLabel(entry)),
maxLines = 1,
)
},
)
}
}
@@ -1452,11 +1478,9 @@ private fun RecurrencePickerDialog(
// Weekday picks — only meaningful on a weekly rule. The 16dp gap
// sits inside the animated block so it grows/shrinks with the card.
AnimatedVisibility(visible = freq == RecurrenceFreq.Weekly) {
Surface(
color = MaterialTheme.colorScheme.surfaceContainerHigh,
shape = groupedShape(Position.Alone),
GroupedSurface(
position = Position.Alone,
modifier = Modifier
.fillMaxWidth()
.padding(top = 16.dp)
.padding(horizontal = 16.dp),
) {
@@ -1487,12 +1511,7 @@ private fun RecurrencePickerDialog(
)
GroupedRow(
title = stringResource(R.string.event_edit_recurrence_end_until),
summary = untilDate?.let {
remember(it, locale) {
DateTimeFormatter.ofLocalizedDate(FormatStyle.MEDIUM)
.withLocale(locale).format(it.toJavaLocalDate())
}
},
summary = untilSummary,
position = Position.Middle,
selected = endMode == RecurrenceEndMode.Until,
onClick = {
@@ -1510,13 +1529,13 @@ private fun RecurrencePickerDialog(
selected = endMode == RecurrenceEndMode.Count,
onClick = { endMode = RecurrenceEndMode.Count },
)
if (endMode == RecurrenceEndMode.Count) {
Surface(
color = MaterialTheme.colorScheme.surfaceContainerHigh,
shape = groupedShape(Position.Bottom),
modifier = Modifier
.fillMaxWidth()
.padding(horizontal = 16.dp),
// Animated for the same reason the weekday card is: this is the
// picker's other collapsible block, and a bare `if` here made
// the ends group snap while the card above it glided.
AnimatedVisibility(visible = endMode == RecurrenceEndMode.Count) {
GroupedSurface(
position = Position.Bottom,
modifier = Modifier.padding(horizontal = 16.dp),
) {
Row(
verticalAlignment = Alignment.CenterVertically,
@@ -1549,7 +1568,13 @@ private fun RecurrencePickerDialog(
untilIso = it.toString()
showUntilPicker = false
},
onDismiss = { showUntilPicker = false },
onDismiss = {
showUntilPicker = false
// Backing out of the date picker with nothing chosen would leave
// "on date" selected but dateless — a state that can't be saved and
// greys out OK with no visible cause. Fall back to "never".
if (untilIso == null) endMode = RecurrenceEndMode.Never
},
)
}
}
@@ -1625,18 +1650,14 @@ private fun recurrenceUnitLabel(freq: RecurrenceFreq): Int = when (freq) {
RecurrenceFreq.Yearly -> R.string.recurrence_unit_years
}
/** Corner shape for a card at [position] in a grouped run (mirrors GroupedRow). */
private fun groupedShape(position: Position, full: Dp = 20.dp, small: Dp = 6.dp): Shape =
when (position) {
Position.Alone -> RoundedCornerShape(full)
Position.Top -> RoundedCornerShape(
topStart = full, topEnd = full, bottomStart = small, bottomEnd = small,
)
Position.Middle -> RoundedCornerShape(small)
Position.Bottom -> RoundedCornerShape(
topStart = small, topEnd = small, bottomStart = full, bottomEnd = full,
)
}
/**
* Corner shape for a card at [position] in a grouped run. Delegates to the
* family primitive with the same radii [GroupedSurface] draws at rest — a local
* copy had drifted to 20dp, so a card sharing a run with GroupedRows had
* visibly tighter outer corners than the rows above it.
*/
private fun groupedShape(position: Position): Shape =
floretGroupedShape(position, full = 22.dp, small = 6.dp)
/** The 2dp gap that visually separates grouped cards (none after the last). */
private fun groupedGap(position: Position): Modifier = when (position) {

View File

@@ -8,9 +8,8 @@ import de.jeanlucmakiola.calendula.data.calendar.NoSuchEventException
import de.jeanlucmakiola.calendula.data.di.IoDispatcher
import de.jeanlucmakiola.calendula.data.prefs.CalendarPrefs
import de.jeanlucmakiola.calendula.data.prefs.SettingsPrefs
import de.jeanlucmakiola.calendula.data.prefs.WeekStartPref
import de.jeanlucmakiola.calendula.data.prefs.firstDayOfWeek
import de.jeanlucmakiola.calendula.data.prefs.resolveDefaultReminder
import de.jeanlucmakiola.calendula.data.prefs.resolveFirstDay
import de.jeanlucmakiola.calendula.domain.AccessLevel
import de.jeanlucmakiola.calendula.domain.Availability
import de.jeanlucmakiola.calendula.domain.CalendarSource
@@ -49,7 +48,6 @@ import kotlinx.datetime.LocalTime
import kotlinx.datetime.TimeZone
import kotlinx.datetime.toInstant
import kotlinx.datetime.toLocalDateTime
import java.util.Locale
import kotlin.coroutines.cancellation.CancellationException
import kotlin.time.Clock
import kotlin.time.Duration.Companion.hours
@@ -276,13 +274,7 @@ class EventEditViewModel @Inject constructor(
* app's "week starts on" preference, matching the calendar views (Auto
* falls back to the locale convention).
*/
val firstDayOfWeek: StateFlow<DayOfWeek> = settingsPrefs.weekStart
.map { it.resolveFirstDay(Locale.getDefault()) }
.stateIn(
scope = viewModelScope,
started = SharingStarted.WhileSubscribed(5_000L),
initialValue = WeekStartPref.Auto.resolveFirstDay(Locale.getDefault()),
)
val firstDayOfWeek: StateFlow<DayOfWeek> = settingsPrefs.firstDayOfWeek(viewModelScope)
/**
* Initialise a fresh form for a new event on [date]. [startMinutes] (minutes

View File

@@ -6,7 +6,7 @@ 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.prefs.SettingsPrefs
import de.jeanlucmakiola.calendula.data.prefs.resolveFirstDay
import de.jeanlucmakiola.calendula.data.prefs.firstDayOfWeek
import de.jeanlucmakiola.calendula.domain.CalendarSource
import de.jeanlucmakiola.calendula.domain.EventInstance
import de.jeanlucmakiola.calendula.domain.FailureReason
@@ -21,7 +21,6 @@ import kotlinx.coroutines.flow.catch
import kotlinx.coroutines.flow.combine
import kotlinx.coroutines.flow.flatMapLatest
import kotlinx.coroutines.flow.flowOn
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.stateIn
import kotlinx.datetime.DateTimeUnit
import kotlinx.datetime.DayOfWeek
@@ -34,7 +33,6 @@ import kotlinx.datetime.minus
import kotlinx.datetime.plus
import kotlinx.datetime.toInstant
import kotlinx.datetime.toLocalDateTime
import java.util.Locale
import kotlin.time.Clock
import kotlin.time.Instant
import javax.inject.Inject
@@ -48,16 +46,9 @@ class MonthViewModel @Inject constructor(
) : ViewModel() {
private val zone = TimeZone.currentSystemDefault()
private val locale: Locale = Locale.getDefault()
/** First day of the week, from the Settings preference (AUTO → locale). */
val weekStart: StateFlow<DayOfWeek> = settingsPrefs.weekStart
.map { it.resolveFirstDay(locale) }
.stateIn(
scope = viewModelScope,
started = SharingStarted.WhileSubscribed(5_000L),
initialValue = DayOfWeek.MONDAY,
)
val weekStart: StateFlow<DayOfWeek> = settingsPrefs.firstDayOfWeek(viewModelScope)
/** Whether to fade events that have already finished (display concern only). */
val dimCompletedEvents: StateFlow<Boolean> = settingsPrefs.dimCompletedEvents

View File

@@ -149,6 +149,9 @@
<string name="recurrence_unit_weeks">weeks</string>
<string name="recurrence_unit_months">months</string>
<string name="recurrence_unit_years">years</string>
<!-- Shown in place of the live rule read-out when an amount field holds a
value outside 1999 (i.e. 0), so the greyed-out OK button has a reason. -->
<string name="event_edit_recurrence_incomplete">Enter a number from 1 to 999</string>
<string name="event_edit_recurrence_ends">Ends</string>
<string name="event_edit_recurrence_end_never">Never</string>
<string name="event_edit_recurrence_end_until">On a date</string>

View File

@@ -0,0 +1,69 @@
package de.jeanlucmakiola.calendula.ui.common
import com.google.common.truth.Truth.assertThat
import java.time.LocalDate
import java.time.ZoneId
import org.junit.jupiter.api.Test
/**
* The read side of the UNTIL round-trip. `SimpleRecurrence.toRRule` deliberately
* writes the end of the chosen *local* day expressed in UTC (see its docs — the
* provider applies UNTIL coarsely), so displaying it means converting back into
* the device zone first. Reading the leading digits raw showed the wrong day for
* zones behind UTC.
*/
class RecurrenceTextTest {
private val berlin = ZoneId.of("Europe/Berlin")
private val losAngeles = ZoneId.of("America/Los_Angeles")
private val utc = ZoneId.of("UTC")
@Test
fun `UTC form converts back to the picked day west of UTC`() {
// toRRule("2026-12-31", America/Los_Angeles) → 23:59:59 local = 07:59:59Z
// on 1 Jan. Reading the digits raw would show 1 Jan 2027.
assertThat(untilLocalDate("20270101T075959Z", losAngeles))
.isEqualTo(LocalDate.of(2026, 12, 31))
}
@Test
fun `UTC form converts back to the picked day east of UTC`() {
// Berlin in December is UTC+1: 23:59:59 local = 22:59:59Z the same day.
assertThat(untilLocalDate("20261231T225959Z", berlin))
.isEqualTo(LocalDate.of(2026, 12, 31))
}
@Test
fun `UTC form is unchanged at UTC itself`() {
assertThat(untilLocalDate("20261231T235959Z", utc))
.isEqualTo(LocalDate.of(2026, 12, 31))
}
@Test
fun `summer offset is honoured, not a fixed one`() {
// Berlin in July is UTC+2, so the same local end-of-day lands at 21:59:59Z.
assertThat(untilLocalDate("20260801T215959Z", berlin))
.isEqualTo(LocalDate.of(2026, 8, 1))
}
@Test
fun `date-only form is already local and passes through`() {
assertThat(untilLocalDate("20261231", losAngeles))
.isEqualTo(LocalDate.of(2026, 12, 31))
}
@Test
fun `floating date-time form uses its date as-is`() {
// No trailing Z, so it isn't an instant — no conversion may be applied.
assertThat(untilLocalDate("20261231T235959", losAngeles))
.isEqualTo(LocalDate.of(2026, 12, 31))
}
@Test
fun `garbage returns null rather than throwing`() {
assertThat(untilLocalDate("", berlin)).isNull()
assertThat(untilLocalDate("nonsense", berlin)).isNull()
assertThat(untilLocalDate("2026", berlin)).isNull()
assertThat(untilLocalDate("20261340", berlin)).isNull()
}
}