On Wed, Jan 20, 2016 at 5:24 PM, Jonathon Jongsma <jjongsma@xxxxxxxxxx> wrote:
> 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() ?
Hmm. I agree.
>
>> > > 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.
I'm doing this just to have a label set in case we fail to get the
SpiceUsbDeviceManager, as it do an earlier return.
>
>> > > + 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.
Okay. "(%d channels free)" works for me.
>
> 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.
I am more than happy to drop the first patch and keep just this one
(and then provide another patch removing the error setting in case
there are no more free channels to redirect).
>
>
>> > > + 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