[PATCH 0/2] MR2560: dinput: Add test and fix EnumObjects callback return value handling.
First commit adds a "todo_wine" test, second commit fixes the behavior and marks the test as no longer "todo". It looks like the same logic is already implemented (and tested) for the EnumDevices method. Commit message for the fix commit: --- This solves an issue in ZUSI 3 settings for DirectInput devices. Delphi defines the True value of the "C-compatible" LongBool type as -1, which wine interpreted to mean DIENUM_STOP because it is != DIENUM_CONTINUE. Change that logic so only an explicit DIENUM_STOP (= 0) return value stops the enumeration of objects. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2560
From: Florian Will <florian.will(a)gmail.com> --- dlls/dinput/tests/device8.c | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/dlls/dinput/tests/device8.c b/dlls/dinput/tests/device8.c index ea0ec8db837..128fb2992f6 100644 --- a/dlls/dinput/tests/device8.c +++ b/dlls/dinput/tests/device8.c @@ -991,6 +991,13 @@ static BOOL CALLBACK check_object_count( const DIDEVICEOBJECTINSTANCEW *obj, voi return DIENUM_CONTINUE; } +static BOOL CALLBACK check_object_count_bad_retval( const DIDEVICEOBJECTINSTANCEW *obj, void *args ) +{ + DWORD *count = args; + *count = *count + 1; + return -1; /* Invalid, but should CONTINUE. Only explicit DIENUM_STOP will stop enumeration. */ +} + static void test_sys_mouse( DWORD version ) { const DIDEVCAPS expect_caps = @@ -1336,6 +1343,11 @@ static void test_sys_mouse( DWORD version ) ok( check_objects_params.index >= check_objects_params.expect_count, "missing %u objects\n", check_objects_params.expect_count - check_objects_params.index ); + res = 0; + hr = IDirectInputDevice8_EnumObjects( device, check_object_count_bad_retval, &res, DIDFT_AXIS ); + ok( hr == DI_OK, "EnumObjects returned %#lx\n", hr ); + todo_wine ok( res == 3, "got %lu expected 3\n", res ); + objinst.dwSize = sizeof(DIDEVICEOBJECTINSTANCEW); res = MAKELONG( HID_USAGE_GENERIC_X, HID_USAGE_PAGE_GENERIC ); hr = IDirectInputDevice8_GetObjectInfo( device, &objinst, res, DIPH_BYUSAGE ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2560
From: Florian Will <florian.will(a)gmail.com> This solves an issue in ZUSI 3 settings for DirectInput devices. Delphi defines the True value of the "C-compatible" LongBool type as -1, which wine interpreted to mean DIENUM_STOP because it is != DIENUM_CONTINUE. Change that logic so only an explicit DIENUM_STOP (= 0) return value stops the enumeration of objects. --- dlls/dinput/device.c | 4 +++- dlls/dinput/tests/device8.c | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/dlls/dinput/device.c b/dlls/dinput/device.c index 19c3241e51f..4db8bd943cb 100644 --- a/dlls/dinput/device.c +++ b/dlls/dinput/device.c @@ -730,7 +730,9 @@ static BOOL enum_objects_callback( struct dinput_device *impl, UINT index, struc struct enum_objects_params *params = data; if (instance->wUsagePage == HID_USAGE_PAGE_PID && !(instance->dwType & DIDFT_NODATA)) return DIENUM_CONTINUE; - return params->callback( instance, params->context ); + + /* Applications may return non-zero values instead of DIENUM_CONTINUE. */ + return params->callback( instance, params->context ) ? DIENUM_CONTINUE : DIENUM_STOP; } static HRESULT WINAPI dinput_device_EnumObjects( IDirectInputDevice8W *iface, LPDIENUMDEVICEOBJECTSCALLBACKW callback, diff --git a/dlls/dinput/tests/device8.c b/dlls/dinput/tests/device8.c index 128fb2992f6..a649fe53b41 100644 --- a/dlls/dinput/tests/device8.c +++ b/dlls/dinput/tests/device8.c @@ -1346,7 +1346,7 @@ static void test_sys_mouse( DWORD version ) res = 0; hr = IDirectInputDevice8_EnumObjects( device, check_object_count_bad_retval, &res, DIDFT_AXIS ); ok( hr == DI_OK, "EnumObjects returned %#lx\n", hr ); - todo_wine ok( res == 3, "got %lu expected 3\n", res ); + ok( res == 3, "got %lu expected 3\n", res ); objinst.dwSize = sizeof(DIDEVICEOBJECTINSTANCEW); res = MAKELONG( HID_USAGE_GENERIC_X, HID_USAGE_PAGE_GENERIC ); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/2560
Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=131412 Your paranoid android. === w7u_adm (32 bit report) === dinput: device8: Timeout
On Mon Apr 3 06:49:14 2023 +0000, **** wrote:
Marvin replied on the mailing list: ``` Hi, It looks like your patch introduced the new failures shown below. Please investigate and fix them before resubmitting your patch. If they are not new, fixing them anyway would help a lot. Otherwise please ask for the known failures list to be updated. The tests also ran into some preexisting test failures. If you know how to fix them that would be helpful. See the TestBot job for the details: The full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=131412 Your paranoid android. === w7u_adm (32 bit report) === dinput: device8: Timeout ``` I'm not sure if my patch to the test could have possibly introduced this new failure? I mean, sure, the behavior when returning -1 from the callback instead of DIENUM_CONTINUE / DIENUM_STOP is not specified in the docs. So I guess win7 would be free to do something that would end up e.g. blocking the thread. I suspect that behavior would be pretty unlikely though, and [it didn't reproduce when I manually resubmitted the test](https://testbot.winehq.org/JobDetails.pl?Key=131414).
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2560#note_28622
On Mon Apr 3 07:07:22 2023 +0000, Florian Will wrote:
I'm not sure if my patch to the test could have possibly introduced this new failure? I mean, sure, the behavior when returning -1 from the callback instead of DIENUM_CONTINUE / DIENUM_STOP is not specified in the docs. So I guess win7 would be free to do something that would end up e.g. blocking the thread. I suspect that behavior would be pretty unlikely though, and [it didn't reproduce when I manually resubmitted the test](https://testbot.winehq.org/JobDetails.pl?Key=131414). Don't worry, this sometimes happens for various reasons.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/2560#note_28625
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/2560
participants (4)
-
Florian Will -
Florian Will (＠w-flo)
-
Marvin -
Rémi Bernon