[Spice-devel] [PATCH spice-gtk 2/4] usb-device-manager: Add a spice_usb_device_get_description() method

[Marc-André Lureau]

some minor nitpicks:

On Wed, Aug 31, 2011 at 3:21 PM, Hans de Goede <hdegoede at redhat.com> wrote:
> This is just a place holder for now. A better implementation requires
> gusb changes, and I hope there will be an official gusb release by the
> time I get around to fixing this up.
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>  gtk/map-file             |    1 +
>  gtk/usb-device-manager.c |   31 ++++++++++++++++++++++++++-----
>  gtk/usb-device-manager.h |    2 ++
>  3 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/gtk/map-file b/gtk/map-file
> index d9765c0..b383edf 100644
> --- a/gtk/map-file
> +++ b/gtk/map-file
> @@ -77,6 +77,7 @@ spice_smartcard_reader_get_type;
>  spice_smartcard_reader_is_software;
>  spice_usbredir_channel_get_type;
>  spice_usb_device_get_type;
> +spice_usb_device_get_description;
>  spice_usb_device_manager_get_type;
>  spice_usb_device_manager_get;
>  spice_usb_device_manager_register_channel;
> diff --git a/gtk/usb-device-manager.c b/gtk/usb-device-manager.c
> index 7557ed8..4e13ae1 100644
> --- a/gtk/usb-device-manager.c
> +++ b/gtk/usb-device-manager.c
> @@ -307,22 +307,22 @@ static gboolean spice_usb_device_manager_source_callback(gpointer user_data)
>  }
>
>  static void spice_usb_device_manager_dev_added(GUsbDeviceList *devlist,
> -                                               GUsbDevice     *device,
> +                                               GUsbDevice     *_device,
>                                                GUdevDevice    *udev,
>                                                gpointer        user_data)
>  {
>     SpiceUsbDeviceManager *manager = user_data;
>     SpiceUsbDeviceManagerPrivate *priv = manager->priv;
> +    SpiceUsbDevice *device = (SpiceUsbDevice *)_device;
>
>     g_ptr_array_add(priv->devices, g_object_ref(device));
>
>     if (priv->auto_connect) {
>         GError *err = NULL;
> -        spice_usb_device_manager_connect_device(manager,
> -                                                (SpiceUsbDevice *)device,
> -                                                &err);
> +        spice_usb_device_manager_connect_device(manager, device, &err);
>         if (err) {
> -            g_warning("Could not auto-redirect USB device: %s", err->message);
> +            g_warning("Could not auto-redirect %s: %s",
> +                      spice_usb_device_get_description(device), err->message);
>             g_error_free(err);
>         }
>     }
> @@ -574,3 +574,24 @@ void spice_usb_device_manager_disconnect_device(SpiceUsbDeviceManager *self,
>         spice_usbredir_channel_disconnect(channel);
>  #endif
>  }
> +
> +/**
> + * spice_usb_device_get_description:
> + * @device: #SpiceUsbDevice to get the description of
> + *
> + * Get a string describing the device which is suitable as a description of
> + * the device for the end user. The returned string should be freed with
> + * g_free() when no longer needed.
> + *
> + * Returns: a newly-allocated string holding the description
> + */
> +gchar *spice_usb_device_get_description(SpiceUsbDevice *device)
> +{
> +    /* FIXME, extend gusb to get vid:pid + usb descriptor strings, use those */
> +    uint8_t bus, address;
> +

- please check arguments of public methods with g_return_val_if_fail(...)

> +    bus = g_usb_device_get_bus((GUsbDevice *)device);
> +    address = g_usb_device_get_address((GUsbDevice *)device);
> +
> +    return g_strdup_printf("USB device at %d-%d", (int)bus, (int)address);
> +}

Wouldn't ("USB device at %u-%u", bus, address) be ok?

> diff --git a/gtk/usb-device-manager.h b/gtk/usb-device-manager.h
> index 855accb..d72e5dd 100644
> --- a/gtk/usb-device-manager.h
> +++ b/gtk/usb-device-manager.h
> @@ -69,6 +69,8 @@ struct _SpiceUsbDeviceManagerClass
>  GType spice_usb_device_get_type(void);
>  GType spice_usb_device_manager_get_type(void);
>
> +gchar *spice_usb_device_get_description(SpiceUsbDevice *device);
> +
>  SpiceUsbDeviceManager *spice_usb_device_manager_get(GMainContext *main_context,
>                                                     GError **err);
>


don't forget to add newly added functions in the right gtk-doc section
doc/reference/spice-gtk-sections.txt

(this is regularly forgotten, and there are some missing API already..)

cheers


-- 
Marc-André Lureau