refactor(mobile): add AssetState and proper asset updating (#2270)

* refactor(mobile): add AssetState and proper asset updating

* generate files

---------

Co-authored-by: Fynn Petersen-Frey <zoodyy@users.noreply.github.com>
Co-authored-by: Alex Tran <alex.tran1502@gmail.com>

mobile/lib/shared/models/asset.dart

@@ -56,6 +56,7 @@ class Asset {
   }
 
   Asset({
+    this.id = Isar.autoIncrement,
     this.remoteId,
     required this.localId,
     required this.deviceId,
@@ -133,6 +134,7 @@ class Asset {
 
   bool isFavorite;
 
+  /// `true` if this [Asset] is present on the device
   bool isLocal;
 
   bool isArchived;
@@ -146,12 +148,26 @@ class Asset {
   @ignore
   String get name => p.withoutExtension(fileName);
 
+  /// `true` if this [Asset] is present on the server
   @ignore
   bool get isRemote => remoteId != null;
 
   @ignore
   bool get isImage => type == AssetType.image;
 
+  @ignore
+  AssetState get storage {
+    if (isRemote && isLocal) {
+      return AssetState.merged;
+    } else if (isRemote) {
+      return AssetState.remote;
+    } else if (isLocal) {
+      return AssetState.local;
+    } else {
+      throw Exception("Asset has illegal state: $this");
+    }
+  }
+
   @ignore
   Duration get duration => Duration(seconds: durationInSeconds);
 
@@ -198,38 +214,113 @@ class Asset {
       isLocal.hashCode ^
       isArchived.hashCode;
 
-  bool updateFromAssetEntity(AssetEntity ae) {
-    // TODO check more fields;
-    // width and height are most important because local assets require these
-    final bool hasChanges =
-        isLocal == false || width != ae.width || height != ae.height;
-    if (hasChanges) {
-      isLocal = true;
-      width = ae.width;
-      height = ae.height;
-    }
-    return hasChanges;
-  }
-
-  Asset withUpdatesFromDto(AssetResponseDto dto) =>
-      Asset.remote(dto).updateFromDb(this);
-
-  Asset updateFromDb(Asset a) {
+  /// Returns `true` if this [Asset] can updated with values from parameter [a]
+  bool canUpdate(Asset a) {
+    assert(isInDb);
     assert(localId == a.localId);
     assert(deviceId == a.deviceId);
-    id = a.id;
-    isLocal |= a.isLocal;
-    remoteId ??= a.remoteId;
-    width ??= a.width;
-    height ??= a.height;
-    exifInfo ??= a.exifInfo;
-    exifInfo?.id = id;
-    if (!isRemote) {
-      isArchived = a.isArchived;
-    }
-    return this;
+    assert(a.storage != AssetState.merged);
+    return a.updatedAt.isAfter(updatedAt) ||
+        a.isRemote && !isRemote ||
+        a.isLocal && !isLocal ||
+        width == null && a.width != null ||
+        height == null && a.height != null ||
+        exifInfo == null && a.exifInfo != null ||
+        livePhotoVideoId == null && a.livePhotoVideoId != null ||
+        !isRemote && a.isRemote && isFavorite != a.isFavorite ||
+        !isRemote && a.isRemote && isArchived != a.isArchived;
   }
 
+  /// Returns a new [Asset] with values from this and merged & updated with [a]
+  Asset updatedCopy(Asset a) {
+    assert(canUpdate(a));
+    if (a.updatedAt.isAfter(updatedAt)) {
+      // take most values from newer asset
+      // keep vales that can never be set by the asset not in DB
+      if (a.isRemote) {
+        return a._copyWith(
+          id: id,
+          isLocal: isLocal,
+          width: a.width ?? width,
+          height: a.height ?? height,
+          exifInfo: a.exifInfo?.copyWith(id: id) ?? exifInfo,
+        );
+      } else {
+        return a._copyWith(
+          id: id,
+          remoteId: remoteId,
+          livePhotoVideoId: livePhotoVideoId,
+          isFavorite: isFavorite,
+          isArchived: isArchived,
+        );
+      }
+    } else {
+      // fill in potentially missing values, i.e. merge assets
+      if (a.isRemote) {
+        // values from remote take precedence
+        return _copyWith(
+          remoteId: a.remoteId,
+          width: a.width,
+          height: a.height,
+          livePhotoVideoId: a.livePhotoVideoId,
+          // isFavorite + isArchived are not set by device-only assets
+          isFavorite: a.isFavorite,
+          isArchived: a.isArchived,
+          exifInfo: a.exifInfo?.copyWith(id: id) ?? exifInfo,
+        );
+      } else {
+        // add only missing values (and set isLocal to true)
+        return _copyWith(
+          isLocal: true,
+          width: width ?? a.width,
+          height: height ?? a.height,
+          exifInfo: exifInfo ?? a.exifInfo?.copyWith(id: id),
+        );
+      }
+    }
+  }
+
+  Asset _copyWith({
+    Id? id,
+    String? remoteId,
+    String? localId,
+    int? deviceId,
+    int? ownerId,
+    DateTime? fileCreatedAt,
+    DateTime? fileModifiedAt,
+    DateTime? updatedAt,
+    int? durationInSeconds,
+    AssetType? type,
+    short? width,
+    short? height,
+    String? fileName,
+    String? livePhotoVideoId,
+    bool? isFavorite,
+    bool? isLocal,
+    bool? isArchived,
+    ExifInfo? exifInfo,
+  }) =>
+      Asset(
+        id: id ?? this.id,
+        remoteId: remoteId ?? this.remoteId,
+        localId: localId ?? this.localId,
+        deviceId: deviceId ?? this.deviceId,
+        ownerId: ownerId ?? this.ownerId,
+        fileCreatedAt: fileCreatedAt ?? this.fileCreatedAt,
+        fileModifiedAt: fileModifiedAt ?? this.fileModifiedAt,
+        updatedAt: updatedAt ?? this.updatedAt,
+        durationInSeconds: durationInSeconds ?? this.durationInSeconds,
+        type: type ?? this.type,
+        width: width ?? this.width,
+        height: height ?? this.height,
+        fileName: fileName ?? this.fileName,
+        livePhotoVideoId: livePhotoVideoId ?? this.livePhotoVideoId,
+        isFavorite: isFavorite ?? this.isFavorite,
+        isLocal: isLocal ?? this.isLocal,
+        isArchived: isArchived ?? this.isArchived,
+        exifInfo: exifInfo ?? this.exifInfo,
+      );
+
   Future<void> put(Isar db) async {
     await db.assets.put(this);
     if (exifInfo != null) {
@@ -311,6 +402,14 @@ extension AssetTypeEnumHelper on AssetTypeEnum {
   }
 }
 
+/// Describes where the information of this asset came from:
+/// only from the local device, only from the remote server or merged from both
+enum AssetState {
+  local,
+  remote,
+  merged,
+}
+
 extension AssetsHelper on IsarCollection<Asset> {
   Future<int> deleteAllByRemoteId(Iterable<String> ids) =>
       ids.isEmpty ? Future.value(0) : _remote(ids).deleteAll();

mobile/lib/shared/models/asset.g.dart

@@ -205,6 +205,7 @@ Asset _assetDeserialize(
     fileModifiedAt: reader.readDateTime(offsets[3]),
     fileName: reader.readString(offsets[4]),
     height: reader.readIntOrNull(offsets[5]),
+    id: id,
     isArchived: reader.readBool(offsets[6]),
     isFavorite: reader.readBool(offsets[7]),
     isLocal: reader.readBool(offsets[8]),
@@ -217,7 +218,6 @@ Asset _assetDeserialize(
     updatedAt: reader.readDateTime(offsets[14]),
     width: reader.readIntOrNull(offsets[15]),
   );
-  object.id = id;
   return object;
 }
 

mobile/lib/shared/models/exif_info.dart

@@ -63,6 +63,7 @@ class ExifInfo {
         description = dto.description;
 
   ExifInfo({
+    this.id,
     this.fileSize,
     this.make,
     this.model,
@@ -78,6 +79,41 @@ class ExifInfo {
     this.country,
     this.description,
   });
+
+  ExifInfo copyWith({
+    Id? id,
+    int? fileSize,
+    String? make,
+    String? model,
+    String? lens,
+    float? f,
+    float? mm,
+    short? iso,
+    float? exposureSeconds,
+    float? lat,
+    float? long,
+    String? city,
+    String? state,
+    String? country,
+    String? description,
+  }) =>
+      ExifInfo(
+        id: id ?? this.id,
+        fileSize: fileSize ?? this.fileSize,
+        make: make ?? this.make,
+        model: model ?? this.model,
+        lens: lens ?? this.lens,
+        f: f ?? this.f,
+        mm: mm ?? this.mm,
+        iso: iso ?? this.iso,
+        exposureSeconds: exposureSeconds ?? this.exposureSeconds,
+        lat: lat ?? this.lat,
+        long: long ?? this.long,
+        city: city ?? this.city,
+        state: state ?? this.state,
+        country: country ?? this.country,
+        description: description ?? this.description,
+      );
 }
 
 double? _exposureTimeToSeconds(String? s) {

mobile/lib/shared/models/exif_info.g.dart

@@ -188,6 +188,7 @@ ExifInfo _exifInfoDeserialize(
     exposureSeconds: reader.readFloatOrNull(offsets[3]),
     f: reader.readFloatOrNull(offsets[4]),
     fileSize: reader.readLongOrNull(offsets[5]),
+    id: id,
     iso: reader.readIntOrNull(offsets[6]),
     lat: reader.readFloatOrNull(offsets[7]),
     lens: reader.readStringOrNull(offsets[8]),
@@ -197,7 +198,6 @@ ExifInfo _exifInfoDeserialize(
     model: reader.readStringOrNull(offsets[12]),
     state: reader.readStringOrNull(offsets[13]),
   );
-  object.id = id;
   return object;
 }
 

mobile/lib/shared/providers/asset.provider.dart

@@ -102,8 +102,7 @@ class AssetNotifier extends StateNotifier<AssetsState> {
         await clearAssetsAndAlbums(_db);
         log.info("Manual refresh requested, cleared assets and albums from db");
       } else if (_stateUpdateLock.enqueued <= 1) {
-        final int cachedCount =
-            await _db.assets.filter().ownerIdEqualTo(me.isarId).count();
+        final int cachedCount = await _userAssetQuery(me.isarId).count();
         if (cachedCount > 0 && cachedCount != state.allAssets.length) {
           await _stateUpdateLock.run(
             () async => _updateAssetsState(await _getUserAssets(me.isarId)),
@@ -121,8 +120,7 @@ class AssetNotifier extends StateNotifier<AssetsState> {
       stopwatch.reset();
       if (!newRemote &&
           !newLocal &&
-          state.allAssets.length ==
-              await _db.assets.filter().ownerIdEqualTo(me.isarId).count()) {
+          state.allAssets.length == await _userAssetQuery(me.isarId).count()) {
         log.info("state is already up-to-date");
         return;
       }
@@ -141,12 +139,13 @@ class AssetNotifier extends StateNotifier<AssetsState> {
     }
   }
 
-  Future<List<Asset>> _getUserAssets(int userId) => _db.assets
-      .filter()
-      .ownerIdEqualTo(userId)
-      .isArchivedEqualTo(false)
-      .sortByFileCreatedAtDesc()
-      .findAll();
+  Future<List<Asset>> _getUserAssets(int userId) =>
+      _userAssetQuery(userId).sortByFileCreatedAtDesc().findAll();
+
+  QueryBuilder<Asset, Asset, QAfterFilterCondition> _userAssetQuery(
+    int userId,
+  ) =>
+      _db.assets.filter().ownerIdEqualTo(userId).isArchivedEqualTo(false);
 
   Future<void> clearAllAsset() {
     state = AssetsState.empty();

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

@@ -101,7 +101,7 @@ class AssetService {
       if (a.isRemote) {
         final dto = await _apiService.assetApi.getAssetById(a.remoteId!);
         if (dto != null && dto.exifInfo != null) {
-          a = a.withUpdatesFromDto(dto);
+          a.exifInfo = Asset.remote(dto).exifInfo!.copyWith(id: a.id);
           if (a.isInDb) {
             _db.writeTxn(() => a.put(_db));
           } else {
@@ -122,7 +122,7 @@ class AssetService {
     final dto =
         await _apiService.assetApi.updateAsset(asset.remoteId!, updateAssetDto);
     if (dto != null) {
-      final updated = Asset.remote(dto).updateFromDb(asset);
+      final updated = asset.updatedCopy(Asset.remote(dto));
       if (updated.isInDb) {
         await _db.writeTxn(() => updated.put(_db));
       }

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

@@ -136,7 +136,7 @@ class SyncService {
     if (match != null) {
       // unify local/remote assets by replacing the
       // local-only asset in the DB with a local&remote asset
-      newAsset.updateFromDb(match);
+      newAsset = match.updatedCopy(newAsset);
     }
     try {
       await _db.writeTxn(() => newAsset.put(_db));
@@ -581,12 +581,12 @@ class SyncService {
       // client and server, thus never reaching "both" case below
       compare: Asset.compareByOwnerDeviceLocalId,
       both: (Asset a, Asset b) {
-        if ((a.isLocal || !b.isLocal) && (a.isRemote || !b.isRemote)) {
+        if (a.canUpdate(b)) {
+          toUpsert.add(a.updatedCopy(b));
+          return true;
+        } else {
           existing.add(a);
           return false;
-        } else {
-          toUpsert.add(b.updateFromDb(a));
-          return true;
         }
       },
       onlyFirst: (Asset a) => _log.finer(
@@ -637,10 +637,8 @@ Triple<List<Asset>, List<Asset>, List<Asset>> _diffAssets(
     assets,
     compare: compare,
     both: (Asset a, Asset b) {
-      if (a.updatedAt.isBefore(b.updatedAt) ||
-          (!a.isLocal && b.isLocal) ||
-          (!a.isRemote && b.isRemote)) {
-        toUpdate.add(b.updateFromDb(a));
+      if (a.canUpdate(b)) {
+        toUpdate.add(a.updatedCopy(b));
         return true;
       }
       return false;