[virt-tools-list] [PATCH virt-viewer 2/5] Introduce ISO List dialog
Eduardo Lima (Etrunko)
etrunko at redhat.com
Mon Jan 23 19:01:34 UTC 2017
On 20/01/17 13:20, Christophe Fergeau wrote:
> Can you expand a bit in the commit log about what this iso dialog is,
> what is your goal for introducing it, .. ? Huge commit with shortlog
> does not look good ;)
>
Alright, it will be done for the next version.
> On Thu, Jan 19, 2017 at 01:42:11PM -0200, Eduardo Lima (Etrunko) wrote:
>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
>> ---
>> po/POTFILES.in | 2 +
>> src/Makefile.am | 3 +
>> src/remote-viewer-iso-list-dialog.c | 365 +++++++++++++++++++++++++++++
>> src/remote-viewer-iso-list-dialog.h | 58 +++++
>> src/resources/ui/remote-viewer-iso-list.ui | 158 +++++++++++++
>> src/resources/virt-viewer.gresource.xml | 1 +
>> 6 files changed, 587 insertions(+)
>> create mode 100644 src/remote-viewer-iso-list-dialog.c
>> create mode 100644 src/remote-viewer-iso-list-dialog.h
>> create mode 100644 src/resources/ui/remote-viewer-iso-list.ui
>>
>> diff --git a/po/POTFILES.in b/po/POTFILES.in
>> index 69d9fef..371c242 100644
>> --- a/po/POTFILES.in
>> +++ b/po/POTFILES.in
>> @@ -1,9 +1,11 @@
>> data/remote-viewer.appdata.xml.in
>> data/remote-viewer.desktop.in
>> data/virt-viewer-mime.xml.in
>> +src/remote-viewer-iso-list-dialog.c
>> src/remote-viewer-main.c
>> src/remote-viewer.c
>> [type: gettext/glade] src/resources/ui/remote-viewer-connect.ui
>> +[type: gettext/glade] src/resources/ui/remote-viewer-iso-list.ui
>> [type: gettext/glade] src/resources/ui/virt-viewer-about.ui
>> src/virt-viewer-app.c
>> src/virt-viewer-auth.c
>> diff --git a/src/Makefile.am b/src/Makefile.am
>> index 272c4ff..9748277 100644
>> --- a/src/Makefile.am
>> +++ b/src/Makefile.am
>> @@ -13,6 +13,7 @@ noinst_DATA = \
>> resources/ui/virt-viewer-vm-connection.ui \
>> resources/ui/virt-viewer-preferences.ui \
>> resources/ui/remote-viewer-connect.ui \
>> + resources/ui/remote-viewer-iso-list.ui \
>> resources/ui/virt-viewer-file-transfer-dialog.ui \
>> $(NULL)
>>
>> @@ -97,6 +98,8 @@ if HAVE_OVIRT
>> libvirt_viewer_la_SOURCES += \
>> ovirt-foreign-menu.h \
>> ovirt-foreign-menu.c \
>> + remote-viewer-iso-list-dialog.c \
>> + remote-viewer-iso-list-dialog.h \
>> $(NULL)
>> endif
>>
>> diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote-viewer-iso-list-dialog.c
>> new file mode 100644
>> index 0000000..bf7c6c7
>> --- /dev/null
>> +++ b/src/remote-viewer-iso-list-dialog.c
>> @@ -0,0 +1,365 @@
>> +/*
>> + * Virt Viewer: A virtual machine console viewer
>> + *
>> + * Copyright (C) 2016 Red Hat, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + */
>> +
>> +#include <config.h>
>> +
>> +#include <glib/gi18n.h>
>> +
>> +#include "remote-viewer-iso-list-dialog.h"
>> +#include "virt-viewer-util.h"
>> +#include "ovirt-foreign-menu.h"
>> +
>> +static void ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu *foreign_menu, GAsyncResult *result, RemoteViewerISOListDialog *self);
>> +static void remote_viewer_iso_list_dialog_show_error(RemoteViewerISOListDialog *self, const gchar *message);
>> +
>> +G_DEFINE_TYPE(RemoteViewerISOListDialog, remote_viewer_iso_list_dialog, GTK_TYPE_DIALOG)
>> +
>> +#define DIALOG_PRIVATE(o) \
>> + (G_TYPE_INSTANCE_GET_PRIVATE((o), REMOTE_VIEWER_TYPE_ISO_LIST_DIALOG, RemoteViewerISOListDialogPrivate))
>> +
>> +struct _RemoteViewerISOListDialogPrivate
>> +{
>> + GtkListStore *list_store;
>> + GtkWidget *status;
>> + GtkWidget *spinner;
>> + GtkWidget *stack;
>> + GtkWidget *tree_view;
>> + OvirtForeignMenu *foreign_menu;
>> +};
>> +
>> +enum RemoteViewerISOListDialogModel
>> +{
>> + ISO_IS_ACTIVE = 0,
>> + ISO_NAME,
>> + FONT_WEIGHT,
>> +};
>> +
>> +enum RemoteViewerISOListDialogProperties {
>> + PROP_0,
>> + PROP_FOREIGN_MENU,
>> +};
>> +
>> +
>> +void remote_viewer_iso_list_dialog_toggled(GtkCellRendererToggle *cell_renderer, gchar *path, gpointer user_data);
>> +void remote_viewer_iso_list_dialog_row_activated(GtkTreeView *view, GtkTreePath *path, GtkTreeViewColumn *col, gpointer user_data);
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_dispose(GObject *object)
>> +{
>> + RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object);
>> + RemoteViewerISOListDialogPrivate *priv = self->priv;
>> +
>> + if (priv->foreign_menu) {
>> + g_signal_handlers_disconnect_by_data(priv->foreign_menu, object);
>> + g_clear_object(&priv->foreign_menu);
>> + }
>> + G_OBJECT_CLASS(remote_viewer_iso_list_dialog_parent_class)->dispose(object);
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_set_property(GObject *object, guint property_id,
>> + const GValue *value, GParamSpec *pspec)
>> +{
>> + RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(object);
>> + RemoteViewerISOListDialogPrivate *priv = self->priv;
>> +
>> + switch (property_id) {
>> + case PROP_FOREIGN_MENU:
>> + priv->foreign_menu = g_value_dup_object(value);
>> + break;
>> + default:
>> + G_OBJECT_WARN_INVALID_PROPERTY_ID (object, property_id, pspec);
>> + }
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_class_init(RemoteViewerISOListDialogClass *klass)
>> +{
>> + GObjectClass *object_class = G_OBJECT_CLASS(klass);
>> +
>> + g_type_class_add_private(klass, sizeof(RemoteViewerISOListDialogPrivate));
>> +
>> + object_class->dispose = remote_viewer_iso_list_dialog_dispose;
>> + object_class->set_property = remote_viewer_iso_list_dialog_set_property;
>> +
>> + g_object_class_install_property(object_class,
>> + PROP_FOREIGN_MENU,
>> + g_param_spec_object("foreign-menu",
>> + "oVirt Foreign Menu",
>> + "Object which is used as interface to oVirt",
>> + OVIRT_TYPE_FOREIGN_MENU,
>> + G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY | G_PARAM_STATIC_STRINGS));
>
> s/G_PARAM_READWRITE/G_PARAM_WRITABLE
Fixed.
>
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_show_files(RemoteViewerISOListDialog *self)
>> +{
>> + RemoteViewerISOListDialogPrivate *priv = self->priv = DIALOG_PRIVATE(self);
>
> I prefer to avoid multiple assignments on a single line. Here you don't
> really need a local 'priv' anyway, so this could just be
>
> self->priv = DIALOG_PRIVATE(self);
Fixed.
>
>> + gtk_stack_set_visible_child_full(GTK_STACK(priv->stack), "iso-list",
>> + GTK_STACK_TRANSITION_TYPE_NONE);
>> + gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, TRUE);
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_foreach(char *name, RemoteViewerISOListDialog *self)
>> +{
>> + RemoteViewerISOListDialogPrivate *priv = self->priv;
>> + gchar *current_iso = ovirt_foreign_menu_get_current_iso_name(self->priv->foreign_menu);
>> + gboolean active = g_strcmp0(current_iso, name) == 0;
>
> I'd prefer
> gboolean active = (g_strcmp0(current_iso, name) == 0);
> imo this makes it easier to notice we're assigning the result of a test
> to the boolean.
>
Ok, this is just a matter of preference, for me it is okay the way it is
right now, but I can change it without problems.
>> + gint weight = active ? PANGO_WEIGHT_BOLD : PANGO_WEIGHT_NORMAL;
>> + GtkTreeIter iter;
>> +
>> + gtk_list_store_append(priv->list_store, &iter);
>> + gtk_list_store_set(priv->list_store, &iter,
>> + ISO_IS_ACTIVE, active,
>> + ISO_NAME, name,
>> + FONT_WEIGHT, weight, -1);
>> +
>> + if (active) {
>> + GtkTreePath *path = gtk_tree_model_get_path(GTK_TREE_MODEL(priv->list_store), &iter);
>> + gtk_tree_view_set_cursor(GTK_TREE_VIEW(priv->tree_view), path, NULL, FALSE);
>> + gtk_tree_view_scroll_to_cell(GTK_TREE_VIEW(priv->tree_view), path, NULL, TRUE, 0.5, 0.5);
>> + gtk_tree_path_free(path);
>> + }
>> +
>> + g_free(current_iso);
>> +}
>> +
>> +static void
>> +fetch_iso_names_cb(OvirtForeignMenu *foreign_menu,
>> + GAsyncResult *result,
>> + RemoteViewerISOListDialog *self)
>> +{
>> + RemoteViewerISOListDialogPrivate *priv = self->priv;
>> + GError *error = NULL;
>> + GList *iso_list;
>> +
>> + iso_list = ovirt_foreign_menu_fetch_iso_names_finish(foreign_menu, result, &error);
>> +
>> + if (!iso_list) {
>> + const gchar *msg = error ? error->message : _("Failed to fetch CD names");
>> + gchar *markup = g_strdup_printf("<b>%s</b>", msg);
>> +
>> + gtk_label_set_markup(GTK_LABEL(priv->status), markup);
>> + gtk_spinner_stop(GTK_SPINNER(priv->spinner));
>> + remote_viewer_iso_list_dialog_show_error(self, msg);
>> + gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, TRUE);
>> + g_free(markup);
>> + g_clear_error(&error);
>
> I'd also close 'self' when the error message is dismissed (so you don't
> really need to set a label in the iso dialog).
I would keep it like this, because it can be a temporary networking
problem, and user can hit the reload button right away, instead of going
to the main menu and opening the dialog again. This happened to me
during one of my tests, where I had a DNS query failure which did not
happen the second time.
>
>> + return;
>> + }
>> +
>> + g_list_foreach(iso_list, (GFunc) remote_viewer_iso_list_dialog_foreach, self);
>> + remote_viewer_iso_list_dialog_show_files(self);
>> +}
>> +
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_refresh_iso_list(RemoteViewerISOListDialog *self)
>> +{
>> + RemoteViewerISOListDialogPrivate *priv = self->priv;
>> +
>> + gtk_list_store_clear(priv->list_store);
>> + ovirt_foreign_menu_fetch_iso_names_async(priv->foreign_menu, NULL,
>> + (GAsyncReadyCallback) fetch_iso_names_cb,
>> + self);
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_response(GtkDialog *dialog,
>> + gint response_id,
>> + gpointer user_data G_GNUC_UNUSED)
>> +{
>> + RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
>> + RemoteViewerISOListDialogPrivate *priv = self->priv;
>> +
>> + if (response_id != GTK_RESPONSE_NONE)
>> + return;
>> +
>> + gtk_spinner_start(GTK_SPINNER(priv->spinner));
>> + gtk_label_set_markup(GTK_LABEL(priv->status), _("<b>Loading...</b>"));
>> + gtk_stack_set_visible_child_full(GTK_STACK(priv->stack), "status",
>> + GTK_STACK_TRANSITION_TYPE_NONE);
>> + gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, FALSE);
>> + remote_viewer_iso_list_dialog_refresh_iso_list(self);
>> +}
>> +
>> +void
>> +remote_viewer_iso_list_dialog_toggled(GtkCellRendererToggle *cell_renderer G_GNUC_UNUSED,
>> + gchar *path,
>> + gpointer user_data)
>> +{
>> + RemoteViewerISOListDialog *self = REMOTE_VIEWER_ISO_LIST_DIALOG(user_data);
>> + RemoteViewerISOListDialogPrivate *priv = self->priv;
>> + GtkTreeModel *model = GTK_TREE_MODEL(priv->list_store);
>> + GtkTreePath *tree_path = gtk_tree_path_new_from_string(path);
>> + GtkTreeIter iter;
>> + gboolean active;
>> + gchar *name;
>> +
>> + gtk_tree_view_set_cursor(GTK_TREE_VIEW(priv->tree_view), tree_path, NULL, FALSE);
>> + gtk_tree_model_get_iter(model, &iter, tree_path);
>> + gtk_tree_model_get(model, &iter,
>> + ISO_IS_ACTIVE, &active,
>> + ISO_NAME, &name, -1);
>> +
>> + gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, FALSE);
>> + gtk_widget_set_sensitive(priv->tree_view, FALSE);
>> +
>> + ovirt_foreign_menu_set_current_iso_name_async(priv->foreign_menu, active ? NULL : name, NULL,
>> + (GAsyncReadyCallback)ovirt_foreign_menu_iso_name_changed,
>> + self);
>> + gtk_tree_path_free(tree_path);
>> + g_free(name);
>> +}
>> +
>> +void
>> +remote_viewer_iso_list_dialog_row_activated(GtkTreeView *view G_GNUC_UNUSED,
>> + GtkTreePath *path,
>> + GtkTreeViewColumn *col G_GNUC_UNUSED,
>> + gpointer user_data)
>> +{
>> + gchar *path_str = gtk_tree_path_to_string(path);
>> + remote_viewer_iso_list_dialog_toggled(NULL, path_str, user_data);
>> + g_free(path_str);
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self)
>> +{
>> + GtkWidget *content = gtk_dialog_get_content_area(GTK_DIALOG(self));
>> + RemoteViewerISOListDialogPrivate *priv = self->priv = DIALOG_PRIVATE(self);
>> + GtkBuilder *builder = virt_viewer_util_load_ui("remote-viewer-iso-list.ui");
>> + GtkCellRendererToggle *cell_renderer;
>> +
>> + gtk_builder_connect_signals(builder, self);
>> +
>> + priv->status = GTK_WIDGET(gtk_builder_get_object(builder, "status"));
>> + priv->spinner = GTK_WIDGET(gtk_builder_get_object(builder, "spinner"));
>> + priv->stack = GTK_WIDGET(gtk_builder_get_object(builder, "stack"));
>> + gtk_box_pack_start(GTK_BOX(content), priv->stack, TRUE, TRUE, 0);
>> +
>> + priv->list_store = GTK_LIST_STORE(gtk_builder_get_object(builder, "liststore"));
>> + priv->tree_view = GTK_WIDGET(gtk_builder_get_object(builder, "view"));
>> + cell_renderer = GTK_CELL_RENDERER_TOGGLE(gtk_builder_get_object(builder, "cellrenderertoggle"));
>> + gtk_cell_renderer_toggle_set_radio(cell_renderer, TRUE);
>> + gtk_cell_renderer_set_padding(GTK_CELL_RENDERER(cell_renderer), 6, 6);
>> +
>> + g_object_unref(builder);
>> +
>> + gtk_dialog_add_buttons(GTK_DIALOG(self),
>> + _("Refresh"), GTK_RESPONSE_NONE,
>> + _("Close"), GTK_RESPONSE_CLOSE,
>> + NULL);
>> +
>> + gtk_dialog_set_default_response(GTK_DIALOG(self), GTK_RESPONSE_CLOSE);
>> + gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, FALSE);
>> + g_signal_connect(self, "response", G_CALLBACK(remote_viewer_iso_list_dialog_response), NULL);
>> +}
>> +
>> +static void
>> +remote_viewer_iso_list_dialog_show_error(RemoteViewerISOListDialog *self,
>> + const gchar *message)
>> +{
>> + GtkWidget *dialog;
>> +
>> + g_warn_if_fail(message != NULL);
>> +
>> + dialog = gtk_message_dialog_new(GTK_WINDOW(self),
>> + GTK_DIALOG_DESTROY_WITH_PARENT,
>> + GTK_MESSAGE_ERROR,
>> + GTK_BUTTONS_CLOSE,
>> + message ? message : _("Unspecified error"));
>> + gtk_dialog_run(GTK_DIALOG(dialog));
>> + gtk_widget_destroy(dialog);
>> +}
>> +
>> +static void
>> +ovirt_foreign_menu_iso_name_changed(OvirtForeignMenu *foreign_menu,
>> + GAsyncResult *result,
>> + RemoteViewerISOListDialog *self)
>> +{
>> + RemoteViewerISOListDialogPrivate *priv = self->priv;
>> + GtkTreeModel *model = GTK_TREE_MODEL(priv->list_store);
>> + gchar *current_iso;
>> + GtkTreeIter iter;
>> + gchar *name;
>> + gboolean active, match = FALSE;
>> + GError *error = NULL;
>> +
>> + /* In the case of error, don't return early, because it is necessary to
>> + * change the ISO back to the original, avoiding a possible inconsistency.
>> + */
>> + if (!ovirt_foreign_menu_set_current_iso_name_finish(foreign_menu, result, &error)) {
>> + remote_viewer_iso_list_dialog_show_error(self, error ? error->message : _("Failed to change CD"));
>> + g_clear_error(&error);
>> + }
>> +
>> + if (!gtk_tree_model_get_iter_first(model, &iter))
>> + return;
>> +
>> + current_iso = ovirt_foreign_menu_get_current_iso_name(foreign_menu);
>> +
>> + do {
>> + gtk_tree_model_get(model, &iter,
>> + ISO_IS_ACTIVE, &active,
>> + ISO_NAME, &name, -1);
>> + match = g_strcmp0(current_iso, name) == 0;
>
>
> match = (g_strcmp0(current_iso, name) == 0);
>
Also fixed.
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etrunko at redhat.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20170123/b27b8512/attachment.sig>
More information about the virt-tools-list
mailing list