[virt-tools-list] [PATCH v5 1/3] Port to GtkApplication API's

Jonathon Jongsma jjongsma at redhat.com
Wed Feb 17 15:22:06 UTC 2016


On Tue, 2016-02-16 at 17:17 +0100, Pavel Grunt wrote:
> On Tue, 2016-02-16 at 13:21 -0200, Eduardo Lima (Etrunko) wrote:
> > On 02/16/2016 10:41 AM, Pavel Grunt wrote:
> > > Hi,
> > > 
> > > On Mon, 2016-02-15 at 19:22 -0200, Eduardo Lima (Etrunko) wrote:
> > > > Most of this patch consists in code being shuffled around to fit
> > > > the
> > > > expected flow while using the new APIs. I tried my best to make
> > > > this
> > > > patch the less intrusive as possible. Main changes are:
> > > > 
> > > > - Updated build requirements
> > > >    * glib version 2.38
> > > >    * gtk+ version 3.10
> > > >    * gio
> > > 
> > > For what we need gio ?
> > 
> > GApplication code lives in gio. Although Gtk+ should already be
> > pulling
> > it anyway, I thought it should be better to have it explicit.
> 
> Sure it is better to have it explicit
> 
> > 
> > > > diff --git a/src/remote-viewer-main.c b/src/remote-viewer-main.c
> > > > index 81cf736..b05f27b 100644
> > > > --- a/src/remote-viewer-main.c
> > > > +++ b/src/remote-viewer-main.c
> > > > @@ -22,184 +22,29 @@
> > > >  
> > > >  #include <config.h>
> > > >  #include <locale.h>
> > > > +#include <gio/gio.h>
> > > >  #include <gtk/gtk.h>
> > > >  #include <glib/gi18n.h>
> > > >  #include <stdlib.h>
> > > > -#ifdef G_OS_WIN32
> > > > -#include <windows.h>
> > > > -#include <io.h>
> > > > -#endif
> > > > -
> > > > -#ifdef HAVE_GTK_VNC
> > > > -#include <vncdisplay.h>
> > > > -#endif
> > > > -#ifdef HAVE_SPICE_GTK
> > > > -#include <spice-option.h>
> > > > -#endif
> > > > -#ifdef HAVE_OVIRT
> > > > -#include <govirt/ovirt-options.h>
> > > > -#endif
> > > >  
> > > >  #include "remote-viewer.h"
> > > > -#include "virt-viewer-app.h"
> > > > -#include "virt-viewer-session.h"
> > > > -
> > > > -static void
> > > > -remote_viewer_version(void)
> > > > -{
> > > > -    g_print(_("remote-viewer version %s"), VERSION BUILDID);
> > > > -#ifdef REMOTE_VIEWER_OS_ID
> > > > -    g_print(" (OS ID: %s)", REMOTE_VIEWER_OS_ID);
> > > > -#endif
> > > > -    g_print("\n");
> > > > -    exit(EXIT_SUCCESS);
> > > > -}
> > > > -
> > > > -static void
> > > > -recent_add(gchar *uri, const gchar *mime_type)
> > > > -{
> > > > -    GtkRecentManager *recent;
> > > > -    GtkRecentData meta = {
> > > > -        .app_name     = (char*)"remote-viewer",
> > > > -        .app_exec     = (char*)"remote-viewer %u",
> > > > -        .mime_type    = (char*)mime_type,
> > > > -    };
> > > > -
> > > > -    if (uri == NULL)
> > > > -        return;
> > > > -
> > > > -    recent = gtk_recent_manager_get_default();
> > > > -    meta.display_name = uri;
> > > > -    if (!gtk_recent_manager_add_full(recent, uri, &meta))
> > > > -        g_warning("Recent item couldn't be added");
> > > > -}
> > > > -
> > > > -static void connected(VirtViewerSession *session,
> > > > -                      VirtViewerApp *self G_GNUC_UNUSED)
> > > > -{
> > > > -    gchar *uri = virt_viewer_session_get_uri(session);
> > > > -    const gchar *mime = virt_viewer_session_mime_type(session);
> > > > -
> > > > -    recent_add(uri, mime);
> > > > -    g_free(uri);
> > > > -}
> > > >  
> > > >  int
> > > >  main(int argc, char **argv)
> > > >  {
> > > 
> > > > -    GOptionContext *context;
> > > > -    GError *error = NULL;
> > > >      int ret = 1;
> > > > -    gchar **args = NULL;
> > > > -    gchar *uri = NULL;
> > > > -    char *title = NULL;
> > > >      RemoteViewer *viewer = NULL;
> > > > -#ifdef HAVE_SPICE_GTK
> > > > -    gboolean controller = FALSE;
> > > > -#endif
> > > > -    VirtViewerApp *app;
> > > > -    const GOptionEntry options [] = {
> > > > -        { "version", 'V', G_OPTION_FLAG_NO_ARG,
> > > > G_OPTION_ARG_CALLBACK,
> > > > -          remote_viewer_version, N_("Display version
> > > > information"),
> > > > NULL },
> > > > -        { "title", 't', 0, G_OPTION_ARG_STRING, &title,
> > > > -          N_("Set window title"), NULL },
> > > > -#ifdef HAVE_SPICE_GTK
> > > > -        { "spice-controller", '\0', 0, G_OPTION_ARG_NONE,
> > > > &controller,
> > > > -          N_("Open connection using Spice controller
> > > > communication"), NULL },
> > > > -#endif
> > > > -        { G_OPTION_REMAINING, '\0', 0,
> > > > G_OPTION_ARG_STRING_ARRAY,
> > > > &args,
> > > > -          NULL, "URI|VV-FILE" },
> > > > -        { NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL }
> > > > -    };
> > > > -    GOptionGroup *app_options = NULL;
> > > >  
> > > >      virt_viewer_util_init(_("Remote Viewer"));
> > > >  
> > > > -    /* Setup command line options */
> > > > -    context = g_option_context_new (NULL);
> > > > -    g_option_context_set_summary(context, _("Remote viewer
> > > > client"));
> > > > -    app_options = virt_viewer_app_get_option_group();
> > > > -    g_option_group_add_entries (app_options, options);
> > > > -    g_option_context_set_main_group (context, app_options);
> > > > -    g_option_context_add_group (context, gtk_get_option_group
> > > > (TRUE));
> > > > -#ifdef HAVE_GTK_VNC
> > > > -    g_option_context_add_group (context,
> > > > vnc_display_get_option_group ());
> > > > -#endif
> > > > -#ifdef HAVE_SPICE_GTK
> > > > -    g_option_context_add_group (context, spice_get_option_group
> > > > ());
> > > > -#endif
> > > > -#ifdef HAVE_OVIRT
> > > > -    g_option_context_add_group (context, ovirt_get_option_group
> > > > ());
> > > > -#endif
> > > > -    g_option_context_parse (context, &argc, &argv, &error);
> > > > -    if (error) {
> > > > -        char *base_name;
> > > > -        base_name = g_path_get_basename(argv[0]);
> > > > -        g_printerr(_("%s\nRun '%s --help' to see a full list of
> > > > available command line options\n"),
> > > > -                   error->message, base_name);
> > > > -        g_free(base_name);
> > > > -        goto cleanup;
> > > > -    }
> > > > -
> > > > -    g_option_context_free(context);
> > > > -
> > > > -#ifdef HAVE_SPICE_GTK
> > > > -    if (controller) {
> > > > -        if (args) {
> > > > -            g_printerr(_("Error: extra arguments given while
> > > > using
> > > > Spice controller\n"));
> > > > -            goto cleanup;
> > > > -        }
> > > > -    } else
> > > > -#endif
> > > > -    if (args) {
> > > > -        if (g_strv_length(args) > 1) {
> > > > -            g_printerr(_("Error: can't handle multiple
> > > > URIs\n"));
> > > > -            goto cleanup;
> > > > -        } else if (g_strv_length(args) == 1) {
> > > > -            uri = g_strdup(args[0]);
> > > > -        }
> > > > -    }
> > > > -
> > > > -#ifdef HAVE_SPICE_GTK
> > > > -    if (controller) {
> > > > -        viewer = remote_viewer_new_with_controller();
> > > > -        g_object_set(viewer, "guest-name", "defined by Spice
> > > > controller", NULL);
> > > > -    } else {
> > > > -#endif
> > > > -        viewer = remote_viewer_new(uri);
> > > > -        if (title)
> > > > -            g_object_set(viewer, "title", title, NULL);
> > > > -#ifdef HAVE_SPICE_GTK
> > > > -    }
> > > > -#endif
> > > > +    viewer = remote_viewer_new();
> > > >      if (viewer == NULL)
> > > 
> > > change the condition to viewer != NULL
> > > 
> > > > -        goto cleanup;
> > > > -
> > > > -    app = VIRT_VIEWER_APP(viewer);
> > > > -
> > > > -    if (!virt_viewer_app_start(app, &error)) {
> > > > -        if (g_error_matches(error, VIRT_VIEWER_ERROR,
> > > > VIRT_VIEWER_ERROR_CANCELLED))
> > > > -            ret = 0;
> > > > -        else if (error) {
> > > > -            virt_viewer_app_simple_message_dialog(app, error-
> > > > > message);
> > > > -        }
> > > > -        goto cleanup;
> > > > -    }
> > > > -
> > > > -    g_signal_connect(virt_viewer_app_get_session(app), "session-
> > > > connected",
> > > > -                     G_CALLBACK(connected), app);
> > > > -
> > > > -    gtk_main();
> > > > -
> > > > -    ret = 0;
> > > > +        goto end;
> > > >  
> > > > - cleanup:
> > > > -    g_free(uri);
> > > > -    if (viewer)
> > > > -        g_object_unref(viewer);
> > > > -    g_strfreev(args);
> > > > -    g_clear_error(&error);
> > > > +    ret = g_application_run(G_APPLICATION(viewer), argc, argv);
> > > > +    g_object_unref(viewer);
> > > 
> > > and put these into the block under it and you don't need goto.
> > > (same for virt-viewer-main.c)
> > > 
> > 
> > 
> > Well, I think this is somewhat a convention to test for errors early
> > and
> > use goto if it is the case, but in this case the function is simple
> > enough to ditch it. I am really in favor of keeping the goto, though.
> 
> The function is really simple, it just "starts" the GApplication
> 
> > 
> > 
> > > >  
> > > > diff --git a/src/remote-viewer.c b/src/remote-viewer.c
> > > > index e712d61..2703a24 100644
> > > > --- a/src/remote-viewer.c
> > > > +++ b/src/remote-viewer.c
> > > > @@ -23,6 +23,7 @@
> > > >   */
> > > >  
> > > >  #include <config.h>
> > > > +#include <gio/gio.h>
> > > >  #include <gtk/gtk.h>
> > > >  #include <glib/gprintf.h>
> > > >  #include <glib/gi18n.h>
> > > > @@ -30,6 +31,7 @@
> > > >  
> > > >  #ifdef HAVE_OVIRT
> > > >  #include <govirt/govirt.h>
> > > > +#include <govirt/ovirt-options.h>
> > > 
> > > this header is included in govirt/govirt.h,
> > > 
> > > although there is no warning (like with spice-gtk), there is no
> > > need to
> > > include it directly.
> > 
> > Ok, I will remove it.
> > 
> > > 
> > > >  #include "ovirt-foreign-menu.h"
> > > >  #include "virt-viewer-vm-connection.h"
> > > >  #endif
> > > > @@ -84,8 +86,9 @@ static OvirtVm * choose_vm(GtkWindow
> > > > *main_window,
> > > >  static gboolean remote_viewer_start(VirtViewerApp *self, GError
> > > > **error);
> > > >  #ifdef HAVE_SPICE_GTK
> > > >  static gboolean remote_viewer_activate(VirtViewerApp *self,
> > > > GError
> > > > **error);
> > > > -static void remote_viewer_window_added(VirtViewerApp *self,
> > > > VirtViewerWindow *win);
> > > > +static void remote_viewer_window_added(GtkApplication *app,
> > > > GtkWindow *w);
> > > >  static void spice_foreign_menu_updated(RemoteViewer *self);
> > > > +static void foreign_menu_title_changed(SpiceCtrlForeignMenu
> > > > *menu,
> > > > GParamSpec *pspec, RemoteViewer *self);
> > > >  #endif
> > > >  
> > > >  static void
> > > > @@ -183,11 +186,128 @@ remote_viewer_deactivated(VirtViewerApp
> > > > *app,
> > > > gboolean connect_error)
> > > >      VIRT_VIEWER_APP_CLASS(remote_viewer_parent_class)-
> > > > > deactivated(app, connect_error);
> > > >  }
> > > >  
> > > > +static gchar **opt_args = NULL;
> > > > +static char *opt_title = NULL;
> > > > +static gboolean opt_controller = FALSE;
> > > > +
> > > > +static gboolean
> > > > +remote_viewer_version (G_GNUC_UNUSED const gchar *option_name,
> > > > +                       G_GNUC_UNUSED const gchar *value,
> > > > +                       G_GNUC_UNUSED gpointer data,
> > > > +                       GError **error)
> > > > +{
> > > > +
> > > > +    g_print(_("%s version %s"), g_get_prgname(), VERSION
> > > > BUILDID);
> > > > +#ifdef REMOTE_VIEWER_OS_ID
> > > > +    g_print(" (OS ID: %s)", REMOTE_VIEWER_OS_ID);
> > > > +#endif
> > > > +    g_print("\n");
> > > > +    g_set_error(error, VIRT_VIEWER_ERROR, VIRT_VIEWER_VERSION,
> > > > +                _("%s version %s"), g_get_prgname(), VERSION
> > > > BUILDID);
> > > > +    return FALSE;
> > > > +}
> > > > +
> > > > +static void
> > > > +remote_viewer_add_option_entries(VirtViewerApp *self,
> > > > GOptionContext
> > > > *context, GOptionGroup *group)
> > > > +{
> > > > +    static const GOptionEntry options[] = {
> > > > +        { "version", 'V', G_OPTION_FLAG_NO_ARG,
> > > > G_OPTION_ARG_CALLBACK,
> > > > +          remote_viewer_version, N_("Display version
> > > > information"),
> > > > NULL },
> > > > +        { "title", 't', 0, G_OPTION_ARG_STRING, &opt_title,
> > > > +          N_("Set window title"), NULL },
> > > > +#ifdef HAVE_SPICE_GTK
> > > > +        { "spice-controller", '\0', 0, G_OPTION_ARG_NONE,
> > > > &opt_controller,
> > > > +          N_("Open connection using Spice controller
> > > > communication"), NULL },
> > > > +#endif
> > > > +        { G_OPTION_REMAINING, '\0', 0,
> > > > G_OPTION_ARG_STRING_ARRAY,
> > > > &opt_args,
> > > > +          NULL, "URI|VV-FILE" },
> > > > +        { NULL, 0, 0, G_OPTION_ARG_NONE, NULL, NULL, NULL }
> > > > +    };
> > > > +
> > > > +    VIRT_VIEWER_APP_CLASS(remote_viewer_parent_class)-
> > > > > add_option_entries(self, context, group);
> > > > +    g_option_context_set_summary(context, _("Remote viewer
> > > > client"));
> > > > +    g_option_group_add_entries(group, options);
> > > > +
> > > > +#ifdef HAVE_OVIRT
> > > > +    g_option_context_add_group (context, ovirt_get_option_group
> > > > ());
> > > > +#endif
> > > > +}
> > > > +
> > > > +static gboolean
> > > > +remote_viewer_local_command_line (GApplication   *gapp,
> > > > +                                  gchar        ***args,
> > > > +                                  int            *status)
> > > > +{
> > > > +#define GOTO_END \
> > > > +    do { \
> > > > +        ret = TRUE; \
> > > > +        *status = 1; \
> > > > +        goto end; \
> > > > +       } while (FALSE)
> > > > +
> > > I don't think this macro improves readability, I would no use it.
> > > (same for virt_viewer_local_command_line)
> > > 
> > 
> > I disagree, but again, I think it is basically a matter of
> > preference.
> 
> It is. From the macro name it is not clear for me how is different
> "GOTO_END" from "goto end"

For what it's worth, I agree with Pavel here. I think it actually obscures
things instead of making them clearer. 


> 
> > Looking at it again, I could improve this macro a bit more, by moving
> > the g_printerr() call inside and receive the message as a parameter:
> 
> any improvements welcome
> 
> Pavel
> 
> > 
> > +#define ERROR_GOTO_END(x, ...) \
> > +    do { \
> > +        g_printerr(x, ## __VA_ARGS__); \
> > +        ret = TRUE; \
> > +        *status = 1; \
> > +        goto end; \
> > +       } while (FALSE)
> > +
> > 
> > Regards, Eduardo.
> > 
> 
> _______________________________________________
> virt-tools-list mailing list
> virt-tools-list at redhat.com
> https://www.redhat.com/mailman/listinfo/virt-tools-list




More information about the virt-tools-list mailing list