[virt-tools-list] [PATCH virt-viewer 4/4] Exit normally when canceling dialog

[Pavel Grunt]

Hi, thanks for the review.

> 
> On Tue, Mar 17, 2015 at 10:08 AM, Pavel Grunt <pgrunt at redhat.com>
> 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
> > ---
> >  src/remote-viewer.c             | 4 ++++
> >  src/virt-viewer-session-spice.c | 2 ++
> >  src/virt-viewer-session-vnc.c   | 1 +
> >  src/virt-viewer.c               | 7 ++++++-
> >  4 files changed, 13 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> > index f9f4f92..c6c151b 100644
> > --- a/src/remote-viewer.c
> > +++ b/src/remote-viewer.c
> > @@ -876,6 +876,7 @@ create_ovirt_session(VirtViewerApp *app, const
> > char *uri, GError **err)
> >
> >      api = ovirt_proxy_fetch_api(proxy, &error);
> >      if (authenticate_info.dialog_cancelled) {
> > +        virt_viewer_app_set_user_cancelled(app, TRUE);
> >          g_clear_error(&error);
> >          goto error;
> >      }
> > @@ -897,6 +898,9 @@ create_ovirt_session(VirtViewerApp *app, const
> > char *uri, GError **err)
> >                         vms,
> >                         &error);
> >          if (vm == NULL) {
> > +            if (error == NULL) {
> > +                virt_viewer_app_set_user_cancelled(app, TRUE);
> > +            }
> >              goto error;
> >          }
> >      }
> > diff --git a/src/virt-viewer-session-spice.c
> > b/src/virt-viewer-session-spice.c
> > index 5eb7234..93906ae 100644
> > --- a/src/virt-viewer-session-spice.c
> > +++ b/src/virt-viewer-session-spice.c
> > @@ -562,6 +562,7 @@
> > virt_viewer_session_spice_main_channel_event(SpiceChannel *channel
> > G_GNUC_UNUSED
> >                                                     username_required
> >                                                     ? &user :
> >                                                     NULL,
> >                                                     &password);
> >          if (!ret) {
> > +
> >            virt_viewer_app_set_user_cancelled(virt_viewer_session_get_app(session),
> > TRUE);
> >              g_signal_emit_by_name(session, "session-cancelled");
> >          } else {
> >              gboolean openfd;
> > @@ -593,6 +594,7 @@
> > virt_viewer_session_spice_main_channel_event(SpiceChannel *channel
> > G_GNUC_UNUSED
> >                                                         "proxy",
> >                                                         NULL,
> >                                                         &user,
> >                                                         &password);
> >              if (!ret) {
> > +
> >                virt_viewer_app_set_user_cancelled(virt_viewer_session_get_app(session),
> > TRUE);
> >                  g_signal_emit_by_name(session,
> >                  "session-cancelled");
> >              } else {
> >                  spice_uri_set_user(proxy, user);
> > diff --git a/src/virt-viewer-session-vnc.c
> > b/src/virt-viewer-session-vnc.c
> > index 5a2dd86..68a6719 100644
> > --- a/src/virt-viewer-session-vnc.c
> > +++ b/src/virt-viewer-session-vnc.c
> > @@ -304,6 +304,7 @@
> > virt_viewer_session_vnc_auth_credential(GtkWidget *src
> > G_GNUC_UNUSED,
> >                                                              wantPassword
> >                                                              ?
> >                                                              &password
> >                                                              :
> >                                                              NULL);
> >
> >          if (!ret) {
> > +
> >            virt_viewer_app_set_user_cancelled(virt_viewer_session_get_app(session),
> > TRUE);
> >              vnc_display_close(self->priv->vnc);
> >              goto cleanup;
> >          }
> > diff --git a/src/virt-viewer.c b/src/virt-viewer.c
> > index acad6c6..65ec4e3 100644
> > --- a/src/virt-viewer.c
> > +++ b/src/virt-viewer.c
> > @@ -730,7 +730,10 @@ virt_viewer_initial_connect(VirtViewerApp
> > *app, GError **error)
> >                              &priv->domkey,
> >                              priv->conn,
> >                              &err);
> > -            if (dom == NULL && err != NULL) {
> > +            if (dom == NULL) {
> > +                if (err == NULL) {
> > +                    virt_viewer_app_set_user_cancelled(app, TRUE);
> > +                }
> 
> Here you changed a bit the logic and I'm not sure if it was
> intentional or not.
> I meant, the only way to go to clean up was: dom == NULL *and* err !=
> NULL. After your changes, dom == NULL is enough to go to clean up.
> Is it intentional? Are you missing an else for the "err == NULL" if?
> 
It was intentional but it should be in different patch.

I will use this:
            if (dom == NULL && err != NULL) {
                goto cleanup;
            }
            if (dom == NULL && err == NULL) {
                virt_viewer_app_set_user_cancelled(app, TRUE);
            }

> >                  goto cleanup;
> >              }
> >          }
> > @@ -910,6 +913,8 @@ virt_viewer_connect(VirtViewerApp *app)
> >              virt_viewer_app_simple_message_dialog(app,
> >              error_message);
> >
> >              g_free(error_message);
> > +        } else {
> > +            virt_viewer_app_set_user_cancelled(app, TRUE);
> >          }
> >
> >          return -1;
> > --
> > 2.3.2
> >
> > _______________________________________________
> > virt-tools-list mailing list
> > virt-tools-list at redhat.com
> > https://www.redhat.com/mailman/listinfo/virt-tools-list
> 
> A more general comment ... I'd set
> virt_viewer_app_set_user_cancelled() in all possible situations,
> being
> TRUE or FALSE and not only when it is TRUE.
> Seems a bit more clear, at least for me :-)
> 
sure, it makes sense. I will send v2.

Thank you,

Pavel