Re: [v3 1/1] winejoystick.drv: Sort virtual joysticks by name on the Mac

[Ken Thomases]

Hi,

On Jul 20, 2016, at 2:15 AM, David Lawrie  wrote:
> +static CFStringRef copy_device_name(IOHIDDeviceRef device)
> +{
> - CFTypeRef ref_name, ref_man;
> - CFStringRef name;
> - long prodID;

I prefer variables to be declared at the narrowest scope in which they're used.
> -
> - if (device)
> - {
> -
>    assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device));
> -
> -
>    ref_name = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey));
> -
> -
>    if (ref_name && CFStringGetTypeID() == CFGetTypeID(ref_name))
> -
>        name = CFStringCreateCopy(kCFAllocatorDefault,ref_name);
> -
>    else
> -
>    {
> -
>        ref_man = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDManufacturerKey));

I think it's simpler to use kIOHIDVendorIDKey with get_device_property_long().  Judging by the HID Calibrator code, kIOHIDManufacturerKey is not reliably available, but kIOHIDVendorIDKey should be.  It's also pretty fundamental to USB/HID.
> -
>        prodID = get_device_property_long(device, CFSTR(kIOHIDProductIDKey));
> -
> -
>        if(ref_man && CFStringGetTypeID() == CFGetTypeID(ref_man))
> -
>            name = CFStringCreateWithFormat(kCFAllocatorDefault, NULL, CFSTR("%@ %lu"), ref_man, prodID);

Better to format vendor and product IDs using "0x%04lx" since that's how they are usually represented (e.g. within HID Calibrator windows and System Profiler).  (Yes, I realize that Copy_DeviceName() which I cited previously formats them as decimal.)

Thanks for persevering in the face of my picky-ness. :)

-Ken

[v3 1/1] winejoystick.drv: Sort virtual joysticks by name on the Mac

[DavidL]

New versions sent!

Cheers,
David

On Thu, Jul 21, 2016 at 10:41 PM, Ken Thomases  wrote:
> Hi,
>
> On Jul 20, 2016, at 2:15 AM, David Lawrie  wrote:
> > +static CFStringRef copy_device_name(IOHIDDeviceRef device)
> > +{
> > - CFTypeRef ref_name, ref_man;
> > - CFStringRef name;
> > - long prodID;
>
> I prefer variables to be declared at the narrowest scope in which they're
> used.
> > -
> > - if (device)
> > - {
> > -
> >    assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device));
> > -
> > -
> >    ref_name = IOHIDDeviceGetProperty(device,
>
> CFSTR(kIOHIDProductKey));
> > -
> > -
> >    if (ref_name && CFStringGetTypeID() == CFGetTypeID(ref_name))
> > -
> >        name = CFStringCreateCopy(kCFAllocatorDefault,ref_name);
> > -
> >    else
> > -
> >    {
> > -
> >        ref_man = IOHIDDeviceGetProperty(device,
>
> CFSTR(kIOHIDManufacturerKey));
>
> I think it's simpler to use kIOHIDVendorIDKey with
> get_device_property_long().  Judging by the HID Calibrator code,
> kIOHIDManufacturerKey is not reliably available, but kIOHIDVendorIDKey
> should be.  It's also pretty fundamental to USB/HID.
> > -
> >        prodID = get_device_property_long(device,
>
> CFSTR(kIOHIDProductIDKey));
> > -
> > -
> >        if(ref_man && CFStringGetTypeID() == CFGetTypeID(ref_man))
> > -
> >            name = CFStringCreateWithFormat(kCFAllocatorDefault,
>
> NULL, CFSTR("%@ %lu"), ref_man, prodID);
>
> Better to format vendor and product IDs using "0x%04lx" since that's how
> they are usually represented (e.g. within HID Calibrator windows and System
> Profiler).  (Yes, I realize that Copy_DeviceName() which I cited previously
> formats them as decimal.)
>
> Thanks for persevering in the face of my picky-ness. :)

:)
> -Ken