[virt-tools-list] [virt-viewer][PATCH 5/5 v2] Leave the app_create_session() errors to the callers

[Jonathon Jongsma]

On Thu, 2015-03-26 at 17:37 +0100, Fabiano Fidêncio wrote:
> On Thu, Mar 26, 2015 at 5:16 PM, Jonathon Jongsma <jjongsma at redhat.com> wrote:
> > On Thu, 2015-03-26 at 16:35 +0100, Fabiano Fidêncio wrote:
> >> Doing this we can avoid to have more than one dialog being shown
> >> reporting the error.
> >>
> >> Related: rhbz#1085216
> >> ---
> >>  src/remote-viewer.c   |  4 +++-
> >>  src/virt-viewer-app.c |  2 --
> >>  src/virt-viewer.c     | 12 +++++++++++-
> >>  3 files changed, 14 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> >> index 4110b1e..5cca24b 100644
> >> --- a/src/remote-viewer.c
> >> +++ b/src/remote-viewer.c
> >> @@ -1224,7 +1224,9 @@ remote_viewer_start(VirtViewerApp *app, GError **err)
> >>
> >>      if (priv->controller) {
> >>          if (virt_viewer_app_create_session(app, "spice") < 0) {
> >> -            virt_viewer_app_simple_message_dialog(app, _("Couldn't create a Spice session"));
> >> +            g_set_error_literal(&error,
> >> +                                VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED,
> >> +                                _("Couldn't create a Spice session" ));
> >>              goto cleanup;
> >>          }
> >>
> >> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> >> index dd0361f..0871ed6 100644
> >> --- a/src/virt-viewer-app.c
> >> +++ b/src/virt-viewer-app.c
> >> @@ -1078,8 +1078,6 @@ virt_viewer_app_create_session(VirtViewerApp *self, const gchar *type)
> >>      {
> >>          virt_viewer_app_trace(self, "Guest %s has unsupported %s display type",
> >>                                priv->guest_name, type);
> >> -        virt_viewer_app_simple_message_dialog(self, _("Unknown graphic type for the guest %s"),
> >> -                                              priv->guest_name);
> >>          return -1;
> >>      }
> >>
> >> diff --git a/src/virt-viewer.c b/src/virt-viewer.c
> >> index 386cdcc..db145d5 100644
> >> --- a/src/virt-viewer.c
> >> +++ b/src/virt-viewer.c
> >> @@ -418,8 +418,18 @@ virt_viewer_extract_connect_info(VirtViewer *self,
> >>          goto cleanup;
> >>      }
> >>
> >> -    if (virt_viewer_app_create_session(app, type) < 0)
> >> +    if (virt_viewer_app_create_session(app, type) < 0) {
> >> +        gchar *msg = NULL;
> >> +        gchar *guest_name = NULL;
> >> +
> >> +        g_object_get(VIRT_VIEWER_APP(self), "guest-name", &guest_name, NULL);
> >> +        msg = g_strdup_printf(_("Unknown graphic type for the guest %s"), guest_name);
> >> +        g_set_error_literal(&error, VIRT_VIEWER_ERROR, VIRT_VIEWER_ERROR_FAILED, msg);
> >> +        g_free(msg);
> >> +        g_free(guest_name);
> >> +
> >
> > This is perhaps a bit nitpicky, but it seems to me that it would be
> > cleaner if the error was passed as an argument to the _create_session()
> > call. That function has all of the logic to determine whether (and why)
> > it can or can't create a session, so it should also be in charge of
> > explaining why the session creation failed (via the error message). But
> > that's mostly a personal preference.
> 
> Makes sense. The only thing to consider here is that we have different
> error messages being set according to who is the caller, all coming
> from the only place where it can return -1 :-)
> For instance:
> https://git.fedorahosted.org/cgit/virt-viewer.git/tree/src/remote-viewer.c#n954
> https://git.fedorahosted.org/cgit/virt-viewer.git/tree/src/remote-viewer.c#n1226
> https://git.fedorahosted.org/cgit/virt-viewer.git/tree/src/remote-viewer.c#n1288

True, but these are all basically the same thing. I don't really see the
need for 3 slightly different error messages here. Reducing that to one
standard message would mean less translating as well. But if anybody
feels strongly that these messages should be customized for these
different cases, I guess I don't care too much.

> 
> I can change it and adapt the error based on type of the connection
> used. If you prefer, I can go for it without any problem.
> 
> >
> > While I'm looking at this, it seems to me that it would be more accurate
> > if the error message said "Unsupported graphic type" rather than
> > "Unknown graphic type". This would change a translated string though.
> >
> 
> Right, I will go for this change as well.
> 
> >
> >>          goto cleanup;
> >> +    }
> >>
> >>      xpath = g_strdup_printf("string(/domain/devices/graphics[@type='%s']/@port)", type);
> >>      gport = virt_viewer_extract_xpath_string(xmldesc, xpath);
> >
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
> 
> 
> Best Regards,