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@xxxxxxxxxxxxxx> 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() ?
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> > > http://lists.freedesktop.org/mailman/listinfo/spice-devel
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel