----- Mail original -----
>
>
> ----- Original Message -----
> > From: "Christophe Fergeau" <cfergeau@xxxxxxxxxx>
> > To: virt-tools-list@xxxxxxxxxx
> > Sent: Wednesday, April 16, 2014 11:59:50 AM
> > Subject: [virt-viewer 3/7] ovirt: Use OvirtForeignMenu
> > class
> >
> > After the previous commit which introduced the OvirtForeignMenu
> > class, we can now make use of it in the remote-viewer UI code.
> > ---
> > src/remote-viewer.c | 61
> > +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 61 insertions(+)
> >
> > diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> > index 4320369..fcd63fc 100644
> > --- a/src/remote-viewer.c
> > +++ b/src/remote-viewer.c
> > @@ -30,6 +30,7 @@
> >
> > #ifdef HAVE_OVIRT
> > #include <govirt/govirt.h>
> > +#include "ovirt-foreign-menu.h"
> > #endif
> >
> > #ifdef HAVE_SPICE_GTK
> > @@ -413,6 +414,50 @@ spice_ctrl_menu_updated(RemoteViewer *self)
> > g_hash_table_foreach(windows, spice_menu_update_each, self);
> > }
> >
> > +#ifdef HAVE_OVIRT
> > +static void
> > +ovirt_foreign_menu_update(VirtViewerWindow *win,
> > + OvirtForeignMenu *foreign_menu)
>
> Can this be renamed? It's nearly identical to
> spice_ovirt_foreign_menu_update() below. It'd be nice if the names gave an
> indication of what the difference was so that you didn't need to inspect the
> implementation. Maybe something like ovirt_foreign_menu_update_for_window()?
Yup, thanks for the name suggestion!
>
> > +{
> > + GtkWidget *menu = g_object_get_data(G_OBJECT(win), "foreign-menu");
> > + GtkWidget *submenu;
> > + GtkMenuShell *shell =
> > GTK_MENU_SHELL(gtk_builder_get_object(virt_viewer_window_get_builder(win),
> > "top-menu"));
> > +
> > + if (menu != NULL)
> > + gtk_widget_destroy(menu);
> > +
> > + menu = gtk_menu_item_new_with_label(_("CDRoms"));
>
> "CDRoms" looks a bit strange. In the user portal, there's a menu item called
> "Change CD". Should we use similar terminology?
>
I agree that CDRoms looks very weird, but using a verb in a toplevel menu is uncommon as well. But I don't have any great suggestions here :(
> > static gboolean
> > create_ovirt_session(VirtViewerApp *app, const char *uri)
> > {
> > @@ -791,6 +844,14 @@ create_ovirt_session(VirtViewerApp *app, const char
> > *uri)
> > goto error;
> > }
> >
> > + {
> > + OvirtForeignMenu *menu = ovirt_foreign_menu_new(proxy);
> > + g_object_set(G_OBJECT(menu), "api", api, "vm", vm, NULL);
> > + g_signal_connect(G_OBJECT(menu), "notify::files",
> > + (GCallback)ovirt_foreign_menu_changed, app);
>
> What about new windows that are created after the notify::files signal fires?
> As far as I can tell, they won't have a foreign menu unless the list of
> files changes.
I haven't tested multiple windows at all with this code, something else to test and fix ;)
Christophe
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
