feat: add support for returning trashed file location#1
feat: add support for returning trashed file location#1dinocosta merged 10 commits intozed-industries:masterfrom
Conversation
* Introduce `trash::delete_with_info` and `trash::delete_all_with_info` * Introduce `trash::TrashContext.delete_with_info` and `trash::TrashContext.delete_all_with_info` * Update `trash::macos::delete_using_file_mgr` in order for it to build `TrashItem`s for the deleted paths, ensuring that we return information regarding where the file was moved to in the system's trash At the moment I believe the refactoring can't be introduced to `trash::macos::delete_using_finder` because it leverages AppleScript and the command's output does not returned the trashed file location. We'll still have to do the same refactoring for both Freedesktop and Windows in order to have access to the trashed files on both platforms.
Populate and return `TrashItem` from `move_to_trash` on Freedesktop, mirroring the existing macOS implementation. Uses `SystemTime` for `time_deleted` to stay consistent across platforms.
Implement IFileOperationProgressSink to capture item IDs during delete operations on Windows, enabling delete_with_info and delete_all_with_info to return TrashItem structs with id, name, original_parent, and time_deleted. The implementation: - Adds TrashProgressSink COM object implementing IFileOperationProgressSink - Collects recycle bin item IDs in PostDeleteItem callback - Looks up full metadata after operations complete (as metadata isn't immediately available during the callback) - Adds windows-core dependency and "implement" feature for COM support Also adds a Windows-specific test for delete_with_info that verifies the returned TrashItem fields and cleans up via purge_all.
* macOS: Ensure that `trash_url` is initialized as `None` instead of `NSURL::new()` to avoid allocation in the case of failure * macOS: Fix tests due to a missing `std::fs::remove_file` import * misc: Update documentation to further specify when are `TrashItem` impossible to construct, even when `with_info` is set to `true` * misc: Prefer use of `map` instead of `match` in `delete_all`
Since `name` is already an OsString, we don't need to convert it from `OsString` to `String` to then convert back to `OsString`. This changes comes out of an exploration to try and get rid of the way that, on Windows, in order to return trashed items, we first need to collect the IDs of the items in `PostDeleteItem` to then fetch the item information one by one, in the Recycle Bin, using the item IDs. Unfortunately this doesn't seem possible. When trying to update the implementation so as to build the `TrashItem` in `PostDeleteItem`, it was noted that, for the newly created `IShellItem`, its extended properties, like the deletion date, are not yet populated at callback time, so the `ERR_NOT_FOUND (0x80070490)` error was being returned when trying to fetch those properties. As such, seems like we need to continue collecting only the IDs during the callback and look up full metadata via `SHCreateItemFromParsingName` after `PerformOperations` completes, so the properties are available.
|
Leaving this in Draft for the time being as I'm trying to also update the implementation for |
Implement trash item info retrieval for the Finder delete method by converting AppleScript Finder object references to POSIX paths via `as alias`. Results are newline-delimited to avoid ambiguity with commas in filenames. This allows the public API to return `Result<TrashItem>` and `Result<Vec<TrashItem>>` instead of `Result<Option<TrashItem>>` and `Result<Option<Vec<TrashItem>>>` since all platforms and configurations now support returning trash item info.
dinocosta
force-pushed
the
5039-delete-with-info
branch
from
9eb7f73 to
63d71ba
Compare
* Move newly introduced macOS tests from `tests/trash.rs` to `src/macos/tests.rs`, as I did not notice we already had macOS-specific tests in this file * Introduce `CleanupPaths` to help ensure that files are removed after tests are run, regardless of whether the tests succeeds or fails
Switch the default delete method on macOS from `Finder` to `NsFileManager`. The NSFileManager approach is faster (no process spawning or IPC), requires no extra permissions, and avoids playing the Finder trash sound.
dinocosta
force-pushed
the
5039-delete-with-info
branch
from
7f1ee02 to
68aa191
Compare
| /// | ||
| /// Simply push the paths for whatever file was created during tests and the | ||
| /// `Drop` implementation will clean up the files after the test. | ||
| struct CleanupPaths(Vec<PathBuf>); |
There was a problem hiding this comment.
you can use https://crates.io/crates/tempfile to get a temporary directory that is always cleaned (including crashes like segfaults).
| fn test_delete_with_info() { | ||
| // Create the test file to be deleted, ensuring that we include the current | ||
| // directory so we can later assert that the `original_parent` is preserved. | ||
| let path = env::current_dir().expect("Should be able to get current directory").join(get_unique_name()); |
There was a problem hiding this comment.
nit: could be tempfile's tempdir instead
Note: no need to change nits! We can just keep going and merge :)
| pub(crate) fn delete_all_canonicalized( | ||
| &self, | ||
| full_paths: Vec<PathBuf>, | ||
| with_info: bool, |
There was a problem hiding this comment.
For future reference, this with_info parameter is here, even though on Zed's side we'll likely always end up setting to true, due to wanting to support undo/redo operations in the project panel, basically because we're not entirely sure yet if this carries a performance penalty for all platforms, as we're now keeping track of extra data which the default trash method was not doing before.
This is even more apparent in the Windows implementation which needs to do this in a 2-step fashion, where we first collect the IDs of the items that were deleted and then we built the TrashItem from the information on that item on the trash.
Since we'd eventually like to upstream these changes, it's probably better to keep this delete_with_info and delete method separate such that users of the trash crate that were simply using delete do not get affected by these changes, as those callers don't need the TrashItem information.
Open to revisit this in the future in case it makes sense to have this as the default 🙂
|
Confirmed with @yara-blue that we still want to go ahead and merge these changes even though we have a couple of comments that we could (and maybe should!) tackle ✅ |
2 participants
These changes introduce two new methods to
TrashContext, as well as matching convenience functions:delete_with_info– Deletes a single file, returning information about where the file ended up in trash after deletion.delete_all_with_info– Same asdelete_with_infobut supports multiple files, returning information for all.Platform Support
Since support and implementation details vary for each platform, here's a quick breakdown of any relevant details for each platform.
macOS
In order to support calling
NSString.lastPathComponent/NSString.stringByDeletingLastPathComponent, theNSPathUtilitiesfeature was added toobjc2-foundation.Windows
Although tracking the trashed file location in Windows is supported, its implementation is a little bit cumbersome. We need to register a
IFileOperationProgressSinkto capture the Item IDs via thePostDeleteItemcallback, then look up the full metadata afterPerformOperationscompletes.This two-phase approach is necessary because properties like the item's deletion timestamp are not available when the
PostDeleteItemcallback is called, and attempting to read these properties results in anERR_NOT_FOUND (0x80070490)error when testing.For the
IFileOperationProgressSinkto be available for implementation, both thewindows-corecrate, as well as theimplementfeature for thewindowscrate had to be added.