diff --git a/mobile/lib/data/db/main/dao/person.dart b/mobile/lib/data/db/main/dao/person.dart index cddf8bfd8..413943663 100644 --- a/mobile/lib/data/db/main/dao/person.dart +++ b/mobile/lib/data/db/main/dao/person.dart @@ -11,14 +11,15 @@ class PeopleDatabaseRepository extends DatabaseAccessor with $PeopleDatab Drift get _db => attachedDatabase; - Future get(String personId) async { + /// The person for the given [personId], if any + Stream watchPerson(String personId) { final query = _db.select(_db.personEntity)..where((row) => row.id.equals(personId)); - final result = await query.getSingleOrNull(); - return result?.toDto(); + return query.map((row) => row.toDto()).watchSingleOrNull(); } - Future> getAssetPeople(String assetId) async { + /// The people associated with a given [assetId] + Stream> watchPeopleForAsset(String assetId) { // An asset can have multiple face records for the same person (e.g., metadata // imports alongside ML detections). Use a subquery instead of a join so each // person is returned once, regardless of how many of their faces are on the asset @@ -33,10 +34,13 @@ class PeopleDatabaseRepository extends DatabaseAccessor with $PeopleDatab final query = _db.select(_db.personEntity) ..where((row) => row.id.isInQuery(faceQuery) & row.isHidden.equals(false)); - return query.map((row) => row.toDto()).get(); + return query.map((row) => row.toDto()).watch(); } - Stream> watch({int minFaces = 3}) { + /// All known people with a known associated face and asset + /// + /// If [minFaces] is provided (defaults to 3), restrict to people having at least that many unique face entries + Stream> watchAll({int minFaces = 3}) { final people = _db.personEntity; final faces = _db.assetFaceEntity; final assets = _db.remoteAssetEntity; diff --git a/mobile/lib/data/store/person.dart b/mobile/lib/data/store/person.dart index dcd76dcc4..205ccd1bc 100644 --- a/mobile/lib/data/store/person.dart +++ b/mobile/lib/data/store/person.dart @@ -1,3 +1,4 @@ +import 'package:collection/collection.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/data/server/person.dart'; import 'package:immich_mobile/data/store/util/cache.dart'; @@ -15,13 +16,13 @@ extension type const PersonStore._(Provider _provider) implemen /// Get the person specified by [personId] /// - /// **NOTE:** This is not reactive to changes, and only hits the local DB - AutoDisposeFutureProvider byId(String personId) => _byIdProvider(personId); + /// **NOTE:** This is only reactive to the local DB + AutoDisposeStreamProvider byId(String personId) => _byIdProvider(personId); /// Get the people present in the asset [assetId] /// - /// **NOTE:** This is not reactive to changes, and only hits the local DB - AutoDisposeFutureProvider> forAsset(String assetId) => _forAssetProvider(assetId); + /// **NOTE:** This is only reactive to the local DB + AutoDisposeStreamProvider> forAsset(String assetId) => _forAssetProvider(assetId); /// Get all known people, honoring the user's minimum detected face count preference /// @@ -31,19 +32,21 @@ extension type const PersonStore._(Provider _provider) implemen final _peopleDb = driftProvider.select((db) => db.peopleDatabaseRepository); -// We have to map from non-reactive existing Drift queries to Riverpod reactivity, so we wrap each call in a provider family -// Note that the only reactivity here is in going from no data (fetch start) to data (fetch completed), and the DB swapping (basically never) -final _byIdProvider = FutureProvider.autoDispose.family( - (ref, personId) => ref.watch(_peopleDb).get(personId), +final _byIdProvider = StreamProvider.autoDispose.family( + (ref, personId) => ref.watch(_peopleDb).watchPerson(personId).distinct(), ); -final _forAssetProvider = FutureProvider.autoDispose.family, String>( - (ref, assetId) => ref.watch(_peopleDb).getAssetPeople(assetId), +final _forAssetProvider = StreamProvider.autoDispose.family, String>( + (ref, assetId) => ref.watch(_peopleDb).watchPeopleForAsset(assetId).distinct(const ListEquality().equals), ); final _allProvider = StreamProvider.autoDispose>((ref) async* { final prefs = await ref.watch(userMetadataPreferencesProvider.future); - yield* ref.watch(_peopleDb).watch(minFaces: prefs?.minimumFaces ?? 3); + + yield* ref + .watch(_peopleDb) + .watchAll(minFaces: prefs?.minimumFaces ?? 3) + .distinct(const ListEquality().equals); }); class PersonMutations extends StoreMutations { diff --git a/mobile/lib/presentation/widgets/asset_viewer/asset_details/people_details.widget.dart b/mobile/lib/presentation/widgets/asset_viewer/asset_details/people_details.widget.dart index 324910f70..d8ae9be94 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/asset_details/people_details.widget.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/asset_details/people_details.widget.dart @@ -28,22 +28,9 @@ class PeopleDetails extends ConsumerWidget { return const SizedBox.shrink(); } - final peopleFuture = ref.watch(Store.people.forAsset(asset.id)); + final people = ref.watch(Store.people.forAsset(asset.id)); - Future showNameEditModal(Person person) async { - await showDialog( - context: context, - useRootNavigator: false, - builder: (BuildContext context) { - return PersonNameEditForm(person: person); - }, - ); - - // TODO(agg23): Remove once state is properly reactive - ref.invalidate(Store.people.forAsset(asset.id)); - } - - return peopleFuture.when( + return people.when( data: (people) { return AnimatedCrossFade( firstChild: const SizedBox.shrink(), @@ -79,7 +66,13 @@ class PeopleDetails extends ConsumerWidget { ContextHelper(context).pop(); unawaited(context.pushRoute(PersonRoute(person: person))); }, - onNameTap: () => showNameEditModal(person), + onNameTap: () => showDialog( + context: context, + useRootNavigator: false, + builder: (BuildContext context) { + return PersonNameEditForm(person: person); + }, + ), ), ], ), diff --git a/mobile/test/data/store/person_store_test.dart b/mobile/test/data/store/person_store_test.dart index 20571b500..24f41b3e8 100644 --- a/mobile/test/data/store/person_store_test.dart +++ b/mobile/test/data/store/person_store_test.dart @@ -1,3 +1,5 @@ +import 'dart:async'; + import 'package:flutter_test/flutter_test.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/data/server/person.dart'; @@ -58,6 +60,48 @@ void main() { expect(people.map((p) => p.id), [named.id]); }); + test('forAsset re-emits when a person is renamed', () async { + final user = await ctx.newUser(); + final asset = await ctx.newRemoteAsset(ownerId: user.id); + final person = await ctx.newPerson(ownerId: user.id, name: 'Old'); + await ctx.newFace(assetId: asset.id, personId: person.id); + + when(() => api.update(person.id, name: 'New')).thenAnswer((_) async => Person(id: person.id, name: 'New')); + + final renamed = Completer(); + + final provider = Store.people.forAsset(asset.id); + container.listen(provider, (_, next) { + if (next.valueOrNull?.single.name == 'New' && !renamed.isCompleted) { + renamed.complete(); + } + }); + await container.read(provider.future); + + await container.read(Store.people).updateName(person.id, 'New'); + + await expectLater(renamed.future, completes); + }); + + test('forAsset does not push updates unnecessarily', () async { + final user = await ctx.newUser(); + final asset = await ctx.newRemoteAsset(ownerId: user.id); + final person = await ctx.newPerson(ownerId: user.id, name: 'Old'); + await ctx.newFace(assetId: asset.id, personId: person.id); + + var emissions = 0; + + final provider = Store.people.forAsset(asset.id); + container.listen(provider, (_, _) => emissions += 1); + await container.read(provider.future); + + await ctx.newPerson(ownerId: user.id, name: 'Unrelated'); + + await pumpEventQueue(); + // Only should have received the first, loaded event + expect(emissions, 1); + }); + test('updateName pushes to the server, then saves locally', () async { final user = await ctx.newUser(); final person = await ctx.newPerson(ownerId: user.id, name: 'Old'); diff --git a/mobile/test/medium/repositories/people_repository_test.dart b/mobile/test/medium/repositories/people_repository_test.dart index b5ff62ba8..ccf748720 100644 --- a/mobile/test/medium/repositories/people_repository_test.dart +++ b/mobile/test/medium/repositories/people_repository_test.dart @@ -16,7 +16,7 @@ void main() { await ctx.dispose(); }); - group('getAssetPeople', () { + group('watchPeopleForAsset', () { test('does not duplicate a person with multiple face records on the same asset', () async { // Regression check for #20585: a join on asset_face_entity returned one row // per face, so a person appeared twice in the asset details panel when the @@ -28,7 +28,7 @@ void main() { await ctx.newFace(assetId: asset.id, personId: person.id); await ctx.newFace(assetId: asset.id, personId: person.id); - final people = await sut.getAssetPeople(asset.id); + final people = await sut.watchPeopleForAsset(asset.id).first; expect(people, hasLength(1)); expect(people.single.id, person.id); @@ -43,7 +43,7 @@ void main() { await ctx.newFace(assetId: asset.id, personId: person1.id); await ctx.newFace(assetId: asset.id, personId: person2.id); - final people = await sut.getAssetPeople(asset.id); + final people = await sut.watchPeopleForAsset(asset.id).first; expect(people, hasLength(2)); expect(people.map((person) => person.id), containsAll([person1.id, person2.id])); @@ -56,7 +56,7 @@ void main() { final hidden = await ctx.newPerson(ownerId: user.id, isHidden: true); await ctx.newFace(assetId: asset.id, personId: hidden.id); - final people = await sut.getAssetPeople(asset.id); + final people = await sut.watchPeopleForAsset(asset.id).first; expect(people, isEmpty); }); @@ -69,7 +69,7 @@ void main() { final person = await ctx.newPerson(ownerId: user.id); await ctx.newFace(assetId: otherAsset.id, personId: person.id); - final people = await sut.getAssetPeople(asset.id); + final people = await sut.watchPeopleForAsset(asset.id).first; expect(people, isEmpty); });