[virt-tools-list] [PATCH] Drop user back to 'open conn' dialog if connecting fails

[Cole Robinson]

Please use [PATCH v2] for version 2 of a patch, etc. git format-patch
--subject-prefix "PATCH v2"

On 08/06/2013 04:44 PM, Giuseppe Scrivano wrote:
> In case of connection failure, the user can either maintain the connection
> or re-edit it.
> 
> Split up `add_connection_to_ui' into `make_conn' and `register_conn'
> to handle separately the object creation and its registration in the
> list of connections (ui and conf).
> 
> Solves: https://bugzilla.redhat.com/show_bug.cgi?id=617386
> Signed-off-by: Giuseppe Scrivano <gscrivano at gnu.org>
> ---
>  virtManager/connect.py | 43 ++++++++++++++++++++++++++++++++-
>  virtManager/engine.py  | 64 ++++++++++++++++++++++++++++++++++++--------------
>  2 files changed, 88 insertions(+), 19 deletions(-)
> 

Behavior looks good now, but a comment inline.

> diff --git a/virtManager/connect.py b/virtManager/connect.py
> index 3aed22f..3b50efd 100644
> --- a/virtManager/connect.py
> +++ b/virtManager/connect.py
> @@ -111,9 +111,11 @@ class vmmConnect(vmmGObjectUI):
>          self.topwin.hide()
>          self.stop_browse()
>  
> -    def show(self, parent):
> +    def show(self, parent, initial_state=None):
>          logging.debug("Showing open connection")
>          self.reset_state()
> +        if initial_state:
> +            self.set_state(initial_state)
>          self.topwin.set_transient_for(parent)
>          self.topwin.present()
>  
> @@ -314,6 +316,45 @@ class vmmConnect(vmmGObjectUI):
>          default_user = default_conn_user(conn)
>          self.widget("username-entry").set_text(default_user)
>  
> +    def set_state(self, state):
> +        hv = None
> +        if state.is_xen():
> +            hv = HV_XEN
> +        elif state.is_qemu():
> +            hv = HV_QEMU
> +        elif state.is_lxc():
> +            hv = HV_LXC
> +        is_remote = bool(state.is_remote())
> +        host = state.get_uri_hostname()
> +        autoconnect = state.get_autoconnect()
> +        connection = None
> +        transport = state.get_transport()
> +        user = transport[1]
> +        if transport[0]:
> +            if "tls" in transport[0]:
> +                connection = CONN_TLS
> +            elif "ssh" in transport[0]:
> +                connection = CONN_SSH
> +            elif "tcp" in transport[0]:
> +                connection = CONN_TCP
> +
> +        self.widget("connection").set_sensitive(is_remote)
> +        if connection:
> +            self.widget("connection").set_active(connection)
> +        self.widget("connect-remote").set_active(is_remote)
> +        self.widget("username-entry").set_sensitive(is_remote)
> +        self.widget("hostname").set_sensitive(is_remote)
> +        if is_remote:
> +            self.widget("hostname").get_child().set_text(host)
> +        if(hv is not None):
> +            self.widget("hypervisor").set_active(hv)
> +        if (user):
> +            self.widget("username-entry").set_text(user)
> +
> +        self.widget("autoconnect").set_sensitive(True)
> +        self.widget("autoconnect").set_active(autoconnect)
> +
> +

This seems overkill. Why can't we add an argument to show() which skips the
reset_state step?

Hmm, I guess someone could reopen the connect wizard while the first
connection is opening, which would mean the state is wrong when we re-show the
wizard.

Thinking some more, what we really want here is:

Launch 'Open Connection', set parameters, hit 'connect'
'Open Connection' window is desensitized, async progress meter dialog opens up
while connection is connecting
If connection fails, we drop back to the still open connection wizard with
same state still listed.

That's how the create/clone/delete wizards work, basically make the operation
synchronous. However truth be told I don't care enough about this bug to
mandate that we do that. If you want to look into it it might be simple, or it
might be a pain.

But the above solution is definitely not ideal since it requires duplicating
update of UI elements in two places (set_state and reset_state) with the
latter in a rarely tested code path.

So I'll take a patch with the show() option bypassing reset_state, or skipping
that entirely and user loses the previous state if we reshow the connect
wizard, or a patch implementing my suggestion above.

Thanks,
Cole