Re: user32: DdeClientTransaction should return 0 in the last param forDDE_DATA
"Jeff Latimer" <lats(a)yless4u.com.au> 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. -- Dmitry.
Dmitry Timoshkov wrote:
"Jeff Latimer" <lats(a)yless4u.com.au> 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
"Jeff Latimer" <lats(a)yless4u.com.au> 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. -- Dmitry.
Dmitry Timoshkov wrote:
"Jeff Latimer" <lats(a)yless4u.com.au> 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.
"Jeff Latimer" <lats(a)yless4u.com.au> 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. -- Dmitry.
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
participants (2)
-
Dmitry Timoshkov -
Jeff Latimer