Re: [virt-tools-list] [PATCH v3 3/5] Port to GtkApplication API's
On Mon, Feb 15, 2016 at 2:57 PM, Eduardo Lima (Etrunko)
wrote:
> On 02/15/2016 11:47 AM, Fabiano Fidêncio wrote:
> [snip]
>
>>>
>>> cleanup:
>>> -g_free(uri);
>>> -if (viewer)
>>> -g_object_unref(viewer);
>>> -g_strfreev(args);
>>> -g_clear_error(&error);
>>> -
>>> +g_object_unref(viewer);
>>
>> g_object_unref() shouldn't be called with a NULL argument. So, you'll
>> need to re-add the check for the viewer before unref'ing the object.
>>
>
> Fixed.
>
>>> +}
>>> +
>>> +g_clear_error(&error);
>>> +g_application_quit(app);
>>
>>
>> And then return here ... ?
>>
>
> I don't think it is necessary, g_application_quit() will end the
> execution of the program.
>
>>> -g_free(uri);
>>> -g_strfreev(args);
>>> -g_free(help_msg);
>>> -g_clear_error(&error);
>>> -
>>> +g_object_unref(viewer);
>>
>> As for remote-viewer, don't remove the check for the viewer before
>> calling g_object_unref().
>>
>
> Also fixed.
>
>>> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
>>> index 3a958f0..14549fd 100644
>>> --- a/src/virt-viewer-window.c
>>> +++ b/src/virt-viewer-window.c
>>> @@ -346,8 +346,7 @@ virt_viewer_window_init (VirtViewerWindow *self)
>>> gtk_window_set_has_resize_grip(GTK_WINDOW(priv->window), FALSE);
>>> priv->accel_enabled = TRUE;
>>>
>>> -accels = gtk_accel_groups_from_object(G_OBJECT(priv->window));
>>> -for ( ; accels ; accels = accels->next) {
>>> +for (accels = gtk_accel_groups_from_object(G_OBJECT(priv-
window)); accels; accels = accels->next) {
>>
>> This change is not related
>>
>
> Yes, I can merge it with previous cleanup patch.
I'd rather live it as it is.
>
>>
>>
>> Didn't have any problem running on Windows and on Linux.
>>
>> The code is in a way better shape than the previous versions. Just a
>> small set of minor comments from me. Also, please, this whole series is
>> breaking "make syntax-check" as pointed out in the #spice channel.
>> +1 for having the code in with the fixes suggested.
>>
>> Please, I also would like to have at least one more review from someone
>> used with the client side (Daniel? Jonathon? Pavel?) before you can
>> consider it as an ACK!
>
> Sure, will post a new version with fixes to comments. Thanks for the review.
Sure, please, rebase everything on git master and re-send and I'll
take a second look (and someone else also will take a look) :-)
>
> --
> Eduardo de Barros Lima (Etrunko)
> Software Engineer - RedHat
> etru...@redhat.com
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH v3 3/5] Port to GtkApplication API's
On 02/15/2016 11:47 AM, Fabiano Fidêncio wrote:
[snip]
>>
>> cleanup:
>> -g_free(uri);
>> -if (viewer)
>> -g_object_unref(viewer);
>> -g_strfreev(args);
>> -g_clear_error(&error);
>> -
>> +g_object_unref(viewer);
>
> g_object_unref() shouldn't be called with a NULL argument. So, you'll
> need to re-add the check for the viewer before unref'ing the object.
>
Fixed.
>> +}
>> +
>> +g_clear_error(&error);
>> +g_application_quit(app);
>
>
> And then return here ... ?
>
I don't think it is necessary, g_application_quit() will end the
execution of the program.
>> -g_free(uri);
>> -g_strfreev(args);
>> -g_free(help_msg);
>> -g_clear_error(&error);
>> -
>> +g_object_unref(viewer);
>
> As for remote-viewer, don't remove the check for the viewer before
> calling g_object_unref().
>
Also fixed.
>> diff --git a/src/virt-viewer-window.c b/src/virt-viewer-window.c
>> index 3a958f0..14549fd 100644
>> --- a/src/virt-viewer-window.c
>> +++ b/src/virt-viewer-window.c
>> @@ -346,8 +346,7 @@ virt_viewer_window_init (VirtViewerWindow *self)
>> gtk_window_set_has_resize_grip(GTK_WINDOW(priv->window), FALSE);
>> priv->accel_enabled = TRUE;
>>
>> -accels = gtk_accel_groups_from_object(G_OBJECT(priv->window));
>> -for ( ; accels ; accels = accels->next) {
>> +for (accels = gtk_accel_groups_from_object(G_OBJECT(priv-
>>> window)); accels; accels = accels->next) {
>
> This change is not related
>
Yes, I can merge it with previous cleanup patch.
>
>
> Didn't have any problem running on Windows and on Linux.
>
> The code is in a way better shape than the previous versions. Just a
> small set of minor comments from me. Also, please, this whole series is
> breaking "make syntax-check" as pointed out in the #spice channel.
> +1 for having the code in with the fixes suggested.
>
> Please, I also would like to have at least one more review from someone
> used with the client side (Daniel? Jonathon? Pavel?) before you can
> consider it as an ACK!
Sure, will post a new version with fixes to comments. Thanks for the review.
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH v3 3/5] Port to GtkApplication API's
On Fri, 2016-02-12 at 09:35 -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
>
> - VirtViewerApp is now a subclass of GtkApplication.
> Some mainloop calls were replaced:
> * gtk_main() -> g_application_run()
> * gtk_quit() -> g_application_quit()
>
> - Unified command line option handling.
> The logic has moved from the main functions and split in common
> options, and specific ones for each application. With this, the
> main
> functions were highly simplified, and now basically responsible for
> instantiating the App object and running the main loop.
>
> - All Window objects must be associated with the Application.
> With this, there is no need to emit our own 'window-added'/'window-
> removed' signals, as those will be emited by GtkApplication
> whenever
> gtk_application_add_window() and gtk_application_remove_window()
> are
> called. Also, 'window-removed' was not being used anywhere.
>
> Signed-off-by: Eduardo Lima (Etrunko)
> ---
> configure.ac | 6 +-
> src/remote-viewer-main.c | 163 +
> src/remote-viewer.c | 207
> +++
> src/remote-viewer.h | 4 +-
> src/virt-viewer-app.c| 144 -
> src/virt-viewer-app.h| 11 +--
> src/virt-viewer-main.c | 104 ++--
> src/virt-viewer-util.h | 1 +
> src/virt-viewer-window.c | 4 +-
> src/virt-viewer.c| 137 +--
> src/virt-viewer.h| 9 +--
> src/virt-viewer.xml | 2 +-
> 12 files changed, 394 insertions(+), 398 deletions(-)
>
> diff --git a/configure.ac b/configure.ac
> index 250a7fe..e09d0cb 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -12,10 +12,10 @@ AC_CANONICAL_HOST
> m4_ifndef([AM_SILENT_RULES], [m4_define([AM_SILENT_RULES],[])])
> AM_SILENT_RULES([yes])
>
> -GLIB2_REQUIRED=2.22.0
> +GLIB2_REQUIRED="2.38.0"
> LIBXML2_REQUIRED="2.6.0"
> LIBVIRT_REQUIRED="0.10.0"
> -GTK3_REQUIRED="3.0"
> +GTK3_REQUIRED="3.10"
> GTK_VNC2_REQUIRED="0.4.0"
> SPICE_GTK_REQUIRED="0.30"
> SPICE_PROTOCOL_REQUIRED="0.12.7"
> @@ -93,7 +93,7 @@ PKG_PROG_PKG_CONFIG
> GLIB_MKENUMS=`$PKG_CONFIG --variable=glib_mkenums glib-2.0`
> AC_SUBST(GLIB_MKENUMS)
>
> -PKG_CHECK_MODULES(GLIB2, glib-2.0 >= $GLIB2_REQUIRED gthread-2.0
> gmodule-export-2.0)
> +PKG_CHECK_MODULES(GLIB2, glib-2.0 >= $GLIB2_REQUIRED gio-2.0
> gthread-2.0 gmodule-export-2.0)
> PKG_CHECK_MODULES(LIBXML2, libxml-2.0 >= $LIBXML2_REQUIRED)
>
> AC_ARG_WITH([libvirt],
> diff --git a/src/remote-viewer-main.c b/src/remote-viewer-main.c
> index 81cf736..b0932c5 100644
> --- a/src/remote-viewer-main.c
> +++ b/src/remote-viewer-main.c
> @@ -22,184 +22,29 @@
>
> #include
> #include
> +#include
> #include
> #include
> #include
> -#ifdef G_OS_WIN32
> -#include
> -#include
> -#endif
> -
> -#ifdef HAVE_GTK_VNC
> -#include
> -#endif
> -#ifdef HAVE_SPICE_GTK
> -#include
> -#endif
> -#ifdef HAVE_OVIRT
> -#include
> -#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_CALLB
Re: [virt-tools-list] [PATCH v3 3/5] Port to GtkApplication API's
On 02/12/2016 03:26 PM, Eduardo Lima (Etrunko) wrote:
> On 02/12/2016 10:09 AM, Eduardo Lima (Etrunko) wrote:
>> Running remote-viewer will throw some warnings:
>>
>> (remote-viewer:546): Gtk-CRITICAL **: gtk_application_get_app_menu:
>> assertion 'GTK_IS_APPLICATION (application)' failed
>>
>> (remote-viewer:546): Gtk-CRITICAL **: gtk_application_get_menubar:
>> assertion 'GTK_IS_APPLICATION (application)' failed
>>
>> This does not happen with virt-viewer. I attached the gdb backtrace to
>> this mail.
>>
>
> Fidencio just pointed out that these warnings won't happen with recent
> versions of glib/gtk as for instance the ones shipped with fedora, and I
> can confirm it. Also, I have some minor additions to this patch that I
> just added:
Following up, I tracked down to this gtk+ commit which makes the
warnings disappear. In this case I think they can be ignored.
commit 1bb880af36d4dfbda743a6fa3c68815963549a49
Author: Matthias Clasen
Date: Sat Jun 7 14:04:57 2014 -0400
GtkApplicationWindow: Avoid a crash
In several places, we were not correctly dealing with the
possibility of application not being set.
diff --git a/gtk/gtkapplicationwindow.c b/gtk/gtkapplicationwindow.c
index bf037ff..aba45a7 100644
--- a/gtk/gtkapplicationwindow.c
+++ b/gtk/gtkapplicationwindow.c
@@ -298,9 +298,10 @@ gtk_application_window_update_shell_shows_app_menu
(GtkApplicationWindow *window
/* the shell does not show it, so make sure we show it */
if (g_menu_model_get_n_items (G_MENU_MODEL
(window->priv->app_menu_section)) == 0)
{
- GMenuModel *app_menu;
+ GMenuModel *app_menu = NULL;
- app_menu = gtk_application_get_app_menu
(gtk_window_get_application (GTK_WINDOW (window)));
+ if (gtk_window_get_application (GTK_WINDOW (window)) != NULL)
+app_menu = gtk_application_get_app_menu
(gtk_window_get_application (GTK_WINDOW (window)));
if (app_menu != NULL)
{
@@ -347,9 +348,10 @@ gtk_application_window_update_shell_shows_menubar
(GtkApplicationWindow *window,
/* the shell does not show it, so make sure we show it */
if (g_menu_model_get_n_items (G_MENU_MODEL
(window->priv->menubar_section)) == 0)
{
- GMenuModel *menubar;
+ GMenuModel *menubar = NULL;
- menubar = gtk_application_get_menubar
(gtk_window_get_application (GTK_WINDOW (window)));
+ if (gtk_window_get_application (GTK_WINDOW (window)) != NULL)
+menubar = gtk_application_get_menubar
(gtk_window_get_application (GTK_WINDOW (window)));
if (menubar != NULL)
g_menu_append_section (window->priv->menubar_section, NULL,
menubar);
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH v3 3/5] Port to GtkApplication API's
On 02/12/2016 10:09 AM, Eduardo Lima (Etrunko) wrote:
> Running remote-viewer will throw some warnings:
>
> (remote-viewer:546): Gtk-CRITICAL **: gtk_application_get_app_menu:
> assertion 'GTK_IS_APPLICATION (application)' failed
>
> (remote-viewer:546): Gtk-CRITICAL **: gtk_application_get_menubar:
> assertion 'GTK_IS_APPLICATION (application)' failed
>
> This does not happen with virt-viewer. I attached the gdb backtrace to
> this mail.
>
Fidencio just pointed out that these warnings won't happen with recent
versions of glib/gtk as for instance the ones shipped with fedora, and I
can confirm it. Also, I have some minor additions to this patch that I
just added:
diff --git a/src/remote-viewer.c b/src/remote-viewer.c
index 1c3cd84..93aa590 100644
--- a/src/remote-viewer.c
+++ b/src/remote-viewer.c
@@ -754,7 +754,7 @@ authenticate_cb(RestProxy *proxy, G_GNUC_UNUSED
RestProxyAuth *auth,
}
static void
-ovirt_foreign_menu_update(GtkApplication *gtkapp, GtkWindow *gtkwin,
gpointer data)
+ovirt_foreign_menu_update(GtkApplication *gtkapp, GtkWindow *gtkwin,
G_GNUC_UNUSED gpointer data)
{
RemoteViewer *app = REMOTE_VIEWER(gtkapp);
VirtViewerWindow *win = g_object_get_data(G_OBJECT(gtkwin),
"virt-viewer-window");
diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
index 5b0e720..fca483a 100644
--- a/src/virt-viewer-app.c
+++ b/src/virt-viewer-app.c
@@ -1892,6 +1892,7 @@ virt_viewer_app_local_command_line (GApplication
*gapp,
GOptionContext *context = g_option_context_new(NULL);
GOptionGroup *group = g_option_group_new("virt-viewer", NULL, NULL,
NULL, NULL);
+*status = 0;
g_option_context_set_main_group(context, group);
VIRT_VIEWER_APP_GET_CLASS(self)->add_option_entries(self, context,
group);
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
___
virt-tools-list mailing list
virt-tools-list@redhat.com
https://www.redhat.com/mailman/listinfo/virt-tools-list
Re: [virt-tools-list] [PATCH v3 3/5] Port to GtkApplication API's
Running remote-viewer will throw some warnings:
(remote-viewer:546): Gtk-CRITICAL **: gtk_application_get_app_menu:
assertion 'GTK_IS_APPLICATION (application)' failed
(remote-viewer:546): Gtk-CRITICAL **: gtk_application_get_menubar:
assertion 'GTK_IS_APPLICATION (application)' failed
This does not happen with virt-viewer. I attached the gdb backtrace to
this mail.
On 02/12/2016 09:35 AM, Eduardo Lima (Etrunko) wrote:
[snip]
> diff --git a/src/virt-viewer-app.c b/src/virt-viewer-app.c
> index 653b30c..5b0e720 100644
> --- a/src/virt-viewer-app.c
> +++ b/src/virt-viewer-app.c
> @@ -1868,25 +1867,81 @@ virt_viewer_app_constructed(GObject *object)
> gtk_accel_map_add_entry("/view/zoom-out", GDK_minus,
> GDK_CONTROL_MASK);
> gtk_accel_map_add_entry("/view/zoom-in", GDK_plus,
> GDK_CONTROL_MASK);
> gtk_accel_map_add_entry("/send/secure-attention", GDK_End,
> GDK_CONTROL_MASK | GDK_MOD1_MASK);
> +
> +if (!virt_viewer_app_start(self, &error)) {
> +if (error && !g_error_matches(error, VIRT_VIEWER_ERROR,
> VIRT_VIEWER_ERROR_CANCELLED)) {
> +virt_viewer_app_simple_message_dialog(self, error->message);
> +}
> +
> +g_clear_error(&error);
> +g_application_quit(app);
> +}
> +
> +g_application_hold(app);
Again, this call to _hold() is only necessary for remote-viewer,
otherwise the application will quit the mainloop before showing the main
window. This is not necessary for virt-viewer, and I tried to track the
reason, without success. Maybe someone can help here.
> +}
> +
> +static gboolean
> +virt_viewer_app_local_command_line (GApplication *gapp,
> +gchar***args,
> +int*status)
> +{
> +VirtViewerApp *self = VIRT_VIEWER_APP(gapp);
> +gboolean ret = FALSE;
> +gint argc = g_strv_length(*args);
> +GError *error = NULL;
> +GOptionContext *context = g_option_context_new(NULL);
> +GOptionGroup *group = g_option_group_new("virt-viewer", NULL, NULL,
> NULL, NULL);
> +
> +g_option_context_set_main_group(context, group);
> +VIRT_VIEWER_APP_GET_CLASS(self)->add_option_entries(self, context,
> group);
> +
> +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
> +
> +if (!g_option_context_parse(context, &argc, args, &error))
> +{
> +if (error && !g_error_matches(error, VIRT_VIEWER_ERROR,
> VIRT_VIEWER_VERSION)) {
> +g_printerr(_("%s\n"), error->message);
> +*status = 1;
> +}
> +
> +g_error_free(error);
> +ret = TRUE;
> +}
> +
> +g_option_context_free(context);
> +return ret;
> }
>
> static void
> virt_viewer_app_class_init (VirtViewerAppClass *klass)
> {
> GObjectClass *object_class = G_OBJECT_CLASS (klass);
> +GApplicationClass *g_app_class = G_APPLICATION_CLASS(klass);
>
> g_type_class_add_private (klass, sizeof (VirtViewerAppPrivate));
>
> -object_class->constructed = virt_viewer_app_constructed;
> object_class->get_property = virt_viewer_app_get_property;
> object_class->set_property = virt_viewer_app_set_property;
> object_class->dispose = virt_viewer_app_dispose;
>
> +g_app_class->local_command_line = virt_viewer_app_local_command_line;
> +g_app_class->startup = virt_viewer_app_startup;
> +g_app_class->command_line = NULL; /* inhibit GApplication default
> handler */
> +
In this case we set the handler to NULL to avoid some warnings that will
be thrown by the default handler. We could have defined a function which
did nothing, as all command line options were already handled in
local_command_line().
--
Eduardo de Barros Lima (Etrunko)
Software Engineer - RedHat
etru...@redhat.com
Breakpoint 1, gtk_application_get_app_menu (application=0x0) at
gtkapplication.c:1102
warning: Source file is more recent than executable.
1102
Missing separate debuginfos, use: dnf debuginfo-install
bzip2-libs-1.0.6-19.fc23.x86_64 libgcc-5.3.1-2.fc23.x86_64
(gdb) bt
#0 gtk_application_get_app_menu (application=0x0) at gtkapplication.c:1102
#1 0x76c0e751 in gtk_application_window_update_shell_shows_app_menu
(window=0x6d6270, settings=) at gtkapplicationwindow.c:319
#2 0x76c0e8e3 in gtk_application_window_real_realize (widget=0x6d6270)
at gtkapplicationwindow.c:752
#3 0x74480ad4 in _g_closure_invoke_va (closure=closure@entry=0x659210,
return_value=return_value@entry=0x0, instance=instance@entry=0x6d6270,
args=args@entry=0x7fffcfe8, n_params=, param_types=0x0) at
gclosure.c:840
#4 0x7449aa1d in g_signal_emit_valist (instance=0x6d6270,
signal_id=, detail=0, var_args=var_args@entry=0x7fffcfe8) at
gsignal.c:3238
#5 0x7449
