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 _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
