-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
feat: opt-in sync of deletes and restores from web to Android #16732
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: opt-in sync of deletes and restores from web to Android #16732
Conversation
…bum added. (Android)
SettingsSwitchListTile( | ||
enabled: true, | ||
valueNotifier: manageLocalMediaAndroid, | ||
title: "Sync remote deletions", |
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.
Please put the string to en-us.json
suggestion for the subtitle string
Automatically delete or restore an asset on this device when that action is taken on the web
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.
Also, I would like to mark this as [EXPERIMENTAL]
Hello, I just ran the first round of functional testing on a Samsung S23, and I don't see the behavior I expected. Let me know if I misunderstood the PR's functionality.
Let me know if I have tested the PR correctly. |
@@ -45,6 +47,13 @@ class AdvancedSettings extends HookConsumerWidget { | |||
title: "advanced_settings_troubleshooting_title".tr(), | |||
subtitle: "advanced_settings_troubleshooting_subtitle".tr(), | |||
), | |||
SettingsSwitchListTile( |
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.
This should only show on Android
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.
You tested it a bit incorrectly. The Media Store API works with media files that are located in the gallery. So, after deletion, we need to check the trash in the gallery, not the trash in the file manager.
When deleting a downloaded file in the web app, the file moves from the album to the trash (gallery), and when restoring it in the web app, it returns to the album from the trash.
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 it is a good idea to try to keep the Trash inside Immich to be in sync with the one on the device. We can configure the number of days after which an asset is permanently deleted from the trash in Immich. However, the System trash on the device defaults to a 30 days window, after which the asset is removed permanently from the device. If the trash days is >30 inside Immich and the user tries to restore the asset after 30 days, there will be no asset, anymore on the device to be restored. It is not a problem with the other app because it defaults to having 30 days as the trash window.
This is a good catch. I do think it's kind of nice that it's viewable in the system trash though. I think there are a few options here:
Do you have an opinion @alextran1502 ? |
I think this is the best compromise. The default is always to trash and have 30 days before deletion. @jrasm91 any thoughts about this? |
Personally, I think if you trash a file in Immich with this option enabled:
Specifically when the Immich's trash policy in longer than 30 days I think it's fine for the item to be deleted on the device and only be restored on the web. When the Immich trash policy is shorter than 30 days, I think it's also fine to leave the trashed photo in the mobile trash. |
I think that's probably pretty close to what happens with this PR today. I guess my question would be what happens if an asset is deleted from one trash and then restored from the other? Will it get synced to the other device? How and when does that happen? I think what Alex suggested is simpler conceptually in that it avoids cases like that. It's been hard to keep this PR in sync with main and it keeps falling down the queue, so I sort of wonder if we shouldn't just merge it as is and figure out any follow ups separately. |
Hi @alextran1502, done |
@aleksandrsovtan I just tested the permission logic and saw the following behaviors
Can you help me resolve those two issues? We are very close now! 😃 |
when (call.method) { | ||
"moveToTrash" -> { | ||
val fileName = call.argument<String>("fileName") | ||
if (fileName != null) { |
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.
Can you re-use the existing method channel in BackgroundServicePlugin
?
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.
@aleksandrsovtan Can you resolve this comment to proceed with the review?
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.
Hi @aleksandrsovtan, Can you help resolve this comment, moving the code to BackgroundServicePlugin
so that we have better separation? I think after this resolved, we can emerge the PR. I just tested again and everything looks good
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.
This might be a challenge, since the background service plugin doesn't have an activity. I'll try to do it ASAP
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.
@shenlong-tanwen Done!
mobile/android/app/src/main/kotlin/app/alextran/immich/MainActivity.kt
Outdated
Show resolved
Hide resolved
mobile/android/app/src/main/kotlin/app/alextran/immich/MainActivity.kt
Outdated
Show resolved
Hide resolved
mobile/android/app/src/main/kotlin/app/alextran/immich/MainActivity.kt
Outdated
Show resolved
Hide resolved
@alextran1502 done!) |
I really hope there is a solution for the permission problem with the playstore. Is there already an issue for this? Or is this the place to discuss it? |
We are in the process of adding it back using a less intrusive permission on Android - #17828. |
I have updated both the android app and the server to 1.132.3, however I still cannot see the experimental setting "Sync remote deletions" in the "Advanced" settings section. What am I missing? Note that my phone is still running Android 11, could that be the reason? |
…-app#16732) * Features: Local file movement to trash and restoration back to the album added. (Android) * Comments fixes * settings button marked as [EXPERIMENTAL] * _moveToTrashMatchedAssets refactored, moveToTrash renamed. * fix: bad merge * Permission check and request for local storage added. * Permission request added on settings switcher * Settings button logic changed * Method channel file_trash moved to BackgroundServicePlugin --------- Co-authored-by: Alex <[email protected]>
* chore: revert immich-app#16732 * lint
Description
Functionality for synchronizing the local media store when remotely deleting to trash and restoring files has been added. (Android)
This solves the issue of file deletion synchronization on web and mobile apps.
Fixes # (issue)
How Has This Been Tested?
To verify this, you need an Android 11+ device.
Checklist:
src/services/
uses repositories implementations for database calls, filesystem operations, etc.src/repositories/
is pretty basic/simple and does not have any immich specific logic (that belongs insrc/services/
)