From 7c57e2767d045e63dfb7e1c0a1aa94bde104d6f0 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Thu, 1 Oct 2026 17:04:35 +0200 Subject: [PATCH] fix(widget): correctness fixes from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - CompleteTaskAction only ever redrew TaskWidget by glanceId; since the callback is shared with GlanceTaskWidget's rows too, and a GlanceId doesn't say which provider it belongs to, that could push the wrong widget's RemoteViews into the other's slot. Redraws both via updateAll instead. - A single-group header is now dropped only when its label would repeat the widget's own title verbatim (e.g. the "Today" smart list showing one "Today" bucket) rather than whenever there's just one group — a real-list widget whose only bucket is "Tomorrow" needs that label, since nothing else on screen says it. - A widget's preset list, like the app's own default list, is checked against the writable/existing list set before seeding a new task — it can otherwise have been deleted or turned read-only since the widget was configured. - The "group by list" toggle is cleared on save once the selection narrows to where its row no longer shows, instead of staying stuck on with no way left to see or undo it. - GlanceTaskWidget's next-task row now shares TaskWidget's due-text logic (bare time when the day's already said), instead of always showing the full date for a same-day due. - Dropped Position/positionOf from WidgetSection now that rows are standalone pills rather than WidgetGroupedRow cards — it was dead weight computed on every redraw, and the doc comment describing the card rendering no longer matched what's there. - Collapsed Ready.singleListName into a lookup on singleListId rather than a second parallel filter switch. --- .../agendula/ui/edit/TaskEditViewModel.kt | 5 +-- .../agendula/widget/GlanceTaskWidget.kt | 9 ++--- .../agendula/widget/TaskWidget.kt | 33 +++++++++++++------ .../widget/TaskWidgetConfigActivity.kt | 5 +++ .../agendula/widget/WidgetData.kt | 27 +++++---------- .../agendula/widget/WidgetDataTest.kt | 7 ++-- 6 files changed, 48 insertions(+), 38 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/ui/edit/TaskEditViewModel.kt b/app/src/main/java/de/jeanlucmakiola/agendula/ui/edit/TaskEditViewModel.kt index 058eee1..304f4ab 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/ui/edit/TaskEditViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/ui/edit/TaskEditViewModel.kt @@ -123,10 +123,11 @@ class TaskEditViewModel @Inject constructor( val settings = settingsPrefs.settings.first() defaultFields = settings.defaultEditFields val lists = runCatching { repository.taskLists().first() }.getOrElse { emptyList() } - // A read-only or vanished default would give the task a list it + // A read-only or vanished default — or a widget's preset list, if it was + // since deleted or turned read-only — would give the task a list it // cannot be saved to. val writable = lists.filter { it.acceptsWrites } - val defaultList = presetListId + val defaultList = presetListId?.takeIf { id -> writable.any { it.id == id } } ?: settings.defaultListId?.takeIf { id -> writable.any { it.id == id } } ?: writable.firstOrNull { !it.isLocal }?.id ?: writable.firstOrNull()?.id diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/widget/GlanceTaskWidget.kt b/app/src/main/java/de/jeanlucmakiola/agendula/widget/GlanceTaskWidget.kt index 847e2d6..bc28270 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/widget/GlanceTaskWidget.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/widget/GlanceTaskWidget.kt @@ -51,8 +51,6 @@ import de.jeanlucmakiola.floret.glance.WidgetActionStyle import de.jeanlucmakiola.floret.glance.WidgetBackground import de.jeanlucmakiola.floret.glance.WidgetSurface import de.jeanlucmakiola.floret.glance.WidgetThemeMode -import de.jeanlucmakiola.floret.time.formatDateTimeCompact -import de.jeanlucmakiola.floret.time.formatTime import de.jeanlucmakiola.floret.glance.R as GlanceR import kotlinx.coroutines.flow.first @@ -199,9 +197,12 @@ private fun GlanceNextTaskRow(row: WidgetTaskRow, use24Hour: Boolean) { maxLines = 2, style = TextStyle(color = GlanceTheme.colors.onSurface, fontSize = 13.sp), ) - row.due?.let { due -> + // This widget is always Today + overdue (see the class doc), and + // GlanceSummary already labels the group "Today" — so, like the main + // Tasks widget's own rows, a same-day due only needs its bare time. + dueText(context, row, use24Hour, suppressDayLabel = true)?.let { due -> Text( - text = due.formatDateTimeCompact(row.isAllDay, use24Hour).ifBlank { due.formatTime(use24Hour) }, + text = due, maxLines = 1, style = TextStyle( color = if (row.isOverdue) GlanceTheme.colors.error else GlanceTheme.colors.onSurfaceVariant, diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/widget/TaskWidget.kt b/app/src/main/java/de/jeanlucmakiola/agendula/widget/TaskWidget.kt index 46d3381..a1e2eae 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/widget/TaskWidget.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/widget/TaskWidget.kt @@ -30,6 +30,7 @@ import androidx.glance.appwidget.lazy.LazyColumn import androidx.glance.appwidget.lazy.items import androidx.glance.appwidget.provideContent import androidx.glance.appwidget.state.getAppWidgetState +import androidx.glance.appwidget.updateAll import androidx.glance.background import androidx.glance.color.ColorProvider import androidx.glance.currentState @@ -128,7 +129,16 @@ class CompleteTaskAction : ActionCallback { runCatching { context.widgetEntryPoint().tasksRepository().setCompleted(taskId, occurrence, true) } - TaskWidget().update(context.applicationContext, glanceId) + // This callback is shared by both TaskWidget's and GlanceTaskWidget's + // rows, and glanceId only tells us the tapped widget's instance, not + // which of the two provider classes it belongs to — updating the wrong + // one by id would push this class's RemoteViews into the other's + // AppWidget slot. updateAll is class-scoped, so redrawing both is the + // immediate, correct option; it also covers every other open instance + // of either widget showing this task. + val appContext = context.applicationContext + TaskWidget().updateAll(appContext) + GlanceTaskWidget().updateAll(appContext) } companion object { @@ -170,12 +180,14 @@ private fun TaskWidgetBody( data.groupByList, data.listNames, ) { kind -> context.getString(dueBucketLabel(kind)) } - // A single group repeats what the widget's own title already - // says (e.g. the "Today" smart list showing one "Today" - // bucket) — the label only earns its place once there's more - // than one group to tell apart. - val singleGroup = allSections.count { it is WidgetSection.Header } <= 1 - val sections = if (singleGroup) allSections.filterNot { it is WidgetSection.Header } else allSections + // A lone group whose label repeats the widget's own title verbatim + // (the "Today" smart list showing one "Today" bucket) earns no + // place of its own. A lone group with a *different* label (a + // real-list widget whose only bucket is "Tomorrow") still has to + // say so — it's the one thing telling the rows apart from today. + val onlyHeader = allSections.singleOrNull { it is WidgetSection.Header } as? WidgetSection.Header + val redundantHeader = onlyHeader != null && onlyHeader.name.equals(title, ignoreCase = true) + val sections = if (redundantHeader) allSections.filterNot { it is WidgetSection.Header } else allSections LazyColumn(modifier = GlanceModifier.fillMaxSize()) { items(sections.size) { index -> when (val section = sections[index]) { @@ -199,8 +211,9 @@ private fun TaskWidgetBody( color = container, colored = rowColor == WidgetRowColor.BY_LIST, // The due-bucket header above already names the day - // (Today / Tomorrow); grouped by list, there's no - // such header, so the row needs to say it itself. + // (Today / Tomorrow) whenever one is shown — and when + // it isn't, that's only because it would have repeated + // the title, which names the same day just as well. suppressDayLabel = !data.groupByList, ) Spacer(GlanceModifier.height(8.dp)) @@ -345,7 +358,7 @@ private fun AgendaTaskRow( * "Tomorrow", or the compact date for anything else. `null` without a due date, * or for an all-day task once its day is already said by the header. */ -private fun dueText(context: Context, row: WidgetTaskRow, use24Hour: Boolean, suppressDayLabel: Boolean): String? { +internal fun dueText(context: Context, row: WidgetTaskRow, use24Hour: Boolean, suppressDayLabel: Boolean): String? { val due = row.due ?: return null val relative = when (row.dueKind) { DueKind.TODAY -> context.getString(R.string.home_due_today) diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/widget/TaskWidgetConfigActivity.kt b/app/src/main/java/de/jeanlucmakiola/agendula/widget/TaskWidgetConfigActivity.kt index 99cb4b6..68d4c5f 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/widget/TaskWidgetConfigActivity.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/widget/TaskWidgetConfigActivity.kt @@ -186,6 +186,11 @@ private fun WidgetFilterPicker( TextButton( enabled = canSave, onClick = { + // The toggle row only shows (and so can only be turned off) with + // more than one list selected; narrowing back down to one, or to + // a smart list, has to clear it here instead; otherwise it's + // stuck on with no way left to see or undo it. + if (selectedLists.size <= 1 && groupByList) onGroupByListChange(false) selectedSmart?.let { onSave(TaskFilter.Smart(it)) } ?: onSave(TaskFilter.OfLists(selectedLists)) }, diff --git a/app/src/main/java/de/jeanlucmakiola/agendula/widget/WidgetData.kt b/app/src/main/java/de/jeanlucmakiola/agendula/widget/WidgetData.kt index b2b4139..8aa1dbd 100644 --- a/app/src/main/java/de/jeanlucmakiola/agendula/widget/WidgetData.kt +++ b/app/src/main/java/de/jeanlucmakiola/agendula/widget/WidgetData.kt @@ -11,8 +11,6 @@ import de.jeanlucmakiola.agendula.domain.Task import de.jeanlucmakiola.agendula.domain.TaskFilter import de.jeanlucmakiola.agendula.domain.calendarDate import de.jeanlucmakiola.agendula.domain.comparator -import de.jeanlucmakiola.floret.components.Position -import de.jeanlucmakiola.floret.components.positionOf import de.jeanlucmakiola.floret.glance.WidgetBackground import de.jeanlucmakiola.floret.glance.WidgetThemeMode import kotlinx.coroutines.flow.Flow @@ -137,13 +135,6 @@ sealed interface TaskWidgetData { val themeMode: ThemeMode, val use24Hour: Boolean, ) : TaskWidgetData { - /** The one list's name, when [filter] names exactly one real list. */ - val singleListName: String? = when (filter) { - is TaskFilter.OfList -> listNames[filter.listId] - is TaskFilter.OfLists -> filter.listIds.singleOrNull()?.let(listNames::get) - is TaskFilter.Smart -> null - } - /** The one list's id, when [filter] names exactly one real list — seeds a widget's add button. */ val singleListId: Long? = when (filter) { is TaskFilter.OfList -> filter.listId @@ -151,32 +142,32 @@ sealed interface TaskWidgetData { is TaskFilter.Smart -> null } + /** [singleListId]'s name. */ + val singleListName: String? = singleListId?.let(listNames::get) + val overdueCount: Int = rows.count { it.isOverdue } } } -/** A run of rows in the widget's list: a task (with its [Position] within the run's card), or a run's header. */ +/** A run of rows in the widget's list: one task, or a run's header. */ internal sealed interface WidgetSection { data class Header(val key: String, val name: String) : WidgetSection - data class TaskItem(val row: WidgetTaskRow, val position: Position) : WidgetSection + data class TaskItem(val row: WidgetTaskRow) : WidgetSection } /** One headed run: its header's stable [key] and display [name], and the rows under it. */ private data class WidgetRun(val key: String, val name: String, val rows: List) -/** Each [WidgetRun] as its own [WidgetSection.Header] followed by its rows, positioned within that run's card. */ +/** Each [WidgetRun] as its own [WidgetSection.Header] followed by its rows. */ private fun headedRuns(runs: List): List = - runs.flatMap { run -> - listOf(WidgetSection.Header(run.key, run.name)) + - run.rows.mapIndexed { index, row -> WidgetSection.TaskItem(row, positionOf(index, run.rows.size)) } - } + runs.flatMap { run -> listOf(WidgetSection.Header(run.key, run.name)) + run.rows.map(WidgetSection::TaskItem) } /** * [rows] split into headed runs: one per list when [groupByList] is on * (already sorted that way by [widgetRows]), otherwise one per [DueKind] * bucket — "Overdue" / "Today" / "Tomorrow" / "Later" / "No date" — in that - * fixed order, since [rows] isn't due-sorted. Each run becomes one - * [WidgetGroupedRow][de.jeanlucmakiola.floret.glance.WidgetGroupedRow] card. + * fixed order, since [rows] isn't due-sorted. Each run becomes one header + * followed by its own standalone, independently coloured task pills. */ internal fun widgetSections( rows: List, diff --git a/app/src/test/java/de/jeanlucmakiola/agendula/widget/WidgetDataTest.kt b/app/src/test/java/de/jeanlucmakiola/agendula/widget/WidgetDataTest.kt index a79b8f6..137d9db 100644 --- a/app/src/test/java/de/jeanlucmakiola/agendula/widget/WidgetDataTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/agendula/widget/WidgetDataTest.kt @@ -8,7 +8,6 @@ import de.jeanlucmakiola.agendula.domain.TaskSorting import de.jeanlucmakiola.agendula.domain.TaskStatus import de.jeanlucmakiola.agendula.domain.allDayInstantOf import de.jeanlucmakiola.agendula.domain.testTask -import de.jeanlucmakiola.floret.components.Position import de.jeanlucmakiola.floret.glance.WidgetBackground import de.jeanlucmakiola.floret.glance.WidgetThemeMode import org.junit.jupiter.api.Test @@ -164,11 +163,11 @@ class WidgetDataTest { } @Test - fun `a bucket of one row is Alone, a bucket of several has Top Middle Bottom`() { + fun `a bucket's rows keep their own order under its one header`() { val rows = listOf(row(1, dueKind = DueKind.TODAY), row(2, dueKind = DueKind.TODAY), row(3, dueKind = DueKind.TODAY)) val sections = widgetSections(rows, groupByList = false, listNames = emptyMap()) { "x" } - val positions = sections.filterIsInstance().map { it.position } - assertThat(positions).containsExactly(Position.Top, Position.Middle, Position.Bottom).inOrder() + assertThat(sections.filterIsInstance()).hasSize(1) + assertThat(sections.filterIsInstance().map { it.row.taskId }).containsExactly(1L, 2L, 3L).inOrder() } @Test