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

[Fabiano Fidêncio]

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

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,
-- 
Fabiano Fidêncio