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@xxxxxxxxxx> >> --- >> 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@xxxxxxxxxx >> https://www.redhat.com/mailman/listinfo/virt-tools-list -- Eduardo de Barros Lima (Etrunko) Software Engineer - RedHat etrunko@xxxxxxxxxx
Attachment:
signature.asc
Description: OpenPGP digital signature
_______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list
