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.
Wrong header, causing build errors on win7, from the docs
Header shobjidl_core.h (include Shobjidl.h)
use Shobjidl.h instead.
Given FOF_ALLOWUNDO seems to do the trick for pre-windows 8, I don't think we need two separate codepaths here.
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?
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.
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.
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.
@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
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 ?).
We really should use command line utilities, similar to Electron:
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.
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.
@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.
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.
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.
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.
Use BLI_sprintfN to avoid having to manually compute the string length.
This will not work if the filepath contains spaces, system will interpret them as separate arguments.
This will try to free a NULL pointer when no desktop environment is detected
I'm currently quite busy, updates may take a while. Hopefully I can squeeze these in next weekend.
Yeah, sorry I'm not familiar with macOS development. I don't know what GHOST is, perhaps somebody else can implement this part.
Right, will add quotation marks.
Which is the same as a no-op. Can change if that you'd like to handle this differently
Yes, I can help with that part if the rest is done.
Strictly speaking, that's still not safe. If the filepath contains quotes you will still have issues.
You could use fork() and execlp() instead:
It's not a no-op, MEM_freeN will print a warning when passing NULL.
- 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>