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

Return trash items that are created by delete #109

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

Conversation

jackpot51
Copy link
Contributor

This allows for restoring those files exactly, even if multiple files with the same path have been trashed. I have only implemented this for the freedesktop platform, which is why it returns an Option. Marking as a draft until all possible platforms are supported, or that is deemed unnecessary.

@Byron Byron linked an issue Jul 12, 2024 that may be closed by this pull request
@sungsphinx
Copy link
Contributor

Can this be rebased so deleting files works on Universal Blue/Fedora Atomic Desktops? Thanks.

Copy link
Owner

@Byron Byron left a comment

Choose a reason for hiding this comment

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

This has been a draft for a long time and to my mind, there isn't anything holding it back, generally at least.

Some concerns might be memory consumption, as I think that will be quite high with hundreds of thousands items deleted, but I suppose that's not usually what people do and it returns only the top-most level items anyway.

@sungsphinx Please feel free to open a new PR with this work rebased, superseding this one.

@jackpot51 jackpot51 marked this pull request as ready for review September 2, 2024 02:12
@jackpot51
Copy link
Contributor Author

This is ready, provided the platform availability is okay. I can rebase it if needed

@Byron
Copy link
Owner

Byron commented Sep 2, 2024

Right, I oversaw that only free desktop is actually implemented, and that tests haven’t been updated either.
From that perspective, I think the PR is indeed not ready and a rebase alone wouldn’t do it.

@eugenesvk
Copy link
Contributor

I've added a Mac's implementation with one change so far: we should return a single item when deleting a single item, not a vec, that's unexpected.

And think this should also be addressed:

Some concerns might be memory consumption, as I think that will be quite high with hundreds of thousands items deleted, but I suppose that's not usually what people do and it returns only the top-most level items anyway.

We could have it both with an extra argument. I think the default should be the simplest one, so I've started splitting functions in two: delete and delete_with_info, where the former is reverted to return nothing, and the latter can be used when trash items are needed.

Though maybe the proper way is for this to be only accessible via trash context similar to how deletemethod on macs work?

@eugenesvk
Copy link
Contributor

By the way, why is the original path stored in parts and requires a separate call to get to that combines sub-parts?

Wouldn't it be better to have a single hashmap keyed by the original path with a (trash_path, delete_time) tuple? The trash_path might be better as key for restoration purposes, but then it's not guaranteed we can get it

Or maybe even better: this could be some trie-type structure that would allow easily answering questions like "what are all the items removed with a path that contains ~\Documents`

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

Successfully merging this pull request may close these issues.

Return path to trashed item
4 participants