[virt-tools-list] [PATCH virt-viewer 6/9] iso-dialog: Implement functionality provided by oVirt foreign menu
Eduardo Lima (Etrunko)
etrunko at redhat.com
Thu Jan 19 14:44:15 UTC 2017
On 19/01/17 10:48, Christophe Fergeau wrote:
> The issues pointed out in
> https://www.redhat.com/archives/virt-tools-list/2017-January/msg00039.html
> do not seem to have been addressed?
>
True, I did overlook that message, thanks for the reminder. I changed
the code so the spinner stops and set the reload button sensitive. This
way the user can try refreshing the list again in case of error. Also it
was not necessary to get the iso_list twice, as you pointed.
> Did you try merging the patches from "Introduce ISO List dialog" to "Run
> iso-dialog when 'Change CD' menu is activated" (without the headerbar
> change)? I think they would make more sense together, did not check if
> that would make the diff far too big or not.
>
I split those two because I thought the xml would be big enough for
itself, but I can merge the patches without problems.
> Christophe
>
> On Wed, Jan 18, 2017 at 12:16:57PM -0200, Eduardo Lima (Etrunko) wrote:
>> Signed-off-by: Eduardo Lima (Etrunko) <etrunko at redhat.com>
>> ---
>> src/remote-viewer-iso-list-dialog.c | 204 ++++++++++++++++++++++++++++-
>> src/resources/ui/remote-viewer-iso-list.ui | 5 +-
>> 2 files changed, 205 insertions(+), 4 deletions(-)
>>
>> diff --git a/src/remote-viewer-iso-list-dialog.c b/src/remote-viewer-iso-list-dialog.c
>> index 858719c..ef60854 100644
>> --- a/src/remote-viewer-iso-list-dialog.c
>> +++ b/src/remote-viewer-iso-list-dialog.c
>> @@ -26,6 +26,9 @@
>> #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, gchar *message);
>> +
>> G_DEFINE_TYPE(RemoteViewerISOListDialog, remote_viewer_iso_list_dialog, GTK_TYPE_DIALOG)
>>
>> #define DIALOG_PRIVATE(o) \
>> @@ -33,17 +36,32 @@ G_DEFINE_TYPE(RemoteViewerISOListDialog, remote_viewer_iso_list_dialog, GTK_TYPE
>>
>> struct _RemoteViewerISOListDialogPrivate
>> {
>> + GtkListStore *list_store;
>> GtkWidget *stack;
>> + GtkWidget *tree_view;
>> OvirtForeignMenu *foreign_menu;
>> };
>>
>> +enum RemoteViewerISOListDialogModel
>> +{
>> + ISO_IS_ACTIVE = 0,
>> + ISO_NAME,
>> + FONT_WEIGHT,
>> +};
>> +
>> +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;
>>
>> - g_clear_object(&priv->foreign_menu);
>> + 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);
>> }
>>
>> @@ -58,6 +76,74 @@ remote_viewer_iso_list_dialog_class_init(RemoteViewerISOListDialogClass *klass)
>> }
>>
>> static void
>> +remote_viewer_iso_list_dialog_show_files(RemoteViewerISOListDialog *self)
>> +{
>> + RemoteViewerISOListDialogPrivate *priv = self->priv = DIALOG_PRIVATE(self);
>> + gtk_stack_set_visible_child_full(GTK_STACK(priv->stack), "iso-list",
>> + GTK_STACK_TRANSITION_TYPE_SLIDE_LEFT);
>
> I don't think I'd use animations here, I'd just switch from spinner to
> iso list, I find the animation distracting.
I liked them, but anyway, using GTK_STACK_TRANSITION_TYPE_NONE from now on.
>
>> + 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;
>> + 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) {
>> + remote_viewer_iso_list_dialog_show_error(self, error ? error->message : _("Failed to fetch CD names"));
>> + g_clear_error(&error);
>> + return;
>> + }
>> +
>> + iso_list = ovirt_foreign_menu_get_iso_names(priv->foreign_menu);
>> + 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)
>> @@ -71,7 +157,47 @@ remote_viewer_iso_list_dialog_response(GtkDialog *dialog,
>> gtk_stack_set_visible_child_full(GTK_STACK(priv->stack), "status",
>> GTK_STACK_TRANSITION_TYPE_SLIDE_LEFT);
>> gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, FALSE);
>> - gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_CLOSE, 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);
>
>
> That is quite similar to remote_viewer_iso_list_dialog_foreach(),
> dunno if part of the code could be shared?
Hmm, not quite, here we get the ISO name the user clicked to be
set/unset, while the code there is used to populate the list, and if
there is one particular ISO set, that one is highlighted.
>
>> +}
>> +
>> +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
>> @@ -80,12 +206,19 @@ 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->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),
>> @@ -95,10 +228,74 @@ remote_viewer_iso_list_dialog_init(RemoteViewerISOListDialog *self)
>>
>> gtk_dialog_set_default_response(GTK_DIALOG(self), GTK_RESPONSE_CLOSE);
>> gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, FALSE);
>> - gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_CLOSE, 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,
>> + 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;
>> +
>> + 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);
>> + }
>
> I think it's intentional that we don't return early on failure so that
> we reselect the correct ISO, maybe it would be worth a comment?
Yes, that's the idea, I added the comment.
>
>> +
>> + 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;
>> +
>> + /* iso is not active anymore */
>> + if (active && !match) {
>> + gtk_list_store_set(priv->list_store, &iter,
>> + ISO_IS_ACTIVE, FALSE,
>> + FONT_WEIGHT, PANGO_WEIGHT_NORMAL, -1);
>> + } else if (match) {
>> + gtk_list_store_set(priv->list_store, &iter,
>> + ISO_IS_ACTIVE, TRUE,
>> + FONT_WEIGHT, PANGO_WEIGHT_BOLD, -1);
>> + }
>> +
>> + g_free(name);
>> + } while (gtk_tree_model_iter_next(model, &iter));
>> +
>> + gtk_dialog_set_response_sensitive(GTK_DIALOG(self), GTK_RESPONSE_NONE, TRUE);
>> + gtk_widget_set_sensitive(priv->tree_view, TRUE);
>> + g_free(current_iso);
>> +}
>> +
>> GtkWidget *
>> remote_viewer_iso_list_dialog_new(GtkWindow *parent, GObject *foreign_menu)
>> {
>> @@ -117,5 +314,6 @@ remote_viewer_iso_list_dialog_new(GtkWindow *parent, GObject *foreign_menu)
>>
>> self = REMOTE_VIEWER_ISO_LIST_DIALOG(dialog);
>> self->priv->foreign_menu = OVIRT_FOREIGN_MENU(g_object_ref(foreign_menu));
>> + remote_viewer_iso_list_dialog_refresh_iso_list(self);
>> return dialog;
>> }
>> diff --git a/src/resources/ui/remote-viewer-iso-list.ui b/src/resources/ui/remote-viewer-iso-list.ui
>> index 624791f..ab1bdc4 100644
>> --- a/src/resources/ui/remote-viewer-iso-list.ui
>> +++ b/src/resources/ui/remote-viewer-iso-list.ui
>> @@ -106,6 +106,7 @@
>> <property name="rules_hint">True</property>
>> <property name="search_column">1</property>
>> <property name="enable_grid_lines">horizontal</property>
>> + <signal name="row-activated" handler="remote_viewer_iso_list_dialog_row_activated" swapped="no"/>
>> <child internal-child="selection">
>> <object class="GtkTreeSelection" id="treeview-selection"/>
>> </child>
>> @@ -114,7 +115,9 @@
>> <property name="sizing">autosize</property>
>> <property name="title" translatable="yes">Selected</property>
>> <child>
>> - <object class="GtkCellRendererToggle" id="cellrenderertoggle"/>
>> + <object class="GtkCellRendererToggle" id="cellrenderertoggle">
>> + <signal name="toggled" handler="remote_viewer_iso_list_dialog_toggled" swapped="no"/>
>> + </object>
>> <attributes>
>> <attribute name="active">0</attribute>
>> </attributes>
>> --
>> 2.9.3
>>
>> _______________________________________________
>> virt-tools-list mailing list
>> virt-tools-list at redhat.com
>> https://www.redhat.com/mailman/listinfo/virt-tools-list
--
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/20170119/776fd92d/attachment.sig>
More information about the virt-tools-list
mailing list