On 06/23/2016 05:21 PM, Peter Hutterer wrote:
> On Wed, Jun 22, 2016 at 09:20:36AM -0700, Jason Gerecke wrote:
>> On 06/21/2016 06:47 PM, Peter Hutterer wrote:
>>> On Tue, Jun 21, 2016 at 03:52:53PM -0700, Jason Gerecke wrote:
>>>> Providing the name of a tablet to show-svg-image can be difficult at
>>>> times, since the name may not be exactly what you expect or there may
>>>> be more than one device which shares the same name. To make its use a
>>>> little easier, allow it to take the device ID of the device to display
>>>> as a command line argument.
>>>>
>>>> Signed-off-by: Jason Gerecke <jason.gere...@wacom.com>
>>>> ---
>>>> tools/show-svg-image.c | 74
>>>> ++++++++++++++++++++++++++++++++++++++++++++------
>>>> 1 file changed, 66 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/tools/show-svg-image.c b/tools/show-svg-image.c
>>>> index 674d27e..152f71f 100644
>>>> --- a/tools/show-svg-image.c
>>>> +++ b/tools/show-svg-image.c
>>>> @@ -355,16 +355,20 @@ main (int argc, char **argv)
>>>> GdkWindow *gdk_win;
>>>> GdkColor black;
>>>> char *tabletname;
>>>> + char *tabletid;
>>>> const char *filename;
>>>> GOptionEntry
>>>> options[] = {
>>>> - {"tablet", 't', 0, G_OPTION_ARG_STRING, &tabletname,
>>>> "Name of the tablet to show", "<string>"},
>>>> + {"tablet", 't', 0, G_OPTION_ARG_STRING, &tabletname,
>>>> "Name of the tablet to show", "[string]"},
>>>> + {"id", 'i', 0, G_OPTION_ARG_STRING, &tabletid, "ID of
>>>> the tablet to show", "[string]"},
>>>> {NULL}
>>>> };
>>>>
>>>> handle = NULL;
>>>> error = NULL;
>>>> + device = NULL;
>>>> tabletname = NULL;
>>>> + tabletid = NULL;
>>>>
>>>> context = g_option_context_new ("- libwacom tablet viewer");
>>>> g_option_context_add_main_entries (context, options, NULL);
>>>> @@ -374,8 +378,12 @@ main (int argc, char **argv)
>>>> g_option_context_free (context);
>>>>
>>>> gtk_init (&argc, &argv);
>>>> - if (tabletname == NULL) {
>>>> - g_warning ("No tablet name provided, exiting");
>>>> + if (tabletname == NULL && tabletid == NULL) {
>>>> + g_warning ("Tablet name or ID must be provided, exiting");
>>>> + return 1;
>>>> + }
>>>> + if (tabletname != NULL && tabletid != NULL) {
>>>> + g_warning ("Tablet name or ID must be provided, exiting");
>>>> return 1;
>>>> }
>>>> db = libwacom_database_new_for_path(TOPSRCDIR"/data");
>>>> @@ -383,15 +391,65 @@ main (int argc, char **argv)
>>>> g_warning ("Failed to load libwacom database, exiting");
>>>> return 1;
>>>> }
>>>> - device = libwacom_new_from_name(db, tabletname, NULL);
>>>> - if (!device) {
>>>> - g_warning ("Device '%s' not found in libwacom database,
>>>> exiting", tabletname);
>>>> - return 1;
>>>> + if (tabletname != NULL) {
>>>> + device = libwacom_new_from_name (db, tabletname, NULL);
>>>> + if (!device) {
>>>> + g_warning ("Device '%s' not found in libwacom database,
>>>> exiting", tabletname);
>>>> + return 1;
>>>> + }
>>>> + }
>>>> + else if (tabletid != NULL) {
>>>> + WacomDevice **devices;
>>>> + WacomDevice **d;
>>>> + WacomBusType bustype;
>>>> + char **tabletid_token;
>>>> + int vid;
>>>> + int pid;
>>>> +
>>>> + tabletid_token = g_strsplit (tabletid, ":", -1);
>>>
>>> sscanf(tabletid, "%s:%x:%x") should work here too? seems easier than
>>> tokenising
>>>
>>
>> Changed locally.
>>
>>>> + if (g_strv_length (tabletid_token) != 3)
>>>> + g_warning ("Incorrectly formatted tablet id, exiting.");
>>>> +
>>>> + if (g_ascii_strcasecmp ("", tabletid_token[0]) == 0)
>>>> + bustype = WBUSTYPE_UNKNOWN;
>>>> + else if (g_ascii_strcasecmp ("usb", tabletid_token[0]) == 0)
>>>> + bustype = WBUSTYPE_USB;
>>>> + else if (g_ascii_strcasecmp ("serial", tabletid_token[0]) == 0)
>>>> + bustype = WBUSTYPE_SERIAL;
>>>> + else if (g_ascii_strcasecmp ("bluetooth", tabletid_token[0]) ==
>>>> 0)
>>>> + bustype = WBUSTYPE_BLUETOOTH;
>>>> + else if (g_ascii_strcasecmp ("i2c", tabletid_token[0]) == 0)
>>>> + bustype = WBUSTYPE_I2C;
>>>> + else {
>>>> + g_warning ("Unknown bus type, exiting.");
>>>> + return 1;
>>>> + }
>>>> +
>>>> + vid = (int)g_ascii_strtoll (tabletid_token[1], NULL, 16);
>>>> + pid = (int)g_ascii_strtoll (tabletid_token[2], NULL, 16);
>>>> +
>>>> + g_strfreev (tabletid_token);
>>>> +
>>>> + devices = libwacom_list_devices_from_database (db, NULL);
>>>> + d = devices;
>>>> + while (*d && !device) {
>>>> + if (libwacom_get_vendor_id (*d) == vid &&
>>>> + libwacom_get_product_id (*d) == pid &&
>>>> + (libwacom_get_bustype (*d) == bustype || bustype ==
>>>> WBUSTYPE_UNKNOWN))
>>>> + device = *d;
>>>> + d++;
>>>> + }
>>>> + free (devices);
>>>
>>> tbh, that looks like something we should have a libwacom_new_from_id() for.
>>> we have from_usbid which has the obvious drawback but either way that seems
>>> like something useful to have around.
>>>
>>
>> I was a bit hesitant to update the API since a thin wrapper around
>> 'libwacom_new' (which already takes vid/pid/bus arguments) seemed so
>> obvious that I suspected there may be a reason it didn't exist. I could
>> certainly either introduce one or corresponding
>> serialid/i2cid/bluetoothid functions.
>
> I *think* the reason it doesn't exist is because we had the new_by_usbid and
> then the bluetooth/i2c tablets came later and we never needed the matching
> call. I'd say rather than adding more bus-specific ones we should deprecate
> the usbid one and add one that takes the bus ID as argument.
>
> Cheers,
> Peter
>
I finished writing the patches, but...
<musing aloud>
It occurred to me that even if a device isn't actually connected over
USB, its still a USB ID that we're referring to. Serial and I2C devices
natively use the PNP/ACPI ID namespace but the database currently refers
to the USB ID that is extracted from the device (e.g. the i2c sensor in
the Lenovo ThinkPad Tablet 10 has an ACPI hardware ID of WCOM0008 but
its HID descriptor has the ID 056a:0114). Bluetooth also uses its own ID
namespace, with us again getting a USB ID from the descriptor.
Given that, is there any reason 'libwacom_new_from_usbid' should
necessarily be limited to devices on the USB bus?
I can still imagine that a caller may want to limit the device match to
a specific bus, but in a world of multiple ID namespaces,
'libwacom_new_from_id' might not be the best name choice in hindsight.
</musing aloud>
Jason
---
Now instead of four in the eights place /
you’ve got three, ‘Cause you added one /
(That is to say, eight) to the two, /
But you can’t take seven from three, /
So you look at the sixty-fours....
>> Let me know which you would prefer and I'll send some patches.
>>
>>> and the last nit: can we please split this block into a helper function.
>>> main() is already quite unwieldly
>>>
>>
>> Changed locally.
>>
>>> I've pushed the other patches, thanks.
>>>
>>> Cheers,
>>> Peter
>>>
>>
>> Thanks.
>>
>> Jason
>> ---
>> Now instead of four in the eights place /
>> you’ve got three, ‘Cause you added one /
>> (That is to say, eight) to the two, /
>> But you can’t take seven from three, /
>> So you look at the sixty-fours....
>>
>>>> +
>>>> + if (!device) {
>>>> + g_warning ("Device ID '%s' not found in libwacom
>>>> database, exiting", tabletid);
>>>> + return 1;
>>>> + }
>>>> }
>>>>
>>>> filename = libwacom_get_layout_filename(device);
>>>> if (filename == NULL) {
>>>> - g_warning ("Device '%s' has no layout available, exiting",
>>>> tabletname);
>>>> + g_warning ("Device '%s' has no layout available, exiting",
>>>> libwacom_get_name(device));
>>>> return 1;
>>>> }
>>>> handle = rsvg_handle_new_from_file (filename, &error);
>>>> --
>>>> 2.8.3
>>>>
------------------------------------------------------------------------------
Attend Shape: An AT&T Tech Expo July 15-16. Meet us at AT&T Park in San
Francisco, CA to explore cutting-edge tech and listen to tech luminaries
present their vision of the future. This family event has something for
everyone, including kids. Get more information and register today.
http://sdm.link/attshape
_______________________________________________
Linuxwacom-devel mailing list
Linuxwacom-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
