[virt-tools-list] [PATCH v2 remote-viewer] Connecting to ovirt without specifying VM name will not fail
Jonathon Jongsma
jjongsma at redhat.com
Fri Sep 19 17:32:07 UTC 2014
I haven't reviewed the code, but I did test the patch and I noticed a
couple of issues:
- I always get a critical warning on the terminal. I'm not sure if this
is a bug in libgovirt or a bug in how we're using libgovirt:
(remote-viewer:6744): libgovirt-CRITICAL **:
ovirt_resource_set_description_from_xml: assertion
'desc_node->content != NULL' failed
- When there are no running vms, it pops up a dialog saying "Couldn't
open oVirt session". That's not really accurate. The oVirt session was
opened successfully, it just didn't find any running vms.
- When you click 'cancel' to close the vm-chooser dialog, it immediately
pops up the same dialog saying "Couldn't open oVirt session". Again,
this is confusing and inaccurate. It should just exit the program
immediately.
- The dialog itself seems very cramped and small. It needs some padding
(see the new remote-viewer startup dialog for comparison).
- I would suggest just removing the text field from this dialog. It
doesn't appear to allow the user to enter text into the field (and it
wouldn't make sense to allow arbitrary strings in this field anyway,
since all of the possibilities are already listed in the treeview).
Right now it just duplicates the information provided by the treeview
selection.
- The heading labels on the dialog appear to be center-aligned. They
should be left-aligned. Again, try to match the new remote-viewer
dialog.
On Fri, 2014-09-19 at 11:30 +0200, Pavel Grunt wrote:
> When a user tries to connect to ovirt without specifying
> VM name (remote-viewer ovirt://ovirt.example.com/) or with
> wrong VM name a list of available virtual machines is shown,
> and the user may pick a machine he wants to connect to.
> ---
> changes:
> - dialog pops up on empty VM name
> - UI of the dialog is in separate file
>
> src/remote-viewer.c | 88 +++++++++++++++++++++-
> src/virt-viewer-vm-connection.xml | 153 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 239 insertions(+), 2 deletions(-)
> create mode 100644 src/virt-viewer-vm-connection.xml
>
> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index 5f5fa3d..5f5ea2d 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -74,6 +74,10 @@ enum {
> PROP_OPEN_RECENT_DIALOG
> };
>
> +#ifdef HAVE_OVIRT
> +static OvirtVm * choose_vm_dialog(char **vm_name, OvirtCollection *vms);
> +#endif
> +
> static gboolean remote_viewer_start(VirtViewerApp *self);
> #ifdef HAVE_SPICE_GTK
> static gboolean remote_viewer_activate(VirtViewerApp *self, GError **error);
> @@ -692,10 +696,13 @@ parse_ovirt_uri(const gchar *uri_str, char **rest_uri, char **name, char **usern
> }
>
> if (uri->path == NULL) {
> + uri->path = g_strdup("/");
> + }
> +
> + if(*uri->path != '/') {
> xmlFreeURI(uri);
> return FALSE;
> }
> - g_return_val_if_fail(*uri->path == '/', FALSE);
>
> /* extract VM name from path */
> path_elements = g_strsplit(uri->path, "/", -1);
> @@ -840,7 +847,10 @@ create_ovirt_session(VirtViewerApp *app, const char *uri)
> goto error;
> }
> vm = OVIRT_VM(ovirt_collection_lookup_resource(vms, vm_name));
> - g_return_val_if_fail(vm != NULL, FALSE);
> + if (vm == NULL && (vm = choose_vm_dialog(&vm_name, vms)) == NULL) {
> + g_debug("no oVirt VM was chosen");
> + goto error;
> + }
> g_object_get(G_OBJECT(vm), "state", &state, NULL);
> if (state != OVIRT_VM_STATE_UP) {
> g_debug("oVirt VM %s is not running", vm_name);
> @@ -980,6 +990,20 @@ recent_item_activated_dialog_cb(GtkRecentChooser *chooser G_GNUC_UNUSED, gpointe
> gtk_dialog_response(GTK_DIALOG (data), GTK_RESPONSE_ACCEPT);
> }
>
> +static void
> +vm_list_selection_changed_dialog_cb(GtkTreeSelection *selection, gpointer data)
> +{
> + GtkTreeIter iter;
> + GtkTreeModel *model;
> + const gchar *vm_name;
> + GtkWidget *entry = data;
> +
> + if (gtk_tree_selection_get_selected (selection, &model, &iter)) {
> + gtk_tree_model_get(model, &iter, 0, &vm_name, -1);
> + gtk_entry_set_text(GTK_ENTRY(entry), vm_name);
> + }
> +}
> +
> static void make_label_light(GtkLabel* label)
> {
> PangoAttrList* attributes = pango_attr_list_new();
> @@ -1092,6 +1116,66 @@ connect_dialog(gchar **uri)
> return retval;
> }
>
> +
> +#ifdef HAVE_OVIRT
> +static OvirtVm *choose_vm_dialog(char **vm_name, OvirtCollection *vms_collection)
> +{
> + GtkBuilder *vm_connection;
> + GtkWidget *dialog, *entry;
> + GtkTreeModel *store;
> + GtkTreeSelection *select;
> + GtkTreeIter iter;
> + GHashTable *vms;
> + GHashTableIter vms_iter;
> + OvirtVm *vm;
> + char *tmp_vm_name;
> + OvirtVmState state;
> + gint vms_running;
> +
> + vms = ovirt_collection_get_resources(vms_collection);
> + g_return_val_if_fail(g_hash_table_size(vms) > 0, NULL);
> +
> + vm_connection = virt_viewer_util_load_ui("virt-viewer-vm-connection.xml");
> + dialog = GTK_WIDGET(gtk_builder_get_object(vm_connection, "vm-connection-dialog"));
> + entry = GTK_WIDGET(gtk_builder_get_object(vm_connection, "entry"));
> + select = GTK_TREE_SELECTION(gtk_builder_get_object(vm_connection, "treeview-selection"));
> + store = GTK_TREE_MODEL(gtk_builder_get_object(vm_connection, "store"));
> +
> + vms_running = 0;
> + g_hash_table_iter_init(&vms_iter, vms);
> + while (g_hash_table_iter_next(&vms_iter, (gpointer *) &tmp_vm_name, (gpointer *) &vm)) {
> + g_object_get(G_OBJECT(vm), "state", &state, NULL);
> + if (state == OVIRT_VM_STATE_UP) {
> + gtk_list_store_append(GTK_LIST_STORE(store), &iter);
> + gtk_list_store_set(GTK_LIST_STORE(store), &iter, 0, tmp_vm_name, -1);
> + ++vms_running;
> + }
> + }
> +
> + g_signal_connect(select, "changed",
> + G_CALLBACK(vm_list_selection_changed_dialog_cb), entry);
> + if (gtk_tree_model_get_iter_first(store, &iter)) {
> + gtk_tree_selection_select_iter(select, &iter);
> + }
> + /* show and wait for response */
> + if (vms_running > 0) {
> + gtk_widget_show_all(dialog);
> + if (gtk_dialog_run(GTK_DIALOG(dialog)) == GTK_RESPONSE_ACCEPT) {
> + *vm_name = g_strdup(gtk_entry_get_text(GTK_ENTRY(entry)));
> + vm = OVIRT_VM(ovirt_collection_lookup_resource(vms_collection, *vm_name));
> + } else {
> + *vm_name = NULL;
> + vm = NULL;
> + }
> + } else {
> + vm = NULL;
> + }
> + gtk_widget_destroy(dialog);
> +
> + return vm;
> +}
> +#endif
> +
> static gboolean
> remote_viewer_start(VirtViewerApp *app)
> {
> diff --git a/src/virt-viewer-vm-connection.xml b/src/virt-viewer-vm-connection.xml
> new file mode 100644
> index 0000000..4c6ced6
> --- /dev/null
> +++ b/src/virt-viewer-vm-connection.xml
> @@ -0,0 +1,153 @@
> +<?xml version="1.0" encoding="UTF-8"?>
> +<!-- Generated with glade 3.16.1 -->
> +<interface>
> + <object class="GtkListStore" id="store">
> + <columns>
> + <!-- column-name name -->
> + <column type="gchararray"/>
> + <!-- column-name state -->
> + <column type="gchararray"/>
> + </columns>
> + </object>
> + <object class="GtkDialog" id="vm-connection-dialog">
> + <property name="can_focus">False</property>
> + <property name="title" translatable="yes">Virtual machine connection details</property>
> + <property name="modal">True</property>
> + <property name="destroy_with_parent">True</property>
> + <property name="type_hint">normal</property>
> + <child internal-child="vbox">
> + <object class="GtkBox" id="dialog-vbox1">
> + <property name="can_focus">False</property>
> + <property name="orientation">vertical</property>
> + <property name="spacing">2</property>
> + <child internal-child="action_area">
> + <object class="GtkButtonBox" id="dialog-action_area1">
> + <property name="can_focus">False</property>
> + <property name="layout_style">end</property>
> + <child>
> + <object class="GtkButton" id="button-cancel">
> + <property name="label">gtk-cancel</property>
> + <property name="visible">True</property>
> + <property name="can_focus">True</property>
> + <property name="receives_default">True</property>
> + <property name="use_stock">True</property>
> + </object>
> + <packing>
> + <property name="expand">False</property>
> + <property name="fill">True</property>
> + <property name="position">0</property>
> + </packing>
> + </child>
> + <child>
> + <object class="GtkButton" id="button-connect">
> + <property name="label">gtk-connect</property>
> + <property name="visible">True</property>
> + <property name="can_focus">True</property>
> + <property name="receives_default">True</property>
> + <property name="use_stock">True</property>
> + </object>
> + <packing>
> + <property name="expand">False</property>
> + <property name="fill">True</property>
> + <property name="position">1</property>
> + </packing>
> + </child>
> + </object>
> + <packing>
> + <property name="expand">False</property>
> + <property name="fill">True</property>
> + <property name="pack_type">end</property>
> + <property name="position">0</property>
> + </packing>
> + </child>
> + <child>
> + <object class="GtkTable" id="table1">
> + <property name="visible">True</property>
> + <property name="can_focus">False</property>
> + <property name="n_rows">4</property>
> + <property name="column_spacing">6</property>
> + <property name="row_spacing">6</property>
> + <child>
> + <object class="GtkLabel" id="label1">
> + <property name="visible">True</property>
> + <property name="can_focus">False</property>
> + <property name="label" translatable="yes">Virtual machine name</property>
> + <property name="ellipsize">end</property>
> + </object>
> + </child>
> + <child>
> + <object class="GtkEntry" id="entry">
> + <property name="visible">True</property>
> + <property name="can_focus">False</property>
> + <property name="editable">False</property>
> + <property name="progress_pulse_step">0.16</property>
> + </object>
> + <packing>
> + <property name="top_attach">1</property>
> + <property name="bottom_attach">2</property>
> + </packing>
> + </child>
> + <child>
> + <object class="GtkLabel" id="label2">
> + <property name="visible">True</property>
> + <property name="can_focus">False</property>
> + <property name="label" translatable="yes">Available virtual machines</property>
> + </object>
> + <packing>
> + <property name="top_attach">2</property>
> + <property name="bottom_attach">3</property>
> + </packing>
> + </child>
> + <child>
> + <object class="GtkTreeView" id="treeview">
> + <property name="visible">True</property>
> + <property name="can_focus">True</property>
> + <property name="model">store</property>
> + <property name="headers_visible">False</property>
> + <property name="enable_grid_lines">horizontal</property>
> + <child internal-child="selection">
> + <object class="GtkTreeSelection" id="treeview-selection"/>
> + </child>
> + <child>
> + <object class="GtkTreeViewColumn" id="treeviewcolumn1">
> + <property name="title" translatable="yes">Name</property>
> + <child>
> + <object class="GtkCellRendererText" id="cellrenderertext1"/>
> + <attributes>
> + <attribute name="text">0</attribute>
> + </attributes>
> + </child>
> + </object>
> + </child>
> + <child>
> + <object class="GtkTreeViewColumn" id="treeviewcolumn2">
> + <property name="title" translatable="yes">State</property>
> + <child>
> + <object class="GtkCellRendererText" id="cellrenderertext2"/>
> + <attributes>
> + <attribute name="text">1</attribute>
> + </attributes>
> + </child>
> + </object>
> + </child>
> + </object>
> + <packing>
> + <property name="top_attach">3</property>
> + <property name="bottom_attach">4</property>
> + </packing>
> + </child>
> + </object>
> + <packing>
> + <property name="expand">False</property>
> + <property name="fill">True</property>
> + <property name="position">1</property>
> + </packing>
> + </child>
> + </object>
> + </child>
> + <action-widgets>
> + <action-widget response="-6">button-cancel</action-widget>
> + <action-widget response="-3">button-connect</action-widget>
> + </action-widgets>
> + </object>
> +</interface>
More information about the virt-tools-list
mailing list