From 960be0c27a3a6d63a888239227bbf6dc16f4a0ae Mon Sep 17 00:00:00 2001 From: Thomas <9749173+uhthomas@users.noreply.github.com> Date: Sat, 4 Apr 2026 23:53:03 +0100 Subject: [PATCH] fix(mobile): don't update search filters in-place (#26965) * fix(mobile): don't update search filters in-place Search filters are currently modified in-place, which can feel quite janky. The chips behind the bottom sheet update instantly, and the search page gets confused because filters have been applied but no search has been initiated. Filters should keep their own copy of the filter when they're opened, and the commit + search on apply. The previous filter and pre-filter concepts were also cleaned up. They added complexity, and `search()` now owns the full life cycle of the filter. This now also reverts the changes from #27155, as this solution should be cleaner. * refactor & color tweak --------- Co-authored-by: Alex --- .../pages/search/drift_search.page.dart | 347 ++++++++---------- .../search_filter/search_filter_chip.dart | 10 +- mobile/pubspec.lock | 8 +- 3 files changed, 165 insertions(+), 200 deletions(-) diff --git a/mobile/lib/presentation/pages/search/drift_search.page.dart b/mobile/lib/presentation/pages/search/drift_search.page.dart index 452f6cc1d5..7e47a742ae 100644 --- a/mobile/lib/presentation/pages/search/drift_search.page.dart +++ b/mobile/lib/presentation/pages/search/drift_search.page.dart @@ -52,24 +52,20 @@ class DriftSearchPage extends HookConsumerWidget { : 'file_name_or_extension'.t(context: context), ); final textSearchController = useTextEditingController(); - final preFilter = ref.watch(searchPreFilterProvider); final filter = useState( SearchFilter( - people: preFilter?.people ?? {}, - location: preFilter?.location ?? SearchLocationFilter(), - camera: preFilter?.camera ?? SearchCameraFilter(), - date: preFilter?.date ?? SearchDateFilter(), - display: preFilter?.display ?? SearchDisplayFilters(isNotInAlbum: false, isArchive: false, isFavorite: false), - rating: preFilter?.rating ?? SearchRatingFilter(), - mediaType: preFilter?.mediaType ?? AssetType.other, + people: {}, + location: SearchLocationFilter(), + camera: SearchCameraFilter(), + date: SearchDateFilter(), + display: SearchDisplayFilters(isNotInAlbum: false, isArchive: false, isFavorite: false), + rating: SearchRatingFilter(), + mediaType: AssetType.other, language: "${context.locale.languageCode}-${context.locale.countryCode}", - assetId: preFilter?.assetId, - tagIds: preFilter?.tagIds ?? [], + tagIds: [], ), ); - final previousFilter = useState(null); - final hasRequestedSearch = useState(false); final dateInputFilter = useState(null); final peopleCurrentFilterWidget = useState(null); @@ -83,68 +79,58 @@ class DriftSearchPage extends HookConsumerWidget { final userPreferences = ref.watch(userMetadataPreferencesProvider); - searchFilter(SearchFilter filter) { - if (preFilter == null && filter == previousFilter.value) { + search(SearchFilter f) { + if (f == filter.value) { return; } + filter.value = f; + ref.read(paginatedSearchProvider.notifier).clear(); - if (filter.isEmpty) { - previousFilter.value = null; - hasRequestedSearch.value = false; - return; + if (!f.isEmpty) { + unawaited(ref.read(paginatedSearchProvider.notifier).search(f)); } - - hasRequestedSearch.value = true; - unawaited(ref.read(paginatedSearchProvider.notifier).search(filter)); - previousFilter.value = filter; } - search() => searchFilter(filter.value); - loadMoreSearchResults() { unawaited(ref.read(paginatedSearchProvider.notifier).search(filter.value)); } - searchPreFilter() { - if (preFilter != null) { - Future.delayed(Duration.zero, () { - filter.value = preFilter; - textSearchController.clear(); - searchFilter(preFilter); - - if (preFilter.location.city != null) { - locationCurrentFilterWidget.value = Text(preFilter.location.city!, style: context.textTheme.labelLarge); - } - }); - } - } - + // TODO: Use ref.listen with `fireImmediately` in the new riverpod version. + final preFilter = ref.watch(searchPreFilterProvider); useEffect(() { - Future.microtask(() => ref.invalidate(paginatedSearchProvider)); - searchPreFilter(); + if (preFilter == null) { + return null; + } + + Future.microtask(() { + textSearchController.clear(); + search(preFilter); + if (preFilter.location.city != null) { + locationCurrentFilterWidget.value = Text(preFilter.location.city!, style: context.textTheme.labelLarge); + } + }); return null; }, [preFilter]); showPeoplePicker() { - handleOnSelect(Set value) { - filter.value = filter.value.copyWith(people: value); + var people = filter.value.people; - final label = value.map((e) => e.name != '' ? e.name : 'no_name'.t(context: context)).join(', '); - if (label.isNotEmpty) { - peopleCurrentFilterWidget.value = Text(label, style: context.textTheme.labelLarge); - } else { - peopleCurrentFilterWidget.value = null; - } + handleOnSelect(Set value) { + people = value; } handleClear() { - filter.value = filter.value.copyWith(people: {}); - peopleCurrentFilterWidget.value = null; - search(); + search(filter.value.copyWith(people: {})); + } + + handleApply() { + final label = people.map((e) => e.name != '' ? e.name : 'no_name'.t(context: context)).join(', '); + peopleCurrentFilterWidget.value = label.isNotEmpty ? Text(label, style: context.textTheme.labelLarge) : null; + search(filter.value.copyWith(people: people)); } showFilterBottomSheet( @@ -155,7 +141,7 @@ class DriftSearchPage extends HookConsumerWidget { child: FilterBottomSheetScaffold( title: 'search_filter_people_title'.t(context: context), expanded: true, - onSearch: search, + onSearch: handleApply, onClear: handleClear, child: PeoplePicker(onSelect: handleOnSelect, filter: filter.value.people), ), @@ -164,23 +150,22 @@ class DriftSearchPage extends HookConsumerWidget { } showTagPicker() { + var tagIds = filter.value.tagIds ?? []; + String tagLabel = ''; + handleOnSelect(Iterable tags) { - filter.value = filter.value.copyWith(tagIds: tags.map((t) => t.id).toList()); - final label = tags.map((t) => t.value).join(', '); - if (label.isEmpty) { - tagCurrentFilterWidget.value = null; - } else { - tagCurrentFilterWidget.value = Text( - label.isEmpty ? 'tags'.t(context: context) : label, - style: context.textTheme.labelLarge, - ); - } + tagIds = tags.map((t) => t.id).toList(); + tagLabel = tags.map((t) => t.value).join(', '); } handleClear() { - filter.value = filter.value.copyWith(tagIds: []); tagCurrentFilterWidget.value = null; - search(); + search(filter.value.copyWith(tagIds: [])); + } + + handleApply() { + tagCurrentFilterWidget.value = tagLabel.isNotEmpty ? Text(tagLabel, style: context.textTheme.labelLarge) : null; + search(filter.value.copyWith(tagIds: tagIds)); } showFilterBottomSheet( @@ -191,7 +176,7 @@ class DriftSearchPage extends HookConsumerWidget { child: FilterBottomSheetScaffold( title: 'search_filter_tags_title'.t(context: context), expanded: true, - onSearch: search, + onSearch: handleApply, onClear: handleClear, child: TagPicker(onSelect: handleOnSelect, filter: (filter.value.tagIds ?? []).toSet()), ), @@ -200,32 +185,27 @@ class DriftSearchPage extends HookConsumerWidget { } showLocationPicker() { + var location = filter.value.location; + handleOnSelect(Map value) { - filter.value = filter.value.copyWith( - location: SearchLocationFilter(country: value['country'], city: value['city'], state: value['state']), - ); - - final locationText = []; - if (value['country'] != null) { - locationText.add(value['country']!); - } - - if (value['state'] != null) { - locationText.add(value['state']!); - } - - if (value['city'] != null) { - locationText.add(value['city']!); - } - - locationCurrentFilterWidget.value = Text(locationText.join(', '), style: context.textTheme.labelLarge); + location = SearchLocationFilter(country: value['country'], city: value['city'], state: value['state']); } handleClear() { - filter.value = filter.value.copyWith(location: SearchLocationFilter()); - locationCurrentFilterWidget.value = null; - search(); + search(filter.value.copyWith(location: SearchLocationFilter())); + } + + handleApply() { + final locationText = [ + if (location.country != null) location.country!, + if (location.state != null) location.state!, + if (location.city != null) location.city!, + ]; + locationCurrentFilterWidget.value = locationText.isNotEmpty + ? Text(locationText.join(', '), style: context.textTheme.labelLarge) + : null; + search(filter.value.copyWith(location: location)); } showFilterBottomSheet( @@ -234,7 +214,7 @@ class DriftSearchPage extends HookConsumerWidget { isDismissible: true, child: FilterBottomSheetScaffold( title: 'search_filter_location_title'.t(context: context), - onSearch: search, + onSearch: handleApply, onClear: handleClear, child: Padding( padding: const EdgeInsets.symmetric(vertical: 16.0), @@ -251,22 +231,24 @@ class DriftSearchPage extends HookConsumerWidget { } showCameraPicker() { - handleOnSelect(Map value) { - filter.value = filter.value.copyWith( - camera: SearchCameraFilter(make: value['make'], model: value['model']), - ); + var camera = filter.value.camera; - cameraCurrentFilterWidget.value = Text( - '${value['make'] ?? ''} ${value['model'] ?? ''}', - style: context.textTheme.labelLarge, - ); + handleOnSelect(Map value) { + camera = SearchCameraFilter(make: value['make'], model: value['model']); } handleClear() { - filter.value = filter.value.copyWith(camera: SearchCameraFilter()); - cameraCurrentFilterWidget.value = null; - search(); + search(filter.value.copyWith(camera: SearchCameraFilter())); + } + + handleApply() { + final make = camera.make ?? ''; + final model = camera.model ?? ''; + cameraCurrentFilterWidget.value = (make.isNotEmpty || model.isNotEmpty) + ? Text('$make $model', style: context.textTheme.labelLarge) + : null; + search(filter.value.copyWith(camera: camera)); } showFilterBottomSheet( @@ -275,7 +257,7 @@ class DriftSearchPage extends HookConsumerWidget { isDismissible: true, child: FilterBottomSheetScaffold( title: 'search_filter_camera_title'.t(context: context), - onSearch: search, + onSearch: handleApply, onClear: handleClear, child: Padding( padding: const EdgeInsets.all(16.0), @@ -288,28 +270,24 @@ class DriftSearchPage extends HookConsumerWidget { datePicked(DateFilterInputModel? selectedDate) { dateInputFilter.value = selectedDate; if (selectedDate == null) { - filter.value = filter.value.copyWith(date: SearchDateFilter()); - dateRangeCurrentFilterWidget.value = null; - unawaited(search()); + search(filter.value.copyWith(date: SearchDateFilter())); return; } final date = selectedDate.asDateTimeRange(); - - filter.value = filter.value.copyWith( - date: SearchDateFilter( - takenAfter: date.start, - takenBefore: date.end.add(const Duration(hours: 23, minutes: 59, seconds: 59)), - ), - ); - dateRangeCurrentFilterWidget.value = Text( selectedDate.asHumanReadable(context), style: context.textTheme.labelLarge, ); - - unawaited(search()); + search( + filter.value.copyWith( + date: SearchDateFilter( + takenAfter: date.start, + takenBefore: date.end.add(const Duration(hours: 23, minutes: 59, seconds: 59)), + ), + ), + ); } showDatePicker() async { @@ -376,31 +354,32 @@ class DriftSearchPage extends HookConsumerWidget { // MEDIA PICKER showMediaTypePicker() { - handleOnSelected(AssetType assetType) { - filter.value = filter.value.copyWith(mediaType: assetType); + var mediaType = filter.value.mediaType; - mediaTypeCurrentFilterWidget.value = Text( - assetType == AssetType.image - ? 'image'.t(context: context) - : assetType == AssetType.video - ? 'video'.t(context: context) - : 'all'.t(context: context), - style: context.textTheme.labelLarge, - ); + handleOnSelected(AssetType assetType) { + mediaType = assetType; } handleClear() { - filter.value = filter.value.copyWith(mediaType: AssetType.other); - mediaTypeCurrentFilterWidget.value = null; - search(); + search(filter.value.copyWith(mediaType: AssetType.other)); + } + + handleApply() { + mediaTypeCurrentFilterWidget.value = mediaType != AssetType.other + ? Text( + mediaType == AssetType.image ? 'image'.t(context: context) : 'video'.t(context: context), + style: context.textTheme.labelLarge, + ) + : null; + search(filter.value.copyWith(mediaType: mediaType)); } showFilterBottomSheet( context: context, child: FilterBottomSheetScaffold( title: 'search_filter_media_type_title'.t(context: context), - onSearch: search, + onSearch: handleApply, onClear: handleClear, child: MediaTypePicker(onSelect: handleOnSelected, filter: filter.value.mediaType), ), @@ -409,19 +388,22 @@ class DriftSearchPage extends HookConsumerWidget { // STAR RATING PICKER showStarRatingPicker() { - handleOnSelected(SearchRatingFilter rating) { - filter.value = filter.value.copyWith(rating: rating); + var rating = filter.value.rating; - ratingCurrentFilterWidget.value = Text( - 'rating_count'.t(args: {'count': rating.rating!}), - style: context.textTheme.labelLarge, - ); + handleOnSelected(SearchRatingFilter value) { + rating = value; } handleClear() { - filter.value = filter.value.copyWith(rating: SearchRatingFilter(rating: null)); ratingCurrentFilterWidget.value = null; - search(); + search(filter.value.copyWith(rating: SearchRatingFilter(rating: null))); + } + + handleApply() { + ratingCurrentFilterWidget.value = rating.rating != null + ? Text('rating_count'.t(args: {'count': rating.rating!}), style: context.textTheme.labelLarge) + : null; + search(filter.value.copyWith(rating: rating)); } showFilterBottomSheet( @@ -429,7 +411,7 @@ class DriftSearchPage extends HookConsumerWidget { isScrollControlled: true, child: FilterBottomSheetScaffold( title: 'rating'.t(context: context), - onSearch: search, + onSearch: handleApply, onClear: handleClear, child: StarRatingPicker(onSelect: handleOnSelected, filter: filter.value.rating), ), @@ -438,79 +420,54 @@ class DriftSearchPage extends HookConsumerWidget { // DISPLAY OPTION showDisplayOptionPicker() { + var display = filter.value.display; + handleOnSelect(Map value) { - final filterText = []; - value.forEach((key, value) { - switch (key) { - case DisplayOption.notInAlbum: - filter.value = filter.value.copyWith(display: filter.value.display.copyWith(isNotInAlbum: value)); - if (value) { - filterText.add('search_filter_display_option_not_in_album'.t(context: context)); - } - break; - case DisplayOption.archive: - filter.value = filter.value.copyWith(display: filter.value.display.copyWith(isArchive: value)); - if (value) { - filterText.add('archive'.t(context: context)); - } - break; - case DisplayOption.favorite: - filter.value = filter.value.copyWith(display: filter.value.display.copyWith(isFavorite: value)); - if (value) { - filterText.add('favorite'.t(context: context)); - } - break; - } - }); - - if (filterText.isEmpty) { - displayOptionCurrentFilterWidget.value = null; - return; - } - - displayOptionCurrentFilterWidget.value = Text(filterText.join(', '), style: context.textTheme.labelLarge); + display = display.copyWith( + isNotInAlbum: value[DisplayOption.notInAlbum], + isArchive: value[DisplayOption.archive], + isFavorite: value[DisplayOption.favorite], + ); } handleClear() { - filter.value = filter.value.copyWith( - display: SearchDisplayFilters(isNotInAlbum: false, isArchive: false, isFavorite: false), - ); - displayOptionCurrentFilterWidget.value = null; - search(); + search( + filter.value.copyWith( + display: SearchDisplayFilters(isNotInAlbum: false, isArchive: false, isFavorite: false), + ), + ); + } + + handleApply() { + final filterText = [ + if (display.isNotInAlbum) 'search_filter_display_option_not_in_album'.t(context: context), + if (display.isArchive) 'archive'.t(context: context), + if (display.isFavorite) 'favorite'.t(context: context), + ]; + displayOptionCurrentFilterWidget.value = filterText.isNotEmpty + ? Text(filterText.join(', '), style: context.textTheme.labelLarge) + : null; + search(filter.value.copyWith(display: display)); } showFilterBottomSheet( context: context, child: FilterBottomSheetScaffold( title: 'display_options'.t(context: context), - onSearch: search, + onSearch: handleApply, onClear: handleClear, child: DisplayOptionPicker(onSelect: handleOnSelect, filter: filter.value.display), ), ); } - handleTextSubmitted(String value) { - switch (textSearchType.value) { - case TextSearchType.context: - filter.value = filter.value.copyWith(filename: '', context: value, description: '', ocr: ''); - - break; - case TextSearchType.filename: - filter.value = filter.value.copyWith(filename: value, context: '', description: '', ocr: ''); - - break; - case TextSearchType.description: - filter.value = filter.value.copyWith(filename: '', context: '', description: value, ocr: ''); - break; - case TextSearchType.ocr: - filter.value = filter.value.copyWith(filename: '', context: '', description: '', ocr: value); - break; - } - - search(); - } + handleTextSubmitted(String value) => search(switch (textSearchType.value) { + TextSearchType.context => filter.value.copyWith(filename: '', context: value, description: '', ocr: ''), + TextSearchType.filename => filter.value.copyWith(filename: value, context: '', description: '', ocr: ''), + TextSearchType.description => filter.value.copyWith(filename: '', context: '', description: value, ocr: ''), + TextSearchType.ocr => filter.value.copyWith(filename: '', context: '', description: '', ocr: value), + }); IconData getSearchPrefixIcon() => switch (textSearchType.value) { TextSearchType.context => Icons.image_search_rounded, @@ -648,8 +605,10 @@ class DriftSearchPage extends HookConsumerWidget { hintText: searchHintText.value, key: const Key('search_text_field'), controller: textSearchController, - contentPadding: preFilter != null ? const EdgeInsets.only(left: 24) : const EdgeInsets.all(8), - prefixIcon: preFilter != null ? null : Icon(getSearchPrefixIcon(), color: context.colorScheme.primary), + contentPadding: filter.value.assetId != null ? const EdgeInsets.only(left: 24) : const EdgeInsets.all(8), + prefixIcon: filter.value.assetId != null + ? null + : Icon(getSearchPrefixIcon(), color: context.colorScheme.primary), onSubmitted: handleTextSubmitted, focusNode: ref.watch(searchInputFocusProvider), ), @@ -724,7 +683,7 @@ class DriftSearchPage extends HookConsumerWidget { ), ), ), - if (!hasRequestedSearch.value) + if (filter.value.isEmpty) const _SearchSuggestions() else _SearchResultGrid(onScrollEnd: loadMoreSearchResults), diff --git a/mobile/lib/widgets/search/search_filter/search_filter_chip.dart b/mobile/lib/widgets/search/search_filter/search_filter_chip.dart index a72b4668dd..d539ee38f3 100644 --- a/mobile/lib/widgets/search/search_filter/search_filter_chip.dart +++ b/mobile/lib/widgets/search/search_filter/search_filter_chip.dart @@ -16,7 +16,7 @@ class SearchFilterChip extends StatelessWidget { onTap: onTap, child: Card( elevation: 0, - color: context.primaryColor.withValues(alpha: .5), + color: context.colorScheme.secondaryContainer, shape: StadiumBorder(side: BorderSide(color: context.colorScheme.secondaryContainer)), child: Padding( padding: const EdgeInsets.symmetric(vertical: 2.0, horizontal: 14.0), @@ -32,7 +32,13 @@ class SearchFilterChip extends StatelessWidget { shape: StadiumBorder(side: BorderSide(color: context.colorScheme.outline.withAlpha(15))), child: Padding( padding: const EdgeInsets.symmetric(vertical: 2.0, horizontal: 14.0), - child: Row(children: [Icon(icon, size: 18), const SizedBox(width: 4.0), Text(label)]), + child: Row( + children: [ + Icon(icon, size: 18), + const SizedBox(width: 4.0), + Text(label, style: TextStyle(color: context.colorScheme.onSecondaryContainer)), + ], + ), ), ), ); diff --git a/mobile/pubspec.lock b/mobile/pubspec.lock index 89a43f328b..9d5f431792 100644 --- a/mobile/pubspec.lock +++ b/mobile/pubspec.lock @@ -1194,10 +1194,10 @@ packages: dependency: transitive description: name: meta - sha256: "23f08335362185a5ea2ad3a4e597f1375e78bce8a040df5c600c8d3552ef2394" + sha256: e3641ec5d63ebf0d9b41bd43201a66e3fc79a65db5f61fc181f04cd27aab950c url: "https://pub.dev" source: hosted - version: "1.17.0" + version: "1.16.0" mime: dependency: transitive description: @@ -1897,10 +1897,10 @@ packages: dependency: transitive description: name: test_api - sha256: ab2726c1a94d3176a45960b6234466ec367179b87dd74f1611adb1f3b5fb9d55 + sha256: "522f00f556e73044315fa4585ec3270f1808a4b186c936e612cab0b565ff1e00" url: "https://pub.dev" source: hosted - version: "0.7.7" + version: "0.7.6" thumbhash: dependency: "direct main" description: