From 51a31e060a8cd9a6f222a85c77b44b3f51a76c22 Mon Sep 17 00:00:00 2001 From: Jean-Luc Makiola Date: Sat, 8 Aug 2026 19:14:47 +0200 Subject: [PATCH] Fix review findings in search result actions (#80) - don't request focus while the selection bar hides the field (crash on rotate) - rank and date all-day hits by their own day, not by UTC - query non-Latin tokens case-by-case instead of wildcarding them into a scan - clear a stale delete receipt when search is reopened - keep the deleted count when write access is revoked mid-batch - match on the raw title, not the untitled placeholder --- .../data/calendar/CalendarDataSource.kt | 61 ++++++---- .../data/calendar/CalendarRepository.kt | 5 +- .../calendula/data/calendar/Projections.kt | 3 +- .../calendula/domain/EventSearch.kt | 105 +++++++++--------- .../jeanlucmakiola/calendula/domain/Models.kt | 5 +- .../calendula/ui/search/SearchScreen.kt | 76 +++++++------ .../calendula/ui/search/SearchViewModel.kt | 24 ++-- app/src/main/res/values/strings.xml | 1 + .../data/calendar/FakeCalendarDataSource.kt | 6 +- .../calendula/domain/EventSearchTest.kt | 62 +++++++++++ .../ui/search/SearchViewModelTest.kt | 24 ++++ 11 files changed, 232 insertions(+), 140 deletions(-) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt index 892977d..1b8507f 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarDataSource.kt @@ -593,15 +593,19 @@ class AndroidCalendarDataSource @Inject constructor( ensureObserversRegistered() val tokens = EventSearch.tokenize(query).take(MAX_SEARCH_TOKENS) if (tokens.isEmpty()) return emptyList() - // Every token has to appear somewhere, so the groups are ANDed. This is - // only a pre-filter — EventSearch re-checks each token with proper - // case folding, and re-checks the tokens dropped by the cap above. - val match = tokens.joinToString(" AND ") { - "(${CalendarContract.Events.TITLE} LIKE ? ESCAPE '\\' OR " + - "${CalendarContract.Events.DESCRIPTION} LIKE ? ESCAPE '\\' OR " + - "${CalendarContract.Events.EVENT_LOCATION} LIKE ? ESCAPE '\\')" + // Only a pre-filter: EventSearch re-checks every token, including the + // ones the cap above dropped. + val patterns = tokens.map { likePatterns(it) } + val match = patterns.joinToString(" AND ") { variants -> + variants.joinToString(" OR ", prefix = "(", postfix = ")") { + "${CalendarContract.Events.TITLE} LIKE ? ESCAPE '\\' OR " + + "${CalendarContract.Events.DESCRIPTION} LIKE ? ESCAPE '\\' OR " + + "${CalendarContract.Events.EVENT_LOCATION} LIKE ? ESCAPE '\\'" + } } - val args = tokens.flatMap { token: String -> List(3) { likePattern(token) } }.toTypedArray() + val args = patterns + .flatMap { variants -> variants.flatMap { pattern -> List(3) { pattern } } } + .toTypedArray() val selection = "($match) AND " + "${CalendarContract.Events.DELETED} = 0 AND " + "${CalendarContract.Events.ORIGINAL_ID} IS NULL" @@ -617,9 +621,8 @@ class AndroidCalendarDataSource @Inject constructor( while (c.moveToNext()) { val description = reader.getString(SearchProjection.IDX_DESCRIPTION) val base = reader.toSearchResult() ?: continue - // A recurring master's DTSTART is the series start; show its - // nearest occurrence instead so the date is the one the user - // actually cares about (and sorting reflects it). + // A recurring master's DTSTART is the series start; date the row + // by its nearest occurrence instead. val event = if (base.isRecurring) { nearestOccurrenceMillis(base.eventId)?.let { (begin, end) -> base.copy( @@ -630,28 +633,40 @@ class AndroidCalendarDataSource @Inject constructor( } else { base } - out += SearchCandidate(event = event, description = description) + // The raw title, so matching can't catch on the placeholder an + // untitled event is drawn with. + out += SearchCandidate( + event = event, + description = description, + title = reader.getString(SearchProjection.IDX_TITLE), + ) } out } ?: emptyList() } /** - * One token as a `LIKE` pattern. SQLite folds case for ASCII only, so a - * non-ASCII cased letter is substituted with the single-character wildcard - * `_` — "ärzte" queries as `%_rzte%` and so still reaches "Ärzte". That - * matches more than it should on purpose; [EventSearch] does the real - * comparison. Uncased scripts stay literal, keeping the filter selective. + * One token as the `LIKE` patterns to OR together. SQLite folds case for + * ASCII only, so a token with cased non-ASCII letters is also queried lower-, + * upper- and title-cased, which is how "ärzte" reaches "Ärzte". Wildcarding + * those letters away instead would make "москва" `%______%` — a full scan. */ + private fun likePatterns(token: String): List { + if (token.none { it.code > 127 && (it.isUpperCase() || it.isLowerCase()) }) { + return listOf(likePattern(token)) + } + val lower = token.lowercase() + return listOf(token, lower, token.uppercase(), lower.replaceFirstChar(Char::uppercaseChar)) + .distinct() + .map(::likePattern) + } + private fun likePattern(token: String): String { val sb = StringBuilder("%") for (c in token) { - when { - c.code > 127 && (c.isUpperCase() || c.isLowerCase()) -> sb.append('_') - // A literal wildcard from the query matches itself. - c == '%' || c == '_' || c == '\\' -> sb.append('\\').append(c) - else -> sb.append(c) - } + // A literal wildcard from the query matches itself. + if (c == '%' || c == '_' || c == '\\') sb.append('\\') + sb.append(c) } return sb.append('%').toString() } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt index 0599d78..b47c83f 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/CalendarRepository.kt @@ -19,9 +19,8 @@ interface CalendarRepository { /** * Candidate matches for [query] with hidden calendars removed; empty when - * [query] is blank. Searches the whole history/future (see - * [CalendarDataSource.searchEvents]) and leaves ranking to - * [de.jeanlucmakiola.calendula.domain.EventSearch]. + * [query] is blank. Searches the whole history/future and leaves matching + * and ranking to [de.jeanlucmakiola.calendula.domain.EventSearch]. */ suspend fun searchEvents(query: String): List diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt index 290b930..ef58672 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/data/calendar/Projections.kt @@ -180,8 +180,7 @@ internal object SearchProjection { // display its nearest occurrence, not the series-start DTSTART. CalendarContract.Events.RRULE, CalendarContract.Events.RDATE, - // Ranked and excerpted, not just filtered on: a description-only hit has - // to be able to show what it matched. + // Excerpted, not just filtered on: a hit has to show what it matched. CalendarContract.Events.DESCRIPTION, ) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/domain/EventSearch.kt b/app/src/main/java/de/jeanlucmakiola/calendula/domain/EventSearch.kt index c77d811..963fd82 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/domain/EventSearch.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/domain/EventSearch.kt @@ -1,8 +1,11 @@ package de.jeanlucmakiola.calendula.domain +import kotlinx.datetime.DateTimeUnit import kotlinx.datetime.LocalDate import kotlinx.datetime.TimeZone +import kotlinx.datetime.atStartOfDayIn import kotlinx.datetime.number +import kotlinx.datetime.plus import kotlinx.datetime.toLocalDateTime import kotlin.time.Instant @@ -10,6 +13,9 @@ import kotlin.time.Instant data class SearchCandidate( val event: EventInstance, val description: String? = null, + /** Null when the event has none: [EventInstance.title] is then a display + * placeholder, which the query must not catch on. */ + val title: String? = event.title, ) /** A matched run inside one of a hit's texts, for highlighting. */ @@ -21,10 +27,7 @@ data class DescriptionSnippet( val spans: List, ) -/** - * One ranked search result: the event, plus where the query matched so the row - * can show why it is in the list. - */ +/** One result: the event, plus where the query matched it. */ data class SearchHit( val event: EventInstance, val titleSpans: List = emptyList(), @@ -42,10 +45,7 @@ data class SearchMonth( val hits: List, ) -/** - * A finished search: a calendar of hits, plus the ones that only turned up - * inside a description and would otherwise clutter it. - */ +/** A finished search: a calendar of hits, plus the description-only ones. */ data class SearchResults( val months: List, val inDescriptions: List, @@ -59,22 +59,15 @@ data class SearchResults( /** * Turning a typed query into the results list. * - * Matching is token-wise: the query splits on whitespace and an event has to - * satisfy *every* token, each in any of title / location / description. Case is - * folded through Kotlin's [String.indexOf] rather than SQL's `LIKE`, which folds - * ASCII only — "ärzte" has to find "Ärzte". + * An event has to carry *every* whitespace-separated token, each in any of + * title / location / description; case is folded in Kotlin rather than by SQL's + * ASCII-only `LIKE`, so "ärzte" finds "Ärzte". * - * Results are a calendar rather than a ranking: hits are grouped by the month - * they fall in, the current month first, then the months ahead, then the months - * behind counting backwards. Inside a month what is still to come reads - * forwards and what has passed follows it, so an event tomorrow sits above one - * from yesterday. The day is the boundary, not the moment — an event that ended - * at 19:00 is still today's business and keeps its place in today's order. - * - * An event the query only reached through its description is held back in - * [SearchResults.inDescriptions]: those are the matches that made search - * confusing when a stray word in a holiday's notes landed among events actually - * named for the query. + * Results are a calendar, not a ranking: months from the current one, then + * forwards, then backwards, and inside a month what is still to come before + * what has passed. The day is the boundary, not the moment. A hit the query + * only reached through its description is held back in + * [SearchResults.inDescriptions] rather than dated among the real ones. */ object EventSearch { @@ -86,20 +79,15 @@ object EventSearch { private val WHITESPACE = Regex("\\s+") - /** - * The query as match tokens: whitespace-separated, blanks dropped. Shared - * with the data layer so its SQL pre-filter tokenises identically. - */ + /** Shared with the data layer so its SQL pre-filter tokenises identically. */ fun tokenize(query: String): List = query.trim().split(WHITESPACE).filter { it.isNotEmpty() } /** * Search [candidates] for [query], dropping any that don't carry every token. - * [now] marks an individual event as over; [todayStart] is where an event - * stops being current, so everything still on today's page keeps its place - * among the upcoming ones. [zone] resolves each event to a calendar month — - * via [spanFirstDay], so an all-day event doesn't slip into the month before - * it west of UTC (#82). + * [now] marks a hit as over, [todayStart] is where it stops being current. + * [zone] places each event on the calendar through the span rule, so an + * all-day event keeps its own day whichever side of UTC we are on (#82). */ fun search( candidates: List, @@ -111,15 +99,17 @@ object EventSearch { val tokens = tokenize(query) if (tokens.isEmpty()) return SearchResults(emptyList(), emptyList()) - val matched = candidates.mapNotNull { candidate -> match(candidate, tokens, now) } - val order = compareBy { if (it.event.end >= todayStart) 0 else 1 } + val today = todayStart.toLocalDateTime(zone).date + val matched = candidates.mapNotNull { candidate -> match(candidate, tokens, now, zone) } + val upcoming = { m: Matched -> m.event.spanLastDay(zone) >= today } + val order = compareBy { if (upcoming(it)) 0 else 1 } .thenComparator { a, b -> // What is left reads forwards; what is behind us reads backwards // from today, nearest first. - if (a.event.end >= todayStart) { - a.event.start.compareTo(b.event.start) + if (upcoming(a)) { + a.event.dayStart(zone).compareTo(b.event.dayStart(zone)) } else { - b.event.start.compareTo(a.event.start) + b.event.dayStart(zone).compareTo(a.event.dayStart(zone)) } } @@ -130,11 +120,7 @@ object EventSearch { ) } - /** - * The dated hits as months: the one we are in, then forwards, then backwards. - * Each month keeps the order it was handed, so a month holding both sides of - * today lists what is left before what has passed. - */ + /** Months from the current one, then forwards, then backwards. */ private fun groupByMonth( dated: List, zone: TimeZone, @@ -171,27 +157,26 @@ object EventSearch { candidate: SearchCandidate, tokens: List, now: Instant, + zone: TimeZone, ): Matched? { - val title = candidate.event.title + val title = candidate.title?.takeIf { it.isNotBlank() } val location = candidate.event.location?.takeIf { it.isNotBlank() } - // Collapsed once so a multi-line description matches and snippets the - // same way it will be drawn. + // Collapsed so a multi-line description matches the way it is drawn. val description = candidate.description ?.replace(WHITESPACE, " ") ?.trim() ?.takeIf { it.isNotEmpty() } - // Mid-word counts as a match — German compounds mean "termin" has to keep - // finding "Zahnarzttermin". + // Mid-word counts: "termin" has to keep finding "Zahnarzttermin". val present = { token: String -> - title.contains(token, ignoreCase = true) || + title?.contains(token, ignoreCase = true) == true || location?.contains(token, ignoreCase = true) == true || description?.contains(token, ignoreCase = true) == true } if (!tokens.all(present)) return null val descriptionSpans = description?.let { spansIn(it, tokens) }.orEmpty() - val titleSpans = spansIn(title, tokens) + val titleSpans = title?.let { spansIn(it, tokens) }.orEmpty() val locationSpans = location?.let { spansIn(it, tokens) }.orEmpty() return Matched( hit = SearchHit( @@ -201,10 +186,10 @@ object EventSearch { descriptionSnippet = description ?.takeIf { descriptionSpans.isNotEmpty() } ?.let { snippet(it, descriptionSpans) }, - isPast = candidate.event.end < now, + isPast = candidate.event.dayEnd(zone) < now, ), - // Nothing in the name or the place the query could have caught on — - // the only reason this event is here is buried in its notes. + // Nothing in the name or the place caught the query: it is only here + // for something in its notes. descriptionOnly = titleSpans.isEmpty() && locationSpans.isEmpty(), ) } @@ -234,9 +219,8 @@ object EventSearch { } /** - * A window of [description] around its first match, elided at both ends, with - * the spans rebased onto the window. Anything that falls outside is dropped — - * the row shows one excerpt, not the whole note. + * A window of [description] around its first match, elided at both ends, + * with the spans rebased onto it and any falling outside dropped. */ private fun snippet(description: String, spans: List): DescriptionSnippet { if (description.length <= SNIPPET_LENGTH) { @@ -258,4 +242,15 @@ object EventSearch { private data class Matched(val hit: SearchHit, val descriptionOnly: Boolean) { val event: EventInstance get() = hit.event } + + /** + * The event's first day as an instant in [zone]. An all-day event sits at + * UTC midnight, so its raw start would rank it on a day it isn't drawn on. + */ + private fun EventInstance.dayStart(zone: TimeZone): Instant = + if (isAllDay) spanFirstDay(zone).atStartOfDayIn(zone) else start + + /** The moment the event's last day is over in [zone], exclusive. */ + private fun EventInstance.dayEnd(zone: TimeZone): Instant = + if (isAllDay) spanLastDay(zone).plus(1, DateTimeUnit.DAY).atStartOfDayIn(zone) else end } diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt b/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt index 647233a..4021109 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/domain/Models.kt @@ -63,9 +63,8 @@ data class EventInstance( val color: Int, val location: String?, /** - * Whether this row's event carries a recurrence rule. Only search results - * (which read the series master) fill this in — the Instances query already - * yields one row per occurrence, so it stays false there. + * Only search results, which read the series master, fill this in — the + * Instances query already yields one row per occurrence. */ val isRecurring: Boolean = false, ) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/search/SearchScreen.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/search/SearchScreen.kt index 123dc65..508dcbe 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/search/SearchScreen.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/search/SearchScreen.kt @@ -114,11 +114,9 @@ import java.time.format.FormatStyle private const val CHIP_MILLIS = 4_000L /** - * Full-text event search (top-bar entry). Type a query → matching events - * (title / location / description) across the whole calendar, newest-relevant - * first; tap a result to open its detail. Long-press a result to enter selection - * mode and delete several at once (#80). A full-screen overlay hosted by - * [de.jeanlucmakiola.calendula.ui.CalendarHost]. + * Full-text event search over title / location / description: tap a result to + * open its detail, long-press to select several and delete them at once (#80). + * A full-screen overlay hosted by [de.jeanlucmakiola.calendula.ui.CalendarHost]. */ @OptIn(ExperimentalMaterial3Api::class) @Composable @@ -138,12 +136,20 @@ fun SearchScreen( val inSelection = selection.isNotEmpty() var showDeleteDialog by remember { mutableStateOf(false) } - // Each fresh open starts blank and straight into typing. The ViewModel is - // activity-scoped so it outlives the overlay; clearing on (re)enter is what - // resets a previous search. Peeking a result doesn't re-run this (the screen - // stays composed under the detail), so backing out keeps the query. + // The ViewModel is activity-scoped, so a re-enter is what resets the last + // search, its selection and any delete receipt still up. Peeking a result + // doesn't re-run this — the screen stays composed under the detail. LaunchedEffect(Unit) { viewModel.setQuery("") + viewModel.clearSelection() + viewModel.consumeDeleteResult() + } + // The field only exists out of selection mode, so a recreation that lands + // mid-selection has to wait for the clear above before it can be focused. + var focusPending by remember { mutableStateOf(true) } + LaunchedEffect(inSelection) { + if (inSelection || !focusPending) return@LaunchedEffect + focusPending = false focusRequester.requestFocus() keyboard?.show() } @@ -170,9 +176,9 @@ fun SearchScreen( } } - // Back leaves selection mode before it leaves the screen — and without the - // predictive-back scale-out, which would read as closing search only to have - // it snap back. The two handlers are mutually exclusive on [inSelection]. + // Back leaves selection mode before it leaves the screen, and without the + // predictive-back scale-out — that would read as search closing and snapping + // back. The two handlers are mutually exclusive on [inSelection]. BackHandler(enabled = inSelection) { viewModel.clearSelection() } Scaffold( @@ -193,9 +199,8 @@ fun SearchScreen( ) }, ) { padding -> - // imePadding shrinks the content by the keyboard, so the centered - // idle/empty message re-centres in the space above it (and the results - // list lifts clear of the keyboard too). + // imePadding so the idle/empty message re-centres above the keyboard and + // the results list lifts clear of it. Box( modifier = Modifier .fillMaxSize() @@ -271,9 +276,8 @@ fun SearchScreen( } /** - * The search field and the contextual selection bar as one bar: entering or - * leaving selection mode keeps the bar itself in place, eases its container - * colour and fades its slots through, rather than cutting between two bars. + * The search field and the contextual selection bar as one bar: the bar stays + * in place across the swap, easing its colour and fading its slots through. */ @OptIn(ExperimentalMaterial3Api::class, ExperimentalMaterial3ExpressiveApi::class) @Composable @@ -298,8 +302,8 @@ private fun SearchTopBar( animationSpec = MaterialTheme.motionScheme.fastEffectsSpec(), label = "search-top-bar-color", ) - // Held past the selection emptying so the count has something to draw while - // it fades out — by then the set is already at zero. + // Held past the selection emptying, so the count has something to draw while + // it fades out. var lastCount by remember { mutableIntStateOf(0) } if (selectedCount > 0) lastCount = selectedCount @@ -408,8 +412,7 @@ private fun SearchResults( ) { LazyColumn( modifier = Modifier.fillMaxSize(), - // No horizontal inset here: GroupedRow already insets itself, and adding - // another 16dp made the results narrower than every other list. + // No horizontal inset: GroupedRow already insets itself. contentPadding = PaddingValues(top = 8.dp, bottom = 96.dp), ) { results.results.months.forEach { month -> @@ -418,8 +421,7 @@ private fun SearchResults( } hitRows(month.hits, results, selection, inSelection, onEventClick, onToggle) } - // Held back from the calendar above: these matched nothing in the name or - // the place, only somewhere in the notes. + // Held back from the calendar above: matched only in the notes. if (results.results.inDescriptions.isNotEmpty()) { stickyHeader(key = "in-descriptions") { SearchSectionHeader(text = stringResource(R.string.search_in_descriptions)) @@ -514,8 +516,8 @@ private fun SearchResultRow( color = if (selected) Color.Unspecified else MaterialTheme.colorScheme.primary, ) GroupedRow( - // Faded like a past event anywhere else in the app: search reaches back - // through the whole history, so an old hit has to read as over. + // Faded like a past event anywhere else in the app — search reaches back + // through the whole history. modifier = if (hit.isPast) modifier.alpha(EventDimAlpha) else modifier, title = marked(event.title, hit.titleSpans, highlight), summary = searchSummary(hit, highlight), @@ -547,9 +549,8 @@ private fun SearchResultRow( } /** - * The batch's receipt: how many events went, and whether any refused. Sits on - * the FAB band so the shortened list stays visible behind it. Deletes aren't - * reversible through the provider, so this carries no Undo. + * The batch's receipt: how many went, and whether any refused. No Undo — the + * provider can't put a deleted event back. */ @Composable private fun DeleteOutcomeChip( @@ -570,7 +571,11 @@ private fun DeleteOutcomeChip( deleteState.deleted, ) } - BulkDeleteUiState.NeedsPermission -> stringResource(R.string.event_delete_write_denied) + is BulkDeleteUiState.NeedsPermission -> if (deleteState.deleted > 0) { + stringResource(R.string.search_delete_denied_partial, deleteState.deleted) + } else { + stringResource(R.string.event_delete_write_denied) + } else -> null } // Held past the state being consumed so the chip has something to draw while @@ -606,10 +611,8 @@ private fun marked(text: String, spans: List, style: SpanStyle): Anno } /** - * "Wed, 17 Jun 2026 · 09:00 · Office" — date, then time (or All day), then - * location, with a second line excerpting the description when that is where the - * query matched. Without it a description-only hit reads as though it arrived - * from nowhere. + * "Wed, 17 Jun 2026 · 09:00 · Office", with a second line excerpting the + * description when that is where the query matched. */ @Composable private fun searchSummary(hit: SearchHit, highlight: SpanStyle): AnnotatedString { @@ -619,9 +622,8 @@ private fun searchSummary(hit: SearchHit, highlight: SpanStyle): AnnotatedString val start = remember(event.start, zone) { JavaInstant.ofEpochMilli(event.start.toEpochMilliseconds()).atZone(zone) } - // From the shared span rule, not [start]: an all-day event sits at UTC - // midnight and would name the day before west of UTC (#82). The clock time - // below stays device-zone — it is only rendered for timed events. + // From the span rule, not [start]: an all-day event sits at UTC midnight and + // would name the day before west of UTC (#82). val dateText = remember(event.start, event.end, event.isAllDay, locale) { DateTimeFormatter.ofLocalizedDate(FormatStyle.MEDIUM).withLocale(locale) .format(event.spanFirstDay(TimeZone.currentSystemDefault()).toJavaLocalDate()) diff --git a/app/src/main/java/de/jeanlucmakiola/calendula/ui/search/SearchViewModel.kt b/app/src/main/java/de/jeanlucmakiola/calendula/ui/search/SearchViewModel.kt index 5eacddf..d410f74 100644 --- a/app/src/main/java/de/jeanlucmakiola/calendula/ui/search/SearchViewModel.kt +++ b/app/src/main/java/de/jeanlucmakiola/calendula/ui/search/SearchViewModel.kt @@ -47,11 +47,7 @@ sealed interface SearchUiState { /** Matches by month — see [de.jeanlucmakiola.calendula.domain.EventSearch]. */ data class Results( val results: SearchResults, - /** - * Calendars among the results that can't be written to (WebCal, birthday - * mirrors, …). Their rows are excluded from selection rather than failing - * at delete time. - */ + /** Rows from these are kept out of selection rather than failing later. */ val readOnlyCalendarIds: Set = emptySet(), ) : SearchUiState { val events: List get() = results.allHits.map { it.event } @@ -70,8 +66,8 @@ sealed interface BulkDeleteUiState { /** Terminal: [deleted] events went, [failed] wouldn't. */ data class Done(val deleted: Int, val failed: Int) : BulkDeleteUiState - /** WRITE_CALENDAR was revoked mid-flight; nothing further was attempted. */ - data object NeedsPermission : BulkDeleteUiState + /** WRITE_CALENDAR was revoked after [deleted] went; the rest wasn't tried. */ + data class NeedsPermission(val deleted: Int) : BulkDeleteUiState } @OptIn(ExperimentalCoroutinesApi::class, FlowPreview::class) @@ -148,10 +144,7 @@ class SearchViewModel @Inject constructor( _selection.value = emptySet() } - /** - * Whether the current selection contains a recurring event, i.e. whether the - * batch needs a [RecurringWriteScope] decision before it can run. - */ + /** Whether the batch needs a [RecurringWriteScope] decision before it runs. */ fun selectionHasRecurring(): Boolean { val results = state.value as? SearchUiState.Results ?: return false val picked = _selection.value @@ -159,10 +152,9 @@ class SearchViewModel @Inject constructor( } /** - * Delete every selected event. [scope] applies to the recurring ones only — - * a one-off is always removed whole, whatever the batch decision was. Runs - * one event at a time so a single failure doesn't take the rest with it; - * the tally lands in [deleteState]. + * Delete every selected event. [scope] reaches the recurring ones only; a + * one-off always goes whole. One at a time, so a single failure doesn't take + * the rest with it — the tally lands in [deleteState]. */ fun deleteSelected(scope: RecurringWriteScope) { if (_deleteState.value == BulkDeleteUiState.Deleting) return @@ -192,7 +184,7 @@ class SearchViewModel @Inject constructor( _selection.value = emptySet() _reload.value += 1 _deleteState.value = if (denied) { - BulkDeleteUiState.NeedsPermission + BulkDeleteUiState.NeedsPermission(deleted = deleted) } else { BulkDeleteUiState.Done(deleted = deleted, failed = failed) } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 51ed4ff..66c5fae 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -336,6 +336,7 @@ %d events deleted %1$d deleted, %2$d couldn\'t be + %1$d deleted, then write access was withdrawn Upcoming diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt index 682f253..a66fd40 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/data/calendar/FakeCalendarDataSource.kt @@ -30,6 +30,10 @@ internal class FakeCalendarDataSource : CalendarDataSource { var existingUidsResult: Set = emptySet() /** Set to make the next write call throw. */ var writeError: Exception? = null + + /** Deletes let through before [writeError] applies, for part-way failures. */ + var deletesBeforeError: Int = 0 + private var deletes = 0 /** Id returned by the next [insertEvent]. */ var nextInsertId: Long = 100L @@ -197,7 +201,7 @@ internal class FakeCalendarDataSource : CalendarDataSource { } override fun deleteEvent(eventId: Long) { - writeError?.let { throw it } + writeError?.let { if (deletes++ >= deletesBeforeError) throw it } deletedEventIds += eventId managedEvents.values.forEach { rows -> rows.removeAll { it.eventId == eventId } } } diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/domain/EventSearchTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/domain/EventSearchTest.kt index 78e9c7e..710bab7 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/domain/EventSearchTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/domain/EventSearchTest.kt @@ -42,6 +42,24 @@ class EventSearchTest { ) } + /** [day] is the event's own date; all-day events sit at UTC midnight. */ + private fun allDayCandidate(title: String, day: String): SearchCandidate { + val begin = Instant.parse("${day}T00:00:00Z") + return SearchCandidate( + event = EventInstance( + instanceId = nextId, + eventId = nextId++, + calendarId = 1L, + title = title, + start = begin, + end = begin.plus(kotlin.time.Duration.parse("24h")), + isAllDay = true, + color = 0, + location = null, + ), + ) + } + private fun search(vararg candidates: SearchCandidate, query: String): SearchResults = EventSearch.search(candidates.toList(), query, now, todayStart, zone) @@ -189,6 +207,50 @@ class EventSearchTest { assertThat(results.months.single().let { it.year to it.monthNumber }).isEqualTo(2027 to 2) } + @Test + fun `an all-day event that has passed does not rank as still to come`() { + // Ahead of UTC its UTC-midnight end lands after the local day started, so + // comparing raw instants floated yesterday's holiday above today's events. + val berlin = TimeZone.of("Europe/Berlin") + val yesterday = allDayCandidate("Test holiday", "2027-01-14") + val tomorrow = candidate("Test soon", start = "2027-01-16T09:00:00Z") + + val results = EventSearch.search( + listOf(yesterday, tomorrow), + "test", + Instant.parse("2027-01-15T07:00:00Z"), + Instant.parse("2027-01-14T23:00:00Z"), + berlin, + ) + + assertThat(datedTitles(results)).containsExactly("Test soon", "Test holiday").inOrder() + assertThat(results.allHits.last().isPast).isTrue() + } + + @Test + fun `an all-day event is not past until its own day is over`() { + // 21:00 in Los Angeles, where the event's UTC-midnight end is already gone. + val results = EventSearch.search( + listOf(allDayCandidate("Test holiday", "2027-01-15")), + "test", + Instant.parse("2027-01-16T05:00:00Z"), + Instant.parse("2027-01-15T08:00:00Z"), + TimeZone.of("America/Los_Angeles"), + ) + + assertThat(results.allHits.single().isPast).isFalse() + } + + @Test + fun `an untitled event's placeholder is not something the query can match`() { + val untitled = candidate("(Ohne Titel)", description = "Titel folgt noch").copy(title = null) + + val results = search(untitled, query = "titel") + + assertThat(datedTitles(results)).isEmpty() + assertThat(results.inDescriptions.single().titleSpans).isEmpty() + } + @Test fun `a hit that has already finished is marked past, including earlier today`() { val earlierToday = candidate("test", start = "2027-01-15T05:00:00Z") diff --git a/app/src/test/java/de/jeanlucmakiola/calendula/ui/search/SearchViewModelTest.kt b/app/src/test/java/de/jeanlucmakiola/calendula/ui/search/SearchViewModelTest.kt index 36b6a7f..f3dff60 100644 --- a/app/src/test/java/de/jeanlucmakiola/calendula/ui/search/SearchViewModelTest.kt +++ b/app/src/test/java/de/jeanlucmakiola/calendula/ui/search/SearchViewModelTest.kt @@ -202,6 +202,30 @@ class SearchViewModelTest { job.cancel() } + @Test + fun `a permission revoked mid-batch still reports what already went`(@TempDir tempDir: Path) = + runTest(dispatcher) { + val fake = FakeCalendarDataSource().apply { + calendarsResult = listOf(cal(1L)) + searchResult = { listOf(hit(1L), hit(2L), hit(3L)) } + writeError = SecurityException("revoked") + deletesBeforeError = 1 + } + val vm = viewModel(tempDir, fake) + val job = activate(vm) + + vm.setQuery("standup") + advanceUntilIdle() + vm.selectAll() + vm.deleteSelected(RecurringWriteScope.AllEvents) + advanceUntilIdle() + + assertThat(fake.deletedEventIds).hasSize(1) + assertThat(vm.deleteState.value) + .isEqualTo(BulkDeleteUiState.NeedsPermission(deleted = 1)) + job.cancel() + } + @Test fun `changing the query drops the selection`(@TempDir tempDir: Path) = runTest(dispatcher) { val fake = FakeCalendarDataSource().apply {