Re: user32: DdeClientTransaction should return 0 in the last param forDDE_DATA

[Dmitry Timoshkov]

"Jeff Latimer"  wrote:
> --- a/dlls/user32/dde_client.c
> +++ b/dlls/user32/dde_client.c
> @@ -520,6 +520,7 @@ static WDML_QUEUE_STATE WDML_HandleRequestReply(WDML_CONV* pConv, MSG* msg, WDML
>     case WM_DDE_DATA:
>         UnpackDDElParam(WM_DDE_DATA, msg->lParam, &uiLo, &uiHi);
>  TRACE("Got the result (%08lx)\n", uiLo);
> - if (ack) *ack = ERROR_SUCCESS;
>
> hsz = WDML_MakeHszFromAtom(pConv->instance, uiHi);
>
> diff --git a/dlls/user32/tests/dde.c b/dlls/user32/tests/dde.c
> index 8ea0525..debbd20 100644
> --- a/dlls/user32/tests/dde.c
> +++ b/dlls/user32/tests/dde.c
> @@ -292,10 +292,7 @@ static void test_ddeml_client(void)
>     hdata = DdeClientTransaction(NULL, 0, conversation, item, CF_TEXT, XTYP_REQUEST, default_timeout, &res);
>     ret = DdeGetLastError(client_pid);
>     ok(ret == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", ret);
> - todo_wine
> - {
> -
>    ok(res == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %08x\n", res);
> - }
> - ok(res == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %08x\n", res);

DdeClientTransaction is not supposed to return ERROR_SUCCESS in res. DDE_FNOTPROCESSED
looks like a more appropriate value. Same for DdeGetLastError, DMLERR_NO_ERROR seems
more reasonable in this case.

Also IMO a more appropriate place to set ack to DDE_FNOTPROCESSED is WDML_HandleReply,
so that all types of replies get covered.

user32: DdeClientTransaction should return 0 in the last param forDDE_DATA

[Jeff Latimer]

Dmitry Timoshkov wrote:
> "Jeff Latimer"  wrote:
> > -
> >
> > if (ack) *ack = ERROR_SUCCESS;
> >
> > ok(ret == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %d\n", ret);
> > - todo_wine
> > - {
> > -
> >    ok(res == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got 
> >
> > %08x\n", res);
> > - }
> > - ok(res == ERROR_SUCCESS, "Expected ERROR_SUCCESS, got %08x\n",
> >
> > res);
>
> DdeClientTransaction is not supposed to return ERROR_SUCCESS in res. 
> DDE_FNOTPROCESSED
> looks like a more appropriate value. Same for DdeGetLastError, 
> DMLERR_NO_ERROR seems
> more reasonable in this case.

Thanks, DMLERR_NO_ERROR looks better.

MSDN does not show DdeCientTransaction as returning in res 
DDE_FNOTPROCESSED.  Is this the standard test to see if it is zero?  
Otherwise an actual test for 0 would be just as good.
> Also IMO a more appropriate place to set ack to DDE_FNOTPROCESSED is 
> WDML_HandleReply,
> so that all types of replies get covered.

I thought this as well when I was hunting for a place for setting ack.  
It seems that handle routines all do something different.  Some don't 
set the ack/res and this causes tests to fail (ie. deadbeef passes 
through some of them).  I opted to place the setting of ack in the 
routine that applies to this test.

Jeff

user32: DdeClientTransaction should return 0 in the last param forDDE_DATA

[Dmitry Timoshkov]

"Jeff Latimer"  wrote:
> MSDN does not show DdeCientTransaction as returning in res 
> DDE_FNOTPROCESSED.  Is this the standard test to see if it is zero?  
> Otherwise an actual test for 0 would be just as good.

Existing tests already check for DDE_FNOTPROCESSED. Using just 0
doesn't say anything, and using meaningful symbol names is always
preferred.
> > Also IMO a more appropriate place to set ack to DDE_FNOTPROCESSED is 
> > WDML_HandleReply,
> > so that all types of replies get covered.
>
> I thought this as well when I was hunting for a place for setting ack.  
> It seems that handle routines all do something different.  Some don't 
> set the ack/res and this causes tests to fail (ie. deadbeef passes 
> through some of them).  I opted to place the setting of ack in the 
> routine that applies to this test.

Although there is no tests for all DdeClientTransaction cases, it should
be pretty safe to assume that all request types behave similar in returning
handling state.

user32: DdeClientTransaction should return 0 in the last param forDDE_DATA

[Jeff Latimer]

Dmitry Timoshkov wrote:
> "Jeff Latimer"  wrote:
> > MSDN does not show DdeCientTransaction as returning in res 
> > DDE_FNOTPROCESSED.  Is this the standard test to see if it is zero?  
> > Otherwise an actual test for 0 would be just as good.
>
> Existing tests already check for DDE_FNOTPROCESSED. Using just 0
> doesn't say anything, and using meaningful symbol names is always
> preferred.

Ok, it will be consistent.
> > > Also IMO a more appropriate place to set ack to DDE_FNOTPROCESSED is 
> > > WDML_HandleReply,
> > > so that all types of replies get covered.
> >
> > I thought this as well when I was hunting for a place for setting 
> > ack.  It seems that handle routines all do something different.  Some 
> > don't set the ack/res and this causes tests to fail (ie. deadbeef 
> > passes through some of them).  I opted to place the setting of ack in 
> > the routine that applies to this test.
>
> Although there is no tests for all DdeClientTransaction cases, it should
> be pretty safe to assume that all request types behave similar in 
> returning
> handling state.

That is not how it looked when I set ack to DMLERR_NO_ERROR at the start 
of the WDML_HandleReply.  Tests started fail != 0xdeadbeef in Windows.  
I would prefer to leave the test where I placed it for that reason.

user32: DdeClientTransaction should return 0 in the last paramforDDE_DATA

[Dmitry Timoshkov]

"Jeff Latimer"  wrote:
> > Although there is no tests for all DdeClientTransaction cases, it should
> > be pretty safe to assume that all request types behave similar in 
> > returning
> > handling state.
>
> That is not how it looked when I set ack to DMLERR_NO_ERROR at the start 
> of the WDML_HandleReply.  Tests started fail != 0xdeadbeef in Windows.  
> I would prefer to leave the test where I placed it for that reason.

I don't see how changing Wine internal implementation could lead to
failing tests under Windows.

user32: DdeClientTransaction should return 0 in the last paramforDDE_DATA

[Jeff Latimer]

Dmitry Timoshkov wrote:
> I don't see how changing Wine internal implementation could lead to
> failing tests under Windows.

My confusion.  Late at night.  Found the problem, the inside the if 
block. Resent the patch.

Jeff