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@xxxxxxxxxx
> https://www.redhat.com/mailman/listinfo/virt-tools-list
_______________________________________________
virt-tools-list mailing list
virt-tools-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/virt-tools-list
