fix: Don't merge with itself

This commit is contained in:
Marvin M 2025-10-05 15:00:51 +02:00
parent ca4a75abdf
commit b15fa4a4ad
4 changed files with 97 additions and 85 deletions

View file

@ -9,11 +9,11 @@
/// 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. /// which other person B we have to display instead.
class PersonMergeTrackerService { class PersonMergeTrackerService {
// Map of merged person ID -> target person ID /// Map of merged person ID -> target person ID
final Map<String, String> _mergeForwardingMap = {}; final Map<String, String> _mergeForwardingMap = {};
// Set of person IDs for which the merge record has been handled (redirected) /// Set of person IDs for which the merge record has been handled (redirected)
// To prevent multiple redirects for the same merge /// To prevent multiple redirects for the same merge
final Set<String> _handledMergeRecords = {}; final Set<String> _handledMergeRecords = {};
/// Record a person merge operation /// Record a person merge operation

View file

@ -71,8 +71,8 @@ class _DriftPersonPageState extends ConsumerState<DriftPersonPage> {
ref.watch(currentRouteNameProvider.select((name) => name ?? DriftPersonRoute.name)); ref.watch(currentRouteNameProvider.select((name) => name ?? DriftPersonRoute.name));
return personAsync.when( return personAsync.when(
data: (personByProvider) { data: (personByIdProvider) {
if (personByProvider == null) { if (personByIdProvider == null) {
// Check if the person was merged and redirect if necessary // Check if the person was merged and redirect if necessary
final shouldRedirect = mergeTracker.shouldRedirectForPerson(_person.id); final shouldRedirect = mergeTracker.shouldRedirectForPerson(_person.id);
final targetPersonId = mergeTracker.getTargetPersonId(_person.id); final targetPersonId = mergeTracker.getTargetPersonId(_person.id);
@ -81,11 +81,12 @@ class _DriftPersonPageState extends ConsumerState<DriftPersonPage> {
bool isOnPersonDetailPage = ModalRoute.of(context)?.isCurrent ?? false; 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 // Only redirect if we're currently on the person detail page, not in a nested view, e.g. image viewer
if (isOnPersonDetailPage) { if (!isOnPersonDetailPage) {
return const Center(child: CircularProgressIndicator());
}
// Person was merged, redirect to the target person // Person was merged, redirect to the target person
WidgetsBinding.instance.addPostFrameCallback((_) { WidgetsBinding.instance.addPostFrameCallback((_) {
if (mounted) { if (mounted) {
// Use the service directly to get the target person
ref ref
.read(driftPeopleServiceProvider) .read(driftPeopleServiceProvider)
.watchPersonById(targetPersonId) .watchPersonById(targetPersonId)
@ -94,8 +95,13 @@ class _DriftPersonPageState extends ConsumerState<DriftPersonPage> {
if (targetPerson != null && mounted) { if (targetPerson != null && mounted) {
// Mark the merge record as handled // Mark the merge record as handled
mergeTracker.markMergeRecordHandled(_person.id); mergeTracker.markMergeRecordHandled(_person.id);
_person = targetPerson;
setState(() {}); // Open the target person's page
if (mounted) {
context.replaceRoute(
DriftPersonRoute(key: ValueKey(targetPerson.id), initialPerson: targetPerson),
);
}
} else { } else {
// Target person not found, go back // Target person not found, go back
context.maybePop(); context.maybePop();
@ -111,13 +117,8 @@ class _DriftPersonPageState extends ConsumerState<DriftPersonPage> {
} }
}); });
return const Center(child: CircularProgressIndicator()); return const Center(child: CircularProgressIndicator());
} else { } else if (shouldRedirect && targetPersonId == null) {
// We're in an image viewer or other nested view, don't redirect yet // This should never happen, but just in case
// Just show loading spinner to indicate something is happening
return const Center(child: CircularProgressIndicator());
}
}
// Person not found and no merge record // Person not found and no merge record
mergeLogger.info( mergeLogger.info(
'Person ${_person.name} (${_person.id}) not found and no merge records exist, it was probably deleted', 'Person ${_person.name} (${_person.id}) not found and no merge records exist, it was probably deleted',
@ -129,8 +130,11 @@ class _DriftPersonPageState extends ConsumerState<DriftPersonPage> {
}); });
return const Center(child: CircularProgressIndicator()); return const Center(child: CircularProgressIndicator());
} }
// Waiting for the personByProvider to load
return const Center(child: CircularProgressIndicator());
}
_person = personByProvider; _person = personByIdProvider;
return ProviderScope( return ProviderScope(
overrides: [ overrides: [
timelineServiceProvider.overrideWith((ref) { timelineServiceProvider.overrideWith((ref) {
@ -154,7 +158,6 @@ class _DriftPersonPageState extends ConsumerState<DriftPersonPage> {
), ),
); );
}, },
// 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()), loading: () => const Center(child: CircularProgressIndicator()),
error: (e, s) => Text('Error: $e'), error: (e, s) => Text('Error: $e'),
); );

View file

@ -61,7 +61,6 @@ class _SheetPeopleDetailsState extends ConsumerState<SheetPeopleDetails> {
final previousRouteData = ref.read(previousRouteDataProvider); final previousRouteData = ref.read(previousRouteDataProvider);
final previousRouteArgs = previousRouteData?.arguments; 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 // Prevent circular navigation
if (previousRouteArgs is DriftPersonRouteArgs && if (previousRouteArgs is DriftPersonRouteArgs &&
previousRouteArgs.initialPerson.id == person.id) { previousRouteArgs.initialPerson.id == person.id) {
@ -72,24 +71,25 @@ class _SheetPeopleDetailsState extends ConsumerState<SheetPeopleDetails> {
context.pushRoute(DriftPersonRoute(initialPerson: person)); context.pushRoute(DriftPersonRoute(initialPerson: person));
}, },
onNameTap: () async { onNameTap: () async {
DriftPerson? newPerson = await showNameEditModal(context, person); // Needs to be before the modal, as this overwrites the previousRouteDataProvider
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 previousRouteData = ref.read(previousRouteDataProvider);
final previousRouteArgs = previousRouteData?.arguments; final previousRouteArgs = previousRouteData?.arguments;
final previousPersonId = previousRouteArgs is DriftPersonRouteArgs final previousPersonId = previousRouteArgs is DriftPersonRouteArgs
? previousRouteArgs.initialPerson.id ? previousRouteArgs.initialPerson.id
: null; : 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) { if (newPerson != null && newPerson.id != person.id && previousPersonId == person.id) {
context.pop(); context.pop();
} }
ref.invalidate(driftPeopleAssetProvider(asset.id));
}, },
), ),
], ],

View file

@ -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/debug_print.dart';
import 'package:immich_mobile/utils/people.utils.dart'; import 'package:immich_mobile/utils/people.utils.dart';
import 'package:immich_mobile/widgets/common/immich_toast.dart'; import 'package:immich_mobile/widgets/common/immich_toast.dart';
import 'package:logging/logging.dart';
class DriftPersonNameEditForm extends ConsumerStatefulWidget { class DriftPersonNameEditForm extends ConsumerStatefulWidget {
final DriftPerson person; final DriftPerson person;
@ -72,7 +73,7 @@ class _DriftPersonNameEditFormState extends ConsumerState<DriftPersonNameEditFor
} }
} }
// TODO: Add diacritic filtering? We would need to add a package. // TODO: Add diacritic filtering?
void _filterPeople(List<DriftPerson> people, String query) { void _filterPeople(List<DriftPerson> people, String query) {
final queryParts = query.toLowerCase().split(' ').where((e) => e.isNotEmpty).toList(); final queryParts = query.toLowerCase().split(' ').where((e) => e.isNotEmpty).toList();
@ -80,7 +81,10 @@ class _DriftPersonNameEditFormState extends ConsumerState<DriftPersonNameEditFor
List<DriftPerson> containsMatches = []; List<DriftPerson> containsMatches = [];
for (final p in people) { for (final p in people) {
if (p.id == widget.person.id) continue;
final nameParts = p.name.toLowerCase().split(' ').where((e) => e.isNotEmpty).toList(); final nameParts = p.name.toLowerCase().split(' ').where((e) => e.isNotEmpty).toList();
final allStart = queryParts.every((q) => nameParts.any((n) => n.startsWith(q))); final allStart = queryParts.every((q) => nameParts.any((n) => n.startsWith(q)));
final allContain = queryParts.every((q) => nameParts.any((n) => n.contains(q))); final allContain = queryParts.every((q) => nameParts.any((n) => n.contains(q)));
@ -94,7 +98,7 @@ class _DriftPersonNameEditFormState extends ConsumerState<DriftPersonNameEditFor
if (!mounted) return; if (!mounted) return;
setState(() { setState(() {
// TODO: What happens if there are more than 3 matches with the exact same name? // TODO: happens if there are more than 3 matches with the exact same name?
_filteredPeople = query.isEmpty ? [] : (startsWithMatches + containsMatches).take(3).toList(); _filteredPeople = query.isEmpty ? [] : (startsWithMatches + containsMatches).take(3).toList();
}); });
} }
@ -102,12 +106,11 @@ class _DriftPersonNameEditFormState extends ConsumerState<DriftPersonNameEditFor
@override @override
Widget build(BuildContext context) { Widget build(BuildContext context) {
final curatedPeople = ref.watch(driftGetAllPeopleProvider); final curatedPeople = ref.watch(driftGetAllPeopleProvider);
List<DriftPerson> people = [];
return AlertDialog( return AlertDialog(
title: const Text("edit_name", style: TextStyle(fontWeight: FontWeight.bold)).tr(), title: const Text("edit_name", style: TextStyle(fontWeight: FontWeight.bold)).tr(),
content: curatedPeople.when( content: SingleChildScrollView(
data: (people) {
return SingleChildScrollView(
child: Column( child: Column(
mainAxisSize: MainAxisSize.min, mainAxisSize: MainAxisSize.min,
children: [ children: [
@ -121,7 +124,10 @@ class _DriftPersonNameEditFormState extends ConsumerState<DriftPersonNameEditFor
onChanged: (value) => _filterPeople(people, value), onChanged: (value) => _filterPeople(people, value),
onTapOutside: (event) => FocusScope.of(context).unfocus(), onTapOutside: (event) => FocusScope.of(context).unfocus(),
), ),
AnimatedSize( curatedPeople.when(
data: (p) {
people = p;
return AnimatedSize(
duration: const Duration(milliseconds: 200), duration: const Duration(milliseconds: 200),
child: SizedBox( child: SizedBox(
width: double.infinity, width: double.infinity,
@ -158,13 +164,16 @@ class _DriftPersonNameEditFormState extends ConsumerState<DriftPersonNameEditFor
), ),
), ),
), ),
),
],
),
); );
}, },
loading: () => const Center(child: CircularProgressIndicator()), loading: () => const Center(child: CircularProgressIndicator()),
error: (err, stack) => Text('Error: $err'), 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'));
},
),
],
),
), ),
actions: [ actions: [
TextButton( TextButton(