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(web): delete all assets associated with stack on asset delete #14618

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leanmiguel
Copy link

An attempt to fix this for the web: #13113
This change deletes both the stack, as well as all the asset ids for a given stack when deleted from either the asset screen, or the photos page.
I went with deleting the stack as I wasn't sure how to get show the stack in the trash page, and thought this was a workable solution. It does have the tradeoff that the user has to restack the images again.

I don't believe this is fully fleshed out yet, but I think the main idea is there.
It would be nice to have some pointers as for what other implications to consider for this PR as I'm not too familiar with the codebase.

I do believe that a query to grab all stack should be added, as opposed to how I have it now (fetch each stack).
I'm also wrapping around the deleteAssets as I'm not sure if this change should be propagated to other parts. The error handling can be improved.

@@ -34,8 +37,36 @@

const handleDelete = async () => {
loading = true;
const ids = [...getOwnedAssets()].map((a) => a.id);
await deleteAssets(force, onAssetDelete, ids);
const ownedAssets = [...getOwnedAssets()];
Copy link
Contributor

Choose a reason for hiding this comment

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

If you have 100,000 assets, will this populate 100,000 element array here?

@jrasm91
Copy link
Contributor

jrasm91 commented Dec 16, 2024

I think it's worth deciding what the expected behavior should be when an asset in a stack in deleted or trashed and then we can determine the best way to accomplish that.

One obvious thing we could change is that trashing the primary asset in a stack trashes the whole stack. In that case, I would expect that logic to exist server-side, not client-side. Also, ideally un-trashing the same asset would likewise untrash the whole stack.

@alextran1502
Copy link
Contributor

The sensible workflow is when you select the stack from the timeline; deleting them with the group of other single assets will also remove the stack. It is confusing at the moment when the said mechanism happens; you return to the timeline to find the stack is still there with only the top asset of the stack was removed

In the detail viewer, there should be an option to remove the asset from the stack and a standard delete option to delete the stack.

I agree with Jason that this mechanism should be handled from the server side.

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

Successfully merging this pull request may close these issues.

3 participants