From 7de9b2f81b946b354be81558bca4bb4b6e1ce31d Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sun, 19 Jul 2026 12:06:40 +0200 Subject: [PATCH] fix(agenda): share the day-aware time label with the widget MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Code-review follow-up. - The multi-day expansion in groupAgendaDays is shared with the agenda widget, but only the screen's summary was made day-aware — so the widget rendered the raw "start – end" on every spanned day, the very bug the screen fix cured. Hoist a pure agendaTimeLabel(event, day, zone) into the shared agenda layer and resolve strings from it in both the screen and the widget, so they label identically. (findings 1, 2) - groupAgendaDays could silently drop an instance whose clamped span was empty (firstDay > lastDay); floor lastDay at firstDay so a returned instance always surfaces on at least its first visible day. (finding 3) - agendaTimeLabel resolves the span days once instead of the summary recomputing them 2–3× per row. (finding 5) Finding 4 (within-day sort) needs no change: sorting by absolute start already places a still-running multi-day event at the top of each day it continues into, which is chronologically correct (it is ongoing from that day's midnight), and at its real start time on its first day. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../calendula/ui/agenda/AgendaScreen.kt | 22 ++---- .../calendula/ui/agenda/AgendaUiState.kt | 36 ++++++++- .../calendula/widget/agenda/AgendaWidget.kt | 29 +++++-- .../ui/agenda/AgendaTimeLabelTest.kt | 76 +++++++++++++++++++ 4 files changed, 140 insertions(+), 23 deletions(-) create mode 100644 app/src/test/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaTimeLabelTest.kt diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaScreen.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaScreen.kt index 697daf5..0f5b5c2 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaScreen.kt @@ -569,23 +569,17 @@ private fun agendaDayLabel(date: LocalDate, today: LocalDate): String { */ @Composable private fun agendaTimeSummary(event: EventInstance, day: LocalDate): String { - val allDayLabel = stringResource(R.string.event_detail_all_day) val is24Hour = LocalUse24HourFormat.current val locale = currentLocale() - val time = if (event.spansMultipleDays(zone)) { - val isFirstDay = day <= event.spanFirstDay(zone) - val isLastDay = day >= event.spanLastDay(zone) - when { - event.isAllDay -> allDayLabel - isFirstDay -> stringResource(R.string.agenda_span_starts, formatTime(event.start, is24Hour, locale)) - isLastDay -> stringResource(R.string.agenda_span_ends, formatTime(event.end, is24Hour, locale)) - else -> allDayLabel // a full middle day - } - } else if (event.isAllDay) { - allDayLabel - } else { - "${formatTime(event.start, is24Hour, locale)} – ${formatTime(event.end, is24Hour, locale)}" + val time = when (val label = agendaTimeLabel(event, day, zone)) { + AgendaTimeLabel.AllDay -> stringResource(R.string.event_detail_all_day) + is AgendaTimeLabel.Starts -> + stringResource(R.string.agenda_span_starts, formatTime(label.start, is24Hour, locale)) + is AgendaTimeLabel.Ends -> + stringResource(R.string.agenda_span_ends, formatTime(label.end, is24Hour, locale)) + is AgendaTimeLabel.Range -> + "${formatTime(label.start, is24Hour, locale)} – ${formatTime(label.end, is24Hour, locale)}" } val location = event.location?.takeIf { it.isNotBlank() } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaUiState.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaUiState.kt index 2e93d30..7f20d44 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaUiState.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaUiState.kt @@ -8,6 +8,7 @@ import kotlinx.datetime.TimeZone import kotlinx.datetime.plus import kotlinx.datetime.toLocalDateTime import kotlin.time.Duration.Companion.milliseconds +import kotlin.time.Instant /** * The zone the event's calendar dates live in. Timed events are resolved in the @@ -36,6 +37,36 @@ fun EventInstance.spanLastDay(zone: TimeZone): LocalDate { fun EventInstance.spansMultipleDays(zone: TimeZone): Boolean = spanFirstDay(zone) != spanLastDay(zone) +/** + * What an agenda row's time line should convey for an event on a given day — + * the part of a multi-day span that [day] falls in. Pure and shared so the + * agenda screen and the agenda widget label multi-day events identically; each + * surface only formats the instants into its own locale/24h string. + */ +sealed interface AgendaTimeLabel { + /** An all-day event, or a whole in-between day of a multi-day span. */ + data object AllDay : AgendaTimeLabel + /** The first day of a multi-day timed event: when it begins. */ + data class Starts(val start: Instant) : AgendaTimeLabel + /** The last day of a multi-day timed event: when it ends. */ + data class Ends(val end: Instant) : AgendaTimeLabel + /** A single-day timed event: its start–end range. */ + data class Range(val start: Instant, val end: Instant) : AgendaTimeLabel +} + +/** The [AgendaTimeLabel] for [event] as it appears on [day], resolved in [zone]. */ +fun agendaTimeLabel(event: EventInstance, day: LocalDate, zone: TimeZone): AgendaTimeLabel { + if (event.isAllDay) return AgendaTimeLabel.AllDay + val firstDay = event.spanFirstDay(zone) + val lastDay = event.spanLastDay(zone) + return when { + firstDay == lastDay -> AgendaTimeLabel.Range(event.start, event.end) + day <= firstDay -> AgendaTimeLabel.Starts(event.start) + day >= lastDay -> AgendaTimeLabel.Ends(event.end) + else -> AgendaTimeLabel.AllDay // a full in-between day + } +} + /** One calendar day with at least one event, for the agenda list. */ data class AgendaDay( val date: LocalDate, @@ -63,7 +94,10 @@ fun groupAgendaDays( val byDay = sortedMapOf>() for (instance in instances) { val firstDay = instance.spanFirstDay(zone).coerceAtLeast(anchor) - val lastDay = instance.spanLastDay(zone).coerceAtMost(windowEnd) + // Never below firstDay: an instance the query returned always surfaces on + // at least its first visible day, even if its end resolves earlier (e.g. a + // zero-length or boundary instant) — otherwise the loop would drop it. + val lastDay = instance.spanLastDay(zone).coerceAtMost(windowEnd).coerceAtLeast(firstDay) var day = firstDay while (day <= lastDay) { byDay.getOrPut(day) { mutableListOf() }.add(instance) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/widget/agenda/AgendaWidget.kt b/app/src/main/java/de/jeanlucmakiola/calendula/widget/agenda/AgendaWidget.kt index 2b048fc..198a21a 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/widget/agenda/AgendaWidget.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/widget/agenda/AgendaWidget.kt @@ -49,6 +49,8 @@ import de.jeanlucmakiola.calendula.data.prefs.parsePastEventDisplay import de.jeanlucmakiola.calendula.domain.EventInstance import de.jeanlucmakiola.calendula.domain.hasEnded import de.jeanlucmakiola.calendula.ui.agenda.AgendaRange +import de.jeanlucmakiola.calendula.ui.agenda.AgendaTimeLabel +import de.jeanlucmakiola.calendula.ui.agenda.agendaTimeLabel import de.jeanlucmakiola.calendula.ui.agenda.anchorTodayIfMissing import de.jeanlucmakiola.calendula.ui.agenda.dayCount import de.jeanlucmakiola.calendula.ui.agenda.parseAgendaRange @@ -127,7 +129,7 @@ class RefreshAgendaAction : ActionCallback { /** Flat row model so the [LazyColumn] can mix day headers and events. */ private sealed interface AgendaRow { data class Header(val date: LocalDate, val today: LocalDate) : AgendaRow - data class Event(val event: EventInstance) : AgendaRow + data class Event(val date: LocalDate, val event: EventInstance) : AgendaRow /** "Nothing left today" line under an anchored, event-less today (#35). */ data class Placeholder(val date: LocalDate) : AgendaRow } @@ -185,7 +187,7 @@ private fun AgendaWidgetBody(data: AgendaWidgetData, dark: Boolean) { if (day.events.isEmpty()) { add(AgendaRow.Placeholder(day.date)) } else { - day.events.forEach { add(AgendaRow.Event(it)) } + day.events.forEach { add(AgendaRow.Event(day.date, it)) } } } } @@ -196,6 +198,7 @@ private fun AgendaWidgetBody(data: AgendaWidgetData, dark: Boolean) { is AgendaRow.Placeholder -> PlaceholderRow(row.date) is AgendaRow.Event -> EventRow( event = row.event, + day = row.date, dark = dark, soften = data.soften, is24Hour = data.is24Hour, @@ -312,6 +315,7 @@ private fun PlaceholderRow(date: LocalDate) { @Composable private fun EventRow( event: EventInstance, + day: LocalDate, dark: Boolean, soften: Boolean, is24Hour: Boolean, @@ -357,7 +361,7 @@ private fun EventRow( style = TextStyle(color = titleColor, fontSize = 14.sp), ) Text( - text = eventTimeSummary(context, event, is24Hour), + text = eventTimeSummary(context, event, day, is24Hour), maxLines = 1, style = TextStyle(color = GlanceTheme.colors.onSurfaceVariant, fontSize = 12.sp), ) @@ -396,11 +400,20 @@ private fun agendaDayLabel(context: Context, date: LocalDate, today: LocalDate): return if (relative != null) "$relative · $formatted" else formatted } -private fun eventTimeSummary(context: Context, event: EventInstance, is24Hour: Boolean): String { - val time = if (event.isAllDay) { - context.getString(R.string.event_detail_all_day) - } else { - "${formatTime(event.start, is24Hour)} – ${formatTime(event.end, is24Hour)}" +private fun eventTimeSummary( + context: Context, + event: EventInstance, + day: LocalDate, + is24Hour: Boolean, +): String { + val time = when (val label = agendaTimeLabel(event, day, zone())) { + AgendaTimeLabel.AllDay -> context.getString(R.string.event_detail_all_day) + is AgendaTimeLabel.Starts -> + context.getString(R.string.agenda_span_starts, formatTime(label.start, is24Hour)) + is AgendaTimeLabel.Ends -> + context.getString(R.string.agenda_span_ends, formatTime(label.end, is24Hour)) + is AgendaTimeLabel.Range -> + "${formatTime(label.start, is24Hour)} – ${formatTime(label.end, is24Hour)}" } val location = event.location?.takeIf { it.isNotBlank() } return if (location != null) "$time · $location" else time diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaTimeLabelTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaTimeLabelTest.kt new file mode 100644 index 0000000..25c71ed --- /dev/null +++ b/app/src/test/java/de/jeanlucmakiola/calendula/ui/agenda/AgendaTimeLabelTest.kt @@ -0,0 +1,76 @@ +package de.jeanlucmakiola.calendula.ui.agenda + +import com.google.common.truth.Truth.assertThat +import de.jeanlucmakiola.calendula.domain.EventInstance +import kotlinx.datetime.LocalDate +import kotlinx.datetime.LocalDateTime +import kotlinx.datetime.TimeZone +import kotlinx.datetime.toInstant +import kotlin.time.Instant +import org.junit.jupiter.api.Test + +class AgendaTimeLabelTest { + + private val zone = TimeZone.UTC + + private fun at(y: Int, mo: Int, d: Int, h: Int = 0, min: Int = 0): Instant = + LocalDateTime(y, mo, d, h, min).toInstant(zone) + + private fun event(start: Instant, end: Instant, isAllDay: Boolean = false) = EventInstance( + instanceId = 1, + eventId = 1, + calendarId = 1, + title = "e", + start = start, + end = end, + isAllDay = isAllDay, + color = 0, + location = null, + ) + + private fun labelOn(y: Int, mo: Int, d: Int, event: EventInstance) = + agendaTimeLabel(event, LocalDate(y, mo, d), zone) + + @Test + fun `a single-day timed event is a start-end range`() { + val e = event(at(2026, 7, 2, 12, 0), at(2026, 7, 2, 13, 0)) + + assertThat(labelOn(2026, 7, 2, e)) + .isEqualTo(AgendaTimeLabel.Range(e.start, e.end)) + } + + @Test + fun `a single-day all-day event is all-day`() { + val e = event(at(2026, 7, 2), at(2026, 7, 3), isAllDay = true) + + assertThat(labelOn(2026, 7, 2, e)).isEqualTo(AgendaTimeLabel.AllDay) + } + + @Test + fun `a multi-day timed event names the start, middle, and end days`() { + val e = event(at(2026, 7, 1, 14, 0), at(2026, 7, 4, 10, 0)) + + assertThat(labelOn(2026, 7, 1, e)).isEqualTo(AgendaTimeLabel.Starts(e.start)) + assertThat(labelOn(2026, 7, 2, e)).isEqualTo(AgendaTimeLabel.AllDay) + assertThat(labelOn(2026, 7, 3, e)).isEqualTo(AgendaTimeLabel.AllDay) + assertThat(labelOn(2026, 7, 4, e)).isEqualTo(AgendaTimeLabel.Ends(e.end)) + } + + @Test + fun `a multi-day all-day event is all-day on every day`() { + val e = event(at(2026, 7, 2), at(2026, 7, 5), isAllDay = true) + + assertThat(labelOn(2026, 7, 2, e)).isEqualTo(AgendaTimeLabel.AllDay) + assertThat(labelOn(2026, 7, 3, e)).isEqualTo(AgendaTimeLabel.AllDay) + assertThat(labelOn(2026, 7, 4, e)).isEqualTo(AgendaTimeLabel.AllDay) + } + + @Test + fun `an event begun before the shown day is not labelled as starting`() { + // Runs 29 Jun 09:00 → 2 Jul 09:00; on 1 Jul it is mid-span, on 2 Jul it ends. + val e = event(at(2026, 6, 29, 9, 0), at(2026, 7, 2, 9, 0)) + + assertThat(labelOn(2026, 7, 1, e)).isEqualTo(AgendaTimeLabel.AllDay) + assertThat(labelOn(2026, 7, 2, e)).isEqualTo(AgendaTimeLabel.Ends(e.end)) + } +}