Application using Delphi runtime with Virtual-TreeView module, like xEdit, pass a NULL IDropSource to SHDoDragDrop, prevnting the drag and drop to works at all, when used in Windos Vista and above compatiblity mode, while windows xp compatibility mode works (as it use a dfferent system). Documentation stated that Windows Vista and above will create an internal IDropSource in this case. Creating a dummy IDropSource with minimal functionality allow the drag and drop to works using the default cursor. In case of xEdit it won't show the cell that it's being dragged like on windows, a functionality that require a full implementation of IDropSource::GiveFeedback and DragSourceHelper2::InitializeFromBitmap.
Solve the functional part of #54923
I'm unsure on how to write tests for this.
From: Lorenzo Ferrillo lorenzofersteam@live.it
--- dlls/shell32/shellord.c | 96 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 2 deletions(-)
diff --git a/dlls/shell32/shellord.c b/dlls/shell32/shellord.c index a007c0e5c60..d9f8f081675 100644 --- a/dlls/shell32/shellord.c +++ b/dlls/shell32/shellord.c @@ -534,6 +534,88 @@ HRESULT WINAPI SHRevokeDragDrop(HWND hWnd) return RevokeDragDrop(hWnd); }
+ +typedef struct IDummySource{ + IDropSource IDropSource_iface; + LONG Ref; +} IDummySource; + +static inline IDummySource *impl_from_IDropSource(IDropSource *iface) +{ + return CONTAINING_RECORD(iface, IDummySource, IDropSource_iface); +} + +static HRESULT WINAPI IDummyDropSource_QueryInterface(IDropSource *iface, REFIID riid, void **ppvObj) +{ + IDummySource *This = impl_from_IDropSource(iface); + TRACE("(%p)->(%s, %p)\n", This, debugstr_guid(riid), ppvObj); + + *ppvObj = NULL; + + if(IsEqualIID(riid, &IID_IDropSource)) + { + *ppvObj = &This->IDropSource_iface; + TRACE("-- Interface: (%p)->(%p)\n",ppvObj,*ppvObj); + return S_OK; + } + + TRACE("-- Interface: E_NOINTERFACE\n"); + return E_NOINTERFACE; +} + +static ULONG WINAPI IDummyDropSource_AddRef(IDropSource *iface) +{ + IDummySource *This = impl_from_IDropSource(iface); + TRACE("(%p)->(%lu)\n", This, This->Ref); + return InterlockedIncrement(&This->Ref); +} + +static ULONG WINAPI IDummyDropSource_Release(IDropSource *iface) +{ + ULONG refcount; + IDummySource *This = impl_from_IDropSource(iface); + TRACE("(%p)->(%lu)\n", This, This->Ref); + refcount = InterlockedDecrement(&This->Ref); + if(refcount == 0){ + free(This); + } + return refcount; +} + +static HRESULT WINAPI IDummyDropSource_QueryContinueDrag( + IDropSource *iface, + BOOL fEscapePressed, + DWORD grfKeyState) +{ + IDummySource *This = impl_from_IDropSource(iface); + TRACE("(%p)\n",This); + + if (fEscapePressed) + return DRAGDROP_S_CANCEL; + else if (!(grfKeyState & MK_LBUTTON) && !(grfKeyState & MK_RBUTTON)) + return DRAGDROP_S_DROP; + else + return S_OK; +} + +static HRESULT WINAPI IDummyDropSource_GiveFeedback( + IDropSource *iface, + DWORD dwEffect) +{ + IDummySource *This = impl_from_IDropSource(iface); + TRACE("(%p)\n",This); + + return DRAGDROP_S_USEDEFAULTCURSORS; +} + +static const IDropSourceVtbl dropsourcevtbl = +{ + IDummyDropSource_QueryInterface, + IDummyDropSource_AddRef, + IDummyDropSource_Release, + IDummyDropSource_QueryContinueDrag, + IDummyDropSource_GiveFeedback +}; /************************************************************************* * SHDoDragDrop [SHELL32.88] * @@ -555,8 +637,18 @@ HRESULT WINAPI SHDoDragDrop( LPDWORD pdwEffect) { FIXME("(%p %p %p 0x%08lx %p):stub.\n", - hWnd, lpDataObject, lpDropSource, dwOKEffect, pdwEffect); - return DoDragDrop(lpDataObject, lpDropSource, dwOKEffect, pdwEffect); + hWnd, lpDataObject, lpDropSource, dwOKEffect, pdwEffect); + if(lpDropSource == NULL) + { + /*Windows Vista or Above expect this function to create a IDropSource object if none was provided. Delphi Windows Vista drag and drop routines actually expect this to work */ + IDummySource* dummy = calloc(1, sizeof(IDummySource)); + if(dummy){ + dummy->IDropSource_iface.lpVtbl = &dropsourcevtbl; + dummy->Ref = 1; + lpDropSource = &dummy->IDropSource_iface; + } + } + return DoDragDrop(lpDataObject, lpDropSource, dwOKEffect, pdwEffect); }
/*************************************************************************
Alfred Agrell (@Alcaro) commented about dlls/shell32/shellord.c:
- LONG Ref;
+} IDummySource;
+static inline IDummySource *impl_from_IDropSource(IDropSource *iface) +{
- return CONTAINING_RECORD(iface, IDummySource, IDropSource_iface);
+}
+static HRESULT WINAPI IDummyDropSource_QueryInterface(IDropSource *iface, REFIID riid, void **ppvObj) +{
- IDummySource *This = impl_from_IDropSource(iface);
- TRACE("(%p)->(%s, %p)\n", This, debugstr_guid(riid), ppvObj);
- *ppvObj = NULL;
- if(IsEqualIID(riid, &IID_IDropSource))
Shouldn't this also accept IUnknown?
Alfred Agrell (@Alcaro) commented about dlls/shell32/shellord.c:
- return E_NOINTERFACE;
+}
+static ULONG WINAPI IDummyDropSource_AddRef(IDropSource *iface) +{
- IDummySource *This = impl_from_IDropSource(iface);
- TRACE("(%p)->(%lu)\n", This, This->Ref);
- return InterlockedIncrement(&This->Ref);
+}
+static ULONG WINAPI IDummyDropSource_Release(IDropSource *iface) +{
- ULONG refcount;
- IDummySource *This = impl_from_IDropSource(iface);
- TRACE("(%p)->(%lu)\n", This, This->Ref);
- refcount = InterlockedDecrement(&This->Ref);
Is that double space intentional?
Alfred Agrell (@Alcaro) commented about dlls/shell32/shellord.c:
LPDWORD pdwEffect) { FIXME("(%p %p %p 0x%08lx %p):stub.\n",
- hWnd, lpDataObject, lpDropSource, dwOKEffect, pdwEffect);
- return DoDragDrop(lpDataObject, lpDropSource, dwOKEffect, pdwEffect);
hWnd, lpDataObject, lpDropSource, dwOKEffect, pdwEffect);
- if(lpDropSource == NULL)
- {
/*Windows Vista or Above expect this function to create a IDropSource object if none was provided. Delphi Windows Vista drag and drop routines actually expect this to work */
IDummySource* dummy = calloc(1, sizeof(IDummySource));
You've got several duplicate spaces and other odd grammar on these two lines too.
Alfred Agrell (@Alcaro) commented about dlls/shell32/shellord.c:
LPDWORD pdwEffect) { FIXME("(%p %p %p 0x%08lx %p):stub.\n",
- hWnd, lpDataObject, lpDropSource, dwOKEffect, pdwEffect);
- return DoDragDrop(lpDataObject, lpDropSource, dwOKEffect, pdwEffect);
hWnd, lpDataObject, lpDropSource, dwOKEffect, pdwEffect);
- if(lpDropSource == NULL)
- {
/*Windows Vista or Above expect this function to create a IDropSource object if none was provided. Delphi Windows Vista drag and drop routines actually expect this to work */
IDummySource* dummy = calloc(1, sizeof(IDummySource));
if(dummy){
dummy->IDropSource_iface.lpVtbl = &dropsourcevtbl;
dummy->Ref = 1;
lpDropSource = &dummy->IDropSource_iface;
I'm not seeing where that object is freed. Am I missing something, or is this a memory leak?
And this should probably return E_OUTOFMEMORY, not call DoDragDrop with bogus arguments.
Alfred Agrell (@Alcaro) commented about dlls/shell32/shellord.c:
return RevokeDragDrop(hWnd);
}
+typedef struct IDummySource{
That's not an interface. I'd drop this I (as well as the ones in the function names).
Alfred Agrell (@Alcaro) commented about dlls/shell32/shellord.c:
+static HRESULT WINAPI IDummyDropSource_QueryInterface(IDropSource *iface, REFIID riid, void **ppvObj) +{
- IDummySource *This = impl_from_IDropSource(iface);
- TRACE("(%p)->(%s, %p)\n", This, debugstr_guid(riid), ppvObj);
- *ppvObj = NULL;
- if(IsEqualIID(riid, &IID_IDropSource))
- {
*ppvObj = &This->IDropSource_iface;
TRACE("-- Interface: (%p)->(%p)\n",ppvObj,*ppvObj);
return S_OK;
- }
- TRACE("-- Interface: E_NOINTERFACE\n");
Isn't E_NOINTERFACE usually a WARN? And I don't think that's the usual QueryInterface trace formatting, but I'm not sure how consistent Wine is.
On Fri May 16 17:44:03 2025 +0000, Alfred Agrell wrote:
Is that double space intentional?
no, it isn't
On Fri May 16 17:44:03 2025 +0000, Alfred Agrell wrote:
Shouldn't this also accept IUnknown?
it can technically be idempotent, it should be possible to just to return the same pointer. I'm not sure if it's needed at all considering it's never exposed outside shell and ole code
On Fri May 16 17:44:03 2025 +0000, Alfred Agrell wrote:
I'm not seeing where that object is freed. Am I missing something, or is this a memory leak? And this should probably return E_OUTOFMEMORY, not call DoDragDrop with bogus arguments.
right on both front. I need to check better the flow regarding freeing the interface, mainly if DoDragDrop is a sort of loop or it return immediately to the caller
On Fri May 16 17:44:03 2025 +0000, Alfred Agrell wrote:
Isn't E_NOINTERFACE usually a WARN? And I don't think that's the usual QueryInterface trace formatting, but I'm not sure how consistent Wine is.
This is replicated from others shell32 interfaces. Not sure what is considered standard QueryItnerface formatting, but if needed I can make it a WARN
On Fri May 16 17:44:03 2025 +0000, Alfred Agrell wrote:
You've got several duplicate spaces and other odd grammar on these two lines too.
Sorry, I'm not an english native speaker, what do you find odd? Sorry for the double spaces
On Fri May 16 19:03:49 2025 +0000, Alfred Agrell wrote:
That's not an interface. I'd drop this I (as well as the ones in the function names).
Do you think InternalDropSource would be a better name?
On Fri May 16 18:47:49 2025 +0000, Lorenzo Ferrillo wrote:
it can technically be idempotent, it should be possible to just to return the same pointer. I'm not sure if it's needed at all considering it's never exposed outside shell and ole code
Necessary, no, but it's inconsistent. It's no more important than other code style issues, but still worth fixing.
Also idempotent means that if you do it twice, the result is the same as doing it once, for example ShowWindow(hwnd, SW_SHOW) or fflush(). QueryInterface is not idempotent, it changes the refcount.
...or, well, it should change the refcount. Looks like you forgot that.
On Fri May 16 18:51:43 2025 +0000, Lorenzo Ferrillo wrote:
right on both front. I need to check better the flow regarding freeing the interface, mainly if DoDragDrop is a sort of loop or it return immediately to the caller
MS docs say it is a loop.
But even if it wasn't, you can just release that reference immediately after DoDragDrop returns. If the function needs to keep it, it'll add its own reference.
(To clarify the E_OUTOFMEMORY thing: Should do that if dummy is null.)
On Fri May 16 18:53:20 2025 +0000, Lorenzo Ferrillo wrote:
This is replicated from others shell32 interfaces. Not sure what is considered standard QueryItnerface formatting, but if needed I can make it a WARN
I'm more used to them looking like https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/quartz/filtergraph.c#....
But you're right, it does vary within Wine. Better be consistent within shell32 and ignore this comment.
On Fri May 16 19:03:27 2025 +0000, Lorenzo Ferrillo wrote:
Sorry, I'm not an english native speaker, what do you find odd? Sorry for the double spaces
Missing space after /*, capitalizing Above, and 'Delphi Windows Vista' feels weird somehow. Change to 'Delphi on Windows Vista', maybe?
On Fri May 16 19:15:49 2025 +0000, Lorenzo Ferrillo wrote:
Do you think InternalDropSource would be a better name?
I was more thinking of DummySource, but InternalDropSource sounds good too. Pick your favorite.
On Fri May 16 19:15:49 2025 +0000, Alfred Agrell wrote:
Necessary, no, but it's inconsistent. It's no more important than other code style issues, but still worth fixing. Also idempotent means that if you do it twice, the result is the same as doing it once, for example ShowWindow(hwnd, SW_SHOW) or fflush(). QueryInterface is not idempotent, it changes the refcount. ...or, well, it should change the refcount. Looks like you forgot that.
you are right, I did put it into the other patch implementing the Bitmap handling too, but should have been here
On Fri May 16 19:15:49 2025 +0000, Alfred Agrell wrote:
Missing space after /*, capitalizing Above, and 'Delphi Windows Vista' feels weird somehow. Change to 'Delphi on Windows Vista', maybe?
IS this better @Alcaro
Delphi's drag and drop functions when it detect Windows Vista or above actually expect this to work
On Fri May 16 20:19:41 2025 +0000, Alfred Agrell wrote:
MS docs say it is a loop. But even if it wasn't, you can just release that reference immediately after DoDragDrop returns. If the function needs to keep it, it'll add its own reference. (To clarify the E_OUTOFMEMORY thing: Should do that if dummy is null.)
Done
On Fri May 16 19:15:49 2025 +0000, Alfred Agrell wrote:
I'm more used to them looking like https://gitlab.winehq.org/wine/wine/-/blob/master/dlls/quartz/filtergraph.c#.... But you're right, it does vary within Wine. Better be consistent within shell32 and ignore this comment.
I saw some interfaces using TRACE and others using FIXME,WARN and ERR for the E_NOTINTERFACE case. Shell32 is totally not consistent, unless the different usages is due to what is encountered on the wild?
On Fri May 16 20:18:43 2025 +0000, Lorenzo Ferrillo wrote:
IS this better @Alcaro
Delphi's drag and drop functions when it detect Windows Vista or above
actually expect this to work
Still a little grammatically awkward, but yes, it's an improvement.
Delphi's drag and drop functions expect this to work when it detects Windows Vista or above
or maybe simplify to
Delphi's drag and drop functions expect this to work on Vista or above
On Fri May 16 20:22:35 2025 +0000, Lorenzo Ferrillo wrote:
I saw some interfaces using TRACE and others using FIXME,WARN and ERR for the E_NOTINTERFACE case. Shell32 is totally not consistent, unless the different usages is due to what is encountered on the wild?
If it's inconsistent even within shell32, then consistency is irrelevant for that part of this MR; just pick your favorite.
Personally I'd go for FIXME, but it makes little or no difference. Especially for internal objects like this.