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

delete() folders as a unit instead of deleting their contents individually on Windows #101

Closed
emmalexandria opened this issue Mar 10, 2024 · 4 comments
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@emmalexandria
Copy link
Contributor

It would be good for UX if calling delete on a folder deleted the folder as a whole (closer to file manager behavior). The current behavior makes it very hard to ergonomically restore an entire folder, which is usually trivial when deleting and restoring items via the file explorer. I'm writing code on Windows. Not sure if the function operates differently on Linux (?).

@emmalexandria emmalexandria changed the title Delete folders as folders instead of deleting their contents and the folder recursively Trash folders as a unit instead of deleting their contents individually Mar 10, 2024
@Byron
Copy link
Owner

Byron commented Mar 11, 2024

I think delete_all is the only way to do this reliably, even though it's likely that it will still resolve a whole directory tree to individual items.

On MacOS, I could validate that delete() can handle directories like one would expect, and I'd think it's the same on Linux.

Ideally, this behaviour would be the same on all platforms to properly support cross-platform applications.

@Byron Byron changed the title Trash folders as a unit instead of deleting their contents individually delete() folders as a unit instead of deleting their contents individually on Windows Mar 11, 2024
@Byron Byron added enhancement New feature or request help wanted Extra attention is needed labels Mar 11, 2024
@emmalexandria
Copy link
Contributor Author

emmalexandria commented Mar 11, 2024

I think delete_all is the only way to do this reliably, even though it's likely that it will still resolve a whole directory tree to individual items.

Calling delete_all does indeed seem to resolve it to individual files.

I must admit I'm unfamiliar with the Windows trash spec, but is there any particular reason to

traverse_paths_recursively(full_paths, &mut collection)?;

in delete_all_canonicalized? The doc comment (/// Removes all files and folder paths recursively.) implies an intentional implementation decision over a necessity. I found a C# example calling into the same API that seems perfectly comfortable with just recycling the folder.

@emmalexandria
Copy link
Contributor Author

emmalexandria commented Mar 11, 2024

From a super quick test, the following code works perfectly:

 pub(crate) fn delete_specified_canonicalized(&self, full_path: PathBuf) -> Result<(), Error> {
        ensure_com_initialized();
        unsafe {
            let pfo: IFileOperation = CoCreateInstance(&FileOperation as *const _, None, CLSCTX_ALL).unwrap();

            pfo.SetOperationFlags(FOF_NO_UI | FOF_ALLOWUNDO | FOF_WANTNUKEWARNING)?;

            let path_prefix = ['\\' as u16, '\\' as u16, '?' as u16, '\\' as u16];
            let wide_path_container = to_wide_path(full_path);
            let wide_path_slice = if wide_path_container.starts_with(&path_prefix) {
                &wide_path_container[path_prefix.len()..]
            } else {
                &wide_path_container[0..]
            };

            let shi: IShellItem = SHCreateItemFromParsingName(PCWSTR(wide_path_slice.as_ptr()), None)?;

            pfo.DeleteItem(&shi, None)?;

            pfo.PerformOperations()?;
            Ok(())
        }
    }

pub(crate) fn delete_all_canonicalized(&self, full_paths: Vec<PathBuf>) -> Result<(), Error> {
        for path in full_paths {
            self.delete_specified_canonicalized(path)?;
        }
        Ok(())
    }

I'll submit a quick PR.

@Byron
Copy link
Owner

Byron commented Mar 12, 2024

That's impressive! Thanks so much for digging in. Heading over to the PR now.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants