From e80d37bf8ff52c3f8e83ceddf9c4744fb6753ec5 Mon Sep 17 00:00:00 2001 From: Fynn Petersen-Frey <10599762+fyfrey@users.noreply.github.com> Date: Tue, 18 Apr 2023 11:47:24 +0200 Subject: [PATCH] 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 Co-authored-by: Alex Tran --- .../album/ui/album_viewer_thumbnail.dart | 7 +- .../providers/archive_asset_provider.dart | 2 +- .../modules/archive/views/archive_page.dart | 70 ++++---- .../favorite/providers/favorite_provider.dart | 4 +- .../home/ui/asset_grid/thumbnail_image.dart | 7 +- mobile/lib/shared/models/asset.dart | 153 ++++++++++++++---- mobile/lib/shared/models/asset.g.dart | 2 +- mobile/lib/shared/models/exif_info.dart | 36 +++++ mobile/lib/shared/models/exif_info.g.dart | 2 +- .../lib/shared/providers/asset.provider.dart | 19 ++- mobile/lib/shared/services/asset.service.dart | 4 +- mobile/lib/shared/services/sync.service.dart | 16 +- mobile/lib/utils/storage_indicator.dart | 14 ++ mobile/test/favorite_provider_test.mocks.dart | 16 ++ 14 files changed, 255 insertions(+), 97 deletions(-) create mode 100644 mobile/lib/utils/storage_indicator.dart diff --git a/mobile/lib/modules/album/ui/album_viewer_thumbnail.dart b/mobile/lib/modules/album/ui/album_viewer_thumbnail.dart index 1a6a614b4066d..38a9644c74a09 100644 --- a/mobile/lib/modules/album/ui/album_viewer_thumbnail.dart +++ b/mobile/lib/modules/album/ui/album_viewer_thumbnail.dart @@ -6,6 +6,7 @@ import 'package:immich_mobile/modules/album/providers/asset_selection.provider.d import 'package:immich_mobile/routing/router.dart'; import 'package:immich_mobile/shared/models/asset.dart'; import 'package:immich_mobile/shared/ui/immich_image.dart'; +import 'package:immich_mobile/utils/storage_indicator.dart'; class AlbumViewerThumbnail extends HookConsumerWidget { final Asset asset; @@ -85,11 +86,7 @@ class AlbumViewerThumbnail extends HookConsumerWidget { right: 10, bottom: 5, child: Icon( - asset.isRemote - ? (asset.isLocal - ? Icons.cloud_done_outlined - : Icons.cloud_outlined) - : Icons.cloud_off_outlined, + storageIcon(asset), color: Colors.white, size: 18, ), diff --git a/mobile/lib/modules/archive/providers/archive_asset_provider.dart b/mobile/lib/modules/archive/providers/archive_asset_provider.dart index 95d2da98dcf45..28d8d74f65b8e 100644 --- a/mobile/lib/modules/archive/providers/archive_asset_provider.dart +++ b/mobile/lib/modules/archive/providers/archive_asset_provider.dart @@ -30,7 +30,7 @@ class ArchiveSelectionNotifier extends StateNotifier> { } Future toggleArchive(Asset asset) async { - if (!asset.isRemote) return; + if (asset.storage == AssetState.local) return; _setArchiveForAssetId(asset.id, !_isArchive(asset.id)); diff --git a/mobile/lib/modules/archive/views/archive_page.dart b/mobile/lib/modules/archive/views/archive_page.dart index 921b55816ebbf..a7b13c69ca88b 100644 --- a/mobile/lib/modules/archive/views/archive_page.dart +++ b/mobile/lib/modules/archive/views/archive_page.dart @@ -65,42 +65,44 @@ class ArchivePage extends HookConsumerWidget { } Widget buildBottomBar() { - return Align( - alignment: Alignment.bottomCenter, - child: SizedBox( - height: 64, - child: Card( - child: Column( - children: [ - ListTile( - shape: RoundedRectangleBorder( - borderRadius: BorderRadius.circular(10), - ), - leading: const Icon( - Icons.unarchive_rounded, - ), - title: - const Text("Unarchive", style: TextStyle(fontSize: 14)), - onTap: () { - if (selection.value.isNotEmpty) { - ref - .watch(assetProvider.notifier) - .toggleArchive(selection.value, false); + return SafeArea( + child: Align( + alignment: Alignment.bottomCenter, + child: SizedBox( + height: 64, + child: Card( + child: Column( + children: [ + ListTile( + shape: RoundedRectangleBorder( + borderRadius: BorderRadius.circular(10), + ), + leading: const Icon( + Icons.unarchive_rounded, + ), + title: + const Text("Unarchive", style: TextStyle(fontSize: 14)), + onTap: () { + if (selection.value.isNotEmpty) { + ref + .watch(assetProvider.notifier) + .toggleArchive(selection.value, false); - final assetOrAssets = - selection.value.length > 1 ? 'assets' : 'asset'; - ImmichToast.show( - context: context, - msg: - 'Moved ${selection.value.length} $assetOrAssets to library', - gravity: ToastGravity.CENTER, - ); - } + final assetOrAssets = + selection.value.length > 1 ? 'assets' : 'asset'; + ImmichToast.show( + context: context, + msg: + 'Moved ${selection.value.length} $assetOrAssets to library', + gravity: ToastGravity.CENTER, + ); + } - selectionEnabledHook.value = false; - }, - ) - ], + selectionEnabledHook.value = false; + }, + ) + ], + ), ), ), ), diff --git a/mobile/lib/modules/favorite/providers/favorite_provider.dart b/mobile/lib/modules/favorite/providers/favorite_provider.dart index 53b024d5267b2..19fe396ce565d 100644 --- a/mobile/lib/modules/favorite/providers/favorite_provider.dart +++ b/mobile/lib/modules/favorite/providers/favorite_provider.dart @@ -26,8 +26,8 @@ class FavoriteSelectionNotifier extends StateNotifier> { } Future toggleFavorite(Asset asset) async { - if (!asset.isRemote) return; // TODO support local favorite assets - + // TODO support local favorite assets + if (asset.storage == AssetState.local) return; _setFavoriteForAssetId(asset.id, !_isFavorite(asset.id)); await assetNotifier.toggleFavorite( diff --git a/mobile/lib/modules/home/ui/asset_grid/thumbnail_image.dart b/mobile/lib/modules/home/ui/asset_grid/thumbnail_image.dart index d5fe564203b16..ec07718ab51e2 100644 --- a/mobile/lib/modules/home/ui/asset_grid/thumbnail_image.dart +++ b/mobile/lib/modules/home/ui/asset_grid/thumbnail_image.dart @@ -6,6 +6,7 @@ import 'package:immich_mobile/modules/favorite/providers/favorite_provider.dart' import 'package:immich_mobile/routing/router.dart'; import 'package:immich_mobile/shared/models/asset.dart'; import 'package:immich_mobile/shared/ui/immich_image.dart'; +import 'package:immich_mobile/utils/storage_indicator.dart'; class ThumbnailImage extends HookConsumerWidget { final Asset asset; @@ -124,11 +125,7 @@ class ThumbnailImage extends HookConsumerWidget { right: 10, bottom: 5, child: Icon( - asset.isRemote - ? (asset.isLocal - ? Icons.cloud_done_outlined - : Icons.cloud_outlined) - : Icons.cloud_off_outlined, + storageIcon(asset), color: Colors.white, size: 18, ), diff --git a/mobile/lib/shared/models/asset.dart b/mobile/lib/shared/models/asset.dart index ed8838095187c..f39a53eb2a5b9 100644 --- a/mobile/lib/shared/models/asset.dart +++ b/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; + 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), + ); + } } - return this; } + 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 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 { Future deleteAllByRemoteId(Iterable ids) => ids.isEmpty ? Future.value(0) : _remote(ids).deleteAll(); diff --git a/mobile/lib/shared/models/asset.g.dart b/mobile/lib/shared/models/asset.g.dart index a67c01b1262a9..bad9e958e8497 100644 --- a/mobile/lib/shared/models/asset.g.dart +++ b/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; } diff --git a/mobile/lib/shared/models/exif_info.dart b/mobile/lib/shared/models/exif_info.dart index 29cc913c2dc89..c694168dbe163 100644 --- a/mobile/lib/shared/models/exif_info.dart +++ b/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) { diff --git a/mobile/lib/shared/models/exif_info.g.dart b/mobile/lib/shared/models/exif_info.g.dart index 825a3e5a0836c..708bf700b6364 100644 --- a/mobile/lib/shared/models/exif_info.g.dart +++ b/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; } diff --git a/mobile/lib/shared/providers/asset.provider.dart b/mobile/lib/shared/providers/asset.provider.dart index cbe3a91b61b0b..02eabded3a8e4 100644 --- a/mobile/lib/shared/providers/asset.provider.dart +++ b/mobile/lib/shared/providers/asset.provider.dart @@ -102,8 +102,7 @@ class AssetNotifier extends StateNotifier { 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 { 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 { } } - Future> _getUserAssets(int userId) => _db.assets - .filter() - .ownerIdEqualTo(userId) - .isArchivedEqualTo(false) - .sortByFileCreatedAtDesc() - .findAll(); + Future> _getUserAssets(int userId) => + _userAssetQuery(userId).sortByFileCreatedAtDesc().findAll(); + + QueryBuilder _userAssetQuery( + int userId, + ) => + _db.assets.filter().ownerIdEqualTo(userId).isArchivedEqualTo(false); Future clearAllAsset() { state = AssetsState.empty(); diff --git a/mobile/lib/shared/services/asset.service.dart b/mobile/lib/shared/services/asset.service.dart index 4d53f7be9d073..8ba88b48f459a 100644 --- a/mobile/lib/shared/services/asset.service.dart +++ b/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)); } diff --git a/mobile/lib/shared/services/sync.service.dart b/mobile/lib/shared/services/sync.service.dart index 41db14f5857ad..664045c2fe497 100644 --- a/mobile/lib/shared/services/sync.service.dart +++ b/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, List> _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; diff --git a/mobile/lib/utils/storage_indicator.dart b/mobile/lib/utils/storage_indicator.dart new file mode 100644 index 0000000000000..590605c2ec864 --- /dev/null +++ b/mobile/lib/utils/storage_indicator.dart @@ -0,0 +1,14 @@ +import 'package:flutter/material.dart'; +import 'package:immich_mobile/shared/models/asset.dart'; + +/// Returns the suitable [IconData] to represent an [Asset]s storage location +IconData storageIcon(Asset asset) { + switch (asset.storage) { + case AssetState.local: + return Icons.cloud_off_outlined; + case AssetState.remote: + return Icons.cloud_outlined; + case AssetState.merged: + return Icons.cloud_done_outlined; + } +} diff --git a/mobile/test/favorite_provider_test.mocks.dart b/mobile/test/favorite_provider_test.mocks.dart index a42bc13429c7e..569c12e791202 100644 --- a/mobile/test/favorite_provider_test.mocks.dart +++ b/mobile/test/favorite_provider_test.mocks.dart @@ -242,6 +242,22 @@ class MockAssetNotifier extends _i1.Mock implements _i2.AssetNotifier { returnValueForMissingStub: _i5.Future.value(false), ) as _i5.Future); @override + _i5.Future toggleArchive( + Iterable<_i4.Asset>? assets, + bool? status, + ) => + (super.noSuchMethod( + Invocation.method( + #toggleArchive, + [ + assets, + status, + ], + ), + returnValue: _i5.Future.value(), + returnValueForMissingStub: _i5.Future.value(), + ) as _i5.Future); + @override bool updateShouldNotify( _i2.AssetsState? old, _i2.AssetsState? current,