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. >> 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. >> >> 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. 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: +#define ERROR_GOTO_END(x, ...) \ + do { \ + g_printerr(x, ## __VA_ARGS__); \ + ret = TRUE; \ + *status = 1; \ + goto end; \ + } while (FALSE) + Regards, Eduardo. -- Eduardo de Barros Lima (Etrunko) Software Engineer - RedHat etrunko@xxxxxxxxxx _______________________________________________ virt-tools-list mailing list virt-tools-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/virt-tools-list