Skip to content
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): separate delete buttons #4505

Merged
merged 11 commits into from
Jan 17, 2024
Merged

Conversation

shenlong-tanwen
Copy link
Member

@shenlong-tanwen shenlong-tanwen commented Oct 17, 2023

Closes #2190 , #4453

Changes made in this PR

  • The delete buttons are separated into 3 different buttons - Delete From Immich, Delete From device and Delete everywhere to reduce confusion when deleting assets.
  • Delete local only will put the local assets into the OS trash. The delete prompt includes a button to either remove only backed up assets or to local files of all assets under selection
  • Delete from Immich will either trash or permanently remove an asset based on whether trash has been enabled or not.
  • Delete everywhere retains the current behavior where removing a file would remove it from both Immich and the local device

Note

  • Deleting assets will not permanently remove them from the device yet. They are moved to the System's internal trash and will be accessible through Google photo's bin feature.
  • This got changed recently in the upstream photo_manager plugin. Updating to it will permanently remove the assets instead of trashing them

Screenshots

Delete buttons Local only deletion
Delete-buttons Local-only-deletion

@vercel
Copy link

vercel bot commented Oct 17, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Ignored Deployment
Name Status Preview Comments Updated (UTC)
immich ⬜️ Ignored (Inspect) Visit Preview Nov 10, 2023 6:59pm

@shenlong-tanwen shenlong-tanwen marked this pull request as ready for review October 18, 2023 18:01
Copy link
Contributor

@fyfrey fyfrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good to me!

maybe we should add a warning if the user wants to delete assets from device that are not yet backed-up

@shenlong-tanwen
Copy link
Member Author

maybe we should add a warning if the user wants to delete assets from device that are not yet backed-up

That'd be good. But I'm not sure where to include this since we might need to handle this in normal deletions as well. In recent versions of android, the user already receives Immich confirmation dialog, followed by the System's dialog for deleting a local asset. Adding one more confirmation would really make sure that the user knows what they're doing, but three confirmations for deletion seems too much

@fyfrey
Copy link
Contributor

fyfrey commented Oct 20, 2023

That'd be good. But I'm not sure where to include this since we might need to handle this in normal deletions as well. In recent versions of android, the user already receives Immich confirmation dialog, followed by the System's dialog for deleting a local asset. Adding one more confirmation would really make sure that the user knows what they're doing, but three confirmations for deletion seems too much

true, three confirm dialogs is too much (two are already weird).

we could add an option (toggle/switch) in the first delete dialog with "delete only assets backed-up successfully" that is enabled by default. if a user want's to remove non-backed assets, the user can easily disable that toggle/switch

@shenlong-tanwen
Copy link
Member Author

we could add an option (toggle/switch) in the first delete dialog with "delete only assets backed-up successfully" that is enabled by default. if a user want's to remove non-backed assets, the user can easily disable that toggle/switch

Updated the handling to display a toggle now with default set to remove only backed up assets. User can toggle it off and remove assets that are not backed up as well.

Confirmation-delete-local

@alextran1502
Copy link
Contributor

Hello, thank you for working on this. I noticed that the slider of "Delete only backed up assets" doesn't do anything. Can you help me explain the functionality of it?

@shenlong-tanwen
Copy link
Member Author

Hello, thank you for working on this. I noticed that the slider of "Delete only backed up assets" doesn't do anything. Can you help me explain the functionality of it?

The "Delete from device" button is primarily used to remove the assets only from the current device. The toggle is used to prevent users from removing a local asset that is not yet backed up. With the toggle on, we filter out local only assets and use only those that are already backed up to the Immich server (i.e, merged assets). Turning off the toggle will allow us to delete local assets that are not backed up yet as well as the local file of the merged assets.

You can try selecting three assets, one that is both available locally and in server, one available only in the server and one available only in the device and tap "Delete from device". If you press "Delete" with the toggle on, only the local file of the asset that is available in both the device and the server will be removed and the asset will become remote only. If you turn the toggle off, the local only file will be removed as well.

@dvbthien
Copy link
Contributor

dvbthien commented Nov 2, 2023

Hi all,
I think the toggle in the dialog will cause confusion for casual users. Following Google Photos, when I remove non-backed-up assets, a warning dialog is displayed. This would be simpler and less confusing. The "Delete only backed-up assets" toggle can be displayed in Immich settings and users can be advised to set it there above the warning dialog. My English isn't good. Thanks for listening to my opinion.

Example image:
IMG_2129

@shenlong-tanwen
Copy link
Member Author

shenlong-tanwen commented Nov 5, 2023

Updated the handling to now show different alert message based on if selection contains non-backed up local assets or not.

Selection without non-backed up local asset Selection with non-backed up local asset
without with

@fyfrey
Copy link
Contributor

fyfrey commented Nov 5, 2023

This is also a good solution.
The two dialogs look too similar, though.

We need to make it clear that the dialog on the right is a potentially dangerous operation. Maybe, we can add a big warning icon somewhere?
And change the text from "delete" to "delete anyway" or something like that.

@shenlong-tanwen
Copy link
Member Author

We need to make it clear that the dialog on the right is a potentially dangerous operation. Maybe, we can add a big warning icon somewhere? And change the text from "delete" to "delete anyway" or something like that.

Updated the previous reply with the new change. Changed the button to "delete anyway" and updated the color of the content to red to let users know that it is a potentially dangerous operation.

@vinicentus
Copy link

vinicentus commented Nov 9, 2023

Is there still an option to delete only backed-up assets, or was it removed with 1270086?
In case it was removed, am I then correct in assuming that the flow for deleting only backed-up assets would be to manually check which ones are backed up, and then carefully select only those? (Or back up all, and then delete all.)

I think that option would be really useful! I'm just not sure where to put the setting slider/checkbox. Or then it could be a separate button: "delete only backed up assets"?

@acjohnson
Copy link

acjohnson commented Nov 10, 2023

Agree with @vinicentus, the original toggle for "Delete only backed up assets" automates building the list of images/videos that can be safely deleted 👍 Changing this PR to require the user to manually verify which images are safe to delete from the device prior to using the "Delete from device" feature is a step backwards IMHO

@fyfrey
Copy link
Contributor

fyfrey commented Nov 10, 2023

@alextran1502 since there are opposing views on how to handle the UI, maybe you want to chime in?

@shenlong-tanwen
Copy link
Member Author

shenlong-tanwen commented Nov 10, 2023

We could keep the current implementation, but instead of a toggle, we can display three buttons when the selection contains non-backed up assets as well?

<Title>

<Warning>

Cancel <> Delete Anyways  <> Delete Backed Up only

@alextran1502
Copy link
Contributor

alextran1502 commented Nov 10, 2023

I think the latest changes from @shenlong-tanwen is good and straightforward.

Maybe we can rephrase the dialog to make it a bit clearer "These items will be permanently removed from your device but still available on the Immich server"

@shenlong-tanwen
Copy link
Member Author

Thank you all for your great suggestions. I've updated the handling to display an additional button when the selection contains local only assets which would allow the user to remove only those that are already backed up. This should hopefully keep the UI simple but also makes it possible to remove local files of only those assets that are backed up successfully.

WIthout Local Only Assets With Local Only Assets
only_merged has_local

@vinicentus
Copy link

This seems like a good solution, at least for now.
The only issue I can see is that since the option to delete all or only backed-up is in the confirmation dialog, it's possible to misclick and accidentally delete all, when the intention was to delete only the backed-up images.
If this option were before the confirmation dialog, then the user would see what they clicked and could decide whether to proceed.

@mchiappinam
Copy link

I'm excited to have this in prod soon! 😍

Copy link

cloudflare-pages bot commented Jan 10, 2024

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: a29e4c3
Status: ✅  Deploy successful!
Preview URL: https://a81eb385.immich.pages.dev
Branch Preview URL: https://feat-mobile-delete-local-onl.immich.pages.dev

View logs

@shenlong-tanwen shenlong-tanwen changed the title feat(mobile): delete assets from device only feat(mobile): separate delete buttons Jan 10, 2024
@shenlong-tanwen shenlong-tanwen linked an issue Jan 10, 2024 that may be closed by this pull request
3 tasks
@alextran1502
Copy link
Contributor

Hi @shenlong-tanwen, I spotted a few issues while making the table. Let me know what you think about them

  • When only the local asset is selected, perform Delete from device and show the Delete Permanently form. I think it should just show the prompt to confirm the deletion or let the native prompt handle that
  • In the scenario of local + remote + merged with the action of delete from Immich, we should see the local asset not being affected, the remote asset goes into the trash, and merged would send the remote part into the trash while the local part should stay. The operation perform correctly, however the state doesn't update correctly for the merged asset, it is removed from the timeline, not being found in the trash, and only show up after logout and log back in again.
  • In the scenario of remote + merged, with the action of delete from Immich, the same behavior is observed for the merged asset above.
  • In the scenario of merged, with the action of delete from Immich, the same behavior is observed for the merged asset above.

@alextran1502
Copy link
Contributor

Here is the table that I came up with, let me know if you agree with the behaviors.

image

@mayeths
Copy link

mayeths commented Jan 13, 2024

I love the table @alextran1502 , you show exactly the behavior of different ACTIONS ("Delete from Immich" or "Delete from device"). The table content is fine, but the header reminds something else for me.

The previous dialog uses the term "Delete Permanently" without emphasizing what the ACTION is performed, which makes me quite nervous to think again whether the user presses the correct button. Even if I read that demo figure, I still have to read the full description in the dialog carefully to understand what will be performed.

Here are my thoughts about this feature, hope these may help:

  • Smooth deleting: Users should be able to delete the photo smoothly if at least one copy exists somewhere, and pop up a simple notification. If the ACTION removes the last copy of the photo, a dialog should be prompted to ask the user to check if this is the desired ACTION.
  • Distinguish the ACTIONS: like what @dvbthien said, I also notice some other apps use the words "MOVE Items to Trash" and "DELETE Items" to tell what is going on. In the context of Immich, it can be:
    • Move to trash locally
    • Move to trash on the server(Immich)
    • Delete from local device
    • Delete from the server(Immich)

With such descriptions, I think users can easily choose which ACTION is their desired one (hide photos by Move or freeing up space by Delete).

Also, a classic scenario is: "I want to free up some space on my phone, so let's [Delete from local device]. (Immich pops up a dialog for the user to choose BLUE[Delete after uploading] or RED[Delete right now]) Oh, I forgot to upload it to the server first, thanks Immich".

For such a scenario, We can also use another intuitive implementation that applies BLUE and RED on the four choices above. BLUE for safe choices, RED for choices that will cause photos lost, and GRAY for unavailable choices (like some selected photos are not presented on the server).

@alextran1502 alextran1502 merged commit 4c2befc into main Jan 17, 2024
21 checks passed
@alextran1502 alextran1502 deleted the feat/mobile-delete-local-only branch January 17, 2024 03:28
@waclaw66
Copy link
Contributor

waclaw66 commented Jan 17, 2024

Does this solve the situation when I want to remove remote uploaded asset and keep the local asset not trashed (un-upload)? When I click to Delete from cloud button, it trashes asset on web and locally as well, that's not intuitive.
On the other hand, when I click to Delete from device, it deletes asset from device and keeps it on server, that's expected behaviour, but how to download the asset back to the device to the original folder? Download button downloads it to Pictures folder instead of Camera folder.

@shenlong-tanwen
Copy link
Member Author

Does this solve the situation when I want to remove remote uploaded asset and keep the local asset not trashed (un-upload)? When I click to Delete from cloud button, it trashes asset on web and locally as well, that's not intuitive.

That behavior is not changed and it does not have anything to do with the delete buttons at all. The asset is still available in Immich server under Trash. Showing it in the timeline with a crossed cloud icon would mean that the asset is unavailable in Immich, but in reality, it does and can be restored easily from the Trash. Deleting it from the trash will however, bring the asset back to the timeline with the crossed cloud icon though.

On the other hand, when I click to Delete from device, it deletes asset from device and keeps it on server, that's expected behaviour, but how to download the asset back to the device to the original folder? Download button downloads it to Pictures folder instead of Camera folder.

It is not possible to download the asset to the original folder since Immich do not store the original path of the asset. I'm not sure how feasible this is as well.

@shenlong-tanwen
Copy link
Member Author

  • Smooth deleting: Users should be able to delete the photo smoothly if at least one copy exists somewhere, and pop up a simple notification. If the ACTION removes the last copy of the photo, a dialog should be prompted to ask the user to check if this is the desired ACTION.

We do not show any prompt to the user unless the action is destructive. i.e, no prompts eill be displayed for trashing / deleting already backed up local assets. However, if you are trying to remove a non-backed local asset or trying to remove assets from the trash, a prompt will be displayed.

With such descriptions, I think users can easily choose which ACTION is their desired one (hide photos by Move or freeing up space by Delete).

We do not have prompt for trashing yet. When we do, we will consider this

For such a scenario, We can also use another intuitive implementation that applies BLUE and RED on the four choices above. BLUE for safe choices, RED for choices that will cause photos lost, and GRAY for unavailable choices (like some selected photos are not presented on the server).

Our prompt should inform them that there are non-backed up assets. Users can then upload the selected local assets using the upload button and proceed to remove their local only files which would not display the prompt now. This should already be intuitive enough.

Thanks a lot for the detailed suggestions :)

@waclaw66
Copy link
Contributor

waclaw66 commented Jan 18, 2024

That behavior is not changed and it does not have anything to do with the delete buttons at all. The asset is still available in Immich server under Trash. Showing it in the timeline with a crossed cloud icon would mean that the asset is unavailable in Immich, but in reality, it does and can be restored easily from the Trash. Deleting it from the trash will however, bring the asset back to the timeline with the crossed cloud icon though.

That't not entirely true, deleting it from Trash from device deletes it for good, it doesn't bring it back, it has to be done from web side though. This is confusing, that a single action does different things. The ui shall be intuitive and user needs to know what action a button does. If it removes an asset to trash, then the button cannot be named "Delete from [cloud]" on the mobile, that button caption implies that an asset is deleted from cloud an local copy stays in device. Btw. such functionality is missing currently, right?
Next when an asset is deleted from Trash from web, then it reappears on the mobile again, when it's deleted from Trash from device, it's deleted for good. Have you tried putting it out there for regular users to use? I got multiple feedbacks, users are confused. Or there shall be a simple explanation with confirmation what action is going to be performed like Google Photos does.

@shenlong-tanwen
Copy link
Member Author

I got multiple feedbacks, users are confused. Or there shall be a simple explanation with confirmation what action is going to be performed like Google Photos does.

Thank you for your valuable feed-backs, and yes, it would indeed be confusing to the users when there is a difference in behavior on how a feature behaves in the mobile app and in the web app.

  • fix(mobile): asset state when delete from trash #6476 should hopefully address this and removing assets from the trash using the app will also only remove the remote part and do not touch the local part.
  • The same PR also changes the text of the bottom app bar based on the state of trash. Looking forward to hear your thoughts on this.

Again, Thanks a lot for your suggestions :)

Btw. such functionality is missing currently, right?

This current PR adds three separate buttons for asset removal. One will remove only the local part and leave the remote part. The other would remove only the remote part and will leave the local part. The last would remove both of them. However, if trash is enabled, the remote part is always moved to the Immich trash. If you still want to permanently remove an asset without putting it in the trash, then this PR might be useful for you where long-pressing the button will permanently remove it, instead of trashing it:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[BUG] Android: trash confirmation dialog ignores user choice
10 participants