Re: GSoC: dinput8 Action Mapping

[Vitaliy Margolen]

On 05/29/2011 04:18 PM, Lucas Zawacki wrote:
> I'm sending a series of patches so everyone can take a
> look before I try commiting them. Please note that these contain a
> partial implementation of EnumDevicesBySemantics, BuildActionMap and
> SetActionMap, and also they're only the A versions.

In general you heading the right direction. In practice these patches can't 
go in as is for number of reasons:
- Too many white space changes. Yes, I know dinput looks terrible, thanks
   to whomever wrote it 10+ years ago. But current Wine policies do not
   allow white space only changes. You can fix few lines around area you
   modifying but not throughout the entire file
- If you modifying line please change tabs to spaces
- I've been working for years to get codding style of dinput to one
   standard - curly braces on their own line. No curlies when you don't need
   them. Please don't break it.
- You have to implement both A & W versions of your functions, or things
   will break.
> - FIXME("(this=%p,%s,%p,%p,%p,%04x): stub\n", This, ptszUserName, lpdiActionFormat,
> - TRACE("(this=%p,%s,%p,%p,%p,%04x): stub\n", This, ptszUserName, lpdiActionFormat,

Don't change fixme to trace without actually implementing stuff. If it's 
partially implemented - change the message to say "semi-stub".
> -
>    if ((r = dinput_devices[i]->enum_deviceA(DI8DEVCLASS_ALL, dwFlags, &devinst, This->dwVersion, j))) {
> -
>        /* note that we have to create the device and set it's data format */
> -
>        if ( GET_DIDEVICE_TYPE(devinst.dwDevType) == DI8DEVTYPE_KEYBOARD ) {

If you looking for keyboard then you should ask for keyboard devices only 
instead of DI8DEVCLASS_ALL. Also note that according to MSDN ans some of my 
tests, keyboard and mouse was always enumerated, regardless of action format.
> -
>            IDirectInputDevice_SetDataFormat(lpdidev,&c_dfDIKeyboard);

This should be done in IDirectInputDevice8AImpl_SetActionMap not 
EnumDevicesBySemantics.
>  ok(SUCCEEDED(hr), "EnumDevicesBySemantics failed: hr=%08x\n",hr);
> - todo_wine ok (ndevices > 0, "EnumDevicesBySemantics did not call the callback. hr=%08x ndevices=%d\n", hr, ndevices);
> - ok(ndevices > 0, "EnumDevicesBySemantics did not call the callback. hr=%08x ndevices=%d\n", hr, ndevices);

I'm not seeing any changes in the dinput code. How did this test got fixed? 
You sure it wasn't broken because of incorrect function call? Then it would 
have failed on Windows.
> -
>        /* set the buffer */
> -
>        if (lpdiActionFormat->dwBufferSize > 0) {
> -
>            IDirectInputDevice_Unacquire(lpdidev);
> -
>            dipdw.diph.dwSize = sizeof(DIPROPDWORD);
> -
>            dipdw.dwData = lpdiActionFormat->dwBufferSize;
> -
>            IDirectInputDevice_SetProperty(lpdidev,DIPROP_BUFFERSIZE,&dipdw.diph);
> -
>        }

This is wrong. You do not need to un-acquire device - it was just created. 
You must specify diph.dwHow = DIPH_DEVICE and diph.dwHeaderSize = 
sizeof(DIPROPHEADER) or this call fail. And this again needs to go into 
SetActionMap.
> - /* each app data is indexed by the device ObjID */
> - DWORD                       actionmap[MAX_DEVICE_OBJECTS];

Wrong data type. Should be UINT_PTR not DWORD.
> I'm not too sure of how to code the tests that require user input...

You can inject input events with SendInput or keybd_event/mouse_event. This 
of course only applies to keyboard and mouse. Joysticks will have to be 
tested manually.

Please combine patches that implement something with tests for it. Also 
don't forget to run your tests on Windows. Or at least on Wine with native 
dinput.dll & dinput8.dll.

Vitaliy