[virt-tools-list] [PATCH virt-viewer 6/9] iso-dialog: Implement functionality provided by oVirt foreign menu

[Christophe Fergeau]

On Thu, Jan 19, 2017 at 12:44:15PM -0200, Eduardo Lima (Etrunko) wrote:
> 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.

Personally, the XML file is just a big opaque thing, so it could be
10.000 lines, or 5 lines, it's the same to me during review, I skip it
:) So maybe having all the changes together would work (once again, I
haven't looked at a diff with all the C changes together).

> >> +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.

Oops, I wanted to add that comment to ovirt_foreign_menu_iso_name_changed(), not to that function :(
So ovirt_foreign_menu_iso_name_changed() and
remote_viewer_iso_list_dialog_foreach() are vaguely similar.


Christophe
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/virt-tools-list/attachments/20170119/24bf4152/attachment.sig>