Support SDL_EVENT_DROP_FILE and SDL_EVENT_DROP_TEXT in Windows

[Dragon-Baroque]

Remove WM_DROPFILES ( supports only SDL_EVENT_DROP_FILE ), use IDropTarget instead ( provides SDL_EVENT_DROP_POSITION, SDL_EVENT_DROP_FILE and SDL_EVENT_DROP_TEXT ).

Description

• In src/video/windows/SDL_windowsevents.c, remove WM_DROPFILES
• In src/video/windows/SDL_windowswindow.c + .h,
  Create IDropTarget and issue SDL_SendDropPosition,
  Handle text/uri-list for SDL_SendDropFile, text/plain;charset=utf-8 and CF_UNICODE_TEXT and CF_TEXT for SDL_SendDropText, CF_HDROP for SDL_SendDropFile.

To be reviewed

• The IDropTarget Drop OLE object requires a full OleInitialize in WIN_AcceptDragAndDrop, as CoInitialize(Ex) are not enough and cause a Memory Management Overflow error. Whereas CoInitialize(Ex) have a centralized support in SDL, OleInitialize has not. What is the position of the SDL Development Team about OleInitialize ?
• The Debug messages print size_t variables, using a cast to 64 bits and a %lld format for 32 and 64 bits support. Should they use %zd instead, like the test automation does, but which requires silencing the warnings -Wformat and -Wformat-extra-args ?

[slouken]

What is the impact of OleInititalize() on the application? Is it safe to call multiple times? Is there cleanup associated with it?
If it's necessary for drag and drop and it's safe for a library to call, it seems like moving that to WIN_VideoInit()/Quit() is reasonable.

For size_t probably the most portable approach is to cast to (Uint64) and use the SDL_PRIu64 formatting macro. Usually we don't bother if we know the values will always be small and just cast to int and use %d.

[Dragon-Baroque]

For size_t probably the most portable approach is to cast to (Uint64) and use the SDL_PRIu64 formatting macro.

I implemented that. Thank you.

What is the impact of OleInititalize() on the application? Is it safe to call multiple times? Is there cleanup associated with it?

I thought that you would have much more experience than I :-)
I did some research and :

• OleInitialize ( like CoInitializeEx ) holds a call counter. You can call it several times. It performs more work the first time it is called.
• OleInitialize actually calls CoInitializeEx with COINIT_APARTMENTTHREADED as the first step , whether or not this is the first time it is called.
  This internal CoInitializeEx with fail and cause OleInitialize to fail too if CoInitializeEx has been called previously with COINIT_MULTITHREADED. OLE is not thread-safe, and it cannot support COINIT_MULTITHREADED.
• Normally OleInitialize calls should be paired with ( the same number of ) OleUninitialize. I have seen applications which don't bother to OleUninitialize and let the application exit perform the cleanup.
• Crucially important is OleInitialize and CoInitializeEx - and their cleanup brothers - are performed on a Thread basis.
  For Drag and Drop to work, say, OleInitialize has to be performed on the same thread that registers the IDropTarget object - in WIN_AcceptDragAndDrop.
  I have reworked that part to perform the OleUninitialize when WIN_AcceptDragAndDrop is called with accept being false.

It seems like moving that to WIN_VideoInit()/Quit() is reasonable.

I am not sure I understand your proposal, for you seem to suggest that OleInitialize would be called unconditionally - I mean, independently of whether the application has enabled the SDL_DROP events - by WIN_VideoInit - and OleUninitialize by WIN_VideoQuit. That would be nice to Drag and Drop, but that could break Windows applications that perform CoInitializeEx with COINIT_MULTITHREADED on their own. Also this supposes that WIN_VideoInit runs on the same thread as WIN_AcceptDragAndDrop, only you can tell that.

Would it be better to create some SDL_UnprepareDragAndDrop to call WIN_AcceptDragAndDrop with accept false and hook it to the window destruction ? Though, as I said, this code has worked without the OleUninitialize so far.

[Dragon-Baroque]

For size_t probably the most portable approach is to cast to (Uint64) and use the SDL_PRIu64 formatting macro.

Fought with that. I would need to disable -Wformat.

Usually we don't bother if we know the values will always be small and just cast to int and use %d.

I ended up with unsigned long and %lu. Sorry about the actions pollution.

[slouken]

For size_t probably the most portable approach is to cast to (Uint64) and use the SDL_PRIu64 formatting macro.

I implemented that. Thank you.

What is the impact of OleInititalize() on the application? Is it safe to call multiple times? Is there cleanup associated with it?

I thought that you would have much more experience than I :-) I did some research and :

• OleInitialize ( like CoInitializeEx ) holds a call counter. You can call it several times. It performs more work the first time it is called.
• OleInitialize actually calls CoInitializeEx with COINIT_APARTMENTTHREADED as the first step , whether or not this is the first time it is called.
  This internal CoInitializeEx with fail and cause OleInitialize to fail too if CoInitializeEx has been called previously with COINIT_MULTITHREADED. OLE is not thread-safe, and it cannot support COINIT_MULTITHREADED.
• Normally OleInitialize calls should be paired with ( the same number of ) OleUninitialize. I have seen applications which don't bother to OleUninitialize and let the application exit perform the cleanup.
• Crucially important is OleInitialize and CoInitializeEx - and their cleanup brothers - are performed on a Thread basis.
  For Drag and Drop to work, say, OleInitialize has to be performed on the same thread that registers the IDropTarget object - in WIN_AcceptDragAndDrop.
  I have reworked that part to perform the OleUninitialize when WIN_AcceptDragAndDrop is called with accept being false.

Good research!

Does this mean that an application that calls CoInitializeEx with COINIT_MULTITHREADED will not get any drag and drop functionality with your change? If so, then maybe we should leave in the WM_DROPFILES support as a fallback?

It seems like moving that to WIN_VideoInit()/Quit() is reasonable.

I am not sure I understand your proposal, for you seem to suggest that OleInitialize would be called unconditionally - I mean, independently of whether the application has enabled the SDL_DROP events - by WIN_VideoInit - and OleUninitialize by WIN_VideoQuit. That would be nice to Drag and Drop, but that could break Windows applications that perform CoInitializeEx with COINIT_MULTITHREADED on their own. Also this supposes that WIN_VideoInit runs on the same thread as WIN_AcceptDragAndDrop, only you can tell that.

It sounds like WIN_VideoInit() should call WIN_CoInitialize(), which will by default initialize COINIT_APARTMENTTHREADED. It should then call OleInitialize(), and if that fails, enable the WM_DROPFILES fallback. Then WIN_VideoQuit() should call OleUninitialize() if OleInitialize() succeeded, and then call WIN_CoUninitialize() if WIN_CoInitialize() succeeded earlier.

SDL requires that you pump events on the main thread (that initialized video), so this should work well, and you don't have to do any specific OLE initialization around the drag and drop code yourself.

[Dragon-Baroque]

It sounds like WIN_VideoInit() should call WIN_CoInitialize(), which will by default initialize COINIT_APARTMENTTHREADED. It should then call OleInitialize(), and if that fails, enable the WM_DROPFILES fallback. Then WIN_VideoQuit() should call OleUninitialize() if OleInitialize() succeeded, and then call WIN_CoUninitialize() if WIN_CoInitialize() succeeded earlier.

That is what I meant by unconditionally. A SDL 3 application under Windows would always connect to COM then connect to OLE, whether or not COM is required because of WASAPI or DirectInput or DirectSound or SensorManager, and whether or not OLE is required because of Drag and Drop - which is disabled by default, am I correct ?

If you assume that, then I am fine with this proposal and I am willing to implement it.

Otherwise I can offer two alternate proposals. They have a common part :

• Leave the OleInitialize and the OleUninitialize logic in WIN_AcceptDragAndDrop, enable DROP_FILES when OleInitialize fails.
• Leave the WIN_AcceptDragAndDrop enablement logic as it is now, that is PrepareDragAndDropSupport for the initial setup and SDL_ToggleDragAndDropSupport when the Drag and Drop events become enabled.
• But hook a disablement call to WIN_AcceptDragAndDrop with accept false on some point of the termination of SDL.

Two proposals for that hook location :

1. Add a call to WIN_AcceptDragAndDrop( false ) into WIN_VideoQuit. Easiest but unsymmetrical - the initial WIN_AcceptDragAndDrop is performed by PrepareDragAndDropSupport from SDL_FinishWindowCreation from SDL_CreateWindowWithProperties, not by SDL_VideoInit or its descendants.
2. Create a parallel chain to PrepareDragAndDropSupport named UnprepareDragAndDropSupport in src/video/SDL_Video.c. UnprepareDragAndDropSupport calling ->AcceptDragAndDrop(false) and being called by some point of SDL_DestroyWindow. More work, but symmetrical, and allowing other Drag and Drop mechanisms to clean up properly too.

[Dragon-Baroque]

Just a logic change to Proposal 1.
Proposal 1. does not work because the WIN_AcceptDragAndDrop records its state onto the internal part of the SDL_Window, which is most certainly freed when WIN_VideoQuit occurs.
Call WIN_AcceptDragAndDrop false from WIN_DestroyWindow instead.

[slouken]

Merged, thanks!

[Dragon-Baroque]

Two down, one to go : Wayland. This one has a tricky issue.
Is the SDL 3.2 schedule a matter of weeks or a matter of months ?

[slouken]

Two down, one to go : Wayland. This one has a tricky issue. Is the SDL 3.2 schedule a matter of weeks or a matter of months ?

The ABI lock is within weeks, but release is farther out. I assume Wayland won't require an ABI change?

[Dragon-Baroque]

I assume Wayland won't require an ABI change ?

I honestly don't see why it would. To be fair, the Wayland Drag and Drop is working now, not requiring and ABI change. However, a section of that code is ugly. Thus a question : If it comes down to that, does a request for a new SDL Hint count as an ABI change ?

[slouken]

I assume Wayland won't require an ABI change ?

I honestly don't see why it would. To be fair, the Wayland Drag and Drop is working now, not requiring and ABI change. However, a section of that code is ugly. Thus a question : If it comes down to that, does a request for a new SDL Hint count as an ABI change ?

No, it doesn't, as long as the default behavior matches how it has always worked.