-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(mobile): preserve mobile album info on upload #11965
base: main
Are you sure you want to change the base?
Conversation
…mirror-upload-album
…pp/immich into feat/mobile/mirror-upload-album
@@ -296,15 +254,7 @@ class BackupAlbumSelectionPage extends HookConsumerWidget { | |||
], | |||
), | |||
), | |||
SliverLayoutBuilder( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was this removed? It is usually a best practise to have a list layout for mobile devices and a grid layout for tablet devices. This will make the UI more adaptive. IMHO, we should update other pages / list views to display a grid while in a tablet rather than removing them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can put it back
@@ -209,7 +207,9 @@ class ManualUploadNotifier extends StateNotifier<ManualUploadState> { | |||
); | |||
} | |||
|
|||
Set<AssetEntity> allUploadAssets = allAssetsFromDevice.nonNulls.toSet(); | |||
Set<BackupCandidate> allUploadAssets = allAssetsFromDevice.nonNulls | |||
.map((asset) => BackupCandidate(asset: asset, albumNames: [])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that manual backups do not preserve the album info even when the setting is turned on? It would be best if uploads, from foreground / background / manual, all respect the sync settings
final query = _db.albums.filter().nameEqualTo(name).sharedEqualTo(false); | ||
|
||
if (remoteOnly) { | ||
return query.localIdIsNull().findFirst(); | ||
} | ||
|
||
return query.findFirst(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The query can be constructed in a single statement using the Isar's optional method
try { | ||
final AssetPathEntity album = | ||
await AssetPathEntity.obtainPathFromProperties( | ||
id: a.id, | ||
optionGroup: filter.copyWith( | ||
updateTimeCond: DateTimeCond( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
This will greatly increase the time of the background upload each time it's executed for large libraries leading to not uploading stuff at all on iOS and wasting battery on Android as it's triggered very often (after any assets changed on device)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found that with this filtering condition, and with the scenario of a fresh installed, and choosing, let's say, "Camera" and "Recent" album for backup. When running in background mode, only the "Recent" album gets processed for the included albums.
Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but something in this PR might have changed the logic. It works fine for multiple albums and backs up all assets on main/release.
I believe this requires some debugging (has the album modified date changed: yes/no, are less assets obtained from the album than expected: yes/no, ...).
It's weird that only the background backup is affected - as the code is identical.
for (int i = 0; i < albums.length; i++) { | ||
final AssetPathEntity? a = albums[i]; | ||
if (a != null && | ||
a.lastModified?.isBefore(backupAlbums[i].lastBackup) != true) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this removed?
This will greatly increase the time of the background upload each time it's executed for large libraries leading to not uploading stuff at all on iOS and wasting battery on Android as it's triggered very often (after any assets changed on device)
|
||
await refreshRemoteAssets(); | ||
final remoteAssets = await _db.assets | ||
.filter() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use where() for one of the two filters (faster because it then uses an index)
…mirror-upload-album
This PR brings the ability to sync the selected album for backup with the Immich server. If an asset is in one of more selected albums, for example, a video in
Videos
andRecents
album, it will be added to both.When this option is toggled, the selected albums and any additional albums selected for backup are created on the server with the same name.
There is also an option to sync uploaded photos to the albums to cover the current Immich user. It can also be used to keep everything in sync if some edge cases happen and some uploaded assets don't make it to the album.