[Spice-devel] [spice-gtk PATCHv2] usb: Add info message when USB dialog is empty

[Hans de Goede]

Hi,

On 09/28/2012 12:56 PM, Christophe Fergeau wrote:
>  From rh bug #804187:
> « The redirection dialog can feel a bit strange when there is no device to
> redirect.
>
> It could be useful to provide a help message indicating that there is no
> device to redirect yet, and that the user can insert a USB device to
> redirect, and some related guidance. »
>
> This commit adds a "No USB devices detected" infobar in the USB
> dialog below the 'Select USB devices to redirect" label.
> Content could probably be improved, but this is a step in the right
> direction ;)
>
> This can be tested with
> diff --git a/gtk/usb-device-widget.c b/gtk/usb-device-widget.c
> index b1bf090..660ea03 100644
> --- a/gtk/usb-device-widget.c
> +++ b/gtk/usb-device-widget.c
> @@ -220,6 +220,11 @@ static GObject *spice_usb_device_widget_constructor(
>                        G_CALLBACK(device_error_cb), self);
>
>       devices = spice_usb_device_manager_get_devices(priv->manager);
> +    if (devices) {
> +        g_ptr_array_unref(devices);
> +        devices = NULL;
> +    }
> +
>       if (!devices)
>           goto end;

This of course no longer is correct ...

> ---
>
> Changes in v2:
> - the 'no usb device' logic has been moved to spice_usb_device_widget_update_status
>    as suggested by Hans
> - removed debugging code
> - instead of adding a device_count member to SpiceUsbDeviceWidget, I've
>    done this through a local variable

Sorry to be a PITA, but I'm not really happy with making it a local variable, that
makes the code more complicated (IMHO), for saving just 4 bytes of ram. Likely the
extra code-size of the new code will amply compensate that ...

Moreover if you want to get rid of the passing of stuff between check_can_redirect
and spice_usb_device_widget_update_status through global state, you should start
with a preparation patch removing the use of err_msg in the global state, as that
is only used between the 2 of them too. But I don't think this is worth it IMHO.

Anyways, please do one of:
1) Split the patch in 2 patches, with the first one removing the use of global
state between check_can_redirect and spice_usb_device_widget_update_status
(err_msg), and the second one adding the actual message on no devices, or
2) Just make count part of the global state

Regards,

Hans


>
>
>
>   gtk/usb-device-widget.c | 41 +++++++++++++++++++++++++++++++----------
>   1 file changed, 31 insertions(+), 10 deletions(-)
>
> diff --git a/gtk/usb-device-widget.c b/gtk/usb-device-widget.c
> index 3ed81e4..1e0df33 100644
> --- a/gtk/usb-device-widget.c
> +++ b/gtk/usb-device-widget.c
> @@ -47,6 +47,7 @@ static void device_removed_cb(SpiceUsbDeviceManager *manager,
>       SpiceUsbDevice *device, gpointer user_data);
>   static void device_error_cb(SpiceUsbDeviceManager *manager,
>       SpiceUsbDevice *device, GError *err, gpointer user_data);
> +static gboolean spice_usb_device_widget_update_status(gpointer user_data);
>
>   /* ------------------------------------------------------------------ */
>   /* gobject glue                                                       */
> @@ -228,6 +229,8 @@ static GObject *spice_usb_device_widget_constructor(
>       g_ptr_array_unref(devices);
>
>   end:
> +    spice_usb_device_widget_update_status(self);
> +
>       return obj;
>   }
>
> @@ -351,21 +354,17 @@ static SpiceUsbDevice *get_usb_device(GtkWidget *widget)
>       return g_object_get_data(G_OBJECT(widget), "usb-device");
>   }
>
> -static void check_can_redirect(GtkWidget *widget, gpointer user_data)
> +static gboolean
> +check_can_redirect(SpiceUsbDeviceWidget *self, SpiceUsbDevice *device)
>   {
> -    SpiceUsbDeviceWidget *self = SPICE_USB_DEVICE_WIDGET(user_data);
>       SpiceUsbDeviceWidgetPrivate *priv = self->priv;
> -    SpiceUsbDevice *device;
>       gboolean can_redirect;
>       GError *err = NULL;
>
> -    device = get_usb_device(widget);
> -    if (!device)
> -        return; /* Non device widget, ie the info_bar */
> +    g_return_val_if_fail(device != NULL, FALSE);
>
>       can_redirect = spice_usb_device_manager_can_redirect_device(priv->manager,
>                                                                   device, &err);
> -    gtk_widget_set_sensitive(widget, can_redirect);
>
>       /* If we can not redirect this device, append the error message to
>          err_msg, but only if it is *not* already there! */
> @@ -384,14 +383,32 @@ static void check_can_redirect(GtkWidget *widget, gpointer user_data)
>       }
>
>       g_clear_error(&err);
> +
> +    return can_redirect;
>   }
>
>   static gboolean spice_usb_device_widget_update_status(gpointer user_data)
>   {
>       SpiceUsbDeviceWidget *self = SPICE_USB_DEVICE_WIDGET(user_data);
>       SpiceUsbDeviceWidgetPrivate *priv = self->priv;
> -
> -    gtk_container_foreach(GTK_CONTAINER(self), check_can_redirect, self);
> +    GList *children;
> +    GList *it;
> +    gsize device_count;
> +
> +    device_count = 0;
> +    children = gtk_container_get_children(GTK_CONTAINER(self));
> +    for (it = children; it != NULL; it = it->next) {
> +        SpiceUsbDevice *device;
> +        gboolean can_redirect;
> +
> +        device = get_usb_device(GTK_WIDGET(it->data));
> +        if (!device)
> +            continue; /* Non device widget, ie the info_bar */
> +        can_redirect = check_can_redirect(self, device);
> +        gtk_widget_set_sensitive(GTK_WIDGET(it->data), can_redirect);
> +        device_count++;
> +    }
> +    g_list_free(children);
>
>       if (priv->err_msg) {
>           spice_usb_device_widget_show_info_bar(self, priv->err_msg,
> @@ -402,6 +419,11 @@ static gboolean spice_usb_device_widget_update_status(gpointer user_data)
>       } else {
>           spice_usb_device_widget_hide_info_bar(self);
>       }
> +
> +    if (device_count == 0)
> +        spice_usb_device_widget_show_info_bar(self, _("No USB devices detected"),
> +                                              GTK_MESSAGE_INFO,
> +                                              GTK_STOCK_DIALOG_INFO);
>       return FALSE;
>   }
>
> @@ -515,7 +537,6 @@ static void device_removed_cb(SpiceUsbDeviceManager *manager,
>
>       gtk_container_foreach(GTK_CONTAINER(self),
>                             destroy_widget_by_usb_device, device);
> -
>       spice_usb_device_widget_update_status(self);
>   }
>
>