fix: four review findings in the settings rework (#69)

- Restoring a backup left the calendar manager standing over the import
  screen, so the target picker sat hidden behind it.
- The past-events preview consumed drags as well as taps, which stopped the
  picker scrolling when the gesture started on the preview.
- The recurrence preview dropped DTSTART when the rule's weekday picks missed
  the start's own weekday; RFC 5545 keeps it in the set.
- "This month" showed the month name in the range picker, reading as the whole
  month next to options that name a concrete span.

The four files also carry their share of the comment trim from the previous
commit.
This commit is contained in:
2026-07-30 21:57:24 +02:00
parent 9bcd08a5bb
commit 4bdcf20c82
6 changed files with 90 additions and 108 deletions

View File

@@ -10,31 +10,18 @@ import kotlinx.datetime.plus
/** /**
* The first [limit] dates a [SimpleRecurrence] fires on, starting at [start] * The first [limit] dates a [SimpleRecurrence] fires on, starting at [start]
* (the event's own start date, i.e. DTSTART), for showing a rule as dates * (DTSTART), for previewing a rule as dates instead of as words. A preview only,
* instead of as words. * kept to the shapes the picker can build; the provider stays the authority.
* *
* A recurrence rule is the one place in the editor where a correct-sounding * Mirrors RFC 5545: [start] is always the first occurrence (§3.8.5.3), even when
* phrase can still mean something the user didn't intend — "every 2 weeks on * the rule's own picks miss it; a monthly or yearly rule *skips* a period the
* Mon, Fri", "monthly" on the 31st — and the only honest answer is the dates * start day doesn't exist in rather than clamping; a weekly rule repeats in
* themselves. This is a *preview*, deliberately kept to the shapes the picker * blocks of `interval` weeks beginning on Monday (the default WKST, since
* can build; the provider remains the authority on what actually gets stored * [toRRule] never writes one); [RecurrenceEnd.Count] counts real occurrences and
* and expanded. * [RecurrenceEnd.Until] is inclusive.
* *
* The rules it mirrors, all from RFC 5545 (the spec the platform's recurrence * Returns fewer than [limit] dates when the series ends first, and an empty list
* engine implements): * only when the rule yields nothing at all (an UNTIL before [start]).
* - a monthly or yearly rule **skips** a period the start day doesn't exist in,
* rather than sliding to the month's last day: the 31st recurring monthly
* lands only in 31-day months, 29 February yearly only in leap years.
* - a weekly rule with weekday picks repeats in blocks of `interval` weeks,
* with the week beginning on **Monday** — the RFC's default WKST, and the one
* that applies here because [toRRule] never writes a WKST part. The
* display-only "first day of week" preference deliberately doesn't enter into
* it, or the preview would disagree with the stored rule.
* - [RecurrenceEnd.Count] counts real occurrences (skipped periods don't
* consume one), [RecurrenceEnd.Until] is inclusive of its date.
*
* Returns fewer than [limit] dates when the series ends first, and an empty
* list when the rule yields nothing at all (an UNTIL before the start).
*/ */
fun SimpleRecurrence.upcomingOccurrences(start: LocalDate, limit: Int): List<LocalDate> { fun SimpleRecurrence.upcomingOccurrences(start: LocalDate, limit: Int): List<LocalDate> {
if (limit <= 0) return emptyList() if (limit <= 0) return emptyList()
@@ -42,15 +29,17 @@ fun SimpleRecurrence.upcomingOccurrences(start: LocalDate, limit: Int): List<Loc
val maxCount = (end as? RecurrenceEnd.Count)?.times ?: Int.MAX_VALUE val maxCount = (end as? RecurrenceEnd.Count)?.times ?: Int.MAX_VALUE
val wanted = minOf(limit, maxCount) val wanted = minOf(limit, maxCount)
if (wanted <= 0) return emptyList() if (wanted <= 0) return emptyList()
if (until != null && start > until) return emptyList()
val result = mutableListOf<LocalDate>() // DTSTART is in the recurrence set whatever the rule picks, so seed with it
// and let the walk skip anything landing on or before it.
val result = mutableListOf(start)
var period = 0 var period = 0
// Periods that produce nothing (a skipped 31st, a weekday block whose picks // Periods can yield nothing (a skipped 31st), so the cap counts periods
// all fall before the start) must not stall the walk, so the cap is on // examined rather than dates found.
// periods examined rather than on dates found.
while (result.size < wanted && period < MAX_PERIODS) { while (result.size < wanted && period < MAX_PERIODS) {
for (date in occurrencesInPeriod(period, start)) { for (date in occurrencesInPeriod(period, start)) {
if (date < start) continue if (date <= start) continue
if (until != null && date > until) return result if (until != null && date > until) return result
result += date result += date
if (result.size == wanted) return result if (result.size == wanted) return result
@@ -67,8 +56,8 @@ private fun SimpleRecurrence.occurrencesInPeriod(period: Int, start: LocalDate):
RecurrenceFreq.Weekly -> weeklyOccurrences(period, start) RecurrenceFreq.Weekly -> weeklyOccurrences(period, start)
RecurrenceFreq.Monthly -> { RecurrenceFreq.Monthly -> {
val month = start.plus(period * interval, DateTimeUnit.MONTH) val month = start.plus(period * interval, DateTimeUnit.MONTH)
// plus() clamps the day into the shorter month; the rule instead // plus() clamps into the shorter month, but the rule skips such a
// skips such a period, so a clamped date means "not this month". // period so a clamped date means "not this month".
listOfNotNull(dateOrNull(month.year, month.month.number, start.day)) listOfNotNull(dateOrNull(month.year, month.month.number, start.day))
} }
RecurrenceFreq.Yearly -> RecurrenceFreq.Yearly ->
@@ -96,13 +85,9 @@ private fun SimpleRecurrence.weeklyOccurrences(period: Int, start: LocalDate): L
} }
/** /**
* Whether a run of [occurrences] starting at [start] leaves its starting year * Whether a run of [occurrences] starting at [start] leaves its starting year,
* i.e. whether showing them without a year would be ambiguous. * i.e. whether showing them without a year would be ambiguous — a yearly rule
* * would otherwise read as the same date repeated.
* Day and month alone are enough for a rule that stays inside one year, and
* dropping the year keeps the line short. They are actively misleading
* otherwise: a yearly rule reads as the same date three times over, and a
* monthly one rolling past December gives no hint that it has.
*/ */
fun occurrencesSpanYears(occurrences: List<LocalDate>, start: LocalDate): Boolean = fun occurrencesSpanYears(occurrences: List<LocalDate>, start: LocalDate): Boolean =
occurrences.any { it.year != start.year } || occurrences.map { it.year }.distinct().size > 1 occurrences.any { it.year != start.year } || occurrences.map { it.year }.distinct().size > 1
@@ -114,9 +99,8 @@ private fun dateOrNull(year: Int, month: Int, day: Int): LocalDate? =
private const val DAYS_PER_WEEK = 7 private const val DAYS_PER_WEEK = 7
/** /**
* How many repetitions to examine before giving up. Generous enough for the * How many repetitions to examine before giving up: generous enough for the
* sparsest rule the picker can build (29 February yearly with a 999-year * sparsest rule the picker can build, bounded so a rule whose occurrences all
* interval is nonsense; 31st monthly needs at most a handful), and bounded so a * fall outside its own UNTIL can't spin.
* rule whose occurrences all fall outside its own UNTIL can't spin.
*/ */
private const val MAX_PERIODS = 2_000 private const val MAX_PERIODS = 2_000

View File

@@ -149,9 +149,8 @@ fun CalendarHost(
// over Settings and survives view switches. // over Settings and survives view switches.
var showCalendars by rememberSaveable { mutableStateOf(false) } var showCalendars by rememberSaveable { mutableStateOf(false) }
// Backup & restore (#69) — like the manager, driven by the calendar list // Backup & restore (#69) — hoisted like the manager, being driven by the
// rather than by preferences, so it is hoisted here instead of living as a // calendar list rather than by preferences. Reached from both.
// Settings sub-section. Reached from Settings and from the manager.
var showBackup by rememberSaveable { mutableStateOf(false) } var showBackup by rememberSaveable { mutableStateOf(false) }
// Event form (v1.2 create) — same held-key pattern as the detail screen: // Event form (v1.2 create) — same held-key pattern as the detail screen:
@@ -486,10 +485,8 @@ fun CalendarHost(
) )
} }
// Calendar manager — declared last so it covers every overlay that can // Declared last so it covers every overlay that can open it: Settings,
// open it: Settings, both event forms, and the .ics import picker (#76). // both event forms, and the .ics import picker (#76).
// Coming back from it leaves the caller exactly as it was, with the
// calendar list already refreshed by the provider's notification.
AnimatedVisibility( AnimatedVisibility(
visible = showCalendars, visible = showCalendars,
enter = slideInHorizontally(slideSpec) { it } + fadeIn(), enter = slideInHorizontally(slideSpec) { it } + fadeIn(),
@@ -509,12 +506,15 @@ fun CalendarHost(
) { ) {
BackupScreen( BackupScreen(
onBack = { showBackup = false }, onBack = { showBackup = false },
// Restoring runs the normal .ics import, and that way round this // Restore runs the normal .ics import, and both this screen and
// screen has to step aside: declared above the import overlays, // the manager that can have opened it are declared above the
// it would otherwise cover the screen it just asked for. Closing // import overlays — so both have to step aside.
// it hands the user back to whatever opened Backup once the onImport = {
// import is done. importUri = it
onImport = { importUri = it; importForceMany = true; showBackup = false }, importForceMany = true
showBackup = false
showCalendars = false
},
) )
} }
} }

View File

@@ -92,20 +92,27 @@ fun parseAgendaRange(stored: String?, default: AgendaRange): AgendaRange = when
* - [AgendaRange.ThisMonth] → month and year ("June 2026") * - [AgendaRange.ThisMonth] → month and year ("June 2026")
* - everything else → "start end" ("27 Jun 3 Jul 2026"), with the start's * - everything else → "start end" ("27 Jun 3 Jul 2026"), with the start's
* year shown too only when it differs from the end's. * year shown too only when it differs from the end's.
*
* [monthAsSpan] puts [AgendaRange.ThisMonth] on the "start end" form as well.
* The agenda's own header browses the whole month, so the month name is right
* there; the range picker previews the window an option opens *today*, which for
* "This month" is only the rest of it.
*/ */
fun agendaRangeWindowSummary( fun agendaRangeWindowSummary(
range: AgendaRange, range: AgendaRange,
start: LocalDate, start: LocalDate,
end: LocalDate, end: LocalDate,
locale: Locale, locale: Locale,
monthAsSpan: Boolean = false,
): String { ): String {
val javaStart = java.time.LocalDate.of(start.year, start.month.ordinal + 1, start.day) val javaStart = java.time.LocalDate.of(start.year, start.month.ordinal + 1, start.day)
val javaEnd = java.time.LocalDate.of(end.year, end.month.ordinal + 1, end.day) val javaEnd = java.time.LocalDate.of(end.year, end.month.ordinal + 1, end.day)
val dayMonth = localizedDateFormatter(locale, "dMMM") val dayMonth = localizedDateFormatter(locale, "dMMM")
val dayMonthYear = localizedDateFormatter(locale, "dMMMy") val dayMonthYear = localizedDateFormatter(locale, "dMMMy")
return when (range) { return when {
AgendaRange.Day -> dayMonthYear.format(javaStart) range == AgendaRange.Day -> dayMonthYear.format(javaStart)
AgendaRange.ThisMonth -> localizedDateFormatter(locale, "LLLLy").format(javaStart) range == AgendaRange.ThisMonth && !monthAsSpan ->
localizedDateFormatter(locale, "LLLLy").format(javaStart)
else -> { else -> {
val startFmt = if (start.year == end.year) dayMonth else dayMonthYear val startFmt = if (start.year == end.year) dayMonth else dayMonthYear
"${startFmt.format(javaStart)} ${dayMonthYear.format(javaEnd)}" "${startFmt.format(javaStart)} ${dayMonthYear.format(javaEnd)}"

View File

@@ -59,10 +59,9 @@ import kotlin.time.Clock
* lead time to the set. Changes apply live via [onSelect]; the user leaves via * lead time to the set. Changes apply live via [onSelect]; the user leaves via
* back. * back.
* *
* [leadTimeSummary] adds a second line to each lead-time row. It exists for the * [leadTimeSummary] adds a second line to each lead-time row, for the all-day
* all-day pickers, where the lead time alone doesn't say when anything happens: * pickers: the hour comes from the separate "show all-day reminders at" setting,
* the hour comes from the separate "show all-day reminders at" setting, so * so the lead time alone doesn't say when anything happens.
* "1 day before" is only half the answer until the row spells out the time.
*/ */
@Composable @Composable
fun ReminderDefaultPicker( fun ReminderDefaultPicker(
@@ -233,10 +232,8 @@ internal fun PickerDescription(text: String) {
* Mirrors [ReminderDefaultPicker]'s custom-expand pattern. * Mirrors [ReminderDefaultPicker]'s custom-expand pattern.
* *
* Every option carries the dates it resolves to today as its summary, computed * Every option carries the dates it resolves to today as its summary, computed
* through the same [dayCount] the agenda windows by (hence [weekStart], which * through the same [dayCount] the agenda windows by (hence [weekStart]) — only
* "This week" depends on). Without it the two groups are near-indistinguishable * the concrete span tells "This week" and "Next 7 days" apart.
* — "This week" and "Next 7 days" name different windows in the same words, and
* only the concrete span tells them apart.
*/ */
@Composable @Composable
fun AgendaRangePicker( fun AgendaRangePicker(
@@ -260,11 +257,11 @@ fun AgendaRangePicker(
val locale = currentLocale() val locale = currentLocale()
val zone = remember { TimeZone.currentSystemDefault() } val zone = remember { TimeZone.currentSystemDefault() }
val today = remember(zone) { Clock.System.now().toLocalDateTime(zone).date } val today = remember(zone) { Clock.System.now().toLocalDateTime(zone).date }
// The window each option would open right now — the agenda's own end-day // The agenda's own end-day arithmetic (anchor + dayCount - 1), so the dates
// arithmetic (anchor + dayCount - 1), so the dates match what it will show. // match what it will show.
val windowSummary: (AgendaRange) -> String = { range -> val windowSummary: (AgendaRange) -> String = { range ->
val end = today.plus(range.dayCount(today, weekStart) - 1, DateTimeUnit.DAY) val end = today.plus(range.dayCount(today, weekStart) - 1, DateTimeUnit.DAY)
agendaRangeWindowSummary(range, today, end, locale) agendaRangeWindowSummary(range, today, end, locale, monthAsSpan = true)
} }
val rangeRow: @Composable (AgendaRange, Position) -> Unit = { option, position -> val rangeRow: @Composable (AgendaRange, Position) -> Unit = { option, position ->
@@ -306,8 +303,8 @@ fun AgendaRangePicker(
} else { } else {
stringResource(R.string.agenda_range_custom) stringResource(R.string.agenda_range_custom)
}, },
// Only a chosen custom window has dates to show; an unset one has no // An unset custom window has no day count yet, so the row stays
// day count yet, so the row stays single-line until it does. // single-line until it does.
summary = if (customSelected) windowSummary(selected) else null, summary = if (customSelected) windowSummary(selected) else null,
position = if (customExpanded) Position.Top else positionOf(rolling.size, rollingRowCount), position = if (customExpanded) Position.Top else positionOf(rolling.size, rollingRowCount),
selected = customSelected, selected = customSelected,

View File

@@ -4,6 +4,7 @@ import androidx.compose.animation.Crossfade
import androidx.compose.animation.core.snap import androidx.compose.animation.core.snap
import androidx.compose.animation.core.tween import androidx.compose.animation.core.tween
import androidx.compose.foundation.background import androidx.compose.foundation.background
import androidx.compose.foundation.gestures.detectTapGestures
import androidx.compose.foundation.layout.Box import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.fillMaxWidth
@@ -17,7 +18,6 @@ import androidx.compose.ui.Alignment
import androidx.compose.ui.Modifier import androidx.compose.ui.Modifier
import androidx.compose.ui.draw.clip import androidx.compose.ui.draw.clip
import androidx.compose.ui.draw.clipToBounds import androidx.compose.ui.draw.clipToBounds
import androidx.compose.ui.input.pointer.PointerEventPass
import androidx.compose.ui.input.pointer.pointerInput import androidx.compose.ui.input.pointer.pointerInput
import androidx.compose.ui.res.stringResource import androidx.compose.ui.res.stringResource
import androidx.compose.ui.semantics.clearAndSetSemantics import androidx.compose.ui.semantics.clearAndSetSemantics
@@ -41,19 +41,9 @@ import kotlinx.datetime.toLocalDateTime
import kotlin.time.Clock import kotlin.time.Clock
/** /**
* The "past events" chooser for the Agenda (#69 follow-up). * The "past events" chooser for the Agenda (#69 follow-up). A stand-in agenda
* * day sits above the options and re-renders as you pick one, using the Agenda's
* "Show / dim / hide" is the case where the word cannot carry the choice — * own rows and filtering. A preview picker, so it stays open on selection.
* *how far* does dimmed fade, and what is left behind when finished events are
* hidden? So a stand-in agenda day sits above the options and re-renders as you
* pick one, using the very rows the Agenda draws: dimming is
* [AgendaEventRow]'s own `dimmed` flag, and hiding drops the row exactly as
* [de.jeanlucmakiola.calendula.ui.agenda.AgendaScreen] filters it — so the
* preview cannot claim an effect the screen doesn't have.
*
* Like the Month style and App name pickers, selecting applies immediately and
* leaves the picker open: closing would hide the thing the screen is for. Back
* exits.
*/ */
@Composable @Composable
internal fun PastEventsPicker( internal fun PastEventsPicker(
@@ -102,7 +92,6 @@ internal fun PastEventsPicker(
} else { } else {
null null
}, },
// Applies straight away; the preview above is the confirmation.
onClick = { onSelect(mode) }, onClick = { onSelect(mode) },
) )
} }
@@ -111,9 +100,8 @@ internal fun PastEventsPicker(
/** /**
* A stand-in agenda day in a given [mode]: three events on today, the first two * A stand-in agenda day in a given [mode]: three events on today, the first two
* already finished. The times are fixed rather than derived from the clock, so * finished. Times are fixed rather than clock-derived, so it reads the same at
* the preview reads the same at any hour — what varies is only what [mode] does * any hour.
* to the two finished rows.
*/ */
@Composable @Composable
private fun PastEventsPreview(mode: PastEventDisplay, modifier: Modifier = Modifier) { private fun PastEventsPreview(mode: PastEventDisplay, modifier: Modifier = Modifier) {
@@ -125,24 +113,17 @@ private fun PastEventsPreview(mode: PastEventDisplay, modifier: Modifier = Modif
stringResource(R.string.settings_past_events_sample_evening), stringResource(R.string.settings_past_events_sample_evening),
) )
val sample = remember(today, zone, titles) { samplePastDay(today, zone, titles) } val sample = remember(today, zone, titles) { samplePastDay(today, zone, titles) }
// Hiding drops the finished rows outright; showing and dimming keep them, // The same three-way split AgendaContent makes.
// and only dimming fades them — the same three-way split AgendaContent makes.
val visible = if (mode == PastEventDisplay.HIDE) sample.filterNot { it.hasPassed } else sample val visible = if (mode == PastEventDisplay.HIDE) sample.filterNot { it.hasPassed } else sample
Column( Column(
modifier = modifier modifier = modifier
.fillMaxWidth() .fillMaxWidth()
.clipToBounds() .clipToBounds()
// A preview is a picture, not a control: swallow touches before the // A picture, not a control: take the taps the fake rows would
// rows' own clickables see them, and keep it out of the reading order — // otherwise handle, and keep it out of the reading order. Taps only
// the options below already say what is selected. // consuming drags would stop the picker scrolling from here.
.pointerInput(Unit) { .pointerInput(Unit) { detectTapGestures { } }
awaitPointerEventScope {
while (true) {
awaitPointerEvent(PointerEventPass.Initial).changes.forEach { it.consume() }
}
}
}
.clearAndSetSemantics { }, .clearAndSetSemantics { },
) { ) {
AgendaDayHeader(date = today, today = today, onOpenDay = {}) AgendaDayHeader(date = today, today = today, onOpenDay = {})
@@ -163,11 +144,8 @@ private fun PastEventsPreview(mode: PastEventDisplay, modifier: Modifier = Modif
private class SampleRow(val event: EventInstance, val hasPassed: Boolean) private class SampleRow(val event: EventInstance, val hasPassed: Boolean)
/** /**
* Today's stand-in events, [titles] in order: two finished (morning, midday) and * Today's stand-in events, [titles] in order: two finished and one to come.
* one still to come. * Colours are raw ARGB, as the provider hands them out.
*
* Colours are raw ARGB on purpose — that is what the provider hands out for an
* event, so a theme token here would misrepresent what the agenda renders.
*/ */
private fun samplePastDay(today: LocalDate, zone: TimeZone, titles: List<String>): List<SampleRow> = private fun samplePastDay(today: LocalDate, zone: TimeZone, titles: List<String>): List<SampleRow> =
listOf( listOf(

View File

@@ -53,13 +53,29 @@ class RecurrenceOccurrencesTest {
byDays = setOf(DayOfWeek.FRIDAY, DayOfWeek.MONDAY), byDays = setOf(DayOfWeek.FRIDAY, DayOfWeek.MONDAY),
).upcomingOccurrences(date(2026, 7, 30), limit = 4) ).upcomingOccurrences(date(2026, 7, 30), limit = 4)
// Thursday start: the Monday of that week is already past, so the first // The Thursday start is an occurrence in its own right (RFC 5545 puts
// hit is Friday 31 Jul, then Mon/Fri of the following weeks. // DTSTART in the set), then Friday 31 Jul and Mon/Fri of the next week.
assertThat(occurrences).containsExactly( assertThat(occurrences).containsExactly(
date(2026, 7, 30),
date(2026, 7, 31), date(2026, 7, 31),
date(2026, 8, 3), date(2026, 8, 3),
date(2026, 8, 7), date(2026, 8, 7),
date(2026, 8, 10), ).inOrder()
}
@Test
fun `the start counts once even when it is also one of the picks`() {
// Thursday start with Thursday picked: the seeded DTSTART and the rule's
// own hit are the same date and must not both be listed.
val occurrences = SimpleRecurrence(
RecurrenceFreq.Weekly,
byDays = setOf(DayOfWeek.THURSDAY),
).upcomingOccurrences(date(2026, 7, 30), limit = 3)
assertThat(occurrences).containsExactly(
date(2026, 7, 30),
date(2026, 8, 6),
date(2026, 8, 13),
).inOrder() ).inOrder()
} }