[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 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.

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.


>          goto cleanup;
> +    }
>  
>      xpath = g_strdup_printf("string(/domain/devices/graphics[@type='%s']/@port)", type);
>      gport = virt_viewer_extract_xpath_string(xmldesc, xpath);