Hey,
This dialog looks very nice! :)
On Tue, Sep 16, 2014 at 01:58:47PM +0200, Pavel Grunt wrote:
> When a user tries to connect to ovirt without specifying VM name (remote-viewer ovirt://ovirt.example.com/) a list of available virtual machines is shown, and the user may pick a machine he wants to connect to.
>
> ---
> src/remote-viewer.c | 130 +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 129 insertions(+), 1 deletion(-)
>
> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index 5f5fa3d..4b21696 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -74,6 +74,10 @@ enum {
> PROP_OPEN_RECENT_DIALOG
> };
>
> +#ifdef HAVE_OVIRT
> +static gint choose_wm_dialog(char **vm_name, OvirtCollection *vms);
It seems you don't really use the return value of choose_wm_dialog so
you could just make it
static char *choose_vm_dialog(OvirtCollection *vms); (nb: vm VS wm)
> +#endif
> +
> static gboolean remote_viewer_start(VirtViewerApp *self);
> #ifdef HAVE_SPICE_GTK
> static gboolean remote_viewer_activate(VirtViewerApp *self, GError **error);
> @@ -840,7 +844,11 @@ 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) {
> + if (choose_wm_dialog(&vm_name, vms) == 0 && vm_name != NULL)
Showing the dialog here will make sure it pops up on an invalid VM name,
but it would be nice to also get the dialog when starting remote-viewer
with an empty VM name:
remote-viewer --ovirt-ca-file=ca.crt ovirt://ovirt.example.com
> + vm = OVIRT_VM(ovirt_collection_lookup_resource(vms, vm_name));
> + g_return_val_if_fail(vm != NULL, FALSE);
> + }
> 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 +988,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 +1114,112 @@ connect_dialog(gchar **uri)
> return retval;
> }
>
> +
> +#ifdef HAVE_OVIRT
> +static gint choose_wm_dialog(char **vm_name, OvirtCollection *vms_collection)
> +{
> + GtkWidget *dialog, *area, *box, *label, *entry, *vm_list;
> +#if !GTK_CHECK_VERSION(3, 0, 0)
> + GtkWidget *alignment;
> +#endif
> + GtkTreeModel *model;
> + GtkListStore *store;
> + GtkTreeSelection *select;
> + GtkTreeIter iter;
> + GHashTable *vms;
> + GHashTableIter vms_iter;
> + OvirtVm *vm;
> + char *tmp_vm_name;
> + OvirtVmState state;
> + gint retval;
> +
> + vms = ovirt_collection_get_resources(vms_collection);
> + g_return_val_if_fail(g_hash_table_size(vms) > 0, -1);
> +
> + dialog = gtk_dialog_new_with_buttons(_("Virtual machine connection details"),
> + NULL,
> + GTK_DIALOG_DESTROY_WITH_PARENT,
> + GTK_STOCK_CANCEL,
> + GTK_RESPONSE_REJECT,
> + GTK_STOCK_CONNECT,
> + GTK_RESPONSE_ACCEPT,
> + NULL);
> + gtk_dialog_set_default_response(GTK_DIALOG(dialog), GTK_RESPONSE_ACCEPT);
> + gtk_container_set_border_width(GTK_CONTAINER(dialog), 5);
> + area = gtk_dialog_get_content_area(GTK_DIALOG(dialog));
> + box = gtk_vbox_new(FALSE, 6);
> + gtk_container_set_border_width(GTK_CONTAINER(box), 5);
> + gtk_box_pack_start(GTK_BOX(area), box, TRUE, TRUE, 0);
> +
> + label = gtk_label_new_with_mnemonic(_("_Virtual machine name"));
> + gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5);
> + gtk_box_pack_start(GTK_BOX(box), label, TRUE, TRUE, 0);
> + entry = GTK_WIDGET(gtk_entry_new());
> + gtk_entry_set_activates_default(GTK_ENTRY(entry), TRUE);
> + g_object_set(entry, "width-request", 200, NULL);
> + g_signal_connect(entry, "changed", G_CALLBACK(entry_changed_cb), entry);
> + g_signal_connect(entry, "icon-release", G_CALLBACK(entry_icon_release_cb), entry);
> + gtk_box_pack_start(GTK_BOX(box), entry, TRUE, TRUE, 0);
> + gtk_label_set_mnemonic_widget(GTK_LABEL(label), entry);
> + make_label_bold(GTK_LABEL(label));
> +
> + label = gtk_label_new_with_mnemonic(_("_Available virtual machines"));
> + make_label_bold(GTK_LABEL(label));
> + gtk_box_pack_start(GTK_BOX(box), label, TRUE, TRUE, 0);
> + gtk_misc_set_alignment(GTK_MISC(label), 0, 0.5);
This is done by hand as the connection dialog does, but it would be
be easier to modify if this was in a UI file (similar to what is done
for the guest details for example).
> +
> + store = gtk_list_store_new(2, G_TYPE_STRING, G_TYPE_STRING);
> + g_hash_table_iter_init(&vms_iter, vms);
> + while (g_hash_table_iter_next(&vms_iter, (gpointer *)&tmp_vm_name, NULL)) {
> + vm = OVIRT_VM(ovirt_collection_lookup_resource(vms_collection, tmp_vm_name));
> + if (vm != NULL) {
> + g_object_get(G_OBJECT(vm), "state", &state, NULL);
> + }
> + gtk_list_store_append(store, &iter);
> + gtk_list_store_set(store, &iter,
> + 0, tmp_vm_name,
> + 1, (vm != NULL && state == OVIRT_VM_STATE_UP) ? "running" : "not running",
> + -1);
You can mark these "running"/"not running" strings for translation with
_() around them: _("running"); I don't think we handle non-running ovirt
VMs really well at the moment, so it's probably better to hide non
running VMs for now.
'vm' must be unref'ed (g_object_unref) after use as
ovirt_collection_lookup_resource() is documented as "transfer full"
which means the caller of that function has ownership of the returned
value.
You can probably skip the call to ovirt_collection_lookup_resource() if
you do:
while (g_hash_table_iter_next(&vms_iter, (gpointer *)&tmp_vm_name, &vm)) {
}
> + }
> + model = GTK_TREE_MODEL(store);
You can also use GTK_TREE_MODEL(store), in the 2 places where it's
needed instead of introducing an additional local variable.
> +
> + vm_list = GTK_WIDGET(gtk_tree_view_new());
I think this can be gtk_tree_view_new_with_model() which saves the call
to _set_model() later on.
> + gtk_label_set_mnemonic_widget(GTK_LABEL(label), vm_list);
> + gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(vm_list), -1, "Name",
> + gtk_cell_renderer_text_new(), "text", 0,
> + NULL);
> + gtk_tree_view_insert_column_with_attributes(GTK_TREE_VIEW(vm_list), -1, "State",
> + gtk_cell_renderer_text_new(), "text", 1,
> + NULL);
> + gtk_tree_view_set_headers_visible(GTK_TREE_VIEW(vm_list), TRUE);
> + gtk_tree_view_set_model(GTK_TREE_VIEW(vm_list), model);
> + gtk_box_pack_start(GTK_BOX(box), vm_list, TRUE, TRUE, 0);
> + gtk_label_set_mnemonic_widget(GTK_LABEL(label), vm_list);
> +
> + select = gtk_tree_view_get_selection(GTK_TREE_VIEW(vm_list));
> + gtk_tree_selection_set_mode (select, GTK_SELECTION_SINGLE);
> + g_signal_connect(select, "changed",
> + G_CALLBACK(vm_list_selection_changed_dialog_cb), entry);
> + if (gtk_tree_model_get_iter_first(model, &iter)) {
> + gtk_tree_selection_select_iter(select, &iter);
> + }
> + g_object_unref(model);
> + /* show and wait for response */
> + 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)));
> + g_strstrip(*vm_name);
Is there any particular reason for calling g_strstrip? Did you get some
extra spaces in your testing?
Christophe
Attachment:
pgpRN5wlMTpUX.pgp
Description: PGP signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
