codeweavers' DDE patch

[Eric Pouech]

Hi Dmitry,

regarding the DDE related patch that Alexandre committed a couple of 
days ago from codeweavers' tree 
(http://cvs.winehq.com/patch.py?root=/home/winehq/opt/cvs-commit&id=8293), 
I have a couple of questions:

1/
diff -u wine/dlls/user/dde/server.c:1.11 wine/dlls/user/dde/server.c:1.12
--- wine/dlls/user/dde/server.c:1.11	Sat May 24 10:42:52 2003
+++ wine/dlls/user/dde/server.c	Sat May 24 10:42:52 2003
@@ -101,7 +101,7 @@
    if (DdeCmpStringHandles(hszItem, pLink->hszItem) == 0)
    {
        hDdeData = WDML_InvokeCallback(pInstance, XTYP_ADVREQ, 
pLink->uFmt, pLink->hConv,
-					   hszTopic, hszItem, 0, count--, 0);
+					   hszTopic, hszItem, 0, 0, 0);

if (hDdeData == (HDDEDATA)CBR_BLOCK)
        {
why did you remove the count-- reference as dwData1 ?
MSDN states:
(http://msdn.microsoft.com/library/default.asp?url=/library/en-us/winui/winui...)
Specifies the count, in the low-order word, of XTYP_ADVREQ transactions 
that remain to be processed on the same topic, item, and format name set 
within the context of the current call to the DdePostAdvise function. 
The count is zero if the current XTYP_ADVREQ transaction is the last 
one. A server can use this count to determine whether to create an 
HDATA_APPOWNED data handle for the advise data.

2/
Index: wine/dlls/user/dde/server.c
diff -u wine/dlls/user/dde/server.c:1.11 wine/dlls/user/dde/server.c:1.12
--- wine/dlls/user/dde/server.c:1.11	Sat May 24 10:42:52 2003
+++ wine/dlls/user/dde/server.c	Sat May 24 10:42:52 2003
@@ -570,7 +581,7 @@
    break;
      default:
          {
-	    HGLOBAL	hMem = WDML_DataHandle2Global(hDdeData, FALSE, FALSE, 
FALSE, FALSE);
+	    HGLOBAL	hMem = WDML_DataHandle2Global(hDdeData, TRUE, TRUE, FALSE, 
FALSE);
        if (!PostMessageA(pConv->hwndClient, WM_DDE_DATA, 
(WPARAM)pConv->hwndServer,
    		      ReuseDDElParam(pXAct->lParam, WM_DDE_REQUEST, WM_DDE_DATA,
    				     (UINT)hMem, (UINT)pXAct->atom)))

which did you change fResponse (or fAckReq) and fRelease to TRUE ?
since the server side isn't ready to handle the ack messages the client 
will in such cases, I'm not sure it's worth it. Moreover, it will 
complexify the memory management and create more leaks than already exist.

A+

[Dmitry Timoshkov]

First of all, Eric, thank you very much for reviewing these patches,
it's very important to do the right thing in such a fragile area as DDE.

"Eric Pouech"  wrote:
> 1/
> diff -u wine/dlls/user/dde/server.c:1.11 wine/dlls/user/dde/server.c:1.12
> --- wine/dlls/user/dde/server.c:1.11 Sat May 24 10:42:52 2003
> +++ wine/dlls/user/dde/server.c Sat May 24 10:42:52 2003
> @@ -101,7 +101,7 @@
>   if (DdeCmpStringHandles(hszItem, pLink->hszItem) == 0)
>   {
>       hDdeData = WDML_InvokeCallback(pInstance, XTYP_ADVREQ, 
> pLink->uFmt, pLink->hConv,
> - hszTopic, hszItem, 0, count--, 0);
> -
>
> hszTopic, hszItem, 0, 0, 0);
>
> if (hDdeData == (HDDEDATA)CBR_BLOCK)
>  {
>
> why did you remove the count-- reference as dwData1 ?

Probably I was fooled by my tests in which I couldn't reproduce a not zero
dwData1. I just retested, actually that part can be reverted, it doesn't effect
the application I'm worrying about.
> 2/
> Index: wine/dlls/user/dde/server.c
> diff -u wine/dlls/user/dde/server.c:1.11 wine/dlls/user/dde/server.c:1.12
> --- wine/dlls/user/dde/server.c:1.11 Sat May 24 10:42:52 2003
> +++ wine/dlls/user/dde/server.c Sat May 24 10:42:52 2003
> @@ -570,7 +581,7 @@
>   break;
>       default:
>           {
> -
> HGLOBAL hMem = WDML_DataHandle2Global(hDdeData, FALSE, FALSE, 
>
> FALSE, FALSE);
> -
> HGLOBAL hMem = WDML_DataHandle2Global(hDdeData, TRUE, TRUE, FALSE, 
>
> FALSE);
>       if (!PostMessageA(pConv->hwndClient, WM_DDE_DATA, 
> (WPARAM)pConv->hwndServer,
>         ReuseDDElParam(pXAct->lParam, WM_DDE_REQUEST, WM_DDE_DATA,
>        (UINT)hMem, (UINT)pXAct->atom)))
>
> which did you change fResponse (or fAckReq) and fRelease to TRUE ?
> since the server side isn't ready to handle the ack messages the client 
> will in such cases, I'm not sure it's worth it. Moreover, it will 
> complexify the memory management and create more leaks than already exist.

It's fResponse and fRelease which were changed.

Since we handle WM_DDE_REQUEST here, we must set fResponse to TRUE according
to MSDN. And I retested again under both win2k and Wine, and fRelease change
can be reverted. Setting it to FALSE doesn't effect my applications.

Is the attached patch acceptable for you?

[Eric Pouech]

Dmitry Timoshkov wrote:
> First of all, Eric, thank you very much for reviewing these patches,
> it's very important to do the right thing in such a fragile area as DDE.

as DDE protocole is a nightmare (from an implementation point of 
view)... but other fixes from your initial are more than welcomed too !
> It's fResponse and fRelease which were changed.
>
> Since we handle WM_DDE_REQUEST here, we must set fResponse to TRUE according
> to MSDN. And I retested again under both win2k and Wine, and fRelease change
> can be reverted. Setting it to FALSE doesn't effect my applications.

you're right here.
> Is the attached patch acceptable for you?

sounds fine yes. thanks

A+