[Spice-devel] [PATCH 1/2] usb-device-widget: Improve "there are no free USB channnels" string
Jonathon Jongsma
jjongsma at redhat.com
Wed Jan 20 07:49:37 PST 2016
On Wed, 2016-01-20 at 10:09 +0100, Fabiano Fidêncio wrote:
> On Wed, Jan 20, 2016 at 9:53 AM, Victor Toso <lists at victortoso.com> wrote:
> > Hi,
> >
> > On Tue, Jan 19, 2016 at 10:36:37PM +0100, Fabiano Fidêncio wrote:
> > > As the message can be understand as an error message, let's replace it
> > > for a more user-friendly message and display it in the infobar as an
> > > info message instead of a warning message.
> > >
> > > Related: rhbz#1298772
> > > ---
> > > src/spice-client.h | 2 ++
> > > src/usb-device-manager.c | 6 ++++--
> > > src/usb-device-widget.c | 10 ++++++++--
> > > 3 files changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/src/spice-client.h b/src/spice-client.h
> > > index 32b79ea..cccb325 100644
> > > --- a/src/spice-client.h
> > > +++ b/src/spice-client.h
> > > @@ -70,6 +70,7 @@ G_BEGIN_DECLS
> > > * @SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME: username is required
> > > * @SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME: password and
> > > username are required
> > > * @SPICE_CLIENT_ERROR_USB_SERVICE: USB service error
> > > + * @SPICE_CLIENT_ERROR_USB_NO_FREE_CHANNELS_AVAILABLE: there are no more
> > > free channels available for redirecting a device
> > > *
> > > * Error codes returned by spice-client API.
> > > */
> > > @@ -82,6 +83,7 @@ typedef enum
> > > SPICE_CLIENT_ERROR_AUTH_NEEDS_USERNAME,
> > > SPICE_CLIENT_ERROR_AUTH_NEEDS_PASSWORD_AND_USERNAME,
> > > SPICE_CLIENT_ERROR_USB_SERVICE,
> > > + SPICE_CLIENT_ERROR_USB_NO_FREE_CHANNELS_AVAILABLE,
> > > } SpiceClientError;
> > >
> > > #ifndef SPICE_DISABLE_DEPRECATED
> > > diff --git a/src/usb-device-manager.c b/src/usb-device-manager.c
> > > index c62f56e..a22d926 100644
> > > --- a/src/usb-device-manager.c
> > > +++ b/src/usb-device-manager.c
> > > @@ -1673,8 +1673,10 @@
> > > spice_usb_device_manager_can_redirect_device(SpiceUsbDeviceManager *self,
> > > break;
> > > }
> > > if (i == priv->channels->len) {
> > > - g_set_error_literal(err, SPICE_CLIENT_ERROR,
> > > SPICE_CLIENT_ERROR_FAILED,
> > > - _("There are no free USB channels"));
> > > + g_set_error_literal(err,
> > > + SPICE_CLIENT_ERROR,
> > > + SPICE_CLIENT_ERROR_USB_NO_FREE_CHANNELS_AVAILABLE,
> > > + _("All available USB channels are in use.\nNo additional
> > > devices can be redirected"));
> >
> > I keep thinking that if they are 'in use' they aren't availabe USB
> > channels. Maybe the first sentence could be "All USB channels are in
> > use" ?
>
> I can take that as well. The sentence used came from Jonathon's
> suggestion. If he also agress with your suggestion, I will change and
> submit a v2.
> Also, I don't link that much of the error name, suggestions are welcome.
Sure, Victor's suggestion is fine.
As for the error name: yes, it's a bit long, but not too bad. Perhaps you could
just drop the FREE so that it's a little more concise:
SPICE_CLIENT_ERROR_USB_NO_CHANNELS_AVAILABLE.
However, does it feel a bit strange to anybody else that a function that just
checks if a channel can be redirected is setting an error? A GError is supposed
to be conceptually similar to an exception in other languages. True, the error
gives us some additional information about why the device cannot be redirected.
But we're not actually trying to redirect anything, we're only asking if it
*can* be redirected. So "throwing an exception" seems to be the wrong thing to
do here. And this is confirmed below by the fact that we have to filter out this
error as not-really-an-error... I'm not really sure
>
> >
> > My english is definetly not good so maybe the former is much better then
> > my suggestion.
> >
> > You changed the identation format but both are fine, patch looks good.
>
> Yeah. If I didin't change the indentation, the content wouldn't fit in
> the max-columns used in this file.
>
> >
> > > return FALSE;
> > > }
> > >
> > > diff --git a/src/usb-device-widget.c b/src/usb-device-widget.c
> > > index fe983c9..830bdce 100644
> > > --- a/src/usb-device-widget.c
> > > +++ b/src/usb-device-widget.c
> > > @@ -74,6 +74,7 @@ struct _SpiceUsbDeviceWidgetPrivate {
> > > GtkWidget *info_bar;
> > > gchar *err_msg;
> > > gsize device_count;
> > > + gboolean is_info_message;
> > > };
> > >
> > > static guint signals[LAST_SIGNAL] = { 0, };
> > > @@ -381,6 +382,10 @@ static void check_can_redirect(GtkWidget *widget,
> > > gpointer user_data)
> > > /* If we cannot redirect this device, append the error message to
> > > err_msg, but only if it is *not* already there! */
> > > if (!can_redirect) {
> > > + priv->is_info_message = g_error_matches(err,
> > > + SPICE_CLIENT_ERROR,
> > > + SPICE_CLIENT_ERROR_USB_NO_FREE_CHANNELS_AVAILABLE);
> > > +
I have quite a few issues with this particular function. First of all, it's
called on the widget rather than on the device, which was surprising to me. In
addition, the caller (_update_status()) calls this in a loop using
gtk_container_foreach(). This means that it is called on all widgets in the
container, not just the ones associated with a usb device. So the called
function has to filter out the nonsense widgets (e.g. gtkinfobar, gtkalignment,
etc) before it can proceed with any checks. In order to get the USB device
associated with a particular widget, it uses GObject data. GObject data can
sometimes be convenient, but I usually find it to be a warning flag that the
code isn't organized very well. So this whole function strikes me as rather
convoluted. But that has nothing much to do with your patch.
As for your change here, you're updating a private member of the object, which
could overwrite a previous "real" error. So I'm not sure it's a good idea. If a
previous call to check_can_redirect() resulted in a real error and then the next
one resulted in a NO_FREE_CHANNELS_AVAILABLE error, we would set is_info_message
and display only an info message even when a real error exists.
> > > if (priv->err_msg) {
> > > if (!strstr(priv->err_msg, err->message)) {
> > > gchar *old_err_msg = priv->err_msg;
Another example of what I don't like about this function (again, not your
fault). It feels like a hack to me. We're just appending new error messages to a
string containing all error messages from previous calls to this function. So
this function is making very specific assumptions about the manner in which it
is called. But it can't check or enforce those assumptions.
> > > @@ -407,8 +412,9 @@ static gboolean
> > > spice_usb_device_widget_update_status(gpointer user_data)
> > >
> > > if (priv->err_msg) {
> > > spice_usb_device_widget_show_info_bar(self, priv->err_msg,
> > > - GTK_MESSAGE_INFO,
> > > - GTK_STOCK_DIALOG_WARNING);
> > > + GTK_MESSAGE_INFO,
> > > + priv->is_info_message ?
> > > + GTK_STOCK_DIALOG_INFO : GTK_STOCK_DIALOG_WARNING);
> > > g_free(priv->err_msg);
> > > priv->err_msg = NULL;
> > > } else {
> > > --
> > > 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
More information about the Spice-devel
mailing list