feat(mobile): add playbackStyle to local asset entity and related database schema#26596
feat(mobile): add playbackStyle to local asset entity and related database schema#26596mertalev merged 11 commits intoimmich-app:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an AssetPlaybackStyle concept to the mobile domain and persists it for local assets via a Drift schema v21 migration, wiring the new field through sync, queries, and repositories to support upcoming animated-image rendering.
Changes:
- Introduce
AssetPlaybackStyleand a deducingplaybackStylegetter onBaseAsset. - Add
playback_stylecolumn tolocal_asset_entitywith Drift schema v21 + migration, and plumb it through inserts/reads. - Extend merged/timeline queries and trashed-asset restore path to carry (or deduce) playback style.
Reviewed changes
Copilot reviewed 14 out of 16 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| mobile/test/drift/main/generated/schema.dart | Registers schema v21 for drift test schemas. |
| mobile/lib/infrastructure/repositories/trashed_local_asset.repository.dart | Writes playbackStyle when restoring trashed assets into local DB. |
| mobile/lib/infrastructure/repositories/timeline.repository.dart | Maps merged query results into LocalAsset.playbackStyle. |
| mobile/lib/infrastructure/repositories/local_album.repository.dart | Persists playbackStyle during local asset upserts. |
| mobile/lib/infrastructure/repositories/db.repository.steps.dart | Generated schema/migration steps updated for v21 and new column. |
| mobile/lib/infrastructure/repositories/db.repository.dart | Bumps schemaVersion to 21 and adds migration to add playback_style. |
| mobile/lib/infrastructure/entities/trashed_local_asset.entity.dart | Deduces playbackStyle for trashed assets when restoring. |
| mobile/lib/infrastructure/entities/merged_asset.drift.dart | Adds playback_style to merged query result mapping. |
| mobile/lib/infrastructure/entities/merged_asset.drift | Updates SQL to include playback_style in UNION projections. |
| mobile/lib/infrastructure/entities/local_asset.entity.drift.dart | Generated table/data updates for playbackStyle column + converters. |
| mobile/lib/infrastructure/entities/local_asset.entity.dart | Adds playbackStyle column to LocalAssetEntity and maps to DTO. |
| mobile/lib/domain/services/local_sync.service.dart | Maps platform playback style onto LocalAsset. |
| mobile/lib/domain/models/asset/local_asset.model.dart | Adds/overrides playbackStyle on LocalAsset. |
| mobile/lib/domain/models/asset/base_asset.model.dart | Introduces AssetPlaybackStyle + deducing getter on BaseAsset. |
| mobile/drift_schemas/main/drift_schema_v21.json | Adds serialized Drift schema for v21. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @override | ||
| final AssetPlaybackStyle? playbackStyle; | ||
|
|
There was a problem hiding this comment.
LocalAsset overrides BaseAsset.playbackStyle with a nullable field. This shadows the new deducing getter in BaseAsset, so any LocalAsset constructed without an explicit playbackStyle (e.g., locally created/edited assets, legacy DB rows after migration) will now return null instead of a deduced value (image/video/livePhoto/etc.). Consider storing the persisted value in a differently named field and overriding the getter to return storedPlaybackStyle ?? super.playbackStyle so existing deduction still works when the DB value is absent.
There was a problem hiding this comment.
Thats intended. As though LocalAssets we can't detect motion photos or other stuff. So we want null in there if the local detection haven't been run for an asset.
There was a problem hiding this comment.
It exists from the start for new syncs, right? I think it's better to have a migration that sets the playbackStyle for existing assets to image/video based on asset type and make it non-nullable.
There was a problem hiding this comment.
@shenlong-tanwen Thoughts on this? This column can be computed from DB metadata in a migration if it's remote, but it can't know if it's animated or a live photo if it's local-only without doing a local sync. Would it be caught in a normal local sync, or would it require a full sync?
There was a problem hiding this comment.
From how I understand the sync code in the local_sync service:
// Faster path - only new assets added
if (await checkAddition(dbAlbum, deviceAlbum)) {
_log.fine("Fast synced device album ${dbAlbum.name}");
return true;
}
// Slower path - full sync
return await fullDiff(dbAlbum, deviceAlbum);It seems normal sync runs as long only assets are added to an album, this probably won't touch old assets.
But if assets where removed, the full sync runs and it will only detect changes if we add playbackStyle to the _assetsEqual method in `local_sync.service.dart.
diffSortedListsSync(
assetsInDb,
assetsInDevice,
compare: (a, b) => a.id.compareTo(b.id),
both: (dbAsset, deviceAsset) {
// Custom comparison to check if the asset has been modified without
// comparing the checksum
if (!_assetsEqual(dbAsset, deviceAsset)) {
assetsToUpsert.add(deviceAsset);
return true;
}
return false;
},
onlyFirst: (dbAsset) => assetsToDelete.add(dbAsset.id),
onlySecond: (deviceAsset) => assetsToUpsert.add(deviceAsset),
);So my guess is that it would require a full sync and changes in _assetEqual method
There was a problem hiding this comment.
Ahhh, thats exactly why I asked in the discord server today when the old code will be removed 😂 This happens to me way too often.
There was a problem hiding this comment.
The way we handle such additions is to fetch all assets that exists locally and update the column in the DB so the model can have the field as non-nullable. _populateLocalAssetTime in migration.dart is one such migration. We can add a similar migration to populate this field
The local sync, even the full sync version, is really fast and so doing this is as a one time migration doesn't add much to the app startup
There was a problem hiding this comment.
@shenlong-tanwen thx for the help.
Is it enough to just update all local assets similar to _populateLocalAssetTime?
Edit: I will change it to update all assets through _populateLocalAssetPlaybackStyle, and for TrashedAssets I will use the db migration/merge with the remote assets
Or are trashedLocalAssets also available through the native sync api?
There was a problem hiding this comment.
@shenlong-tanwen thx for the help.
Or are trashedLocalAssets also available through the native sync api?
You can get the list of assets in the trash using the following NativeSyncApi::getTrashedAssets. So update all assets similar to _populateLocalAssetTime and another call to getTrashedAssets to update the remaining assets
mobile/lib/infrastructure/entities/trashed_local_asset.entity.dart
Outdated
Show resolved
Hide resolved
|
@mertalev relevant changes that where made:
|
- only backfill local assets playbackStyle in flutter/dart code - only update trashed local assets in db migration
550c6b8 to
c7838d7
Compare
This is PR 3 to get animated images working. This one focuses on getting the playbackStyle into the database and having the playbackStyle property on the Assets (Local/Remote).
TrashedLocalAssetEntityDataDomainExtensionadded aAssetPlaybackStyle? get _deducedPlaybackStyleto deduce the playback style from deleted assets when restoring.Roadmap:
If this and the load encoded images pr #26584 gets merged. I then can implement the actual changes to show animated images with an MultiFrameImageStreamCompleter 🎉