Page MenuHome

Move files to OS recycling bin when deleted in file explorer
ClosedPublic

Authored by Robert Guetzkow (rjg) on Mar 25 2019, 1:30 PM.

Details

Summary

This is a patch for T61412 which introduces a soft-delete for the file explorer on Windows. The IFileOperation interface is used for Windows 8 and later, older Windows versions use the legacy interface. The patch includes code from D4579 @Kris (Metricity) who implemented the function using the legacy API, is used for older Windows versions. I have tested the patch on Windows 10 using long paths, including Umlaute in filenames.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Correct error messages

@Campbell Barton (campbellbarton) I think this patch is done, if we want to limit the supported desktop environments as @Brecht Van Lommel (brecht) suggested. And if it works properly on macOS which I haven't been able to test.

Since I've implemented the full spec anyway, I've published it on GitHub. Perhaps somebody finds it useful.

Brecht Van Lommel (brecht) requested changes to this revision.May 5 2019, 11:55 AM
Brecht Van Lommel (brecht) added inline comments.
source/blender/blenlib/intern/fileops.c
798–812

This will not compile as objective-C code needs to be in its own .m or .mm file. Adding in GHOST may be the easiest solution.

806

No need to use NSLog, use printf instead.

Ideally what should happen for all platforms is that any error messages are reported back to the user, not logged or printed to the terminal.

825–826

I think we can use gio always when it's available, even if the desktop environment is not GNOME or there is no detect desktop environment. That way it works with e.g. XFCE.

827–831

Use BLI_sprintfN to avoid having to manually compute the string length.

828–831

This will not work if the filepath contains spaces, system will interpret them as separate arguments.

863

This will try to free a NULL pointer when no desktop environment is detected

This revision now requires changes to proceed.May 5 2019, 11:55 AM

I'm currently quite busy, updates may take a while. Hopefully I can squeeze these in next weekend.

source/blender/blenlib/intern/fileops.c
798–812

Yeah, sorry I'm not familiar with macOS development. I don't know what GHOST is, perhaps somebody else can implement this part.

828–831

Right, will add quotation marks.

863

Which is the same as a no-op. Can change if that you'd like to handle this differently

source/blender/blenlib/intern/fileops.c
798–812

Yes, I can help with that part if the rest is done.

828–831

Strictly speaking, that's still not safe. If the filepath contains quotes you will still have issues.

You could use fork() and execlp() instead:
https://stackoverflow.com/a/8539271

863

It's not a no-op, MEM_freeN will print a warning when passing NULL.

source/blender/blenlib/intern/fileops.c
828–831

Yes this is a better and safer alternative to system().

863

My bad, yes there are additional checks in Blender's free.

Since 64acb70e7f0f removes the ability to delete files, is development on this patch still of interest?

@Robert Guetzkow (rjg) I think so. If you can make sure that it only affects Windows, or can also add Mac/Linux support, then I can't see why we shouldn't still add this. Preferably we should try to add support for all three at once I think.

  • Simplified Linux soft delete, now using fork and execvp
  • Added keymap entries back
  • macOS currently commented, because changes necessary to make Objective-C work
  • removed duplicate include for <sys/stat.h>

I've done some more research and it seems we could use the runtime with pure C on OSX. Certainly not the most elegant approach. @Brecht Van Lommel (brecht) do you want to take this from here?

This comment was removed by Robert Guetzkow (rjg).

@Brecht Van Lommel (brecht) This implementation using only C (and not Objective-C) could work. It would require linking CoreFoundation and Cocoa.

Let me know if you'd like to see this implementation for macOS in Blender or what other approach should be used.

This seems overall fine, but it will have to wait for 2.81 since it wasn't finished before the feature freeze.

macOS implementation of soft delete using the cocoa runtime.

This would be great to get in for 2.81, with the updated File Browser work happening, it fits well.

Robert Guetzkow (rjg) added a comment.EditedSep 2 2019, 5:21 PM

Sure, just let me know what additional changes are necessary. I'm currently a bit busy with a project for university, but I'm sure I'll find some time to update the patch.

One area that would need improvement is the feedback to the user when an error occurs.

Rebase patch on latest master

I'd like to get this in for 2.81, it makes sense with the recent file browser changes. You've picked a nice but tricky task, given that this is so OS dependent :)

Only tested the Linux implementation, I can test on Windows 10 later.

  • For me, on Sway (Wayland based, tiling window manager) this doesn't work at all, noted reason in an inline comment.
  • There should definitely be better error reporting. At least on Linux, with the missing standardization between DEs, I smell this would always fail on some systems. We could pass an const char **r_error_message as argument to the soft delete functions which the caller, i.e. file_delete_exec() can put into a BKE_report() call on non-0 return value (BTW, could just return bool?). We'd only assign string literals to it, so we don't need to care for dynamic allocation.
  • The soft delete should also be added to the new file browser context menu, see FILEBROWSER_MT_context_menu in space_filebrowser.py.
release/scripts/presets/keyconfig/keymap_data/industry_compatible_data.py
1186

Backspace is set twice now (see just above). I guess it should just be used for navigating to previous location, since AFAIK that's what common file browsers do.

source/blender/blenlib/intern/fileops.c
44

This should be #elif defined(__APPLE__)

847–848

Assigning the string literals to this I get warnings on GCC: "assignment discards ‘const’ qualifier from pointer target type", so these should be const char *.

Due to some API tradeoff accepted for exec functions (see rationale here), you'll still have to cast then to avoid warnings. Also, no need to store args[0] separately:
execvp(args[0], (char **)args);

850–855

For me, this fails, because you have to use XDG_SESSION_DESKTOP rather than XDG_CURRENT_DESKTOP. So both would have to be checked here.

However, as Brecht suggested earlier, we might not want to early-exit if the DE can't be identified and just try to run the gio command. If this failed (e.g. because gio wasn't found), we can still fail with an error message.

Julian Eisel (Severin) requested changes to this revision.Mon, Sep 30, 1:08 PM
This revision now requires changes to proceed.Mon, Sep 30, 1:08 PM

I'm currently on a very tight time budget, because I have to complete a university project. I'll try to squeeze this in, but can't make any promises. Believe me when I say I'd rather work on this than the other project. If I can't get it done in time, feel free to take over this revision.

I'd like to get this in for 2.81, it makes sense with the recent file browser changes. You've picked a nice but tricky task, given that this is so OS dependent :)

Yes especially the macOS version was a bit complicated to implement.

For me, on Sway (Wayland based, tiling window manager) this doesn't work at all, noted reason in an inline comment.
There should definitely be better error reporting. At least on Linux, with the missing standardization between DEs, I smell this would always fail on some systems.

Checking the other environment variable is no big deal. The actual issue is that we are currently relying on installed tools that are (hopefully) available, instead of solving this for every system regardless of the desktop environment that is used. This is of course much simpler to implement but supports fewer systems.

We could pass an const char **r_error_message as argument to the soft delete functions which the caller, i.e. file_delete_exec() can put into a BKE_report() call on non-0 return value (BTW, could just return bool?). We'd only assign string literals to it, so we don't need to care for dynamic allocation.

Sounds good, although I have to take another look at the code and possibly ask some questions to ensure that there is no misunderstanding in the intended API. Would you like to have the error reporting use X-macros similar to how I implemented this in libtrashcan?

The soft delete should also be added to the new file browser context menu, see FILEBROWSER_MT_context_menu in space_filebrowser.py.

Ok.

release/scripts/presets/keyconfig/keymap_data/industry_compatible_data.py
1186

I agree.

source/blender/blenlib/intern/fileops.c
847–848

Absolutely right. The Linux implementation was a bit quick and dirty.

850–855

Should XDG_SESSION_DESKTOP take precedence over XDG_CURRENT_DESKTOP or the other way around? The current approach is not really nice, since it doesn't properly solve this for all desktop environments, however I'm on-board with attempting to use gio in case there is neither environment variable set. Having a proper solution which is independent of the desktop would be nice (e.g. if my implementation would be more mature https://github.com/robertguetzkow/libtrashcan).

source/blender/blenlib/intern/fileops.c
44

I might be wrong here, but I don't think the define is necessary if it's an elif to an ifdef.

Removed duplicate entry for the backspace key and made small adjustments to the soft delete function for Linux.

Julian Eisel (Severin) retitled this revision from Windows move files to recycling bin when deleted in file explorer to Move files to OS recycling bin when deleted in file explorer.Fri, Oct 4, 11:36 AM
Robert Guetzkow (rjg) marked an inline comment as done.

Integration into the new file browser.

I've got another patch update for the error messages, but I'm unsure how useful it actually is. Besides Windows, there are very few extra information we can give to the user on macOS and Linux. The main benefit on Linux is that we can check when the exec failed hard, indicating that there is no gio available.

Patch for the error messages and slightly different error handling. It still needs to be tested, especially on Linux and macOS.

There are errors in the latest diff update. They will be fixed in about an hour.

Fixed errors in previous update.

  • now using defined(__APPLE__)
  • missing semicolon added
  • casting argument to execvp

Works fine on Sway now :) I'd also say the Linux implementation is ready. We could try to run gio first, and only check for the KDE variant if that fails, but in practice it probably doesn't make a difference.

Only one thing I'd suggest tweaking: The operator description should clarify that it only moves to the trash, and doesn't hard delete.
Maybe it's not in scope of this patch, but it would also be nice if we could (soft!) delete directories.

Still didn't test on Windows, will try to do soon.

source/blender/blenlib/intern/fileops.c
44

It's probably fine, but GCC happens to warn about it.

Updated the description of FILE_OT_delete.

The current delete_soft() implementation would allow to delete directories as well. There are adjustments necessary to the way BLI_delete_soft() is called, which is why I'd like to put that in a separate diff.

Robert Guetzkow (rjg) marked 11 inline comments as done.Mon, Oct 7, 6:46 PM
Robert Guetzkow (rjg) marked 2 inline comments as done.Mon, Oct 7, 7:15 PM

It would be great if the patch could be thoroughly tested on all platforms, especially on macOS because I don't have Apple hardware. All feedback is appreciated.

Julian Eisel (Severin) accepted this revision.EditedThu, Oct 10, 4:00 PM

Ran some quick tests on Windows 10, worked fine, even with troublesome file paths.
Also quickly checking the Win32 code, didn't note anything obviously wrong.

Brecht wanted to test on macOS, if that's fine, this can go in today I guess.

  • Fix build error on macOS.
  • Don't show confirmation popup when using Delete from the context menu.
This revision was not accepted when it landed; it landed in state Needs Review.Thu, Oct 10, 5:35 PM
This revision was automatically updated to reflect the committed changes.

@Brecht Van Lommel (brecht) thank you for testing this on macOS and noticing the error in the includes.

For people who have come here from the latest Blender Today with @Pablo Vazquez (pablovazquez): The delete feature for folders will be added. It will require more changes though, since all previous versions (also back in 2.79) didn't allow to delete folders. I've got some more tasks to do, but perhaps I'm able to implement it before the Blender Conference.