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

[Jonathon Jongsma]

On Tue, 2015-03-17 at 17:58 -0400, Pavel Grunt wrote:
> 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?
> 

I guess I was not very clear. My point was that since user_cancelled
avoids calling gtk_main() at all, anything thing that sets
user_cancelled after the mainloop is already running is pointless
because we never check the value of user_cancelled after that. 

Initially I thought that authenticate_db() was only triggered as part of
an asynchronous network communication (i.e. while the mainloop was
running). But now I see that it can happen synchronously before the
mainloop runs. But I think my other comments (regarding
virt_viewer_session_spice_main_channel_event(), etc.) are still
accurate. These callbacks only happen while the mainloop is iterating,
so setting user_cancelled there should have no effect on the behavior of
the application.

Jonathon