[virt-tools-list] [PATCH v5 1/3] Port to GtkApplication API's
Eduardo Lima (Etrunko)
etrunko at redhat.com
Tue Feb 16 15:21:33 UTC 2016
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 at redhat.com
More information about the virt-tools-list
mailing list