From 0d60199514babc2f2cf547feee8d048f7fbda83a Mon Sep 17 00:00:00 2001 From: Brandon Wees Date: Tue, 12 Aug 2025 14:46:50 -0500 Subject: [PATCH] fix(mobile): newest/oldest album sort (#20743) * fix(mobile): newest/oldest album sort * chore: use sqlite to determine album asset timestamps * Fix missing future Co-authored-by: Alex * fix: async handling of sort * chore: tests * chore: code review changes * fix: use created at for newest asset * fix: use localDateTime for sorting * chore: cleanup * chore: use final * feat: loading indicator --------- Co-authored-by: Alex --- i18n/en.json | 1 + .../domain/services/remote_album.service.dart | 75 ++++++++++- .../repositories/remote_album.repository.dart | 22 ++++ .../widgets/album/album_selector.widget.dart | 25 +++- .../infrastructure/remote_album.provider.dart | 5 +- mobile/lib/utils/remote_album.utils.dart | 64 ---------- .../domain/services/album.service_test.dart | 116 ++++++++++++++++++ .../test/infrastructure/repository.mock.dart | 6 + 8 files changed, 240 insertions(+), 74 deletions(-) delete mode 100644 mobile/lib/utils/remote_album.utils.dart create mode 100644 mobile/test/domain/services/album.service_test.dart diff --git a/i18n/en.json b/i18n/en.json index 116dfaeb4a..57a380579e 100644 --- a/i18n/en.json +++ b/i18n/en.json @@ -1856,6 +1856,7 @@ "sort_created": "Date created", "sort_items": "Number of items", "sort_modified": "Date modified", + "sort_newest": "Newest photo", "sort_oldest": "Oldest photo", "sort_people_by_similarity": "Sort people by similarity", "sort_recent": "Most recent photo", diff --git a/mobile/lib/domain/services/remote_album.service.dart b/mobile/lib/domain/services/remote_album.service.dart index 6c5d974485..4d85119b74 100644 --- a/mobile/lib/domain/services/remote_album.service.dart +++ b/mobile/lib/domain/services/remote_album.service.dart @@ -1,12 +1,12 @@ import 'dart:async'; +import 'package:collection/collection.dart'; import 'package:immich_mobile/domain/models/album/album.model.dart'; import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/user.model.dart'; import 'package:immich_mobile/infrastructure/repositories/remote_album.repository.dart'; import 'package:immich_mobile/models/albums/album_search.model.dart'; import 'package:immich_mobile/repositories/drift_album_api_repository.dart'; -import 'package:immich_mobile/utils/remote_album.utils.dart'; class RemoteAlbumService { final DriftRemoteAlbumRepository _repository; @@ -26,8 +26,21 @@ class RemoteAlbumService { return _repository.get(albumId); } - List sortAlbums(List albums, RemoteAlbumSortMode sortMode, {bool isReverse = false}) { - return sortMode.sortFn(albums, isReverse); + Future> sortAlbums( + List albums, + RemoteAlbumSortMode sortMode, { + bool isReverse = false, + }) async { + final List sorted = switch (sortMode) { + RemoteAlbumSortMode.created => albums.sortedBy((album) => album.createdAt), + RemoteAlbumSortMode.title => albums.sortedBy((album) => album.name), + RemoteAlbumSortMode.lastModified => albums.sortedBy((album) => album.updatedAt), + RemoteAlbumSortMode.assetCount => albums.sortedBy((album) => album.assetCount), + RemoteAlbumSortMode.mostRecent => await _sortByNewestAsset(albums), + RemoteAlbumSortMode.mostOldest => await _sortByOldestAsset(albums), + }; + + return (isReverse ? sorted.reversed : sorted).toList(); } List searchAlbums( @@ -143,4 +156,60 @@ class RemoteAlbumService { Future getCount() { return _repository.getCount(); } + + Future> _sortByNewestAsset(List albums) async { + // map album IDs to their newest asset dates + final Map> assetTimestampFutures = {}; + for (final album in albums) { + assetTimestampFutures[album.id] = _repository.getNewestAssetTimestamp(album.id); + } + + // await all database queries + final entries = await Future.wait( + assetTimestampFutures.entries.map((entry) async => MapEntry(entry.key, await entry.value)), + ); + final assetTimestamps = Map.fromEntries(entries); + + final sorted = albums.sorted((a, b) { + final aDate = assetTimestamps[a.id] ?? DateTime.fromMillisecondsSinceEpoch(0); + final bDate = assetTimestamps[b.id] ?? DateTime.fromMillisecondsSinceEpoch(0); + return aDate.compareTo(bDate); + }); + + return sorted; + } + + Future> _sortByOldestAsset(List albums) async { + // map album IDs to their oldest asset dates + final Map> assetTimestampFutures = { + for (final album in albums) album.id: _repository.getOldestAssetTimestamp(album.id), + }; + + // await all database queries + final entries = await Future.wait( + assetTimestampFutures.entries.map((entry) async => MapEntry(entry.key, await entry.value)), + ); + final assetTimestamps = Map.fromEntries(entries); + + final sorted = albums.sorted((a, b) { + final aDate = assetTimestamps[a.id] ?? DateTime.fromMillisecondsSinceEpoch(0); + final bDate = assetTimestamps[b.id] ?? DateTime.fromMillisecondsSinceEpoch(0); + return aDate.compareTo(bDate); + }); + + return sorted.reversed.toList(); + } +} + +enum RemoteAlbumSortMode { + title("library_page_sort_title"), + assetCount("library_page_sort_asset_count"), + lastModified("library_page_sort_last_modified"), + created("library_page_sort_created"), + mostRecent("sort_newest"), + mostOldest("sort_oldest"); + + final String key; + + const RemoteAlbumSortMode(this.key); } diff --git a/mobile/lib/infrastructure/repositories/remote_album.repository.dart b/mobile/lib/infrastructure/repositories/remote_album.repository.dart index acb1eddda5..4e5d53a49b 100644 --- a/mobile/lib/infrastructure/repositories/remote_album.repository.dart +++ b/mobile/lib/infrastructure/repositories/remote_album.repository.dart @@ -265,6 +265,28 @@ class DriftRemoteAlbumRepository extends DriftDatabaseRepository { }).watchSingleOrNull(); } + Future getNewestAssetTimestamp(String albumId) { + final query = _db.remoteAlbumAssetEntity.selectOnly() + ..where(_db.remoteAlbumAssetEntity.albumId.equals(albumId)) + ..addColumns([_db.remoteAssetEntity.localDateTime.max()]) + ..join([ + innerJoin(_db.remoteAssetEntity, _db.remoteAssetEntity.id.equalsExp(_db.remoteAlbumAssetEntity.assetId)), + ]); + + return query.map((row) => row.read(_db.remoteAssetEntity.localDateTime.max())).getSingleOrNull(); + } + + Future getOldestAssetTimestamp(String albumId) { + final query = _db.remoteAlbumAssetEntity.selectOnly() + ..where(_db.remoteAlbumAssetEntity.albumId.equals(albumId)) + ..addColumns([_db.remoteAssetEntity.localDateTime.min()]) + ..join([ + innerJoin(_db.remoteAssetEntity, _db.remoteAssetEntity.id.equalsExp(_db.remoteAlbumAssetEntity.assetId)), + ]); + + return query.map((row) => row.read(_db.remoteAssetEntity.localDateTime.min())).getSingleOrNull(); + } + Future getCount() { return _db.managers.remoteAlbumEntity.count(); } diff --git a/mobile/lib/presentation/widgets/album/album_selector.widget.dart b/mobile/lib/presentation/widgets/album/album_selector.widget.dart index 53871b48ae..aacede789e 100644 --- a/mobile/lib/presentation/widgets/album/album_selector.widget.dart +++ b/mobile/lib/presentation/widgets/album/album_selector.widget.dart @@ -7,6 +7,7 @@ import 'package:flutter/material.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/domain/models/album/album.model.dart'; import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; +import 'package:immich_mobile/domain/services/remote_album.service.dart'; import 'package:immich_mobile/extensions/build_context_extensions.dart'; import 'package:immich_mobile/extensions/theme_extensions.dart'; import 'package:immich_mobile/extensions/translate_extensions.dart'; @@ -18,7 +19,6 @@ import 'package:immich_mobile/providers/infrastructure/current_album.provider.da import 'package:immich_mobile/providers/timeline/multiselect.provider.dart'; import 'package:immich_mobile/providers/user.provider.dart'; import 'package:immich_mobile/routing/router.dart'; -import 'package:immich_mobile/utils/remote_album.utils.dart'; import 'package:immich_mobile/widgets/common/confirm_dialog.dart'; import 'package:immich_mobile/widgets/common/immich_toast.dart'; import 'package:immich_mobile/widgets/common/search_field.dart'; @@ -138,21 +138,28 @@ class _SortButton extends ConsumerStatefulWidget { class _SortButtonState extends ConsumerState<_SortButton> { RemoteAlbumSortMode albumSortOption = RemoteAlbumSortMode.lastModified; bool albumSortIsReverse = true; + bool isSorting = false; - void onMenuTapped(RemoteAlbumSortMode sortMode) { + Future onMenuTapped(RemoteAlbumSortMode sortMode) async { final selected = albumSortOption == sortMode; // Switch direction if (selected) { setState(() { albumSortIsReverse = !albumSortIsReverse; + isSorting = true; }); - ref.read(remoteAlbumProvider.notifier).sortFilteredAlbums(sortMode, isReverse: albumSortIsReverse); + await ref.read(remoteAlbumProvider.notifier).sortFilteredAlbums(sortMode, isReverse: albumSortIsReverse); } else { setState(() { albumSortOption = sortMode; + isSorting = true; }); - ref.read(remoteAlbumProvider.notifier).sortFilteredAlbums(sortMode, isReverse: albumSortIsReverse); + await ref.read(remoteAlbumProvider.notifier).sortFilteredAlbums(sortMode, isReverse: albumSortIsReverse); } + + setState(() { + isSorting = false; + }); } @override @@ -230,6 +237,16 @@ class _SortButtonState extends ConsumerState<_SortButton> { color: context.colorScheme.onSurface.withAlpha(225), ), ), + isSorting + ? SizedBox( + width: 22, + height: 22, + child: CircularProgressIndicator( + strokeWidth: 2, + color: context.colorScheme.onSurface.withAlpha(225), + ), + ) + : const SizedBox.shrink(), ], ), ); diff --git a/mobile/lib/providers/infrastructure/remote_album.provider.dart b/mobile/lib/providers/infrastructure/remote_album.provider.dart index a9c1b88a15..a48a1c30e4 100644 --- a/mobile/lib/providers/infrastructure/remote_album.provider.dart +++ b/mobile/lib/providers/infrastructure/remote_album.provider.dart @@ -5,7 +5,6 @@ import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/user.model.dart'; import 'package:immich_mobile/domain/services/remote_album.service.dart'; import 'package:immich_mobile/models/albums/album_search.model.dart'; -import 'package:immich_mobile/utils/remote_album.utils.dart'; import 'package:logging/logging.dart'; import 'package:riverpod_annotation/riverpod_annotation.dart'; @@ -71,8 +70,8 @@ class RemoteAlbumNotifier extends Notifier { state = state.copyWith(filteredAlbums: state.albums); } - void sortFilteredAlbums(RemoteAlbumSortMode sortMode, {bool isReverse = false}) { - final sortedAlbums = _remoteAlbumService.sortAlbums(state.filteredAlbums, sortMode, isReverse: isReverse); + Future sortFilteredAlbums(RemoteAlbumSortMode sortMode, {bool isReverse = false}) async { + final sortedAlbums = await _remoteAlbumService.sortAlbums(state.filteredAlbums, sortMode, isReverse: isReverse); state = state.copyWith(filteredAlbums: sortedAlbums); } diff --git a/mobile/lib/utils/remote_album.utils.dart b/mobile/lib/utils/remote_album.utils.dart deleted file mode 100644 index b63df899ce..0000000000 --- a/mobile/lib/utils/remote_album.utils.dart +++ /dev/null @@ -1,64 +0,0 @@ -import 'package:collection/collection.dart'; -import 'package:immich_mobile/domain/models/album/album.model.dart'; - -typedef AlbumSortFn = List Function(List albums, bool isReverse); - -class _RemoteAlbumSortHandlers { - const _RemoteAlbumSortHandlers._(); - - static const AlbumSortFn created = _sortByCreated; - static List _sortByCreated(List albums, bool isReverse) { - final sorted = albums.sortedBy((album) => album.createdAt); - return (isReverse ? sorted.reversed : sorted).toList(); - } - - static const AlbumSortFn title = _sortByTitle; - static List _sortByTitle(List albums, bool isReverse) { - final sorted = albums.sortedBy((album) => album.name); - return (isReverse ? sorted.reversed : sorted).toList(); - } - - static const AlbumSortFn lastModified = _sortByLastModified; - static List _sortByLastModified(List albums, bool isReverse) { - final sorted = albums.sortedBy((album) => album.updatedAt); - return (isReverse ? sorted.reversed : sorted).toList(); - } - - static const AlbumSortFn assetCount = _sortByAssetCount; - static List _sortByAssetCount(List albums, bool isReverse) { - final sorted = albums.sorted((a, b) => a.assetCount.compareTo(b.assetCount)); - return (isReverse ? sorted.reversed : sorted).toList(); - } - - static const AlbumSortFn mostRecent = _sortByMostRecent; - static List _sortByMostRecent(List albums, bool isReverse) { - final sorted = albums.sorted((a, b) { - // For most recent, we sort by updatedAt in descending order - return b.updatedAt.compareTo(a.updatedAt); - }); - return (isReverse ? sorted.reversed : sorted).toList(); - } - - static const AlbumSortFn mostOldest = _sortByMostOldest; - static List _sortByMostOldest(List albums, bool isReverse) { - final sorted = albums.sorted((a, b) { - // For oldest, we sort by createdAt in ascending order - return a.createdAt.compareTo(b.createdAt); - }); - return (isReverse ? sorted.reversed : sorted).toList(); - } -} - -enum RemoteAlbumSortMode { - title("library_page_sort_title", _RemoteAlbumSortHandlers.title), - assetCount("library_page_sort_asset_count", _RemoteAlbumSortHandlers.assetCount), - lastModified("library_page_sort_last_modified", _RemoteAlbumSortHandlers.lastModified), - created("library_page_sort_created", _RemoteAlbumSortHandlers.created), - mostRecent("sort_recent", _RemoteAlbumSortHandlers.mostRecent), - mostOldest("sort_oldest", _RemoteAlbumSortHandlers.mostOldest); - - final String key; - final AlbumSortFn sortFn; - - const RemoteAlbumSortMode(this.key, this.sortFn); -} diff --git a/mobile/test/domain/services/album.service_test.dart b/mobile/test/domain/services/album.service_test.dart new file mode 100644 index 0000000000..88e8b3824c --- /dev/null +++ b/mobile/test/domain/services/album.service_test.dart @@ -0,0 +1,116 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/domain/models/album/album.model.dart'; +import 'package:immich_mobile/domain/services/remote_album.service.dart'; +import 'package:immich_mobile/infrastructure/repositories/remote_album.repository.dart'; +import 'package:immich_mobile/repositories/drift_album_api_repository.dart'; +import 'package:mocktail/mocktail.dart'; + +import '../../infrastructure/repository.mock.dart'; + +void main() { + late RemoteAlbumService sut; + late DriftRemoteAlbumRepository mockRemoteAlbumRepo; + late DriftAlbumApiRepository mockAlbumApiRepo; + + setUp(() { + mockRemoteAlbumRepo = MockRemoteAlbumRepository(); + mockAlbumApiRepo = MockDriftAlbumApiRepository(); + sut = RemoteAlbumService(mockRemoteAlbumRepo, mockAlbumApiRepo); + + when(() => mockRemoteAlbumRepo.getNewestAssetTimestamp(any())).thenAnswer((invocation) { + // Simulate a timestamp for the newest asset in the album + final albumID = invocation.positionalArguments[0] as String; + + if (albumID == '1') { + return Future.value(DateTime(2023, 1, 1)); + } else if (albumID == '2') { + return Future.value(DateTime(2023, 2, 1)); + } + + return Future.value(DateTime.fromMillisecondsSinceEpoch(0)); + }); + + when(() => mockRemoteAlbumRepo.getOldestAssetTimestamp(any())).thenAnswer((invocation) { + // Simulate a timestamp for the oldest asset in the album + final albumID = invocation.positionalArguments[0] as String; + + if (albumID == '1') { + return Future.value(DateTime(2019, 1, 1)); + } else if (albumID == '2') { + return Future.value(DateTime(2019, 2, 1)); + } + + return Future.value(DateTime.fromMillisecondsSinceEpoch(0)); + }); + }); + + final albumA = RemoteAlbum( + id: '1', + name: 'Album A', + description: "", + isActivityEnabled: false, + order: AlbumAssetOrder.asc, + assetCount: 1, + createdAt: DateTime(2023, 1, 1), + updatedAt: DateTime(2023, 1, 2), + ownerId: 'owner1', + ownerName: "Test User", + ); + + final albumB = RemoteAlbum( + id: '2', + name: 'Album B', + description: "", + isActivityEnabled: false, + order: AlbumAssetOrder.desc, + assetCount: 2, + createdAt: DateTime(2023, 2, 1), + updatedAt: DateTime(2023, 2, 2), + ownerId: 'owner2', + ownerName: "Test User", + ); + + group('sortAlbums', () { + test('should sort correctly based on name', () async { + final albums = [albumB, albumA]; + + final result = await sut.sortAlbums(albums, RemoteAlbumSortMode.title); + expect(result, [albumA, albumB]); + }); + + test('should sort correctly based on createdAt', () async { + final albums = [albumB, albumA]; + + final result = await sut.sortAlbums(albums, RemoteAlbumSortMode.created); + expect(result, [albumA, albumB]); + }); + + test('should sort correctly based on updatedAt', () async { + final albums = [albumB, albumA]; + + final result = await sut.sortAlbums(albums, RemoteAlbumSortMode.lastModified); + expect(result, [albumA, albumB]); + }); + + test('should sort correctly based on assetCount', () async { + final albums = [albumB, albumA]; + + final result = await sut.sortAlbums(albums, RemoteAlbumSortMode.assetCount); + expect(result, [albumA, albumB]); + }); + + test('should sort correctly based on newestAssetTimestamp', () async { + final albums = [albumB, albumA]; + + final result = await sut.sortAlbums(albums, RemoteAlbumSortMode.mostRecent); + expect(result, [albumA, albumB]); + }); + + test('should sort correctly based on oldestAssetTimestamp', () async { + final albums = [albumB, albumA]; + + final result = await sut.sortAlbums(albums, RemoteAlbumSortMode.mostOldest); + expect(result, [albumB, albumA]); + }); + }); +} diff --git a/mobile/test/infrastructure/repository.mock.dart b/mobile/test/infrastructure/repository.mock.dart index 29ef9462a8..1fe3af689f 100644 --- a/mobile/test/infrastructure/repository.mock.dart +++ b/mobile/test/infrastructure/repository.mock.dart @@ -2,12 +2,14 @@ import 'package:immich_mobile/infrastructure/repositories/device_asset.repositor import 'package:immich_mobile/infrastructure/repositories/local_album.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/local_asset.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/log.repository.dart'; +import 'package:immich_mobile/infrastructure/repositories/remote_album.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/storage.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/store.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/sync_api.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/sync_stream.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/user.repository.dart'; import 'package:immich_mobile/infrastructure/repositories/user_api.repository.dart'; +import 'package:immich_mobile/repositories/drift_album_api_repository.dart'; import 'package:mocktail/mocktail.dart'; class MockStoreRepository extends Mock implements IsarStoreRepository {} @@ -22,6 +24,8 @@ class MockSyncStreamRepository extends Mock implements SyncStreamRepository {} class MockLocalAlbumRepository extends Mock implements DriftLocalAlbumRepository {} +class MockRemoteAlbumRepository extends Mock implements DriftRemoteAlbumRepository {} + class MockLocalAssetRepository extends Mock implements DriftLocalAssetRepository {} class MockStorageRepository extends Mock implements StorageRepository {} @@ -30,3 +34,5 @@ class MockStorageRepository extends Mock implements StorageRepository {} class MockUserApiRepository extends Mock implements UserApiRepository {} class MockSyncApiRepository extends Mock implements SyncApiRepository {} + +class MockDriftAlbumApiRepository extends Mock implements DriftAlbumApiRepository {}