From ca4a75abdfd63bd3fc08130b09a514b18360bb20 Mon Sep 17 00:00:00 2001 From: Marvin M <39344769+M123-dev@users.noreply.github.com> Date: Wed, 1 Oct 2025 20:38:29 +0200 Subject: [PATCH 1/8] feat: Allow merging faces on mobile --- .../lib/domain/services/people.service.dart | 9 ++ .../person_merge_tracker.service.dart | 48 +++++++ .../repositories/people.repository.dart | 21 +++ .../pages/drift_people_collection.page.dart | 2 +- .../presentation/pages/drift_person.page.dart | 136 +++++++++++++----- .../sheet_people_details.widget.dart | 42 +++--- .../person_edit_birthday_modal.widget.dart | 6 +- .../people/person_edit_name_modal.widget.dart | 128 +++++++++++++++-- .../people/person_merge_modal.widget.dart | 136 ++++++++++++++++++ .../widgets/people/person_tile.widget.dart | 58 ++++++++ .../infrastructure/people.provider.dart | 9 ++ .../repositories/person_api.repository.dart | 4 + mobile/lib/routing/router.gr.dart | 14 +- mobile/lib/utils/people.utils.dart | 15 +- .../search/search_filter/people_picker.dart | 58 +++----- 15 files changed, 566 insertions(+), 120 deletions(-) create mode 100644 mobile/lib/domain/services/person_merge_tracker.service.dart create mode 100644 mobile/lib/presentation/widgets/people/person_merge_modal.widget.dart create mode 100644 mobile/lib/presentation/widgets/people/person_tile.widget.dart diff --git a/mobile/lib/domain/services/people.service.dart b/mobile/lib/domain/services/people.service.dart index d45f710d7b..08d3f570a0 100644 --- a/mobile/lib/domain/services/people.service.dart +++ b/mobile/lib/domain/services/people.service.dart @@ -14,6 +14,10 @@ class DriftPeopleService { return _repository.getAssetPeople(assetId); } + Stream watchPersonById(String personId) { + return _repository.watchPersonById(personId); + } + Future> getAllPeople() { return _repository.getAllPeople(); } @@ -23,6 +27,11 @@ class DriftPeopleService { return _repository.updateName(personId, name); } + Future mergePeople({required String targetPersonId, required List mergePersonIds}) async { + await _personApiRepository.merge(targetPersonId, mergePersonIds); + return _repository.mergePeople(targetPersonId, mergePersonIds); + } + Future updateBrithday(String personId, DateTime birthday) async { await _personApiRepository.update(personId, birthday: birthday); return _repository.updateBirthday(personId, birthday); diff --git a/mobile/lib/domain/services/person_merge_tracker.service.dart b/mobile/lib/domain/services/person_merge_tracker.service.dart new file mode 100644 index 0000000000..04d90aedf6 --- /dev/null +++ b/mobile/lib/domain/services/person_merge_tracker.service.dart @@ -0,0 +1,48 @@ +/// Why do we need this? +/// Say we open the profile page (drift_person.page.dart) for Person A, and then nested above +/// a image viewer for an image that belongs to Person A. +/// +/// When the users now merges user A into user B, we cant just listen to +/// the changes in the profile page, we have to keep track where the user A (now B) +/// can be found in the DB. +/// +/// So when popping back to the profile page (and the user is missing) we check +/// which other person B we have to display instead. +class PersonMergeTrackerService { + // Map of merged person ID -> target person ID + final Map _mergeForwardingMap = {}; + + // Set of person IDs for which the merge record has been handled (redirected) + // To prevent multiple redirects for the same merge + final Set _handledMergeRecords = {}; + + /// Record a person merge operation + void recordMerge({required String mergedPersonId, required String targetPersonId}) { + _mergeForwardingMap[mergedPersonId] = targetPersonId; + } + + /// Get the target person ID for a merged person + String? getTargetPersonId(String personId) { + return _mergeForwardingMap[personId]; + } + + /// Check if a person ID has been merged + bool isPersonMerged(String personId) { + return _mergeForwardingMap.containsKey(personId); + } + + /// Check if a merge record has been handled (redirected) + bool isMergeRecordHandled(String personId) { + return _handledMergeRecords.contains(personId); + } + + /// Check if we should redirect for this person (merged but not yet handled) + bool shouldRedirectForPerson(String personId) { + return isPersonMerged(personId) && !isMergeRecordHandled(personId); + } + + /// Mark a merge record as handled (for tracking purposes) + void markMergeRecordHandled(String personId) { + _handledMergeRecords.add(personId); + } +} diff --git a/mobile/lib/infrastructure/repositories/people.repository.dart b/mobile/lib/infrastructure/repositories/people.repository.dart index e2b8646dba..7402dd55cd 100644 --- a/mobile/lib/infrastructure/repositories/people.repository.dart +++ b/mobile/lib/infrastructure/repositories/people.repository.dart @@ -1,5 +1,6 @@ import 'package:drift/drift.dart'; import 'package:immich_mobile/domain/models/person.model.dart'; +import 'package:immich_mobile/infrastructure/entities/asset_face.entity.drift.dart'; import 'package:immich_mobile/infrastructure/entities/person.entity.drift.dart'; import 'package:immich_mobile/infrastructure/repositories/db.repository.dart'; @@ -36,6 +37,12 @@ class DriftPeopleRepository extends DriftDatabaseRepository { }).get(); } + Stream watchPersonById(String personId) { + return (_db.select( + _db.personEntity, + )..where((tbl) => tbl.id.equals(personId))).watchSingleOrNull().map((entity) => entity?.toDto()); + } + Future updateName(String personId, String name) { final query = _db.update(_db.personEntity)..where((row) => row.id.equals(personId)); @@ -47,6 +54,20 @@ class DriftPeopleRepository extends DriftDatabaseRepository { return query.write(PersonEntityCompanion(birthDate: Value(birthday), updatedAt: Value(DateTime.now()))); } + + Future mergePeople(String targetPersonId, List mergePersonIds) async { + return _db.transaction(() async { + // Update AssetFaceEntity to point to the target person + final updateQuery = _db.update(_db.assetFaceEntity)..where((row) => row.personId.isIn(mergePersonIds)); + final updatedCount = await updateQuery.write(AssetFaceEntityCompanion(personId: Value(targetPersonId))); + + // Delete merged persons + final deleteQuery = _db.delete(_db.personEntity)..where((row) => row.id.isIn(mergePersonIds)); + final deletedCount = await deleteQuery.go(); + + return updatedCount + deletedCount; + }); + } } extension on PersonEntityData { diff --git a/mobile/lib/presentation/pages/drift_people_collection.page.dart b/mobile/lib/presentation/pages/drift_people_collection.page.dart index ca4e20aad0..aa86a7afe2 100644 --- a/mobile/lib/presentation/pages/drift_people_collection.page.dart +++ b/mobile/lib/presentation/pages/drift_people_collection.page.dart @@ -83,7 +83,7 @@ class _DriftPeopleCollectionPageState extends ConsumerState createState() => _DriftPersonPageState(); @@ -23,30 +27,20 @@ class DriftPersonPage extends ConsumerStatefulWidget { class _DriftPersonPageState extends ConsumerState { late DriftPerson _person; + final Logger mergeLogger = Logger("PersonMerge"); + @override initState() { super.initState(); - _person = widget.person; + _person = widget.initialPerson; } Future handleEditName(BuildContext context) async { - final newName = await showNameEditModal(context, _person); - - if (newName != null && newName.isNotEmpty) { - setState(() { - _person = _person.copyWith(name: newName); - }); - } + await showNameEditModal(context, _person); } Future handleEditBirthday(BuildContext context) async { - final birthday = await showBirthdayEditModal(context, _person); - - if (birthday != null) { - setState(() { - _person = _person.copyWith(birthDate: birthday); - }); - } + await showBirthdayEditModal(context, _person); } void showOptionSheet(BuildContext context) { @@ -72,27 +66,97 @@ class _DriftPersonPageState extends ConsumerState { @override Widget build(BuildContext context) { - return ProviderScope( - overrides: [ - timelineServiceProvider.overrideWith((ref) { - final user = ref.watch(currentUserProvider); - if (user == null) { - throw Exception('User must be logged in to view person timeline'); + final personAsync = ref.watch(driftGetPersonByIdProvider(_person.id)); + final mergeTracker = ref.read(personMergeTrackerProvider); + ref.watch(currentRouteNameProvider.select((name) => name ?? DriftPersonRoute.name)); + + return personAsync.when( + data: (personByProvider) { + if (personByProvider == null) { + // Check if the person was merged and redirect if necessary + final shouldRedirect = mergeTracker.shouldRedirectForPerson(_person.id); + final targetPersonId = mergeTracker.getTargetPersonId(_person.id); + + if (shouldRedirect && targetPersonId != null) { + bool isOnPersonDetailPage = ModalRoute.of(context)?.isCurrent ?? false; + + // Only redirect if we're currently on the person detail page, not in a nested view, e.g. image viewer + if (isOnPersonDetailPage) { + // Person was merged, redirect to the target person + WidgetsBinding.instance.addPostFrameCallback((_) { + if (mounted) { + // Use the service directly to get the target person + ref + .read(driftPeopleServiceProvider) + .watchPersonById(targetPersonId) + .first + .then((targetPerson) { + if (targetPerson != null && mounted) { + // Mark the merge record as handled + mergeTracker.markMergeRecordHandled(_person.id); + _person = targetPerson; + setState(() {}); + } else { + // Target person not found, go back + context.maybePop(); + } + }) + .catchError((error) { + // If we can't load the target person, go back + mergeLogger.severe("Error during read of targetPerson", error); + if (mounted) { + context.maybePop(); + } + }); + } + }); + return const Center(child: CircularProgressIndicator()); + } else { + // We're in an image viewer or other nested view, don't redirect yet + // Just show loading spinner to indicate something is happening + return const Center(child: CircularProgressIndicator()); + } } - final timelineService = ref.watch(timelineFactoryProvider).person(user.id, _person.id); - ref.onDispose(timelineService.dispose); - return timelineService; - }), - ], - child: Timeline( - appBar: PersonSliverAppBar( - person: _person, - onNameTap: () => handleEditName(context), - onBirthdayTap: () => handleEditBirthday(context), - onShowOptions: () => showOptionSheet(context), - ), - ), + // Person not found and no merge record + mergeLogger.info( + 'Person ${_person.name} (${_person.id}) not found and no merge records exist, it was probably deleted', + ); + WidgetsBinding.instance.addPostFrameCallback((_) { + if (mounted) { + context.maybePop(); + } + }); + return const Center(child: CircularProgressIndicator()); + } + + _person = personByProvider; + return ProviderScope( + overrides: [ + timelineServiceProvider.overrideWith((ref) { + final user = ref.watch(currentUserProvider); + if (user == null) { + throw Exception('User must be logged in to view person timeline'); + } + + final timelineService = ref.read(timelineFactoryProvider).person(user.id, _person.id); + ref.onDispose(timelineService.dispose); + return timelineService; + }), + ], + child: Timeline( + appBar: PersonSliverAppBar( + person: _person, + onNameTap: () => handleEditName(context), + onBirthdayTap: () => handleEditBirthday(context), + onShowOptions: () => showOptionSheet(context), + ), + ), + ); + }, + // TODO(m123): Show initialPerson data while loading new data (optimistic ui update, but we need to handle scroll state etc) + loading: () => const Center(child: CircularProgressIndicator()), + error: (e, s) => Text('Error: $e'), ); } } diff --git a/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart b/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart index fee34bca1b..0871db3510 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart @@ -5,14 +5,13 @@ import 'package:immich_mobile/domain/models/asset/base_asset.model.dart'; import 'package:immich_mobile/domain/models/person.model.dart'; import 'package:immich_mobile/extensions/build_context_extensions.dart'; import 'package:immich_mobile/extensions/translate_extensions.dart'; -import 'package:immich_mobile/presentation/widgets/people/person_edit_name_modal.widget.dart'; import 'package:immich_mobile/providers/infrastructure/asset_viewer/current_asset.provider.dart'; import 'package:immich_mobile/providers/infrastructure/people.provider.dart'; import 'package:immich_mobile/providers/routes.provider.dart'; import 'package:immich_mobile/routing/router.dart'; import 'package:immich_mobile/services/api.service.dart'; -import 'package:immich_mobile/utils/people.utils.dart'; import 'package:immich_mobile/utils/image_url_builder.dart'; +import 'package:immich_mobile/utils/people.utils.dart'; class SheetPeopleDetails extends ConsumerStatefulWidget { const SheetPeopleDetails({super.key}); @@ -31,18 +30,6 @@ class _SheetPeopleDetailsState extends ConsumerState { final peopleFuture = ref.watch(driftPeopleAssetProvider(asset.id)); - Future showNameEditModal(DriftPerson person) async { - await showDialog( - context: context, - useRootNavigator: false, - builder: (BuildContext context) { - return DriftPersonNameEditForm(person: person); - }, - ); - - ref.invalidate(driftPeopleAssetProvider(asset.id)); - } - return peopleFuture.when( data: (people) { return AnimatedCrossFade( @@ -74,15 +61,36 @@ class _SheetPeopleDetailsState extends ConsumerState { final previousRouteData = ref.read(previousRouteDataProvider); final previousRouteArgs = previousRouteData?.arguments; + // TODO: Check what happens if the person id from the previous route is not the correct one anymore e.g. after a merge // Prevent circular navigation - if (previousRouteArgs is DriftPersonRouteArgs && previousRouteArgs.person.id == person.id) { + if (previousRouteArgs is DriftPersonRouteArgs && + previousRouteArgs.initialPerson.id == person.id) { context.back(); return; } context.pop(); - context.pushRoute(DriftPersonRoute(person: person)); + context.pushRoute(DriftPersonRoute(initialPerson: person)); + }, + onNameTap: () async { + DriftPerson? newPerson = await showNameEditModal(context, person); + + ref.invalidate(driftPeopleAssetProvider(asset.id)); + + // If the name edit resulted in a new person (e.g. from merging) + // And if we are currently nested below the drift person page if said + // old person id, we need to pop, otherwise the timeline provider + // complains because the indexes are off + // TODO: Preferably we would replace the timeline provider, and let it listen to the new person id (Relevant function is the ```TimelineService person(String userId, String personId)``` in timeline.service.dart) + final previousRouteData = ref.read(previousRouteDataProvider); + final previousRouteArgs = previousRouteData?.arguments; + final previousPersonId = previousRouteArgs is DriftPersonRouteArgs + ? previousRouteArgs.initialPerson.id + : null; + + if (newPerson != null && newPerson.id != person.id && previousPersonId == person.id) { + context.pop(); + } }, - onNameTap: () => showNameEditModal(person), ), ], ), diff --git a/mobile/lib/presentation/widgets/people/person_edit_birthday_modal.widget.dart b/mobile/lib/presentation/widgets/people/person_edit_birthday_modal.widget.dart index dd6390406b..358be76d2f 100644 --- a/mobile/lib/presentation/widgets/people/person_edit_birthday_modal.widget.dart +++ b/mobile/lib/presentation/widgets/people/person_edit_birthday_modal.widget.dart @@ -6,9 +6,9 @@ import 'package:immich_mobile/domain/models/person.model.dart'; import 'package:immich_mobile/extensions/build_context_extensions.dart'; import 'package:immich_mobile/extensions/translate_extensions.dart'; import 'package:immich_mobile/providers/infrastructure/people.provider.dart'; +import 'package:immich_mobile/utils/debug_print.dart'; import 'package:immich_mobile/widgets/common/immich_toast.dart'; import 'package:scroll_date_picker/scroll_date_picker.dart'; -import 'package:immich_mobile/utils/debug_print.dart'; class DriftPersonBirthdayEditForm extends ConsumerStatefulWidget { final DriftPerson person; @@ -16,10 +16,10 @@ class DriftPersonBirthdayEditForm extends ConsumerStatefulWidget { const DriftPersonBirthdayEditForm({super.key, required this.person}); @override - ConsumerState createState() => _DriftPersonNameEditFormState(); + ConsumerState createState() => _DriftPersonBirthdayEditFormState(); } -class _DriftPersonNameEditFormState extends ConsumerState { +class _DriftPersonBirthdayEditFormState extends ConsumerState { late DateTime _selectedDate; @override diff --git a/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart b/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart index 6de19000e0..28a0f055d7 100644 --- a/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart +++ b/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart @@ -5,9 +5,12 @@ import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/domain/models/person.model.dart'; import 'package:immich_mobile/extensions/build_context_extensions.dart'; import 'package:immich_mobile/extensions/translate_extensions.dart'; +import 'package:immich_mobile/pages/common/large_leading_tile.dart'; +import 'package:immich_mobile/presentation/widgets/people/person_tile.widget.dart'; import 'package:immich_mobile/providers/infrastructure/people.provider.dart'; -import 'package:immich_mobile/widgets/common/immich_toast.dart'; import 'package:immich_mobile/utils/debug_print.dart'; +import 'package:immich_mobile/utils/people.utils.dart'; +import 'package:immich_mobile/widgets/common/immich_toast.dart'; class DriftPersonNameEditForm extends ConsumerStatefulWidget { final DriftPerson person; @@ -20,6 +23,7 @@ class DriftPersonNameEditForm extends ConsumerStatefulWidget { class _DriftPersonNameEditFormState extends ConsumerState { late TextEditingController _formController; + List _filteredPeople = []; @override void initState() { @@ -27,12 +31,30 @@ class _DriftPersonNameEditFormState extends ConsumerState(response); + } + } + return; + } + + void onEdit(DriftPerson person, String newName) async { try { - final result = await ref.read(driftPeopleServiceProvider).updateName(personId, newName); + final result = await ref.read(driftPeopleServiceProvider).updateName(person.id, newName); if (result != 0) { ref.invalidate(driftGetAllPeopleProvider); - context.pop(newName); + if (mounted) { + context.pop(person); + } } } catch (error) { dPrint(() => 'Error updating name: $error'); @@ -50,17 +72,99 @@ class _DriftPersonNameEditFormState extends ConsumerState people, String query) { + final queryParts = query.toLowerCase().split(' ').where((e) => e.isNotEmpty).toList(); + + List startsWithMatches = []; + List containsMatches = []; + + for (final p in people) { + final nameParts = p.name.toLowerCase().split(' ').where((e) => e.isNotEmpty).toList(); + final allStart = queryParts.every((q) => nameParts.any((n) => n.startsWith(q))); + final allContain = queryParts.every((q) => nameParts.any((n) => n.contains(q))); + + if (allStart) { + // Prioritize names that start with the query + startsWithMatches.add(p); + } else if (allContain) { + containsMatches.add(p); + } + } + + if (!mounted) return; + setState(() { + // TODO: What happens if there are more than 3 matches with the exact same name? + _filteredPeople = query.isEmpty ? [] : (startsWithMatches + containsMatches).take(3).toList(); + }); + } + @override Widget build(BuildContext context) { + final curatedPeople = ref.watch(driftGetAllPeopleProvider); + return AlertDialog( title: const Text("edit_name", style: TextStyle(fontWeight: FontWeight.bold)).tr(), - content: SingleChildScrollView( - child: TextFormField( - controller: _formController, - textCapitalization: TextCapitalization.words, - autofocus: true, - decoration: InputDecoration(hintText: 'name'.tr(), border: const OutlineInputBorder()), - ), + content: curatedPeople.when( + data: (people) { + return SingleChildScrollView( + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + TextFormField( + autofocus: true, + controller: _formController, + decoration: InputDecoration( + hintText: 'add_a_name'.tr(), + border: const OutlineInputBorder(borderRadius: BorderRadius.all(Radius.circular(8))), + ), + onChanged: (value) => _filterPeople(people, value), + onTapOutside: (event) => FocusScope.of(context).unfocus(), + ), + AnimatedSize( + duration: const Duration(milliseconds: 200), + child: SizedBox( + width: double.infinity, + child: _filteredPeople.isEmpty + // Tile instead of a blank space to avoid horizontal layout shift + ? LargeLeadingTile( + leading: const SizedBox.shrink(), + onTap: () {}, + title: const SizedBox.shrink(), + disabled: true, + ) + : Container( + margin: const EdgeInsets.only(top: 8), + decoration: BoxDecoration(borderRadius: BorderRadius.circular(8)), + child: Column( + mainAxisSize: MainAxisSize.min, + children: _filteredPeople.map((person) { + return PersonTile( + isSelected: false, + onTap: () { + if (!mounted) return; + setState(() { + _formController.text = person.name; + }); + _formController.selection = TextSelection.fromPosition( + TextPosition(offset: _formController.text.length), + ); + onMerge(context: context, person: widget.person, mergeTarget: person); + }, + personName: person.name, + personId: person.id, + ); + }).toList(), + ), + ), + ), + ), + ], + ), + ); + }, + loading: () => const Center(child: CircularProgressIndicator()), + error: (err, stack) => Text('Error: $err'), ), actions: [ TextButton( @@ -71,7 +175,7 @@ class _DriftPersonNameEditFormState extends ConsumerState onEdit(widget.person.id, _formController.text), + onPressed: () => onEdit(widget.person, _formController.text), child: Text( "save", style: TextStyle(color: context.primaryColor, fontWeight: FontWeight.bold), diff --git a/mobile/lib/presentation/widgets/people/person_merge_modal.widget.dart b/mobile/lib/presentation/widgets/people/person_merge_modal.widget.dart new file mode 100644 index 0000000000..7f9bf5444a --- /dev/null +++ b/mobile/lib/presentation/widgets/people/person_merge_modal.widget.dart @@ -0,0 +1,136 @@ +import 'package:easy_localization/easy_localization.dart'; +import 'package:flutter/material.dart'; +import 'package:fluttertoast/fluttertoast.dart'; +import 'package:hooks_riverpod/hooks_riverpod.dart'; +import 'package:immich_mobile/domain/models/person.model.dart'; +import 'package:immich_mobile/providers/infrastructure/people.provider.dart'; +import 'package:immich_mobile/services/api.service.dart'; +import 'package:immich_mobile/utils/image_url_builder.dart'; +import 'package:immich_mobile/widgets/common/immich_toast.dart'; + +class DriftPersonMergeForm extends ConsumerStatefulWidget { + final DriftPerson person; + final DriftPerson mergeTarget; + + const DriftPersonMergeForm({super.key, required this.person, required this.mergeTarget}); + + @override + ConsumerState createState() => _DriftPersonMergeFormState(); +} + +class _DriftPersonMergeFormState extends ConsumerState { + bool _isMerging = false; + + Future _mergePeople(BuildContext context) async { + setState(() => _isMerging = true); + try { + await ref + .read(driftPeopleServiceProvider) + .mergePeople(targetPersonId: widget.mergeTarget.id, mergePersonIds: [widget.person.id]); + + // Record the merge in the tracker service + ref + .read(personMergeTrackerProvider) + .recordMerge(mergedPersonId: widget.person.id, targetPersonId: widget.mergeTarget.id); + + if (mounted) { + Navigator.of(context).pop(widget.mergeTarget); + ImmichToast.show( + context: context, + msg: "merge_people_successfully".tr(), + gravity: ToastGravity.BOTTOM, + toastType: ToastType.success, + ); + } + ref.invalidate(driftGetAllPeopleProvider); + } catch (e) { + if (mounted) { + setState(() => _isMerging = false); + ImmichToast.show( + context: context, + msg: "error_title".tr(), + gravity: ToastGravity.BOTTOM, + toastType: ToastType.error, + ); + } + } + } + + @override + Widget build(BuildContext context) { + final headers = ApiService.getRequestHeaders(); + + return AlertDialog( + title: const Text("merge_people", style: TextStyle(fontWeight: FontWeight.bold)).tr(), + content: Column( + mainAxisSize: MainAxisSize.min, + children: [ + Row( + mainAxisAlignment: MainAxisAlignment.center, + children: [ + CircleAvatar( + radius: 32, + backgroundImage: NetworkImage(getFaceThumbnailUrl(widget.person.id), headers: headers), + ), + const SizedBox(width: 16), + const RotatedBox(quarterTurns: 1, child: Icon(Icons.merge_type, size: 32)), + const SizedBox(width: 16), + CircleAvatar( + radius: 32, + backgroundImage: NetworkImage(getFaceThumbnailUrl(widget.mergeTarget.id), headers: headers), + ), + ], + ), + const SizedBox(height: 24), + const Text( + "are_these_the_same_person", + style: TextStyle(fontWeight: FontWeight.w600, fontSize: 18), + textAlign: TextAlign.center, + ).tr(), + const SizedBox(height: 8), + const Text( + "they_will_be_merged_together", + style: TextStyle(fontSize: 14, color: Colors.black54), + textAlign: TextAlign.center, + ).tr(), + const SizedBox(height: 24), + Row( + children: [ + Expanded( + child: ElevatedButton( + style: ElevatedButton.styleFrom( + backgroundColor: Theme.of(context).colorScheme.onSurface, + foregroundColor: Theme.of(context).colorScheme.onInverseSurface, + elevation: 0, + ), + onPressed: _isMerging ? null : () => Navigator.of(context).pop(), + child: const Text("no", style: TextStyle(fontWeight: FontWeight.bold)).tr(), + ), + ), + const SizedBox(width: 12), + Expanded( + child: ElevatedButton( + style: ElevatedButton.styleFrom( + backgroundColor: Theme.of(context).colorScheme.primary, + foregroundColor: Theme.of(context).colorScheme.onPrimary, + ), + onPressed: _isMerging ? null : () => _mergePeople(context), + child: _isMerging + ? SizedBox( + height: 20, + width: 20, + child: CircularProgressIndicator( + strokeWidth: 2, + color: Theme.of(context).colorScheme.onPrimary, + ), + ) + : const Text("yes", style: TextStyle(fontWeight: FontWeight.bold)).tr(), + ), + ), + ], + ), + ], + ), + ); + } +} diff --git a/mobile/lib/presentation/widgets/people/person_tile.widget.dart b/mobile/lib/presentation/widgets/people/person_tile.widget.dart new file mode 100644 index 0000000000..bf5f6361f6 --- /dev/null +++ b/mobile/lib/presentation/widgets/people/person_tile.widget.dart @@ -0,0 +1,58 @@ +import 'package:flutter/material.dart'; +import 'package:immich_mobile/extensions/build_context_extensions.dart'; +import 'package:immich_mobile/pages/common/large_leading_tile.dart'; +import 'package:immich_mobile/services/api.service.dart'; +import 'package:immich_mobile/utils/image_url_builder.dart'; + +// TODO: Only pass person object, instead of id and name when PersonDto and DriftPerson are unified +class PersonTile extends StatelessWidget { + final bool isSelected; + final String personId; + final String personName; + final double imageSize; + final Function() onTap; + + const PersonTile({ + super.key, + required this.isSelected, + required this.personId, + required this.personName, + this.imageSize = 60.0, + required this.onTap, + }); + + @override + Widget build(BuildContext context) { + final headers = ApiService.getRequestHeaders(); + + return Padding( + padding: const EdgeInsets.only(bottom: 2.0), + child: LargeLeadingTile( + title: Text( + personName, + style: context.textTheme.bodyLarge?.copyWith( + fontSize: 20, + fontWeight: FontWeight.w500, + color: isSelected ? context.colorScheme.onPrimary : context.colorScheme.onSurface, + ), + ), + leading: SizedBox( + height: imageSize, + child: Material( + shape: const CircleBorder(side: BorderSide.none), + elevation: 3, + child: CircleAvatar( + maxRadius: imageSize / 2, + backgroundImage: NetworkImage(getFaceThumbnailUrl(personId), headers: headers), + ), + ), + ), + onTap: () => onTap(), + + selected: isSelected, + selectedTileColor: context.primaryColor, + tileColor: context.primaryColor.withAlpha(25), + ), + ); + } +} diff --git a/mobile/lib/providers/infrastructure/people.provider.dart b/mobile/lib/providers/infrastructure/people.provider.dart index 94a1b2447f..6d23f3db8f 100644 --- a/mobile/lib/providers/infrastructure/people.provider.dart +++ b/mobile/lib/providers/infrastructure/people.provider.dart @@ -1,6 +1,7 @@ import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/domain/models/person.model.dart'; import 'package:immich_mobile/domain/services/people.service.dart'; +import 'package:immich_mobile/domain/services/person_merge_tracker.service.dart'; import 'package:immich_mobile/infrastructure/repositories/people.repository.dart'; import 'package:immich_mobile/providers/infrastructure/db.provider.dart'; import 'package:immich_mobile/repositories/person_api.repository.dart'; @@ -22,3 +23,11 @@ final driftGetAllPeopleProvider = FutureProvider>((ref) async final service = ref.watch(driftPeopleServiceProvider); return service.getAllPeople(); }); + +final driftGetPersonByIdProvider = StreamProvider.family((ref, personId) { + return ref.watch(driftPeopleServiceProvider).watchPersonById(personId); +}); + +final personMergeTrackerProvider = Provider( + (ref) => PersonMergeTrackerService(), +); diff --git a/mobile/lib/repositories/person_api.repository.dart b/mobile/lib/repositories/person_api.repository.dart index 04e4bd2a2c..63ba15c604 100644 --- a/mobile/lib/repositories/person_api.repository.dart +++ b/mobile/lib/repositories/person_api.repository.dart @@ -21,6 +21,10 @@ class PersonApiRepository extends ApiRepository { return _toPerson(dto); } + Future merge(String targetId, List mergeIds) async { + await checkNull(_api.mergePerson(targetId, MergePersonDto(ids: mergeIds))); + } + static PersonDto _toPerson(PersonResponseDto dto) => PersonDto( birthDate: dto.birthDate, id: dto.id, diff --git a/mobile/lib/routing/router.gr.dart b/mobile/lib/routing/router.gr.dart index 4e488a30c7..2dbb2f0840 100644 --- a/mobile/lib/routing/router.gr.dart +++ b/mobile/lib/routing/router.gr.dart @@ -1280,12 +1280,12 @@ class DriftPeopleCollectionRoute extends PageRouteInfo { /// [DriftPersonPage] class DriftPersonRoute extends PageRouteInfo { DriftPersonRoute({ + required DriftPerson initialPerson, Key? key, - required DriftPerson person, List? children, }) : super( DriftPersonRoute.name, - args: DriftPersonRouteArgs(key: key, person: person), + args: DriftPersonRouteArgs(initialPerson: initialPerson, key: key), initialChildren: children, ); @@ -1295,21 +1295,21 @@ class DriftPersonRoute extends PageRouteInfo { name, builder: (data) { final args = data.argsAs(); - return DriftPersonPage(key: args.key, person: args.person); + return DriftPersonPage(args.initialPerson, key: args.key); }, ); } class DriftPersonRouteArgs { - const DriftPersonRouteArgs({this.key, required this.person}); + const DriftPersonRouteArgs({required this.initialPerson, this.key}); + + final DriftPerson initialPerson; final Key? key; - final DriftPerson person; - @override String toString() { - return 'DriftPersonRouteArgs{key: $key, person: $person}'; + return 'DriftPersonRouteArgs{initialPerson: $initialPerson, key: $key}'; } } diff --git a/mobile/lib/utils/people.utils.dart b/mobile/lib/utils/people.utils.dart index ddd1867269..7f4224b7ab 100644 --- a/mobile/lib/utils/people.utils.dart +++ b/mobile/lib/utils/people.utils.dart @@ -3,6 +3,7 @@ import 'package:immich_mobile/domain/models/person.model.dart'; import 'package:immich_mobile/extensions/translate_extensions.dart'; import 'package:immich_mobile/presentation/widgets/people/person_edit_birthday_modal.widget.dart'; import 'package:immich_mobile/presentation/widgets/people/person_edit_name_modal.widget.dart'; +import 'package:immich_mobile/presentation/widgets/people/person_merge_modal.widget.dart'; String formatAge(DateTime birthDate, DateTime referenceDate) { int ageInYears = _calculateAge(birthDate, referenceDate); @@ -33,8 +34,8 @@ int _calculateAgeInMonths(DateTime birthDate, DateTime referenceDate) { (referenceDate.day < birthDate.day ? 1 : 0); } -Future showNameEditModal(BuildContext context, DriftPerson person) { - return showDialog( +Future showNameEditModal(BuildContext context, DriftPerson person) { + return showDialog( context: context, useRootNavigator: false, builder: (BuildContext context) { @@ -52,3 +53,13 @@ Future showBirthdayEditModal(BuildContext context, DriftPerson person }, ); } + +Future showMergeModal(BuildContext context, DriftPerson person, DriftPerson mergeTarget) { + return showDialog( + context: context, + useRootNavigator: false, + builder: (BuildContext context) { + return DriftPersonMergeForm(person: person, mergeTarget: mergeTarget); + }, + ); +} diff --git a/mobile/lib/widgets/search/search_filter/people_picker.dart b/mobile/lib/widgets/search/search_filter/people_picker.dart index b2a7a18c7c..4d9c0ba2cd 100644 --- a/mobile/lib/widgets/search/search_filter/people_picker.dart +++ b/mobile/lib/widgets/search/search_filter/people_picker.dart @@ -1,14 +1,12 @@ -import 'package:flutter/material.dart'; import 'package:easy_localization/easy_localization.dart'; +import 'package:flutter/material.dart'; import 'package:flutter_hooks/flutter_hooks.dart'; import 'package:hooks_riverpod/hooks_riverpod.dart'; import 'package:immich_mobile/domain/models/person.model.dart'; import 'package:immich_mobile/extensions/asyncvalue_extensions.dart'; import 'package:immich_mobile/extensions/build_context_extensions.dart'; -import 'package:immich_mobile/pages/common/large_leading_tile.dart'; +import 'package:immich_mobile/presentation/widgets/people/person_tile.widget.dart'; import 'package:immich_mobile/providers/search/people.provider.dart'; -import 'package:immich_mobile/services/api.service.dart'; -import 'package:immich_mobile/utils/image_url_builder.dart'; import 'package:immich_mobile/widgets/common/search_field.dart'; class PeoplePicker extends HookConsumerWidget { @@ -20,10 +18,9 @@ class PeoplePicker extends HookConsumerWidget { @override Widget build(BuildContext context, WidgetRef ref) { final formFocus = useFocusNode(); - final imageSize = 60.0; + final searchQuery = useState(''); final people = ref.watch(getAllPeopleProvider); - final headers = ApiService.getRequestHeaders(); final selectedPeople = useState>(filter ?? {}); return Column( @@ -57,42 +54,19 @@ class PeoplePicker extends HookConsumerWidget { .toList()[index]; final isSelected = selectedPeople.value.contains(person); - return Padding( - padding: const EdgeInsets.only(bottom: 2.0), - child: LargeLeadingTile( - title: Text( - person.name, - style: context.textTheme.bodyLarge?.copyWith( - fontSize: 20, - fontWeight: FontWeight.w500, - color: isSelected ? context.colorScheme.onPrimary : context.colorScheme.onSurface, - ), - ), - leading: SizedBox( - height: imageSize, - child: Material( - shape: const CircleBorder(side: BorderSide.none), - elevation: 3, - child: CircleAvatar( - maxRadius: imageSize / 2, - backgroundImage: NetworkImage(getFaceThumbnailUrl(person.id), headers: headers), - ), - ), - ), - onTap: () { - if (selectedPeople.value.contains(person)) { - selectedPeople.value.remove(person); - } else { - selectedPeople.value.add(person); - } - - selectedPeople.value = {...selectedPeople.value}; - onSelect(selectedPeople.value); - }, - selected: isSelected, - selectedTileColor: context.primaryColor, - tileColor: context.primaryColor.withAlpha(25), - ), + return PersonTile( + isSelected: isSelected, + personId: person.id, + personName: person.name, + onTap: () { + if (selectedPeople.value.contains(person)) { + selectedPeople.value.remove(person); + } else { + selectedPeople.value.add(person); + } + selectedPeople.value = {...selectedPeople.value}; + onSelect(selectedPeople.value); + }, ); }, ); From b15fa4a4add58c2170aa72aff05a3cb02dfbde39 Mon Sep 17 00:00:00 2001 From: Marvin M <39344769+M123-dev@users.noreply.github.com> Date: Sun, 5 Oct 2025 15:00:51 +0200 Subject: [PATCH 2/8] fix: Don't merge with itself --- .../person_merge_tracker.service.dart | 6 +- .../presentation/pages/drift_person.page.dart | 97 ++++++++++--------- .../sheet_people_details.widget.dart | 20 ++-- .../people/person_edit_name_modal.widget.dart | 59 ++++++----- 4 files changed, 97 insertions(+), 85 deletions(-) diff --git a/mobile/lib/domain/services/person_merge_tracker.service.dart b/mobile/lib/domain/services/person_merge_tracker.service.dart index 04d90aedf6..a53d39cbe9 100644 --- a/mobile/lib/domain/services/person_merge_tracker.service.dart +++ b/mobile/lib/domain/services/person_merge_tracker.service.dart @@ -9,11 +9,11 @@ /// So when popping back to the profile page (and the user is missing) we check /// which other person B we have to display instead. class PersonMergeTrackerService { - // Map of merged person ID -> target person ID + /// Map of merged person ID -> target person ID final Map _mergeForwardingMap = {}; - // Set of person IDs for which the merge record has been handled (redirected) - // To prevent multiple redirects for the same merge + /// Set of person IDs for which the merge record has been handled (redirected) + /// To prevent multiple redirects for the same merge final Set _handledMergeRecords = {}; /// Record a person merge operation diff --git a/mobile/lib/presentation/pages/drift_person.page.dart b/mobile/lib/presentation/pages/drift_person.page.dart index bbce31d8ba..ea8d12d3b3 100644 --- a/mobile/lib/presentation/pages/drift_person.page.dart +++ b/mobile/lib/presentation/pages/drift_person.page.dart @@ -71,8 +71,8 @@ class _DriftPersonPageState extends ConsumerState { ref.watch(currentRouteNameProvider.select((name) => name ?? DriftPersonRoute.name)); return personAsync.when( - data: (personByProvider) { - if (personByProvider == null) { + data: (personByIdProvider) { + if (personByIdProvider == null) { // Check if the person was merged and redirect if necessary final shouldRedirect = mergeTracker.shouldRedirectForPerson(_person.id); final targetPersonId = mergeTracker.getTargetPersonId(_person.id); @@ -81,56 +81,60 @@ class _DriftPersonPageState extends ConsumerState { bool isOnPersonDetailPage = ModalRoute.of(context)?.isCurrent ?? false; // Only redirect if we're currently on the person detail page, not in a nested view, e.g. image viewer - if (isOnPersonDetailPage) { - // Person was merged, redirect to the target person - WidgetsBinding.instance.addPostFrameCallback((_) { - if (mounted) { - // Use the service directly to get the target person - ref - .read(driftPeopleServiceProvider) - .watchPersonById(targetPersonId) - .first - .then((targetPerson) { - if (targetPerson != null && mounted) { - // Mark the merge record as handled - mergeTracker.markMergeRecordHandled(_person.id); - _person = targetPerson; - setState(() {}); - } else { - // Target person not found, go back - context.maybePop(); - } - }) - .catchError((error) { - // If we can't load the target person, go back - mergeLogger.severe("Error during read of targetPerson", error); - if (mounted) { - context.maybePop(); - } - }); - } - }); - return const Center(child: CircularProgressIndicator()); - } else { - // We're in an image viewer or other nested view, don't redirect yet - // Just show loading spinner to indicate something is happening + if (!isOnPersonDetailPage) { return const Center(child: CircularProgressIndicator()); } - } + // Person was merged, redirect to the target person + WidgetsBinding.instance.addPostFrameCallback((_) { + if (mounted) { + ref + .read(driftPeopleServiceProvider) + .watchPersonById(targetPersonId) + .first + .then((targetPerson) { + if (targetPerson != null && mounted) { + // Mark the merge record as handled + mergeTracker.markMergeRecordHandled(_person.id); - // Person not found and no merge record - mergeLogger.info( - 'Person ${_person.name} (${_person.id}) not found and no merge records exist, it was probably deleted', - ); - WidgetsBinding.instance.addPostFrameCallback((_) { - if (mounted) { - context.maybePop(); - } - }); + // Open the target person's page + if (mounted) { + context.replaceRoute( + DriftPersonRoute(key: ValueKey(targetPerson.id), initialPerson: targetPerson), + ); + } + } else { + // Target person not found, go back + context.maybePop(); + } + }) + .catchError((error) { + // If we can't load the target person, go back + mergeLogger.severe("Error during read of targetPerson", error); + if (mounted) { + context.maybePop(); + } + }); + } + }); + return const Center(child: CircularProgressIndicator()); + } else if (shouldRedirect && targetPersonId == null) { + // This should never happen, but just in case + // Person not found and no merge record + mergeLogger.info( + 'Person ${_person.name} (${_person.id}) not found and no merge records exist, it was probably deleted', + ); + WidgetsBinding.instance.addPostFrameCallback((_) { + if (mounted) { + context.maybePop(); + } + }); + return const Center(child: CircularProgressIndicator()); + } + // Waiting for the personByProvider to load return const Center(child: CircularProgressIndicator()); } - _person = personByProvider; + _person = personByIdProvider; return ProviderScope( overrides: [ timelineServiceProvider.overrideWith((ref) { @@ -154,7 +158,6 @@ class _DriftPersonPageState extends ConsumerState { ), ); }, - // TODO(m123): Show initialPerson data while loading new data (optimistic ui update, but we need to handle scroll state etc) loading: () => const Center(child: CircularProgressIndicator()), error: (e, s) => Text('Error: $e'), ); diff --git a/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart b/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart index 0871db3510..50443848a0 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart @@ -61,7 +61,6 @@ class _SheetPeopleDetailsState extends ConsumerState { final previousRouteData = ref.read(previousRouteDataProvider); final previousRouteArgs = previousRouteData?.arguments; - // TODO: Check what happens if the person id from the previous route is not the correct one anymore e.g. after a merge // Prevent circular navigation if (previousRouteArgs is DriftPersonRouteArgs && previousRouteArgs.initialPerson.id == person.id) { @@ -72,24 +71,25 @@ class _SheetPeopleDetailsState extends ConsumerState { context.pushRoute(DriftPersonRoute(initialPerson: person)); }, onNameTap: () async { - DriftPerson? newPerson = await showNameEditModal(context, person); - - ref.invalidate(driftPeopleAssetProvider(asset.id)); - - // If the name edit resulted in a new person (e.g. from merging) - // And if we are currently nested below the drift person page if said - // old person id, we need to pop, otherwise the timeline provider - // complains because the indexes are off - // TODO: Preferably we would replace the timeline provider, and let it listen to the new person id (Relevant function is the ```TimelineService person(String userId, String personId)``` in timeline.service.dart) + // Needs to be before the modal, as this overwrites the previousRouteDataProvider final previousRouteData = ref.read(previousRouteDataProvider); final previousRouteArgs = previousRouteData?.arguments; final previousPersonId = previousRouteArgs is DriftPersonRouteArgs ? previousRouteArgs.initialPerson.id : null; + DriftPerson? newPerson = await showNameEditModal(context, person); + + // If the name edit resulted in a new person (e.g. from merging) + // And if we are currently nested below the drift person page if said + // old person id, we need to pop, otherwise the timeline provider complains + // and the asset viewer goes black + // TODO: Preferably we would replace the timeline provider, and let it listen to the new person id (Relevant function is the ```TimelineService person(String userId, String personId)``` in timeline.service.dart) if (newPerson != null && newPerson.id != person.id && previousPersonId == person.id) { context.pop(); } + + ref.invalidate(driftPeopleAssetProvider(asset.id)); }, ), ], diff --git a/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart b/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart index 28a0f055d7..9d3b9f736c 100644 --- a/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart +++ b/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart @@ -11,6 +11,7 @@ import 'package:immich_mobile/providers/infrastructure/people.provider.dart'; import 'package:immich_mobile/utils/debug_print.dart'; import 'package:immich_mobile/utils/people.utils.dart'; import 'package:immich_mobile/widgets/common/immich_toast.dart'; +import 'package:logging/logging.dart'; class DriftPersonNameEditForm extends ConsumerStatefulWidget { final DriftPerson person; @@ -72,7 +73,7 @@ class _DriftPersonNameEditFormState extends ConsumerState people, String query) { final queryParts = query.toLowerCase().split(' ').where((e) => e.isNotEmpty).toList(); @@ -80,7 +81,10 @@ class _DriftPersonNameEditFormState extends ConsumerState containsMatches = []; for (final p in people) { + if (p.id == widget.person.id) continue; + final nameParts = p.name.toLowerCase().split(' ').where((e) => e.isNotEmpty).toList(); + final allStart = queryParts.every((q) => nameParts.any((n) => n.startsWith(q))); final allContain = queryParts.every((q) => nameParts.any((n) => n.contains(q))); @@ -94,7 +98,7 @@ class _DriftPersonNameEditFormState extends ConsumerState people = []; return AlertDialog( title: const Text("edit_name", style: TextStyle(fontWeight: FontWeight.bold)).tr(), - content: curatedPeople.when( - data: (people) { - return SingleChildScrollView( - child: Column( - mainAxisSize: MainAxisSize.min, - children: [ - TextFormField( - autofocus: true, - controller: _formController, - decoration: InputDecoration( - hintText: 'add_a_name'.tr(), - border: const OutlineInputBorder(borderRadius: BorderRadius.all(Radius.circular(8))), - ), - onChanged: (value) => _filterPeople(people, value), - onTapOutside: (event) => FocusScope.of(context).unfocus(), - ), - AnimatedSize( + content: SingleChildScrollView( + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + TextFormField( + autofocus: true, + controller: _formController, + decoration: InputDecoration( + hintText: 'add_a_name'.tr(), + border: const OutlineInputBorder(borderRadius: BorderRadius.all(Radius.circular(8))), + ), + onChanged: (value) => _filterPeople(people, value), + onTapOutside: (event) => FocusScope.of(context).unfocus(), + ), + curatedPeople.when( + data: (p) { + people = p; + return AnimatedSize( duration: const Duration(milliseconds: 200), child: SizedBox( width: double.infinity, @@ -158,13 +164,16 @@ class _DriftPersonNameEditFormState extends ConsumerState const Center(child: CircularProgressIndicator()), + error: (err, stack) { + Logger('PersonEditNameModal').warning('Error loading people for name edit modal', err, stack); + return Center(child: Text('Error loading people for name edit modal: $err')); + }, ), - ); - }, - loading: () => const Center(child: CircularProgressIndicator()), - error: (err, stack) => Text('Error: $err'), + ], + ), ), actions: [ TextButton( From 69c4460a03051e32b3b5acb8beb386bca3b60ba4 Mon Sep 17 00:00:00 2001 From: Marvin M <39344769+M123-dev@users.noreply.github.com> Date: Sun, 5 Oct 2025 15:14:49 +0200 Subject: [PATCH 3/8] chore: Prefer 'const BorderRadius.all' to non-const constructors. --- .../widgets/people/person_edit_name_modal.widget.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart b/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart index 9d3b9f736c..11b31154a9 100644 --- a/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart +++ b/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart @@ -141,7 +141,7 @@ class _DriftPersonNameEditFormState extends ConsumerState Date: Sun, 5 Oct 2025 15:18:07 +0200 Subject: [PATCH 4/8] chore: Comments --- .../lib/domain/services/person_merge_tracker.service.dart | 6 +++--- mobile/lib/presentation/pages/drift_person.page.dart | 1 - 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/mobile/lib/domain/services/person_merge_tracker.service.dart b/mobile/lib/domain/services/person_merge_tracker.service.dart index a53d39cbe9..ca9171c284 100644 --- a/mobile/lib/domain/services/person_merge_tracker.service.dart +++ b/mobile/lib/domain/services/person_merge_tracker.service.dart @@ -2,11 +2,11 @@ /// Say we open the profile page (drift_person.page.dart) for Person A, and then nested above /// a image viewer for an image that belongs to Person A. /// -/// When the users now merges user A into user B, we cant just listen to -/// the changes in the profile page, we have to keep track where the user A (now B) +/// When the users now merges user A into user B, we can't just listen to +/// the changes in the profile page, we have to keep track of where the user A (now B) /// can be found in the DB. /// -/// So when popping back to the profile page (and the user is missing) we check +/// So when popping back to the profile page (and the user is missing), we check /// which other person B we have to display instead. class PersonMergeTrackerService { /// Map of merged person ID -> target person ID diff --git a/mobile/lib/presentation/pages/drift_person.page.dart b/mobile/lib/presentation/pages/drift_person.page.dart index ea8d12d3b3..12e7e506e1 100644 --- a/mobile/lib/presentation/pages/drift_person.page.dart +++ b/mobile/lib/presentation/pages/drift_person.page.dart @@ -93,7 +93,6 @@ class _DriftPersonPageState extends ConsumerState { .first .then((targetPerson) { if (targetPerson != null && mounted) { - // Mark the merge record as handled mergeTracker.markMergeRecordHandled(_person.id); // Open the target person's page From d6520350549632ab18a6610f8024186de402eed6 Mon Sep 17 00:00:00 2001 From: Marvin M <39344769+M123-dev@users.noreply.github.com> Date: Sun, 5 Oct 2025 15:19:42 +0200 Subject: [PATCH 5/8] chore: Comments --- .../widgets/people/person_edit_name_modal.widget.dart | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart b/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart index 11b31154a9..875b52471a 100644 --- a/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart +++ b/mobile/lib/presentation/widgets/people/person_edit_name_modal.widget.dart @@ -98,7 +98,7 @@ class _DriftPersonNameEditFormState extends ConsumerState Date: Sun, 5 Oct 2025 15:39:13 +0200 Subject: [PATCH 6/8] test: PersonMergeTrackerService --- .../person_merge_tracker_service_test.dart | 30 +++++++++++++++++++ 1 file changed, 30 insertions(+) create mode 100644 mobile/test/person_merge_tracker_service_test.dart diff --git a/mobile/test/person_merge_tracker_service_test.dart b/mobile/test/person_merge_tracker_service_test.dart new file mode 100644 index 0000000000..bd1a1111a8 --- /dev/null +++ b/mobile/test/person_merge_tracker_service_test.dart @@ -0,0 +1,30 @@ +import 'package:flutter_test/flutter_test.dart'; +import 'package:immich_mobile/domain/services/person_merge_tracker.service.dart'; + +void main() { + group('PersonMergeTrackerService', () { + late PersonMergeTrackerService mergeTracker; + + setUp(() { + mergeTracker = PersonMergeTrackerService(); + }); + + test('records and retrieves merge correctly', () { + mergeTracker.recordMerge(mergedPersonId: 'A', targetPersonId: 'B'); + + expect(mergeTracker.isPersonMerged('A'), isTrue); + expect(mergeTracker.getTargetPersonId('A'), equals('B')); + expect(mergeTracker.shouldRedirectForPerson('A'), isTrue); + + mergeTracker.markMergeRecordHandled('A'); + expect(mergeTracker.isMergeRecordHandled('A'), isTrue); + expect(mergeTracker.shouldRedirectForPerson('A'), isFalse); + }); + + test('returns false for non-merged person', () { + expect(mergeTracker.isPersonMerged('X'), isFalse); + expect(mergeTracker.getTargetPersonId('X'), isNull); + expect(mergeTracker.shouldRedirectForPerson('X'), isFalse); + }); + }); +} From db727eb2e8c35eacc8d64eff775a9a01f8184330 Mon Sep 17 00:00:00 2001 From: Marvin M <39344769+M123-dev@users.noreply.github.com> Date: Sun, 5 Oct 2025 16:25:37 +0200 Subject: [PATCH 7/8] chore: Format people_provider --- mobile/lib/providers/infrastructure/people.provider.dart | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/mobile/lib/providers/infrastructure/people.provider.dart b/mobile/lib/providers/infrastructure/people.provider.dart index 6d23f3db8f..962e6da4ea 100644 --- a/mobile/lib/providers/infrastructure/people.provider.dart +++ b/mobile/lib/providers/infrastructure/people.provider.dart @@ -28,6 +28,4 @@ final driftGetPersonByIdProvider = StreamProvider.family(( return ref.watch(driftPeopleServiceProvider).watchPersonById(personId); }); -final personMergeTrackerProvider = Provider( - (ref) => PersonMergeTrackerService(), -); +final personMergeTrackerProvider = Provider((ref) => PersonMergeTrackerService()); From 0300f23834c723eb2a2d148300f37708d1ab7872 Mon Sep 17 00:00:00 2001 From: Marvin M <39344769+M123-dev@users.noreply.github.com> Date: Sat, 11 Oct 2025 12:29:29 +0200 Subject: [PATCH 8/8] chore: Simplification --- .../person_merge_tracker.service.dart | 24 --------------- .../presentation/pages/drift_person.page.dart | 19 ++++-------- .../sheet_people_details.widget.dart | 2 +- .../person_merge_tracker_service_test.dart | 30 ------------------- 4 files changed, 6 insertions(+), 69 deletions(-) delete mode 100644 mobile/test/person_merge_tracker_service_test.dart diff --git a/mobile/lib/domain/services/person_merge_tracker.service.dart b/mobile/lib/domain/services/person_merge_tracker.service.dart index ca9171c284..6347e14f37 100644 --- a/mobile/lib/domain/services/person_merge_tracker.service.dart +++ b/mobile/lib/domain/services/person_merge_tracker.service.dart @@ -12,10 +12,6 @@ class PersonMergeTrackerService { /// Map of merged person ID -> target person ID final Map _mergeForwardingMap = {}; - /// Set of person IDs for which the merge record has been handled (redirected) - /// To prevent multiple redirects for the same merge - final Set _handledMergeRecords = {}; - /// Record a person merge operation void recordMerge({required String mergedPersonId, required String targetPersonId}) { _mergeForwardingMap[mergedPersonId] = targetPersonId; @@ -25,24 +21,4 @@ class PersonMergeTrackerService { String? getTargetPersonId(String personId) { return _mergeForwardingMap[personId]; } - - /// Check if a person ID has been merged - bool isPersonMerged(String personId) { - return _mergeForwardingMap.containsKey(personId); - } - - /// Check if a merge record has been handled (redirected) - bool isMergeRecordHandled(String personId) { - return _handledMergeRecords.contains(personId); - } - - /// Check if we should redirect for this person (merged but not yet handled) - bool shouldRedirectForPerson(String personId) { - return isPersonMerged(personId) && !isMergeRecordHandled(personId); - } - - /// Mark a merge record as handled (for tracking purposes) - void markMergeRecordHandled(String personId) { - _handledMergeRecords.add(personId); - } } diff --git a/mobile/lib/presentation/pages/drift_person.page.dart b/mobile/lib/presentation/pages/drift_person.page.dart index 12e7e506e1..d399b66a67 100644 --- a/mobile/lib/presentation/pages/drift_person.page.dart +++ b/mobile/lib/presentation/pages/drift_person.page.dart @@ -74,10 +74,8 @@ class _DriftPersonPageState extends ConsumerState { data: (personByIdProvider) { if (personByIdProvider == null) { // Check if the person was merged and redirect if necessary - final shouldRedirect = mergeTracker.shouldRedirectForPerson(_person.id); final targetPersonId = mergeTracker.getTargetPersonId(_person.id); - - if (shouldRedirect && targetPersonId != null) { + if (targetPersonId != null) { bool isOnPersonDetailPage = ModalRoute.of(context)?.isCurrent ?? false; // Only redirect if we're currently on the person detail page, not in a nested view, e.g. image viewer @@ -93,13 +91,10 @@ class _DriftPersonPageState extends ConsumerState { .first .then((targetPerson) { if (targetPerson != null && mounted) { - mergeTracker.markMergeRecordHandled(_person.id); - // Open the target person's page if (mounted) { - context.replaceRoute( - DriftPersonRoute(key: ValueKey(targetPerson.id), initialPerson: targetPerson), - ); + context.pop(); + context.pushRoute(DriftPersonRoute(initialPerson: targetPerson)); } } else { // Target person not found, go back @@ -116,12 +111,11 @@ class _DriftPersonPageState extends ConsumerState { } }); return const Center(child: CircularProgressIndicator()); - } else if (shouldRedirect && targetPersonId == null) { - // This should never happen, but just in case - // Person not found and no merge record + } else { mergeLogger.info( 'Person ${_person.name} (${_person.id}) not found and no merge records exist, it was probably deleted', ); + WidgetsBinding.instance.addPostFrameCallback((_) { if (mounted) { context.maybePop(); @@ -129,10 +123,7 @@ class _DriftPersonPageState extends ConsumerState { }); return const Center(child: CircularProgressIndicator()); } - // Waiting for the personByProvider to load - return const Center(child: CircularProgressIndicator()); } - _person = personByIdProvider; return ProviderScope( overrides: [ diff --git a/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart b/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart index 50443848a0..4d60da36d0 100644 --- a/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart +++ b/mobile/lib/presentation/widgets/asset_viewer/bottom_sheet/sheet_people_details.widget.dart @@ -86,7 +86,7 @@ class _SheetPeopleDetailsState extends ConsumerState { // and the asset viewer goes black // TODO: Preferably we would replace the timeline provider, and let it listen to the new person id (Relevant function is the ```TimelineService person(String userId, String personId)``` in timeline.service.dart) if (newPerson != null && newPerson.id != person.id && previousPersonId == person.id) { - context.pop(); + await context.maybePop(); } ref.invalidate(driftPeopleAssetProvider(asset.id)); diff --git a/mobile/test/person_merge_tracker_service_test.dart b/mobile/test/person_merge_tracker_service_test.dart deleted file mode 100644 index bd1a1111a8..0000000000 --- a/mobile/test/person_merge_tracker_service_test.dart +++ /dev/null @@ -1,30 +0,0 @@ -import 'package:flutter_test/flutter_test.dart'; -import 'package:immich_mobile/domain/services/person_merge_tracker.service.dart'; - -void main() { - group('PersonMergeTrackerService', () { - late PersonMergeTrackerService mergeTracker; - - setUp(() { - mergeTracker = PersonMergeTrackerService(); - }); - - test('records and retrieves merge correctly', () { - mergeTracker.recordMerge(mergedPersonId: 'A', targetPersonId: 'B'); - - expect(mergeTracker.isPersonMerged('A'), isTrue); - expect(mergeTracker.getTargetPersonId('A'), equals('B')); - expect(mergeTracker.shouldRedirectForPerson('A'), isTrue); - - mergeTracker.markMergeRecordHandled('A'); - expect(mergeTracker.isMergeRecordHandled('A'), isTrue); - expect(mergeTracker.shouldRedirectForPerson('A'), isFalse); - }); - - test('returns false for non-merged person', () { - expect(mergeTracker.isPersonMerged('X'), isFalse); - expect(mergeTracker.getTargetPersonId('X'), isNull); - expect(mergeTracker.shouldRedirectForPerson('X'), isFalse); - }); - }); -}