Re: [v1 1/1] dinput: Sort virtual joysticks by name on the Mac

[Ken Thomases]

On Jul 16, 2016, at 3:15 AM, David Lawrie <david.dljunk(a)gmail.com> wrote:
> Tested on OS X 10.10.5.
>
> Signed-off-by: David Lawrie <david.dljunk(a)gmail.com>
> ---
> dlls/dinput/joystick_osx.c | 35 ++++++++++++++++++++++++++++++++---
> 1 file changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/dlls/dinput/joystick_osx.c b/dlls/dinput/joystick_osx.c
> index 637692b..22bdc4f 100644
> --- a/dlls/dinput/joystick_osx.c
> +++ b/dlls/dinput/joystick_osx.c
> @@ -193,6 +193,26 @@ static long get_device_property_long(IOHIDDeviceRef device, CFStringRef key)
>     return result;
> }
>
> +static CFStringRef get_device_name(IOHIDDeviceRef device)
> +{
> +    CFTypeRef ref;
> +    CFStringRef name = CFSTR("Default Name");

You can probably do better for a fallback.  For example, you can format the vendor ID and product ID into a string.  For an extreme take on this, see the Copy_DeviceName() function in Apple's HID Calibrator sample code:
https://developer.apple.com/library/mac/samplecode/HID_Calibrator/Listings/H...
> +
> +    if(device)
> +    {
> +        assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device));
> +
> +        ref = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey));
> +
> +        if (ref && CFStringGetTypeID() == CFGetTypeID(ref))
> +            name = ref;
> +        else if (ref)
> +            name = CFCopyDescription(ref);

This introduces a memory-management problem, a.k.a. a leak.  This copy is never released.

If you follow my suggestion above for formatting a string as a fallback if the product name isn't available, you'd also have to release that.

So, you would change this function's name to copy_device_name() and make sure all code paths return an owned reference to the caller, which the caller has to release.  Basically, in the case where IOHIDDeviceGetProperty() gives you a string, you'd have to copy it to match the other code paths.  Of course, you'd have to make sure the callers do, in fact, release the references.
> +    }
> +
> +    return name;
> +}
> +
> static long get_device_location_ID(IOHIDDeviceRef device)
> {
>     return get_device_property_long(device, CFSTR(kIOHIDLocationIDKey));
> @@ -203,7 +223,15 @@ static void copy_set_to_array(const void *value, void *context)
>     CFArrayAppendValue(context, value);
> }
>
> -static CFComparisonResult device_location_comparator(const void *val1, const void *val2, void *context)
> +static CFComparisonResult device_name_comparator(IOHIDDeviceRef device1, IOHIDDeviceRef device2)
> +{
> +    CFStringRef name1 = get_device_name(device1), name2 = get_device_name(device2);
> +    CFStringCompareFlags compareOptions = kCFCompareForcedOrdering | kCFCompareNumerically;

Any reason you've added this separate variable instead of just inlining the options in the call?
> +
> +    return CFStringCompare(name1, name2, compareOptions);
> +}

-Ken

[v1 1/1] dinput: Sort virtual joysticks by name on the Mac

[Ken Thomases]

On Jul 17, 2016, at 4:08 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
> On Jul 16, 2016, at 3:15 AM, David Lawrie <david.dljunk(a)gmail.com> wrote:
> > +static CFStringRef get_device_name(IOHIDDeviceRef device)
> > +{
> > +    CFTypeRef ref;
> > +    CFStringRef name = CFSTR("Default Name");
>
> You can probably do better for a fallback.  For example, you can format the vendor ID and product ID into a string.  For an extreme take on this, see the Copy_DeviceName() function in Apple's HID Calibrator sample code:
> https://developer.apple.com/library/mac/samplecode/HID_Calibrator/Listings/H...

Oh, and for the case where device is NULL, it's fine to return NULL.  It should never happen and, if it does, we want a crash to figure out why and fix things in a better manner, rather than papering over it with a bogus value.

-Ken

[v1 1/1] dinput: Sort virtual joysticks by name on the Mac

[DavidL]

Right-o! I was wondering what the CFRelease was doing in the debugstr_cf
function. :)

Cheers,
David

On Sun, Jul 17, 2016 at 2:11 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
> On Jul 17, 2016, at 4:08 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
> > On Jul 16, 2016, at 3:15 AM, David Lawrie <david.dljunk(a)gmail.com>
>
> wrote:
> > > +static CFStringRef get_device_name(IOHIDDeviceRef device)
> > > +{
> > > +    CFTypeRef ref;
> > > +    CFStringRef name = CFSTR("Default Name");
> >
> > You can probably do better for a fallback.  For example, you can format
> > the vendor ID and product ID into a string.  For an extreme take on this,
> > see the Copy_DeviceName() function in Apple's HID Calibrator sample code:
>
> https://developer.apple.com/library/mac/samplecode/HID_Calibrator/Listings/H...
>
> Oh, and for the case where device is NULL, it's fine to return NULL.  It
> should never happen and, if it does, we want a crash to figure out why and
> fix things in a better manner, rather than papering over it with a bogus
> value.
>
> -Ken

[v1 1/1] dinput: Sort virtual joysticks by name on the Mac

[DavidL]

Hi Ken,

So I wanted to try to hew as close to the behavior of debugstr_cf as
possible since that is the output that the user sees already in trace logs.
Here's how I decided handle each case:

static CFStringRef copy_device_name(IOHIDDeviceRef device)
> {
>     CFTypeRef ref;
>     CFStringRef name = NULL;
>
> if(device)
>     {
>         assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device));
>         ref = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey));
>         if (ref && CFStringGetTypeID() == CFGetTypeID(ref))
>             name = CFStringCreateCopy(kCFAllocatorDefault,ref);
>         else if (ref)
>             name = CFCopyDescription(ref);
>         else
>             name = CFStringCreateCopy(kCFAllocatorDefault,CFSTR("(null)"));
>     }
>     else
>         ERR("Invalid Device requested %p\n",device);
>
> return name;
> }

> static CFComparisonResult device_name_comparator(IOHIDDeviceRef device1,
> IOHIDDeviceRef device2)
> {
>     CFStringRef name1 = copy_device_name(device1), name2 =
> copy_device_name(device2);
>     CFComparisonResult result = CFStringCompare(name1, name2,
> (kCFCompareForcedOrdering | kCFCompareNumerically));
>     if(name1)
>         CFRelease(name1);
>     if(name2)
>         CFRelease(name2);
>     return  result;
> }

What do you think?

Cheers,
David

On Sun, Jul 17, 2016 at 8:20 PM, DavidL <david.dljunk(a)gmail.com> wrote:
> Right-o! I was wondering what the CFRelease was doing in the debugstr_cf
> function. :)
>
> Cheers,
> David
>
> On Sun, Jul 17, 2016 at 2:11 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
> > On Jul 17, 2016, at 4:08 PM, Ken Thomases <ken(a)codeweavers.com> wrote:
> > > On Jul 16, 2016, at 3:15 AM, David Lawrie <david.dljunk(a)gmail.com>
> >
> > wrote:
> > > > +static CFStringRef get_device_name(IOHIDDeviceRef device)
> > > > +{
> > > > +    CFTypeRef ref;
> > > > +    CFStringRef name = CFSTR("Default Name");
> > >
> > > You can probably do better for a fallback.  For example, you can format
> > > the vendor ID and product ID into a string.  For an extreme take on this,
> > > see the Copy_DeviceName() function in Apple's HID Calibrator sample code:
> >
> > https://developer.apple.com/library/mac/samplecode/HID_Calibrator/Listings/H...
> >
> > Oh, and for the case where device is NULL, it's fine to return NULL.  It
> > should never happen and, if it does, we want a crash to figure out why and
> > fix things in a better manner, rather than papering over it with a bogus
> > value.
> >
> > -Ken