[virt-tools-list] [virt-viewer] Fix --without-spice-gtk --with-ovirt build

[Jonathon Jongsma]

On Tue, 2014-09-09 at 11:57 +0200, Christophe Fergeau wrote:
> The oVirt foreign menu support reused some existing bits from the older
> SPICE controller foreign menu code. However, this controller code is
> only built when spice-gtk support is built, while the oVirt foreign menu
> code could be used with VNC as well. Trying to build the ovirt foreign
> menu code without spice-gtk causes build issues due to missing
> functions, or missing declarations, ...
> 
> The libgovirt/spice-gtk code which is entangled is the code to update
> the foreign menu when its content changes, or when a new window is
> opened. Making the oVirt-specific code independant from the
> spice-gtk-specific code is not too complicated, but this comes at the
> expense of a bit of code duplication, but this is only simple code
> iterating over the GHashTable storing the opened windows.
> ---
>  src/remote-viewer.c | 83 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 47 insertions(+), 36 deletions(-)
> 
> diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> index 5f5fa3d..a67f9f8 100644
> --- a/src/remote-viewer.c
> +++ b/src/remote-viewer.c
> @@ -422,34 +422,6 @@ 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(RemoteViewer *app, VirtViewerWindow *win)
> -{
> -    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 (app->priv->ovirt_foreign_menu == NULL) {
> -        /* nothing to do */
> -        return;
> -    }
> -    if (menu == NULL) {
> -        menu = gtk_menu_item_new_with_label(_("_Change CD"));
> -        gtk_menu_item_set_use_underline(GTK_MENU_ITEM(menu), TRUE);
> -        gtk_menu_shell_append(shell, menu);
> -        g_object_set_data_full(G_OBJECT(win), "foreign-menu",
> -                               g_object_ref(menu),
> -                               (GDestroyNotify)gtk_widget_destroy);
> -    }
> -
> -    submenu = ovirt_foreign_menu_get_gtk_menu(app->priv->ovirt_foreign_menu);
> -    gtk_menu_item_set_submenu(GTK_MENU_ITEM(menu), submenu);
> -
> -    gtk_widget_show_all(menu);
> -}
> -#endif
> -
>  static void
>  foreign_menu_update(RemoteViewer *self, VirtViewerWindow *win)

we now have 2 different 'foreign menu' implementations. Before this
change, this function updated both of these implementations. But after
this patch, it only updates one of them, so it no longer seems
appropriate to maintain the generic name foreign_menu_update(). Can we
rename these functions so that it's explicitly clear which
implementation they refer to:

foreign_menu_update()
foreign_menu_update_each()


>  {
> @@ -487,10 +459,6 @@ foreign_menu_update_each(gpointer key G_GNUC_UNUSED,
>                           gpointer user_data)
>  {
>      foreign_menu_update(REMOTE_VIEWER(user_data), VIRT_VIEWER_WINDOW(value));
> -#ifdef HAVE_OVIRT
> -    ovirt_foreign_menu_update(REMOTE_VIEWER(user_data),
> -                              VIRT_VIEWER_WINDOW(value));
> -#endif
>  }
>  
>  static void
> @@ -662,9 +630,6 @@ remote_viewer_window_added(VirtViewerApp *app,
>  {
>      spice_menu_update(REMOTE_VIEWER(app), win);
>      foreign_menu_update(REMOTE_VIEWER(app), win);
> -#ifdef HAVE_OVIRT
> -    ovirt_foreign_menu_update(REMOTE_VIEWER(app), win);
> -#endif
>  }
>  #endif
>  
> @@ -757,13 +722,57 @@ authenticate_cb(RestProxy *proxy, G_GNUC_UNUSED RestProxyAuth *auth,
>      return success;
>  }
>  
> +static void
> +ovirt_foreign_menu_update(RemoteViewer *app, VirtViewerWindow *win)
> +{
> +    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 (app->priv->ovirt_foreign_menu == NULL) {
> +        /* nothing to do */
> +        return;
> +    }
> +    if (menu == NULL) {
> +        menu = gtk_menu_item_new_with_label(_("_Change CD"));
> +        gtk_menu_item_set_use_underline(GTK_MENU_ITEM(menu), TRUE);
> +        gtk_menu_shell_append(shell, menu);
> +        g_object_set_data_full(G_OBJECT(win), "foreign-menu",
> +                               g_object_ref(menu),
> +                               (GDestroyNotify)gtk_widget_destroy);
> +    }
> +
> +    submenu = ovirt_foreign_menu_get_gtk_menu(app->priv->ovirt_foreign_menu);
> +    gtk_menu_item_set_submenu(GTK_MENU_ITEM(menu), submenu);
> +
> +    gtk_widget_show_all(menu);
> +}
> +
> +static void
> +ovirt_foreign_menu_update_each(gpointer key G_GNUC_UNUSED,
> +                               gpointer value,
> +                               gpointer user_data)
> +{
> +    ovirt_foreign_menu_update(REMOTE_VIEWER(user_data),
> +                              VIRT_VIEWER_WINDOW(value));
> +}
> +
> +static void
> +ovirt_foreign_menu_updated(RemoteViewer *self)
> +{
> +    GHashTable *windows = virt_viewer_app_get_windows(VIRT_VIEWER_APP(self));
> +
> +    g_debug("Spice foreign menu updated");
> +
> +    g_hash_table_foreach(windows, ovirt_foreign_menu_update_each, self);
> +}
>  
>  static void
>  ovirt_foreign_menu_changed(OvirtForeignMenu *foreign_menu G_GNUC_UNUSED,
>                             GParamSpec *pspec G_GNUC_UNUSED,
>                             VirtViewerApp *app)
>  {
> -    spice_foreign_menu_updated(REMOTE_VIEWER(app));
> +    ovirt_foreign_menu_updated(REMOTE_VIEWER(app));
>  }
>  
> 
> @@ -784,6 +793,8 @@ virt_viewer_app_set_ovirt_foreign_menu(VirtViewerApp *app,
>                       (GCallback)ovirt_foreign_menu_changed, app);
>      g_signal_connect(G_OBJECT(foreign_menu), "notify::files",
>                       (GCallback)ovirt_foreign_menu_changed, app);
> +    g_signal_connect(G_OBJECT(app), "window-added",
> +                     (GCallback)ovirt_foreign_menu_update, NULL);
>      ovirt_foreign_menu_start(foreign_menu);
>  }
>  


It appears that you will leak priv->ovirt_foreign_menu in the case where
we're building with spice-gtk disabled, because the dispose vfunc is
only registered within an #ifdef HAVE_SPICE_GTK block.

ACK with those two changes.