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