Byron / trash-rs

A Rust library for moving files to the Recycle Bin
MIT License
170 stars 28 forks source link

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

Closed emmalexandria closed 7 months ago

emmalexandria commented 7 months ago

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 (?).

Byron commented 7 months ago

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.

emmalexandria commented 7 months ago

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 commented 7 months ago

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 commented 7 months ago

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