fix(mobile): proper syncing with Recents album on iOS (#2020)

* fix(mobile): deal with Recents album on iOS

* feature(mobile): local asset sync logging

* add comments

* delete ExifInfo when deleting Asset

---------

Co-authored-by: Fynn Petersen-Frey <zoodyy@users.noreply.github.com>

mobile/lib/modules/album/services/album.service.dart

@@ -1,4 +1,6 @@
 import 'dart:async';
+import 'dart:collection';
+import 'dart:io';
 
 import 'package:collection/collection.dart';
 import 'package:flutter/foundation.dart';
@@ -15,6 +17,7 @@ import 'package:immich_mobile/shared/services/api.service.dart';
 import 'package:immich_mobile/shared/services/sync.service.dart';
 import 'package:immich_mobile/shared/services/user.service.dart';
 import 'package:isar/isar.dart';
+import 'package:logging/logging.dart';
 import 'package:openapi/api.dart';
 import 'package:photo_manager/photo_manager.dart';
 
@@ -34,6 +37,7 @@ class AlbumService {
   final SyncService _syncService;
   final Isar _db;
   final BackupService _backupService;
+  final Logger _log = Logger('AlbumService');
   Completer<bool> _localCompleter = Completer()..complete(false);
   Completer<bool> _remoteCompleter = Completer()..complete(false);
 
@@ -50,6 +54,7 @@ class AlbumService {
   Future<bool> refreshDeviceAlbums() async {
     if (!_localCompleter.isCompleted) {
       // guard against concurrent calls
+      _log.info("refreshDeviceAlbums is already in progress");
       return _localCompleter.future;
     }
     _localCompleter = Completer();
@@ -68,22 +73,45 @@ class AlbumService {
         hasAll: true,
         filterOption: FilterOptionGroup(containsPathModified: true),
       );
+      _log.info("Found ${onDevice.length} device albums");
+      Set<String>? excludedAssets;
       if (excludedIds.isNotEmpty) {
+        if (Platform.isIOS) {
+          // iOS and Android device album working principle differ significantly
+          // on iOS, an asset can be in multiple albums
+          // on Android, an asset can only be in exactly one album (folder!) at the same time
+          // thus, on Android, excluding an album can be done by ignoring that album
+          // however, on iOS, it it necessary to load the assets from all excluded
+          // albums and check every asset from any selected album against the set
+          // of excluded assets
+          excludedAssets = await _loadExcludedAssetIds(onDevice, excludedIds);
+          _log.info("Found ${excludedAssets.length} assets to exclude");
+        }
         // remove all excluded albums
         onDevice.removeWhere((e) => excludedIds.contains(e.id));
+        _log.info(
+          "Ignoring ${excludedIds.length} excluded albums resulting in ${onDevice.length} device albums",
+        );
       }
       final hasAll = selectedIds
           .map((id) => onDevice.firstWhereOrNull((a) => a.id == id))
           .whereNotNull()
           .any((a) => a.isAll);
       if (hasAll) {
-        // remove the virtual "Recents" album and keep and individual albums
-        onDevice.removeWhere((e) => e.isAll);
+        if (Platform.isAndroid) {
+          // remove the virtual "Recent" album and keep and individual albums
+          // on Android, the virtual "Recent" `lastModified` value is always null
+          onDevice.removeWhere((e) => e.isAll);
+          _log.info("'Recents' is selected, keeping all individual albums");
+        }
       } else {
         // keep only the explicitly selected albums
         onDevice.removeWhere((e) => !selectedIds.contains(e.id));
+        _log.info("'Recents' is not selected, keeping only selected albums");
       }
-      changes = await _syncService.syncLocalAlbumAssetsToDb(onDevice);
+      changes =
+          await _syncService.syncLocalAlbumAssetsToDb(onDevice, excludedAssets);
+      _log.info("Syncing completed. Changes: $changes");
     } finally {
       _localCompleter.complete(changes);
     }
@@ -91,6 +119,21 @@ class AlbumService {
     return changes;
   }
 
+  Future<Set<String>> _loadExcludedAssetIds(
+    List<AssetPathEntity> albums,
+    List<String> excludedAlbumIds,
+  ) async {
+    final Set<String> result = HashSet<String>();
+    for (AssetPathEntity a in albums) {
+      if (excludedAlbumIds.contains(a.id)) {
+        final List<AssetEntity> assets =
+            await a.getAssetListRange(start: 0, end: 0x7fffffffffffffff);
+        result.addAll(assets.map((e) => e.id));
+      }
+    }
+    return result;
+  }
+
   /// Checks remote albums (owned if `isShared` is false) for changes,
   /// updates the local database and returns `true` if there were any changes
   Future<bool> refreshRemoteAlbums({required bool isShared}) async {

mobile/lib/shared/models/album.dart

@@ -144,6 +144,9 @@ class Album {
     }
     return a;
   }
+
+  @override
+  String toString() => name;
 }
 
 extension AssetsHelper on IsarCollection<Album> {
@@ -160,8 +163,15 @@ extension AssetPathEntityHelper on AssetPathEntity {
   Future<List<Asset>> getAssets({
     int start = 0,
     int end = 0x7fffffffffffffff,
+    Set<String>? excludedAssets,
   }) async {
     final assetEntities = await getAssetListRange(start: start, end: end);
+    if (excludedAssets != null) {
+      return assetEntities
+          .where((e) => !excludedAssets.contains(e.id))
+          .map(Asset.local)
+          .toList();
+    }
     return assetEntities.map(Asset.local).toList();
   }
 }

mobile/lib/shared/services/sync.service.dart

@@ -13,6 +13,7 @@ import 'package:immich_mobile/utils/async_mutex.dart';
 import 'package:immich_mobile/utils/diff.dart';
 import 'package:immich_mobile/utils/tuple.dart';
 import 'package:isar/isar.dart';
+import 'package:logging/logging.dart';
 import 'package:openapi/api.dart';
 import 'package:photo_manager/photo_manager.dart';
 
@@ -22,6 +23,7 @@ final syncServiceProvider =
 class SyncService {
   final Isar _db;
   final AsyncMutex _lock = AsyncMutex();
+  final Logger _log = Logger('SyncService');
 
   SyncService(this._db);
 
@@ -73,8 +75,11 @@ class SyncService {
 
   /// Syncs all device albums and their assets to the database
   /// Returns `true` if there were any changes
-  Future<bool> syncLocalAlbumAssetsToDb(List<AssetPathEntity> onDevice) =>
-      _lock.run(() => _syncLocalAlbumAssetsToDb(onDevice));
+  Future<bool> syncLocalAlbumAssetsToDb(
+    List<AssetPathEntity> onDevice, [
+    Set<String>? excludedAssets,
+  ]) =>
+      _lock.run(() => _syncLocalAlbumAssetsToDb(onDevice, excludedAssets));
 
   /// returns all Asset IDs that are not contained in the existing list
   List<int> sharedAssetsToRemove(
@@ -155,7 +160,10 @@ class SyncService {
     if (isShared && toDelete.isNotEmpty) {
       final List<int> idsToRemove = sharedAssetsToRemove(toDelete, existing);
       if (idsToRemove.isNotEmpty) {
-        await _db.writeTxn(() => _db.assets.deleteAll(idsToRemove));
+        await _db.writeTxn(() async {
+          await _db.assets.deleteAll(idsToRemove);
+          await _db.exifInfos.deleteAll(idsToRemove);
+        });
       }
     } else {
       assert(toDelete.isEmpty);
@@ -275,6 +283,7 @@ class SyncService {
     List<Asset> deleteCandidates,
   ) async {
     if (album.isLocal) {
+      _log.info("Removing local album $album from DB");
       // delete assets in DB unless they are remote or part of some other album
       deleteCandidates.addAll(
         await album.assets.filter().remoteIdIsNull().findAll(),
@@ -286,13 +295,22 @@ class SyncService {
         await album.assets.filter().not().ownerIdEqualTo(user.isarId).findAll(),
       );
     }
-    final bool ok = await _db.writeTxn(() => _db.albums.delete(album.id));
-    assert(ok);
+    try {
+      final bool ok = await _db.writeTxn(() => _db.albums.delete(album.id));
+      assert(ok);
+      _log.info("Removed local album $album from DB");
+    } catch (e) {
+      _log.warning("Failed to remove local album $album from DB");
+    }
   }
 
   /// Syncs all device albums and their assets to the database
   /// Returns `true` if there were any changes
-  Future<bool> _syncLocalAlbumAssetsToDb(List<AssetPathEntity> onDevice) async {
+  Future<bool> _syncLocalAlbumAssetsToDb(
+    List<AssetPathEntity> onDevice, [
+    Set<String>? excludedAssets,
+  ]) async {
+    _log.info("Syncing ${onDevice.length} albums from device: $onDevice");
     onDevice.sort((a, b) => a.id.compareTo(b.id));
     final List<Album> inDb =
         await _db.albums.where().localIdIsNotNull().sortByLocalId().findAll();
@@ -302,17 +320,27 @@ class SyncService {
       onDevice,
       inDb,
       compare: (AssetPathEntity a, Album b) => a.id.compareTo(b.localId!),
-      both: (AssetPathEntity ape, Album album) =>
-          _syncAlbumInDbAndOnDevice(ape, album, deleteCandidates, existing),
-      onlyFirst: (AssetPathEntity ape) => _addAlbumFromDevice(ape, existing),
+      both: (AssetPathEntity ape, Album album) => _syncAlbumInDbAndOnDevice(
+        ape,
+        album,
+        deleteCandidates,
+        existing,
+        excludedAssets,
+      ),
+      onlyFirst: (AssetPathEntity ape) =>
+          _addAlbumFromDevice(ape, existing, excludedAssets),
       onlySecond: (Album a) => _removeAlbumFromDb(a, deleteCandidates),
     );
     final pair = _handleAssetRemoval(deleteCandidates, existing);
     if (pair.first.isNotEmpty || pair.second.isNotEmpty) {
       await _db.writeTxn(() async {
         await _db.assets.deleteAll(pair.first);
+        await _db.exifInfos.deleteAll(pair.first);
         await _db.assets.putAll(pair.second);
       });
+      _log.info(
+        "Removed ${pair.first.length} and updated ${pair.second.length} local assets from DB",
+      );
     }
     return anyChanges;
   }
@@ -325,21 +353,33 @@ class SyncService {
     Album album,
     List<Asset> deleteCandidates,
     List<Asset> existing, [
+    Set<String>? excludedAssets,
     bool forceRefresh = false,
   ]) async {
     if (!forceRefresh && !await _hasAssetPathEntityChanged(ape, album)) {
       return false;
     }
-    if (!forceRefresh && await _syncDeviceAlbumFast(ape, album)) {
+    if (!forceRefresh &&
+        excludedAssets == null &&
+        await _syncDeviceAlbumFast(ape, album)) {
       return true;
     }
 
-    // general case, e.g. some assets have been deleted
+    // general case, e.g. some assets have been deleted or there are excluded albums on iOS
     final inDb = await album.assets.filter().sortByLocalId().findAll();
-    final List<Asset> onDevice = await ape.getAssets();
+    final List<Asset> onDevice =
+        await ape.getAssets(excludedAssets: excludedAssets);
     onDevice.sort(Asset.compareByLocalId);
     final d = _diffAssets(onDevice, inDb, compare: Asset.compareByLocalId);
     final List<Asset> toAdd = d.first, toUpdate = d.second, toDelete = d.third;
+    if (toAdd.isEmpty &&
+        toUpdate.isEmpty &&
+        toDelete.isEmpty &&
+        album.name == ape.name &&
+        album.modifiedAt == ape.lastModified) {
+      // changes only affeted excluded albums
+      return false;
+    }
     final result = await _linkWithExistingFromDb(toAdd);
     deleteCandidates.addAll(toDelete);
     existing.addAll(result.first);
@@ -359,8 +399,9 @@ class SyncService {
         album.thumbnail.value ??= await album.assets.filter().findFirst();
         await album.thumbnail.save();
       });
+      _log.info("Synced changes of local album $ape to DB");
     } on IsarError catch (e) {
-      debugPrint(e.toString());
+      _log.warning("Failed to update synced album $ape in DB: $e");
     }
 
     return true;
@@ -395,8 +436,10 @@ class SyncService {
         await album.assets.update(link: result.first + result.second);
         await _db.albums.put(album);
       });
+      _log.info("Fast synced local album $ape to DB");
     } on IsarError catch (e) {
-      debugPrint(e.toString());
+      _log.warning("Failed to fast sync local album $ape to DB: $e");
+      return false;
     }
 
     return true;
@@ -406,10 +449,17 @@ class SyncService {
   /// assets already existing in the database to the list of `existing` assets
   Future<void> _addAlbumFromDevice(
     AssetPathEntity ape,
-    List<Asset> existing,
-  ) async {
+    List<Asset> existing, [
+    Set<String>? excludedAssets,
+  ]) async {
+    _log.info("Syncing a new local album to DB: $ape");
     final Album a = Album.local(ape);
-    final result = await _linkWithExistingFromDb(await ape.getAssets());
+    final result = await _linkWithExistingFromDb(
+      await ape.getAssets(excludedAssets: excludedAssets),
+    );
+    _log.info(
+      "${result.first.length} assets already existed in DB, to upsert ${result.second.length}",
+    );
     await _upsertAssetsWithExif(result.second);
     existing.addAll(result.first);
     a.assets.addAll(result.first);
@@ -418,8 +468,9 @@ class SyncService {
     a.thumbnail.value = thumb;
     try {
       await _db.writeTxn(() => _db.albums.store(a));
+      _log.info("Added a new local album to DB: $ape");
     } on IsarError catch (e) {
-      debugPrint(e.toString());
+      _log.warning("Failed to add new local album $ape to DB: $e");
     }
   }
 
@@ -476,8 +527,11 @@ class SyncService {
         }
         await _db.exifInfos.putAll(exifInfos);
       });
+      _log.info("Upserted ${assets.length} assets into the DB");
     } on IsarError catch (e) {
-      debugPrint(e.toString());
+      _log.warning(
+        "Failed to upsert ${assets.length} assets into the DB: ${e.toString()}",
+      );
     }
   }
 }