[PATCH 0/1] MR8063: shell32: Create an internal IDropSource in SHDoDragDrop if it wasn't passed by the caller

[Lorenzo Ferrillo (＠llde)]

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.

[PATCH 1/1] shell32: Create an internal IDropSource in SHDoDragDrop if it wasn't passed by the caller

[Lorenzo Ferrillo]

From: Lorenzo Ferrillo

---
 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)]

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)]

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)]

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)]

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)]

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)]

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.

[Lorenzo Ferrillo (＠llde)]

On Fri May 16 17:44:03 2025 +0000, Alfred Agrell wrote:
> Is that double space intentional?

no, it isn't

[Lorenzo Ferrillo (＠llde)]

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

[Lorenzo Ferrillo (＠llde)]

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

[Lorenzo Ferrillo (＠llde)]

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

[Lorenzo Ferrillo (＠llde)]

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

[Lorenzo Ferrillo (＠llde)]

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?

[Alfred Agrell (＠Alcaro)]

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.

[Alfred Agrell (＠Alcaro)]

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.)

[Alfred Agrell (＠Alcaro)]

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.

[Alfred Agrell (＠Alcaro)]

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?

[Alfred Agrell (＠Alcaro)]

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.

[Lorenzo Ferrillo (＠llde)]

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

[Lorenzo Ferrillo (＠llde)]

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

[Lorenzo Ferrillo (＠llde)]

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

[Lorenzo Ferrillo (＠llde)]

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?

[Alfred Agrell (＠Alcaro)]

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

[Alfred Agrell (＠Alcaro)]

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.