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

[Ken Thomases]

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.
> - {
> -
>    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.
> -
>    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?

Also, the same spacing issue as above.
> - }
> - 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"?
> -
> - 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.

-Ken

[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

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

[Ken Thomases]

On Jul 19, 2016, at 2:59 AM, DavidL  wrote:
> On Mon, Jul 18, 2016 at 10:59 PM, Ken Thomases <[e-mail] > wrote:
> On Jul 18, 2016, at 1:57 AM, David Lawrie <[e-mail] > wrote:
> > -
> >    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.

I don't think it's particularly important that they match to that degree.  First, users generally don't see the trace logs.  Second, it's good enough that they match when the product property is non-null.  There's no strong reason to make them match for the null case.  It's fine to have this function return something more useful than that, especially since it's currently used as a sort key and "(null)" is useless for that.
> 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

I have no objection if you want to make debugstr_device() also include the vendor/product ID as a fallback, but I don't think it's especially important.  The result from copy_device_name() is not currently logged, so there's no way for a reader of the logs to benefit if the two functions match in that way, nor lose anything if they don't match.
> ... 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?

I don't know how important it is for get_osx_device_name() to return a non-empty name.  It could be changed to call copy_device_name() to get the name and then extract that to the buffer.  Mind you, it's currently buggy in how it treats the CFString length vs. the buffer length.  The former is in number of UTF-16 code units, the latter in terms of ASCII characters, so they're not directly comparable.  (Also, nothing ever checks the function's return value, so returning the supposed required length doesn't achieve anything.)
> > @@ -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.

Testing isn't appropriate.  It could work in some versions of the frameworks and not in others.  The authority on the design contract is the documentation (including header comments), with the understanding that if it doesn't explicitly say that NULL is acceptable, then it's not.
> 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.

Returning the empty string is probably simplest.  If you go the pointer testing route, then surely (!name1 && !name2) implies equal, not less-than.

-Ken