[virt-tools-list] [PATCH virt-viewer v2 2/3] Exit normally when canceling dialog

[Pavel Grunt]

Jonathon, thank you for your comments

> 
> To be completely honest, I think these get_user_cancelled() /
> set_user_cancelled() functions seem overly complicated. It is
> basically
> just used as a way to differentiate between different types of error
> conditions, which could be done by simply returning a different error
> code. I know that Marc-Andre removed the
> VIRT_VIEWER_VM_CHOOSE_DIALOG_CANCELLED error code a while back, but I
> really think it would be much cleaner to simply add it back again (or
> a
> simpler VIRT_VIEWER_ERROR_CANCELLED) rather than adding new API to
> test
> whether the dialog was cancelled or not. There's plenty of precedence
> for CANCELLED to be treated as a different error type
> (G_IO_ERROR_CANCELLED, VIRT_ERR_AUTH_CANCELLED, etc).
> 

Well I didn't think about reintroducing CANCELLED because it was removed. I can change it to GError (it will be g_error_matches and g_error_new instead of get/set_user_cancelled).

> Further comments inline.
> 
> On Tue, 2015-03-17 at 15:42 +0100, Pavel Grunt wrote:
> > This applies for:
> >  session authentication dialog
> >  libvirt authentication dialog (e.g. virt-viewer --attach guest)
> >  oVirt authentication dialog (e.g. remote-viewer
> >  ovirt://host/guest)
> >  'vm choose' dialog when connecting specifying the vm name
> > 
> > Related to https://bugzilla.redhat.com/show_bug.cgi?id=1201604
> > ---
> > v2: set virt_viewer_app_set_user_cancelled() in all possible
> > situation
> >     squashed
> >     https://www.redhat.com/archives/virt-tools-list/2015-March/msg00101.html
> > ---
> >  src/remote-viewer-main.c        |  6 +++++-
> >  src/remote-viewer.c             |  2 ++
> >  src/virt-viewer-app.c           | 15 +++++++++++++++
> >  src/virt-viewer-app.h           |  2 ++
> >  src/virt-viewer-main.c          |  6 +++++-
> >  src/virt-viewer-session-spice.c |  2 ++
> >  src/virt-viewer-session-vnc.c   |  1 +
> >  src/virt-viewer.c               |  2 ++
> >  8 files changed, 34 insertions(+), 2 deletions(-)
> > 
> > diff --git a/src/remote-viewer-main.c b/src/remote-viewer-main.c
> > index e8784ba..9db2a17 100644
> > --- a/src/remote-viewer-main.c
> > +++ b/src/remote-viewer-main.c
> > @@ -174,8 +174,12 @@ main(int argc, char **argv)
> >  
> >      app = VIRT_VIEWER_APP(viewer);
> >  
> > -    if (!virt_viewer_app_start(app))
> > +    if (!virt_viewer_app_start(app)) {
> 
> if we do re-introduce the CANCELLED error code, we'll obviously need
> to
> modify virt_viewer_app_start() to take a GError** output parameter.
> 
> > +        if (virt_viewer_app_get_user_cancelled(app)) {
> > +            ret = 0;
> > +        }
> >          goto cleanup;
> > +    }
> >  
> >      g_signal_connect(virt_viewer_app_get_session(app),
> >      "session-connected",
> >                       G_CALLBACK(connected), app);
> > diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> > index 4541515..35493f3 100644
> > --- a/src/remote-viewer.c
> > +++ b/src/remote-viewer.c
> > @@ -724,6 +724,7 @@ authenticate_cb(RestProxy *proxy, G_GNUC_UNUSED
> > RestProxyAuth *auth,
> >                                                     "oVirt",
> >                                                     NULL,
> >                                                     &username,
> >                                                     &password);
> > +    virt_viewer_app_set_user_cancelled(VIRT_VIEWER_APP(user_data),
> > !success);
> 
> As far as I can tell, setting this status does not actually do
> anything.
> authenticate_cb() obviously happens while gtk_main() is iterating,

Authenticate callback might happen as reaction for ovirt_proxy_fetch_api()

> but
> the only place in this patch where you actually call
> _get_user_cancelled() is in remote-viewer-main.c and
> virt-viewer-main.c,
> and these calls all occur before we even enter the mainloop. Or am I
> missing something?
> 
I am not sure what do you mean - virt_viewer_app_start() happens before gtk_main(), and setting user_cancelled avoids calling gtk_main(). How would it be different if we use GError?

Thanks,

Pavel