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