On Wed, Jan 20, 2016 at 5:28 PM, Marc-André Lureau <mlureau@xxxxxxxxxx> wrote:
> 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@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() ?
>
> What about making these object properties instead?
Hmmm. It actually sounds good. Let me go for this.
>
>>
>> > > > 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
>>
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel