Re: dinput - Allow the use of the standard Joystick GUID when calling CreateDevice

[Vitaliy Margolen]

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.