[Spice-devel] [PATCH 2/2] usb-device-widget: Add counter of free channels
Marc-André Lureau
mlureau at redhat.com
Wed Jan 20 08:28:06 PST 2016
Hi
----- Original Message -----
> On Wed, 2016-01-20 at 10:11 +0100, Fabiano Fidêncio wrote:
> > On Wed, Jan 20, 2016 at 9:59 AM, Victor Toso <lists at victortoso.com> wrote:
> > > Hi,
> > >
> > > On Tue, Jan 19, 2016 at 10:36:38PM +0100, Fabiano Fidêncio wrote:
> > > > As the message showed when the last usbredir channel is taken can be a
> > > > bit confusing, let's add a counter of free channels to the widget's
> > > > label.
> > > > In order to add the counter, two new helper functions got introduced
> > > > spice_usb_device_manager_get_{free,total}_channels().
> > > >
> > > > Related: rhbz#1298772
> > > > ---
> > > > src/map-file | 2 ++
> > > > src/spice-glib-sym-file | 2 ++
> > > > src/usb-device-manager.c | 52
> > > > ++++++++++++++++++++++++++++++++++++++++++++++++
> > > > src/usb-device-manager.h | 6 ++++++
> > > > src/usb-device-widget.c | 34 +++++++++++++++++++++++--------
> > > > 5 files changed, 88 insertions(+), 8 deletions(-)
> > > >
> > > > diff --git a/src/map-file b/src/map-file
> > > > index 62cdb51..babe0be 100644
> > > > --- a/src/map-file
> > > > +++ b/src/map-file
> > > > @@ -130,6 +130,8 @@ spice_usb_device_manager_disconnect_device;
> > > > spice_usb_device_manager_get;
> > > > spice_usb_device_manager_get_devices;
> > > > spice_usb_device_manager_get_devices_with_filter;
> > > > +spice_usb_device_manager_get_total_channels;
> > > > +spice_usb_device_manager_get_free_channels;
>
> I think I would prefer to use something like get_n_foo similar to other glib
> -type function name. So, perhaps:
>
> _get_n_channels()
> _get_n_free_channels() ?
What about making these object properties instead?
>
> > > > spice_usb_device_manager_get_type;
> > > > spice_usb_device_manager_is_device_connected;
> > > > spice_usb_device_widget_get_type;
> > > > diff --git a/src/spice-glib-sym-file b/src/spice-glib-sym-file
> > > > index ae365cd..784f50e 100644
> > > > --- a/src/spice-glib-sym-file
> > > > +++ b/src/spice-glib-sym-file
> > > > @@ -107,6 +107,8 @@ spice_usb_device_manager_disconnect_device
> > > > spice_usb_device_manager_get
> > > > spice_usb_device_manager_get_devices
> > > > spice_usb_device_manager_get_devices_with_filter
> > > > +spice_usb_device_manager_get_total_channels
> > > > +spice_usb_device_manager_get_free_channels
> > > > spice_usb_device_manager_get_type
> > > > spice_usb_device_manager_is_device_connected
> > > > spice_usbredir_channel_get_type
> > > > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > > > index a22d926..21d3a25 100644
> > > > --- a/src/usb-device-manager.c
> > > > +++ b/src/usb-device-manager.c
> > > > @@ -1689,6 +1689,58 @@
> > > > spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager
> > > > *self,
> > > > }
> > > >
> > > > /**
> > > > + * spice_usb_device_manager_get_total_channels:
> > > > + * @self: the #SpiceUsbDeviceManager manager
> > > > + *
> > > > + * Get the total number of redirecting channels.
> > > > + *
> > > > + * Returns: The total number of redirecting channels.
> > > > + */
> > > > +int
> > > > +spice_usb_device_manager_get_total_channels(SpiceUsbDeviceManager
> > > > *self)
> > > > +{
> > > > +#ifdef USE_USBREDIR
> > > > + SpiceUsbDeviceManagerPrivate *priv = self->priv;
> > >
> > > Here you are accessing priv before the check bellow.
> > >
> > > > +
> > > > + g_return_val_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self), FALSE);
> > > > +
> > > > + return priv->channels->len;
> > > > +#else
> > > > + return 0;
> > > > +#endif
> > > > +}
> > > > +
> > > > +/**
> > > > + * spice_usb_device_manager_get_total_channels:
> > > > + * @self: the #SpiceUsbDeviceManager manager
> > > > + *
> > > > + * Get the number of redirecting channels that are available.
> > > > + *
> > > > + * Returns: The number of redirecting channels that are available.
> > > > + */
> > > > +int
> > > > +spice_usb_device_manager_get_free_channels(SpiceUsbDeviceManager
> > > > *self)
> > > > +{
> > > > +#ifdef USE_USBREDIR
> > > > + SpiceUsbDeviceManagerPrivate *priv = self->priv;
> > >
> > > Ditto
> >
> > I will incorporate these suggestions and send a v2 (as soon as we have
> > more comments).
> >
> > >
> > > Looks good otherwise
> >
> > I still don't like that much the string being used (%d of %d channels
> > free) doesn't sound clear enough for me, but I don't have a better
> > idea either :-\
> >
> > >
> > > > + int i, free_channels = 0;
> > > > +
> > > > + g_return_val_if_fail(SPICE_IS_USB_DEVICE_MANAGER(self), FALSE);
> > > > +
> > > > + for (i = 0; i < priv->channels->len; i++) {
> > > > + SpiceUsbredirChannel *channel =
> > > > g_ptr_array_index(priv->channels,
> > > > i);
> > > > +
> > > > + if (!spice_usbredir_channel_get_device(channel))
> > > > + free_channels++;
> > > > + }
> > > > +
> > > > + return free_channels;
> > > > +#else
> > > > + return 0;
> > > > +#endif
> > > > +}
> > > > +
> > > > +/**
> > > > * spice_usb_device_get_description:
> > > > * @device: #SpiceUsbDevice to get the description of
> > > > * @format: (allow-none): an optional printf() format string with
> > > > diff --git a/src/usb-device-manager.h b/src/usb-device-manager.h
> > > > index e05ebae..90fa413 100644
> > > > --- a/src/usb-device-manager.h
> > > > +++ b/src/usb-device-manager.h
> > > > @@ -127,6 +127,12 @@
> > > > spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager
> > > > *self,
> > > > SpiceUsbDevice
> > > > *device,
> > > > GError
> > > > **err);
> > > >
> > > > +int
> > > > +spice_usb_device_manager_get_total_channels(SpiceUsbDeviceManager
> > > > *self);
> > > > +
> > > > +int
> > > > +spice_usb_device_manager_get_free_channels(SpiceUsbDeviceManager
> > > > *self);
> > > > +
> > > > G_END_DECLS
> > > >
> > > > #endif /* __SPICE_USB_DEVICE_MANAGER_H__ */
> > > > diff --git a/src/usb-device-widget.c b/src/usb-device-widget.c
> > > > index 830bdce..fffee44 100644
> > > > --- a/src/usb-device-widget.c
> > > > +++ b/src/usb-device-widget.c
> > > > @@ -72,6 +72,7 @@ struct _SpiceUsbDeviceWidgetPrivate {
> > > > gchar *device_format_string;
> > > > SpiceUsbDeviceManager *manager;
> > > > GtkWidget *info_bar;
> > > > + GtkWidget *label;
> > > > gchar *err_msg;
> > > > gsize device_count;
> > > > gboolean is_info_message;
> > > > @@ -182,8 +183,7 @@ static GObject
> > > > *spice_usb_device_widget_constructor(
> > > > SpiceUsbDeviceWidgetPrivate *priv;
> > > > GPtrArray *devices = NULL;
> > > > GError *err = NULL;
> > > > - GtkWidget *label;
> > > > - gchar *str;
> > > > + gchar *str, *markup_str;
> > > > int i;
> > > >
> > > > {
> > > > @@ -198,12 +198,12 @@ static GObject
> > > > *spice_usb_device_widget_constructor(
> > > > if (!priv->session)
> > > > g_error("SpiceUsbDeviceWidget constructed without a session");
> > > >
> > > > - label = gtk_label_new(NULL);
> > > > - str = g_strdup_printf("<b>%s</b>", _("Select USB devices to
> > > > redirect"));
> > > > - gtk_label_set_markup(GTK_LABEL (label), str);
> > > > - g_free(str);
> > > > - gtk_misc_set_alignment(GTK_MISC(label), 0.0, 0.5);
> > > > - gtk_box_pack_start(GTK_BOX(self), label, FALSE, FALSE, 0);
> > > > + priv->label = gtk_label_new(NULL);
> > > > + markup_str = g_strdup_printf("<b>%s</b>", _("Select USB devices to
> > > > redirect"));
> > > > + gtk_label_set_markup(GTK_LABEL (priv->label), markup_str);
>
> I don't understand why you're setting the label markup here and then
> immediately
> overwriting it below.
>
> > > > + g_free(markup_str);
> > > > + gtk_misc_set_alignment(GTK_MISC(priv->label), 0.0, 0.5);
> > > > + gtk_box_pack_start(GTK_BOX(self), priv->label, FALSE, FALSE, 0);
> > > >
> > > > priv->manager = spice_usb_device_manager_get(priv->session, &err);
> > > > if (err) {
> > > > @@ -214,6 +214,14 @@ static GObject
> > > > *spice_usb_device_widget_constructor(
> > > > return obj;
> > > > }
> > > >
> > > > + str = g_strdup_printf(_("Select USB devices to redirect (%d of %d
> > > > channels free)"),
>
> Perhaps you can omit the total number and just say "(%d channels free)"? or
> "(%d
> free channels remaining)"? I don't think your original suggestion is so bad
> either.
>
> However, when all available channels are used, there is some information
> duplication, because then you end up with something like:
>
> Select USB devices to redirect (0 of 1 channels free)
> [ [info] ALL available USB channels are in use. ]
> [ [icon] No additional devices can be redirected ]
>
> That's not a big deal, but it does feel a bit redundant to me.
>
>
> > > > +
> > > > spice_usb_device_manager_get_free_channels(priv
> > > > ->manager),
> > > > +
> > > > spice_usb_device_manager_get_total_channels(priv->manager));
> > > > + markup_str = g_strdup_printf("<b>%s</b>", str);
> > > > + gtk_label_set_markup(GTK_LABEL (priv->label), markup_str);
> > > > + g_free(markup_str);
> > > > + g_free(str);
> > > > +
> > > > g_signal_connect(priv->manager, "device-added",
> > > > G_CALLBACK(device_added_cb), self);
> > > > g_signal_connect(priv->manager, "device-removed",
> > > > @@ -469,6 +477,7 @@ static void checkbox_clicked_cb(GtkWidget *check,
> > > > gpointer user_data)
> > > > SpiceUsbDeviceWidget *self = SPICE_USB_DEVICE_WIDGET(user_data);
> > > > SpiceUsbDeviceWidgetPrivate *priv = self->priv;
> > > > SpiceUsbDevice *device;
> > > > + gchar *str, *markup_str;
> > > >
> > > > device = g_object_get_data(G_OBJECT(check), "usb-device");
> > > >
> > > > @@ -485,6 +494,15 @@ static void checkbox_clicked_cb(GtkWidget *check,
> > > > gpointer user_data)
> > > > spice_usb_device_manager_disconnect_device(priv->manager,
> > > > device);
> > > > }
> > > > +
> > > > + str = g_strdup_printf(_("Select USB devices to redirect (%d of %d
> > > > channels free)"),
> > > > +
> > > > spice_usb_device_manager_get_free_channels(priv
> > > > ->manager),
> > > > +
> > > > spice_usb_device_manager_get_total_channels(priv->manager));
> > > > + markup_str = g_strdup_printf("<b>%s</b>", str);
> > > > + gtk_label_set_markup(GTK_LABEL (priv->label), markup_str);
> > > > + g_free(markup_str);
> > > > + g_free(str);
> > > > +
> > > > spice_usb_device_widget_update_status(self);
> > > > }
> > > >
> > > > --
> > > > 2.5.0
> > > >
> > > > _______________________________________________
> > > > Spice-devel mailing list
> > > > Spice-devel at lists.freedesktop.org
> > > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> > _______________________________________________
> > Spice-devel mailing list
> > Spice-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list