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

[DavidL]

On Mon, Jul 18, 2016 at 10:59 PM, Ken Thomases  wrote:
> On Jul 18, 2016, at 1:57 AM, David Lawrie  wrote:
> > +static CFStringRef copy_device_name(IOHIDDeviceRef device)
> > +{
> > - CFTypeRef ref;
> > - CFStringRef name = NULL;
> > -
> > - if(device)
>
> Maybe didn't notice this in the previous version, but put a space between
> "if" and the parenthesis.

will do
> > - {
> > -
> >    assert(IOHIDDeviceGetTypeID() == CFGetTypeID(device));
> > -
> > -
> >    ref = IOHIDDeviceGetProperty(device, CFSTR(kIOHIDProductKey));
> > -
> > -
> >    if (ref && CFStringGetTypeID() == CFGetTypeID(ref))
> > -
> >        name = CFStringCreateCopy(kCFAllocatorDefault,ref);
>
> Please use a space after the comma in argument lists in function calls.

will do
> > -
> >    else if (ref)
> > -
> >        name = CFCopyDescription(ref);
> > -
> >    else
> > -
> >        name =
>
> CFStringCreateCopy(kCFAllocatorDefault,CFSTR("(null)"));
>
> You don't want to fall back to a string based on the vendor and product ID?

I thought about it, but then 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. This label "(null)" is how debugstr handles
that case. The alternative of course is changing both functions to try to
get the venture/product ID - though I think the concern is if it doesn't
have aProductKey it may not have a proper productId or vendorID either ...
also there is a function get_os_x_device_name (in dinput only) which
doesn't handle this case at all I don't think. What's your recommendation?
> Also, the same spacing issue as above.

will do
> > - }
> > - else
> > -
> >    ERR("Invalid Device requested %p\n",device);
>
> At this point, device is known to be NULL, so the log message can say that
> rather than the vague "invalid", and doesn't need to include its value.  Or
> did you mean to be checking something else in the "if"?

Ah right, good point! :)
> > -
> > - return name;
> >
> > +}
> > -
> >
> > static long get_device_location_ID(IOHIDDeviceRef device)
> > {
> >     return get_device_property_long(device, CFSTR(kIOHIDLocationIDKey));
> > @@ -203,7 +227,18 @@ 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 = copy_device_name(device1), name2 =
>
> copy_device_name(device2);
> > - CFComparisonResult result = CFStringCompare(name1, name2,
>
> (kCFCompareForcedOrdering | kCFCompareNumerically));
> > - if(name1)
> > -
> >    CFRelease(name1);
> > - if(name2)
> > -
> >    CFRelease(name2);
>
> If name1 or name2 might be NULL as these tests imply, then what does the
> preceding CFStringCompare() call do?  Probably crashes.  So, either
> assume/assure that copy_device_name() doesn't return NULL in normal
> operation or handle the possibility that it might more thoroughly.

Actually I think it doesn't crash, though I should test it to be sure.
However, maybe for completeness/robustness I should include if statements
dealing with all the possible name-NULL combinations and return - something
like:

if ((!name1 && !name2) || !name1)
   return lessThan;
else if (!name2)
   return greaterThan;

Then it wouldn't matter what CFStringCompare's behavior was if it crashes
... or I could set name equal to an empty string in copy_device_name which
would have the same effect as the above few lines.
> -Ken

--David