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] 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); - }); - }); -}