Page MenuHome

Windows move files to recycling bin when deleted in file explorer
Needs ReviewPublic

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

It supports longer paths and is the current API. I'm not sure what Microsoft will do with the legacy SHFileOperationW in future versions. The new API exists since Vista, unfortunately the flag to move files to the recycling bin was included later in Windows 8.

LazyDodo (LazyDodo) requested changes to this revision.Mar 25 2019, 4:01 PM
LazyDodo (LazyDodo) added inline comments.
source/blender/blenlib/intern/fileops.c
39

Wrong header, causing build errors on win7, from the docs

Header	shobjidl_core.h (include Shobjidl.h)

use Shobjidl.h instead.

298

CoInitializeEx is ideally called once per thread and once a threading model has been chosen it cannot be changed. @Brecht Van Lommel (brecht) / @Campbell Barton (campbellbarton) any bright ideas here?

304

none of the FOFX_ flags are defined when building on earlier windows so that'll give build errors.

Also i gave it a quick shake and

	hr = pfo->lpVtbl->SetOperationFlags(pfo,
										FOF_NO_UI | FOF_ALLOWUNDO |
										FOF_NOERRORUI |
										FOF_SILENT );

Seems to work just fine for windows 7.

324

Potentially breaking other COM users, CoUninitialize should only be called at application shutdown time.

362

Given FOF_ALLOWUNDO seems to do the trick for pre-windows 8, I don't think we need two separate codepaths here.

378

Not as much as a comment as a question, the file browser didn't seem give me the option to delete a folder, so I'm not sure how to test this?

This revision now requires changes to proceed.Mar 25 2019, 4:01 PM

Fixed header include, should now work on older platforms as well.

Robert Guetzkow (rjg) marked an inline comment as done and an inline comment as not done.Mar 25 2019, 4:38 PM
Robert Guetzkow (rjg) added inline comments.
source/blender/blenlib/intern/fileops.c
298

Yes it's better if some more experienced developers take a look at the COM interaction.

304

You're right I missed that the flags aren't available when build on the older platforms. Does it move the file to the recycling bin though?

I think it's fine to call CoInitializeEx multiple times, the main issue is ensuring we don't call it with different values. I'm not sure there is a good way to automatically ensure it's not called with different values. If someone adds another call to CoInitialize in the future I would hope they read the documentation and then search for any similar calls in the code and verify it's ok.

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

Can confirm this works on Windows 10 as well. It doesn't seem to be documented behavior to move files to the recycling bin with this flag though, at least not for the new interface.

I think it's fine to call CoInitializeEx multiple times, the main issue is ensuring we don't call it with different values. I'm not sure there is a good way to automatically ensure it's not called with different values. If someone adds another call to CoInitialize in the future I would hope they read the documentation and then search for any similar calls in the code and verify it's ok.

Allright lets keep it as is then, also keep the call to CoUninitialize since they need to be balanced.

The COM handling seems to be a bit difficult with the CoInitializeEx() CoUninitialize(). As per the documentation I'm not exactly sure what they do on CoUnitialize that requires it to be called before closing the application (and not sooner). I'm not sure how we could satisfy this besides moving them to Blender's startup/exit code. Perhaps using the old interface is the better solution for now, if that causes too much of an headache.

I've tested multiple subsequent file deletions and there was no problem, even with how CoUninitialize() is currently used. Not sure how sound this solution is, but it's working in my test environment.

They do some expensive cleanup when the refcount hits zero apparently, but i'll be honest, it's not like we'll be calling this 100.000 times a second on a regular basis....

@LazyDodo (LazyDodo) That's what I thought. I wish they would properly document their API and give reason as to why certain recommendation are made (or even whether that is mandatory or a recommendation). I'll do some tests with the backward compatible flags and if some others should be included as well, like FOF_WANTNUKEWARNING. It would be great if someone could test this on Vista, just in case something behaves differently.

  • Only use backward compatible FOF_* flags
  • Make progress silent, but show system dialogs when an error occurs, especially when a file would be permanently deleted because it can't be moved to the bin
  • Removed legacy interface, since code should work for Windows Vista and newer versions
Robert Guetzkow (rjg) marked 4 inline comments as done.Mar 25 2019, 7:19 PM

Corrected error handling. Only CoUninitialize() when CoInitializeEx() returns either S_OK or S_FALSE both of which have a positive return value.

I'm not familier with the WIN32 API, some topics regarding this patch.

  • We should prepare a solution for Linux/macOS/WIN32 in a branch and commit it into master for all platforms at once since having delete behave differently on each platform is no good.

    At the very least we shouldn't release with some platforms unsupported.
  • We could support 'hard' delete still (shift-delete for eg) - although this can be applied later.
  • Wouldn't use the name 'recycle', but thats easy to change (could call BLI_delete_soft ?).

@Campbell Barton (campbellbarton) I agree. I'll have to read up on how to implement this for macOS or perhaps somebody more familiar with the platform could join in. I think @Kris (Metricity) would like to contribute to this as well.

Currently a bit busy, but this isn't forgotten. When I got some time I'll implement the Linux soft delete based on the approach that trash-cli uses.

I would like to implement support for $topdir, but in case (1) and (2) fail (see spec) refuse to trash. This means we can simply use rename since we don't move files over across devices. This makes the code simpler and doesn't require reimplementing mv.

We really should use command line utilities, similar to Electron:
https://github.com/electron/electron/blob/master/atom/common/platform_util_linux.cc#L100

I do not want to have an implementation of the trash specification in Blender, it's too complex, we shouldn't be maintaining this kind of thing ourselves.

I think the complexity is manageable, but that is your decision. The problem that I see with the Electron approach is finding out what desktop environment we're on (which implies what CLI utility can be used). Does Blender already have an equivalent of libgtkui::GetDesktopName?

I think we can do this rather simple, not supporting older desktop environment versions. In such cases there would just be no move to trash function in the file browser.

If getenv('XDG_CURRENT_DESKTOP') equals KDE we use kioclient5 and otherwise use gio.

That would be an option, although I'd favor a general solution that works on all Linux distributions and desktop environments. @Campbell Barton (campbellbarton) @LazyDodo (LazyDodo) Do you agree with this approach?

@Brecht Van Lommel (brecht) just in case you change your mind: I have implemented the trash specification (currently without directory size cache).

I will not accept a patch with an implementation of the trash specification, especially not a custom implementation. Correctly handling all the errors and corner cases is hard, we should use a mature implementation rather than spend time on this ourselves.

We should just use gio and similar, then we can make the implementation very small and reliable.

Sure, no problem. I'll implement it today. Do you have any special security policies regarding system() for calling the CLI utilities?

Work in progress implementation of soft delete for Linux and macOS

Several small fixes. Sorry for spamming the tracker.

Removed unused argument from Windows delete_soft()

I can't test the macOS implementation, perhaps somebody can help with that . I'll test the Linux implementation later this day.

@Brecht Van Lommel (brecht) let me know if you want to change anything.

Fixed wrong debug print function for Linux.

Added missing include for macOS

Replaced malloc with Blender's allocator.

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.