[virt-tools-list] [virt-viewer 3/7] ovirt: Use OvirtForeignMenu class

[Christophe Fergeau]

----- Mail original -----
> 
> 
> ----- Original Message -----
> > From: "Christophe Fergeau" <cfergeau at redhat.com>
> > To: virt-tools-list at redhat.com
> > Sent: Wednesday, April 16, 2014 11:59:50 AM
> > Subject: [virt-tools-list] [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