Peter Dons Tychsen wrote:
> On Sun, 2007-07-29 at 11:15 -0600, Vitaliy Margolen wrote:
>> Peter Dons Tychsen wrote:
>>> Hello again Wine!
>>>
>>> Here is a small fix for dinput which fixes application which call
>>> CreateDevice with the standard Joystick GUID instead of the GUID
>>> supplied in the enumeration callback.
>> The patch looks good, and I see what you trying to fix. It would be
>> nice if you add some tests for this. MSDN is pretty explicit about
>> what GUIDs could be used there.
>>
>> Few nitpicks about your patch:
>>
>>> -static BOOL IsJoystickGUID(REFGUID guid)
>>> +/******************************************************************************
>>> + * Acquire : gets exclusive control of the joystick
>>> + */
>>> +static CHAR IsJoystickGUID(REFGUID guid)
>>> {
>>>
>> Comment are good, but only when they are correct. Also you probably should
>> rename this function to something like get_joystick_index.
>>
>>> + return -1;
>>> }
>>>
>>> static HRESULT joydev_create_deviceA(IDirectInputImpl *dinput, REFGUID rguid, REFIID riid, LPDIRECTINPUTDEVICEA* pdev)
>>> {
>>> - if ((IsEqualGUID(&GUID_Joystick,rguid)) ||
>>> - (IsJoystickGUID(rguid))) {
>>> + CHAR index;
>> Why CHAR? the Data3 defined as "unsigned short Data3;". That is what
>> you really should use. BTW be careful with that "return -1;" it won't
>> be < 0 for unsigned anymore.
>>
>>> - GUID wine_joystick = DInput_Wine_Joystick_GUID;
>>> - GUID dev_guid = *guid;
>>> + GUID wine_joystick = DInput_Wine_Joystick_GUID;
>>> + GUID dev_guid = *guid;
>> Please do not reformat the code. It should be 4-spaces indentation, not 2.
>>
>> And while you changing this, don't forget to change joystick_linuxinput.c as well.
>> They should be kept in sync.
>>
>>
>> Vitaliy.
>
> Hi Vitaliy.
>
> OK, thanks for the input:
First of all, please bottom post on this list. We are not business.
>
> I have re-sent the patch with all your remarks fixed, except for the one
> regarding joystick_linuxinput.c, which cannot be done with the current
> code as alloc_device() is too different in the two versions for the
> patch to work. I have however made sure that the version in
> joystick_linuxinput.c does not have this problem.
>
Why do you think they are so different? Looks almost the same to me.
Few more things to fix:
> -#undef MAX_JOYSTICKS
You removed this, but you haven't added it back.
> GetJoystickIndex
All the functions around that place named properly. Don't use up-casing names.
I suggested using "get_joystick_index" - all lover case, with underscores.
> -static HRESULT alloc_device(REFGUID rguid, const void *jvt, IDirectInputImpl *dinput, LPDIRECTINPUTDEVICEA* pdev)
> +static HRESULT alloc_device(REFGUID rguid, const void *jvt, IDirectInputImpl *dinput, LPDIRECTINPUTDEVICEA* pdev, unsigned short index)
This is a really long line, please make it shorter.