[Spice-devel,spice-gtk,2/4] Add a desktop-integration helper class

Submitted by Hans de Goede on June 21, 2012, 8:09 p.m.

Details

Message ID 1340309356-28960-3-git-send-email-hdegoede@redhat.com
State New
Headers show

Not browsing as part of any series.

Commit Message

Hans de Goede June 21, 2012, 8:09 p.m.
We need to integrate closely with the desktop environment of the user in
several cases. Some examples are disabling auto-mounting when auto-usbredir
is active (rhbz#812972), and disabling the screensaver when fullscreen
(fdo#34793).

Unfortuntely these kinds of things require desktop environment specific
handling. Therefor this patch introduces a desktop-integration helper class,
which is to server as a container for all sort of desktop environment specific
functions.

For now it just supports disabling automounting under Gnome, but this will be
extended in the future.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 configure.ac              |   20 ++++
 gtk/Makefile.am           |    4 +
 gtk/desktop-integration.c |  260 +++++++++++++++++++++++++++++++++++++++++++++
 gtk/desktop-integration.h |   67 ++++++++++++
 gtk/spice-session-priv.h  |    2 +
 gtk/spice-session.c       |    1 +
 6 files changed, 354 insertions(+)
 create mode 100644 gtk/desktop-integration.c
 create mode 100644 gtk/desktop-integration.h

Patch hide | download patch | download mbox

diff --git a/configure.ac b/configure.ac
index 09129b7..3841c56 100644
--- a/configure.ac
+++ b/configure.ac
@@ -566,6 +566,26 @@  fi
 
 AM_CONDITIONAL(WITH_PYTHON, [test "$WITH_PYTHON" = "yes"])
 
+
+AC_ARG_ENABLE([dbus],
+  AS_HELP_STRING([--enable-dbus=@<:@auto/yes/no@:>@],
+                 [Enable dbus support for desktop integration (disabling automount) @<:@default=auto@:>@]),
+  [],
+  [enable_dbus="auto"])
+
+if test "x$enable_dbus" != "xno"; then
+  PKG_CHECK_MODULES([DBUS_GLIB], [dbus-glib-1],
+                                 [have_dbus=yes],
+                                 [have_dbus=no])
+  if test "x$enable_dbus" = "xyes" && test "x$have_dbus" = "xno"; then
+    AC_MSG_ERROR([D-Bus support explicitly requested, but some required packages are not available])
+  fi
+
+  if test "x$have_dbus" = "xyes"; then
+    AC_DEFINE(USE_DBUS, [1], [Define if supporting dbus])
+  fi
+fi
+
 dnl ===========================================================================
 dnl check compiler flags
 
diff --git a/gtk/Makefile.am b/gtk/Makefile.am
index 0327d65..f5f6bc6 100644
--- a/gtk/Makefile.am
+++ b/gtk/Makefile.am
@@ -77,6 +77,7 @@  SPICE_COMMON_CPPFLAGS =						\
 	$(GLIB2_CFLAGS)						\
 	$(GIO_CFLAGS)						\
 	$(GOBJECT2_CFLAGS)					\
+	$(DBUS_GLIB_CFLAGS)					\
 	$(SSL_CFLAGS)						\
 	$(SASL_CFLAGS)						\
 	$(GST_CFLAGS)						\
@@ -100,6 +101,7 @@  SPICE_GTK_LIBADD_COMMON =		\
 	libspice-client-glib-2.0.la	\
 	$(GTK_LIBS)			\
 	$(CAIRO_LIBS)			\
+	$(DBUS_GLIB_LIBS)		\
 	$(XRANDR_LIBS)			\
 	$(LIBM)				\
 	$(NULL)
@@ -114,6 +116,8 @@  SPICE_GTK_SOURCES_COMMON =		\
 	vncdisplaykeymap.h		\
 	spice-grabsequence.c		\
 	spice-grabsequence.h		\
+	desktop-integration.c		\
+	desktop-integration.h		\
 	usb-device-widget.c		\
 	$(NULL)
 
diff --git a/gtk/desktop-integration.c b/gtk/desktop-integration.c
new file mode 100644
index 0000000..8c5ac75
--- /dev/null
+++ b/gtk/desktop-integration.c
@@ -0,0 +1,260 @@ 
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2012 Red Hat, Inc.
+
+   Red Hat Authors:
+   Hans de Goede <hdegoede@redhat.com>
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+
+#include "config.h"
+#include <glib-object.h>
+#include <glib/gi18n.h>
+#ifdef USE_DBUS
+#include <dbus/dbus-glib.h>
+#endif
+#include "glib-compat.h"
+#include "spice-session-priv.h"
+#include "desktop-integration.h"
+
+#define GNOME_SESSION_INHIBIT_AUTOMOUNT 16
+
+/* ------------------------------------------------------------------ */
+/* gobject glue                                                       */
+
+#define SPICE_DESKTOP_INTEGRATION_GET_PRIVATE(obj)                                  \
+    (G_TYPE_INSTANCE_GET_PRIVATE ((obj), SPICE_TYPE_DESKTOP_INTEGRATION, SpiceDesktopIntegrationPrivate))
+
+struct _SpiceDesktopIntegrationPrivate {
+#ifdef USE_DBUS
+    DBusGConnection *dbus_conn;
+    DBusGProxy *gnome_session_proxy;
+    GData *gnome_automount_inhibit_cookie_list;
+#endif
+};
+
+G_DEFINE_TYPE(SpiceDesktopIntegration, spice_desktop_integration, G_TYPE_OBJECT);
+
+/* ------------------------------------------------------------------ */
+/* Gnome specific code                                                */
+
+#ifdef USE_DBUS
+
+static void handle_dbus_call_error(const char *call, GError **_error)
+{
+    GError *error = *_error;
+    const char *message = error->message;
+
+    if (error->domain == DBUS_GERROR &&
+            error->code == DBUS_GERROR_REMOTE_EXCEPTION)
+        message = dbus_g_error_get_name(error);
+    g_debug("Error calling '%s': %s", call, message);
+    g_clear_error(_error);
+}
+
+static void gnome_integration_init(SpiceDesktopIntegration *self)
+{
+    SpiceDesktopIntegrationPrivate *priv = self->priv;
+    GError *error = NULL;
+
+    g_datalist_init(&priv->gnome_automount_inhibit_cookie_list);
+
+    if (!priv->dbus_conn)
+        return;
+
+    /* We use for_name_owner, to resolve the name now, as we may not be
+       running under gnome-session manager at all! */
+    priv->gnome_session_proxy = dbus_g_proxy_new_for_name_owner(
+                                            priv->dbus_conn,
+                                            "org.gnome.SessionManager",
+                                            "/org/gnome/SessionManager",
+                                            "org.gnome.SessionManager",
+                                            &error);
+    if (error) {
+        g_debug("Could not create org.gnome.SessionManager dbus proxy: %s",
+                error->message);
+        g_clear_error(&error);
+    }
+}
+
+static void gnome_integration_inhibit_automount(SpiceDesktopIntegration *self,
+                                                guint toplevel_window_id)
+{
+    SpiceDesktopIntegrationPrivate *priv = self->priv;
+    GError *error = NULL;
+    guint cookie;
+
+    if (!priv->gnome_session_proxy)
+        return;
+
+    cookie = GPOINTER_TO_UINT(g_datalist_id_get_data(
+                                  &priv->gnome_automount_inhibit_cookie_list,
+                                  toplevel_window_id));
+    /* If a cookie exists for the toplevel we are already inhibitting */
+    if (cookie != 0)
+        return;
+
+    if (!dbus_g_proxy_call(
+                priv->gnome_session_proxy, "Inhibit", &error,
+                G_TYPE_STRING, "spice-gtk-app",
+                G_TYPE_UINT, toplevel_window_id,
+                G_TYPE_STRING,
+                 _("Automounting has been inhibited for USB auto-redirecting"),
+                G_TYPE_UINT, GNOME_SESSION_INHIBIT_AUTOMOUNT,
+                G_TYPE_INVALID,
+                G_TYPE_UINT, &cookie,
+                G_TYPE_INVALID)) {
+        handle_dbus_call_error("org.gnome.SessionManager.Inhibit", &error);
+        return;
+    }
+
+    g_datalist_id_set_data(&priv->gnome_automount_inhibit_cookie_list,
+                           toplevel_window_id, GUINT_TO_POINTER(cookie));
+}
+
+static void gnome_integration_do_uninhibit_automount(GQuark id,
+                                                     gpointer data,
+                                                     gpointer user_data)
+{
+    SpiceDesktopIntegration *self = SPICE_DESKTOP_INTEGRATION(user_data);
+    SpiceDesktopIntegrationPrivate *priv = self->priv;
+    guint cookie = GPOINTER_TO_UINT(data);
+    GError *error = NULL;
+
+    if (!dbus_g_proxy_call(
+                priv->gnome_session_proxy, "Uninhibit", &error,
+                G_TYPE_UINT, cookie,
+                G_TYPE_INVALID,
+                G_TYPE_INVALID))
+        handle_dbus_call_error("org.gnome.SessionManager.Uninhibit", &error);
+}
+
+static void gnome_integration_uninhibit_automount(
+                                                SpiceDesktopIntegration *self,
+                                                guint toplevel_window_id)
+{
+    SpiceDesktopIntegrationPrivate *priv = self->priv;
+    gpointer cookie;
+
+    if (!priv->gnome_session_proxy)
+        return;
+
+    cookie = g_datalist_id_get_data(&priv->gnome_automount_inhibit_cookie_list,
+                                    toplevel_window_id);
+
+    /* If there is no cookie we are not inhibitting automount */
+    if (cookie == 0)
+        return;
+
+    gnome_integration_do_uninhibit_automount(toplevel_window_id, cookie, self);
+
+    g_datalist_id_remove_data(&priv->gnome_automount_inhibit_cookie_list,
+                              toplevel_window_id);
+}
+
+static void gnome_integration_dispose(SpiceDesktopIntegration *self)
+{
+    SpiceDesktopIntegrationPrivate *priv = self->priv;
+
+    g_datalist_foreach(&priv->gnome_automount_inhibit_cookie_list,
+                       gnome_integration_do_uninhibit_automount, self);
+    g_datalist_clear(&priv->gnome_automount_inhibit_cookie_list);
+    g_clear_object(&priv->gnome_session_proxy);
+}
+
+#endif
+
+/* ------------------------------------------------------------------ */
+/* gobject glue                                                       */
+
+static void spice_desktop_integration_init(SpiceDesktopIntegration *self)
+{
+    SpiceDesktopIntegrationPrivate *priv;
+#ifdef USE_DBUS
+    GError *error = NULL;
+#endif
+
+    priv = SPICE_DESKTOP_INTEGRATION_GET_PRIVATE(self);
+    self->priv = priv;
+
+#ifdef USE_DBUS
+    priv->dbus_conn = dbus_g_bus_get (DBUS_BUS_SESSION, &error);
+    if (!priv->dbus_conn) {
+       g_warning("Error connecting to session dbus: %s", error->message);
+       g_clear_error(&error);
+    }
+    gnome_integration_init(self);
+#endif
+}
+
+static void spice_desktop_integration_dispose(GObject *gobject)
+{
+    SpiceDesktopIntegration *self = SPICE_DESKTOP_INTEGRATION(gobject);
+
+#ifdef USE_DBUS
+    gnome_integration_dispose(self);
+#endif
+
+    /* Chain up to the parent class */
+    if (G_OBJECT_CLASS(spice_desktop_integration_parent_class)->dispose)
+        G_OBJECT_CLASS(spice_desktop_integration_parent_class)->dispose(gobject);
+}
+
+static void spice_desktop_integration_class_init(SpiceDesktopIntegrationClass *klass)
+{
+    GObjectClass *gobject_class = G_OBJECT_CLASS (klass);
+
+    gobject_class->dispose      = spice_desktop_integration_dispose;
+
+    g_type_class_add_private(klass, sizeof(SpiceDesktopIntegrationPrivate));
+}
+
+/* ------------------------------------------------------------------ */
+/* public methods                                                     */
+
+SpiceDesktopIntegration *spice_desktop_integration_get(SpiceSession *session)
+{
+    SpiceDesktopIntegration *self;
+    static GStaticMutex mutex = G_STATIC_MUTEX_INIT;
+
+    g_return_val_if_fail(session != NULL, NULL);
+
+    g_static_mutex_lock(&mutex);
+    self = session->priv->desktop_integration;
+    if (self == NULL) {
+        self = g_object_new(SPICE_TYPE_DESKTOP_INTEGRATION, NULL);
+        session->priv->desktop_integration = self;
+    }
+    g_static_mutex_unlock(&mutex);
+
+    return self;
+}
+
+void spice_desktop_integration_inhibit_automount(SpiceDesktopIntegration *self,
+                                                 guint toplevel_window_id)
+{
+#ifdef USE_DBUS
+    gnome_integration_inhibit_automount(self, toplevel_window_id);
+#endif
+}
+
+void spice_desktop_integration_uninhibit_automount(
+                                                 SpiceDesktopIntegration *self,
+                                                 guint toplevel_window_id)
+{
+#ifdef USE_DBUS
+    gnome_integration_uninhibit_automount(self, toplevel_window_id);
+#endif
+}
diff --git a/gtk/desktop-integration.h b/gtk/desktop-integration.h
new file mode 100644
index 0000000..7191483
--- /dev/null
+++ b/gtk/desktop-integration.h
@@ -0,0 +1,67 @@ 
+/* -*- Mode: C; c-basic-offset: 4; indent-tabs-mode: nil -*- */
+/*
+   Copyright (C) 2012 Red Hat, Inc.
+
+   Red Hat Authors:
+   Hans de Goede <hdegoede@redhat.com>
+
+   This library is free software; you can redistribute it and/or
+   modify it under the terms of the GNU Lesser General Public
+   License as published by the Free Software Foundation; either
+   version 2.1 of the License, or (at your option) any later version.
+
+   This library is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+   Lesser General Public License for more details.
+
+   You should have received a copy of the GNU Lesser General Public
+   License along with this library; if not, see <http://www.gnu.org/licenses/>.
+*/
+#ifndef __SPICE_DESKTOP_INTEGRATION_H__
+#define __SPICE_DESKTOP_INTEGRATION_H__
+
+#include "spice-client.h"
+
+G_BEGIN_DECLS
+
+#define SPICE_TYPE_DESKTOP_INTEGRATION            (spice_desktop_integration_get_type ())
+#define SPICE_DESKTOP_INTEGRATION(obj)            (G_TYPE_CHECK_INSTANCE_CAST ((obj), SPICE_TYPE_DESKTOP_INTEGRATION, SpiceDesktopIntegration))
+#define SPICE_DESKTOP_INTEGRATION_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST ((klass), SPICE_TYPE_DESKTOP_INTEGRATION, SpiceDesktopIntegrationClass))
+#define SPICE_IS_DESKTOP_INTEGRATION(obj)         (G_TYPE_CHECK_INSTANCE_TYPE ((obj), SPICE_TYPE_DESKTOP_INTEGRATION))
+#define SPICE_IS_DESKTOP_INTEGRATION_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE ((klass), SPICE_TYPE_DESKTOP_INTEGRATION))
+#define SPICE_DESKTOP_INTEGRATION_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS ((obj), SPICE_TYPE_DESKTOP_INTEGRATION, SpiceDesktopIntegrationClass))
+
+typedef struct _SpiceDesktopIntegration SpiceDesktopIntegration;
+typedef struct _SpiceDesktopIntegrationClass SpiceDesktopIntegrationClass;
+typedef struct _SpiceDesktopIntegrationPrivate SpiceDesktopIntegrationPrivate;
+
+/*
+ * SpiceDesktopIntegration offers helper-functions to do desktop environment
+ * and/or platform specific tasks like disabling automount, disabling the
+ * screen-saver, etc. SpiceDesktopIntegration is for internal spice-gtk usage
+ * only!
+ */
+struct _SpiceDesktopIntegration
+{
+    GObject parent;
+
+    SpiceDesktopIntegrationPrivate *priv;
+};
+
+struct _SpiceDesktopIntegrationClass
+{
+    GObjectClass parent_class;
+};
+
+GType spice_desktop_integration_get_type(void);
+SpiceDesktopIntegration *spice_desktop_integration_get(SpiceSession *session);
+void spice_desktop_integration_inhibit_automount(SpiceDesktopIntegration *self,
+                                                 guint toplevel_window_id);
+void spice_desktop_integration_uninhibit_automount(
+                                                 SpiceDesktopIntegration *self,
+                                                 guint toplevel_window_id);
+
+G_END_DECLS
+
+#endif /* __SPICE_DESKTOP_INTEGRATION_H__ */
diff --git a/gtk/spice-session-priv.h b/gtk/spice-session-priv.h
index 5cef264..c24ef8e 100644
--- a/gtk/spice-session-priv.h
+++ b/gtk/spice-session-priv.h
@@ -20,6 +20,7 @@ 
 
 #include <glib.h>
 #include <gio/gio.h>
+#include "desktop-integration.h"
 #include "spice-session.h"
 #include "spice-gtk-session.h"
 #include "spice-channel-cache.h"
@@ -97,6 +98,7 @@  struct _SpiceSessionPrivate {
 
     /* associated objects */
     SpiceAudio        *audio_manager;
+    SpiceDesktopIntegration *desktop_integration;
     SpiceGtkSession   *gtk_session;
     SpiceUsbDeviceManager *usb_manager;
 };
diff --git a/gtk/spice-session.c b/gtk/spice-session.c
index fface67..995b2ed 100644
--- a/gtk/spice-session.c
+++ b/gtk/spice-session.c
@@ -155,6 +155,7 @@  spice_session_dispose(GObject *gobject)
     }
 
     g_clear_object(&s->audio_manager);
+    g_clear_object(&s->desktop_integration);
     g_clear_object(&s->gtk_session);
     g_clear_object(&s->usb_manager);
 

Comments

Hi

On Thu, Jun 21, 2012 at 10:09 PM, Hans de Goede <hdegoede@redhat.com> wrote:
> +
> +AC_ARG_ENABLE([dbus],
> +  AS_HELP_STRING([--enable-dbus=@<:@auto/yes/no@:>@],
> +                 [Enable dbus support for desktop integration (disabling automount) @<:@default=auto@:>@]),
> +  [],
> +  [enable_dbus="auto"])

It would be more reliable to have a --with-desktop=<gnome/none> and
checking all the needed dependencies.

+void spice_desktop_integration_inhibit_automount(SpiceDesktopIntegration *self,
+                                                 guint toplevel_window_id)
+{
+#ifdef USE_DBUS
+    gnome_integration_inhibit_automount(self, toplevel_window_id);
+#endif
+}


Because this will need to be updated as soon as USE_DBUS is used for
something else, and also, calling this function when !USE_DBUS will
just be silent and do nothing. We should at least add a SPICE_DEBUG if
nothing is done there.

it might be cleaner to define a base class and derive it for the
various desktops, but we can do that later.

> +if test "x$enable_dbus" != "xno"; then
> +  PKG_CHECK_MODULES([DBUS_GLIB], [dbus-glib-1],

Since we are targetting GNOME3 with those changes, we shouldn't use
dbus-glib which has been deprecated for a while in favour of gdbus

Finally, I think the GData list isn't very appropriate structure for
mapping int -> pointer, why not use a hashtable? My understanding of
this logic could also be made with a refcount.
Hi,

Thanks for the comments.

On 06/22/2012 11:06 AM, Marc-André Lureau wrote:
> Hi
>
> On Thu, Jun 21, 2012 at 10:09 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>> +
>> +AC_ARG_ENABLE([dbus],
>> +  AS_HELP_STRING([--enable-dbus=@<:@auto/yes/no@:>@],
>> +                 [Enable dbus support for desktop integration (disabling automount) @<:@default=auto@:>@]),
>> +  [],
>> +  [enable_dbus="auto"])
>
> It would be more reliable to have a --with-desktop=<gnome/none> and
> checking all the needed dependencies.

That is not going to work, as once we support multiple desktops we will want to
compile in support for more then one. So that the same binary can run under
gnome and under say kde, and do the right thing in both cases.

Also note that the code has been written so as to *not* complain (only g_debug)
when things fail, to allow running under none gnome without polluting the console
with error messages, this is by design.

>
> +void spice_desktop_integration_inhibit_automount(SpiceDesktopIntegration *self,
> +                                                 guint toplevel_window_id)
> +{
> +#ifdef USE_DBUS
> +    gnome_integration_inhibit_automount(self, toplevel_window_id);
> +#endif
> +}
>
>
> Because this will need to be updated as soon as USE_DBUS is used for
> something else,

Lets say we manage to get KDE upstream to provide us with an automount-inhibit
interface, and it is also using dbus, then the above code will simply change to:

oid spice_desktop_integration_inhibit_automount(SpiceDesktopIntegration *self,
                                                 guint toplevel_window_id)
{
#ifdef USE_DBUS
     gnome_integration_inhibit_automount(self, toplevel_window_id);
     kde_integration_inhibit_automount(self, toplevel_window_id);
#endif
}

Yes both will get called, and one of them will simply do nothing, just like
gnome_integration_inhibit_automount now does nothing when not running under
gnome-session.

> and also, calling this function when !USE_DBUS will
> just be silent and do nothing. We should at least add a SPICE_DEBUG if
> nothing is done there.

The problem then becomes defining "nothing is done there", what if we are
compiled with dbus but not running under a supported desktop environment?

I guess I could add a warning to spice_desktop_integration_init when we are
missing desktop integration for the users environment, should this be a
warning, or a debug ?

> to define a base class and derive it for the
> various desktops, but we can do that later.

We would then need a factory capable of figuring out which de it is running
under, also what if say XFCE supports gnome's automount inhibit and kde's
screensaver inhibit? I think just calling all implementations, and let them
figure out themselves if they can handle the call is better.

>
>> +if test "x$enable_dbus" != "xno"; then
>> +  PKG_CHECK_MODULES([DBUS_GLIB], [dbus-glib-1],
>
> Since we are targetting GNOME3 with those changes, we shouldn't use
> dbus-glib which has been deprecated for a while in favour of gdbus

We are targeting gnome >= 2.28 (and yes gnome-session in 2.28 does have
the Inhibitor interface), so again this was a conscious choice, I would
rather have 1 implementation then 2, until we drop support for gnome 2.28.

> Finally, I think the GData list isn't very appropriate structure for
> mapping int -> pointer, why not use a hashtable?

Because:
1) GData is meant exactly for mapping uints (quarks) to pointers, iow it
is being used as intended (well I need mapping (unique) uints to uints, but
mapping uints to pointers does the job nicely).
2) Using a hashtable would needlessly complicate the code, since a hashtable
is not intended for this.

> My understanding of this logic could also be made with a refcount.
No we need to cookie associated to the specific toplevel window id to
uninhibit.

Regards,

Hans
Hi

----- Mensaje original -----
> > It would be more reliable to have a --with-desktop=<gnome/none> and
> > checking all the needed dependencies.
> 
> That is not going to work, as once we support multiple desktops we
> will want to
> compile in support for more then one. So that the same binary can run
> under
> gnome and under say kde, and do the right thing in both cases.

Yes, indeed.

> Also note that the code has been written so as to *not* complain
> (only g_debug)
> when things fail, to allow running under none gnome without polluting
> the console
> with error messages, this is by design.

ok

> #ifdef USE_DBUS
>      gnome_integration_inhibit_automount(self, toplevel_window_id);
>      kde_integration_inhibit_automount(self, toplevel_window_id);
> #endif
> }
> 
> Yes both will get called, and one of them will simply do nothing,
> just like
> gnome_integration_inhibit_automount now does nothing when not running
> under
> gnome-session.

ok

> The problem then becomes defining "nothing is done there", what if we
> are
> compiled with dbus but not running under a supported desktop
> environment?
> 
> I guess I could add a warning to spice_desktop_integration_init when
> we are
> missing desktop integration for the users environment, should this be
> a
> warning, or a debug ?

A warning or a debug, but not something silent.

> >> +if test "x$enable_dbus" != "xno"; then
> >> +  PKG_CHECK_MODULES([DBUS_GLIB], [dbus-glib-1],
> >
> > Since we are targetting GNOME3 with those changes, we shouldn't use
> > dbus-glib which has been deprecated for a while in favour of gdbus
> 
> We are targeting gnome >= 2.28 (and yes gnome-session in 2.28 does
> have
> the Inhibitor interface), so again this was a conscious choice, I
> would
> rather have 1 implementation then 2, until we drop support for gnome
> 2.28.

Argh, rhel6 is still glib 2.22, and gdbus is 2.26. ok :)

> > Finally, I think the GData list isn't very appropriate structure
> > for
> > mapping int -> pointer, why not use a hashtable?
> 
> Because:
> 1) GData is meant exactly for mapping uints (quarks) to pointers, iow
> it
> is being used as intended (well I need mapping (unique) uints to
> uints, but
> mapping uints to pointers does the job nicely).

quarks are "non-zero integer which uniquely identifies a particular string", not exactly uint.

> 2) Using a hashtable would needlessly complicate the code, since a
> hashtable
> is not intended for this.

Many people use it to do int -> pointer (and also int -> int G(U)INT_TO_POINTER)

> > My understanding of this logic could also be made with a refcount.
> No we need to cookie associated to the specific toplevel window id to
> uninhibit.

ah right, /blame me, I didn't spend enough time reviewing the code..

Could we simplify the API and the logic if we used a hidden window? have you thought about it? That way, we don't need to pass a window id around, we don't need to call (un)inhibit many times, we don't need to maintain int->int mapping, we only keep one cookie and a refcount.
Hi,

On 06/22/2012 02:37 PM, Marc-André Lureau wrote:

<snip?

>> The problem then becomes defining "nothing is done there", what if we
>> are
>> compiled with dbus but not running under a supported desktop
>> environment?
>>
>> I guess I could add a warning to spice_desktop_integration_init when
>> we are
>> missing desktop integration for the users environment, should this be
>> a
>> warning, or a debug ?
>
> A warning or a debug, but not something silent.

Well, debug is silent by default, so I guess a warning?

<snip>

>>> Finally, I think the GData list isn't very appropriate structure
>>> for
>>> mapping int -> pointer, why not use a hashtable?
>>
>> Because:
>> 1) GData is meant exactly for mapping uints (quarks) to pointers, iow
>> it
>> is being used as intended (well I need mapping (unique) uints to
>> uints, but
>> mapping uints to pointers does the job nicely).
>
> quarks are "non-zero integer which uniquely identifies a particular string", not exactly uint.

And windows id's are non-zero integers which uniquely identify a particular window.

>> 2) Using a hashtable would needlessly complicate the code, since a
>> hashtable
>> is not intended for this.
>
> Many people use it to do int -> pointer (and also int -> int G(U)INT_TO_POINTER)

Just because many people use hash-tables for something does not make them the
right choice, I looked at using hash-tables first, before I choose to use
GData and I found GData to be a better match.

>>> My understanding of this logic could also be made with a refcount.
>> No we need to cookie associated to the specific toplevel window id to
>> uninhibit.
>
> ah right, /blame me, I didn't spend enough time reviewing the code..
>
> Could we simplify the API and the logic if we used a hidden window? have you thought about it? That way, we don't need to pass a window id around, we don't need to call (un)inhibit many times, we don't need to maintain int->int mapping, we only keep one cookie and a refcount.

Erm, no, the whole purpose of the toplevel window id is so that if the window goes
away gnome-session can automatically uninhibit if the app does not do so properly,
keeping a hidden window around for this would break this.

Regards,

Hans
----- Mensaje original -----
> Hi,
> 
> On 06/22/2012 02:37 PM, Marc-André Lureau wrote:
> 
> <snip?
> 
> >> The problem then becomes defining "nothing is done there", what if
> >> we
> >> are
> >> compiled with dbus but not running under a supported desktop
> >> environment?
> >>
> >> I guess I could add a warning to spice_desktop_integration_init
> >> when
> >> we are
> >> missing desktop integration for the users environment, should this
> >> be
> >> a
> >> warning, or a debug ?
> >
> > A warning or a debug, but not something silent.
> 
> Well, debug is silent by default, so I guess a warning?
> 
> <snip>
> 
> >>> Finally, I think the GData list isn't very appropriate structure
> >>> for
> >>> mapping int -> pointer, why not use a hashtable?
> >>
> >> Because:
> >> 1) GData is meant exactly for mapping uints (quarks) to pointers,
> >> iow
> >> it
> >> is being used as intended (well I need mapping (unique) uints to
> >> uints, but
> >> mapping uints to pointers does the job nicely).
> >
> > quarks are "non-zero integer which uniquely identifies a particular
> > string", not exactly uint.
> 
> And windows id's are non-zero integers which uniquely identify a
> particular window.
> >> 2) Using a hashtable would needlessly complicate the code, since a
> >> hashtable
> >> is not intended for this.
> >
> > Many people use it to do int -> pointer (and also int -> int
> > G(U)INT_TO_POINTER)
> 
> Just because many people use hash-tables for something does not make
> them the
> right choice, I looked at using hash-tables first, before I choose to
> use
> GData and I found GData to be a better match.

Right, but if nobody does something this way, I am very suspicious :)

GData list seems to be a simple growing array of elements with GQuark key, and the API includes "gchar *key". Not really fond of using something meant for various string representations for windows xid.

I don't see why a GHashTable would be more complicated.

> >>> My understanding of this logic could also be made with a
> >>> refcount.
> >> No we need to cookie associated to the specific toplevel window id
> >> to
> >> uninhibit.
> >
> > ah right, /blame me, I didn't spend enough time reviewing the
> > code..
> >
> > Could we simplify the API and the logic if we used a hidden window?
> > have you thought about it? That way, we don't need to pass a
> > window id around, we don't need to call (un)inhibit many times, we
> > don't need to maintain int->int mapping, we only keep one cookie
> > and a refcount.
> 
> Erm, no, the whole purpose of the toplevel window id is so that if
> the window goes
> away gnome-session can automatically uninhibit if the app does not do
> so properly,
> keeping a hidden window around for this would break this.

By go away, you mean destroyed? I suppose the manager doesn't care if the window is shown/visible or not, but that it exists and is destroyed either intentionally, or unintentionally after abort etc.. I don't see the need to pass around each and every toplevel window to the manager (visible or not)
Hi,

On 06/22/2012 05:41 PM, Marc-André Lureau wrote:

<snip>

>> Just because many people use hash-tables for something does not make
>> them the
>> right choice, I looked at using hash-tables first, before I choose to
>> use
>> GData and I found GData to be a better match.
>
> Right, but if nobody does something this way, I am very suspicious :)
>
> GData list seems to be a simple growing array of elements with GQuark key, and the API includes "gchar *key". Not really fond of using something meant for various string representations for windows xid.
>
> I don't see why a GHashTable would be more complicated.

Ok, I'll convert this bit to use GHashTables

<snip>

>>> Could we simplify the API and the logic if we used a hidden window?
>>> have you thought about it? That way, we don't need to pass a
>>> window id around, we don't need to call (un)inhibit many times, we
>>> don't need to maintain int->int mapping, we only keep one cookie
>>> and a refcount.
>>
>> Erm, no, the whole purpose of the toplevel window id is so that if
>> the window goes
>> away gnome-session can automatically uninhibit if the app does not do
>> so properly,
>> keeping a hidden window around for this would break this.
>
> By go away, you mean destroyed? I suppose the manager doesn't care if
 > the window is shown/visible or not, but that it exists and is destroyed
> either intentionally, or unintentionally after abort etc.. I don't see
 > the need to pass around each and every toplevel window to the manager
 > (visible or not)

Passing, what for all intends and purposes is a *fake* toplevel window id,
is just gaming the system, this is not how the inhibit API is supposed to
be used! The whole toplevel id is there to avoid inhibits sticking around
when they should not. And we should simply not try to game the system!

Regards,

Hans
On Sat, Jun 23, 2012 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote:
> Passing, what for all intends and purposes is a *fake* toplevel window id,
> is just gaming the system, this is not how the inhibit API is supposed to
> be used! The whole toplevel id is there to avoid inhibits sticking around
> when they should not. And we should simply not try to game the system!

There is no game, it can be simplified, resulting in simpler API and less code.
Hi,

On 06/23/2012 02:46 PM, Marc-André Lureau wrote:
> On Sat, Jun 23, 2012 at 9:05 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> Passing, what for all intends and purposes is a *fake* toplevel window id,
>> is just gaming the system, this is not how the inhibit API is supposed to
>> be used! The whole toplevel id is there to avoid inhibits sticking around
>> when they should not. And we should simply not try to game the system!
>
> There is no game

??

 > it can be simplified, resulting in simpler API and less code.

No it cannot, using a hidden window for this is just plain wrong! One is
supposed to use real toplevel windows with the inhibit API.

Regards,

Hans
Hi

On Sun, Jun 24, 2012 at 1:39 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>> it can be simplified, resulting in simpler API and less code.
>
> No it cannot, using a hidden window for this is just plain wrong! One is
> supposed to use real toplevel windows with the inhibit API.
>

Well, in fact that toplevel_xid is optionnal, and is used for
eventually taking application screenshots (only if possible).

gnome-session/gsm-inhibit-dialog.c
        xid = gsm_inhibitor_peek_toplevel_xid (inhibitor);
        g_debug ("GsmInhibitDialog: inhibitor has XID %u", xid);
        if (xid > 0 && dialog->priv->have_xrender) {
                pixbuf = get_pixbuf_for_window (gdkdisplay, xid,
DEFAULT_SNAPSHOT_SIZE, DEFAULT_SNAPSHOT_SIZE);
                if (pixbuf == NULL) {
                        g_debug ("GsmInhibitDialog: unable to read
pixbuf from %u", xid);
                }


You may just pass XID 0:

gdbus call --session --dest org.gnome.SessionManager --object-path
/org/gnome/SessionManager --method org.gnome.SessionManager.Inhibit
gnome-boxes 0 foobar 1

The toplevel window of a spice-gtk widget may change, so there is no
reliable way to keep an Inhibitor with a valid XID, except by creating
a private one, which is not useful, since the XID isn't even
necessary.

Let's just use 0 there
Hi,

On 06/24/2012 02:07 AM, Marc-André Lureau wrote:
> Hi
>
> On Sun, Jun 24, 2012 at 1:39 AM, Hans de Goede <hdegoede@redhat.com> wrote:
>>> it can be simplified, resulting in simpler API and less code.
>>
>> No it cannot, using a hidden window for this is just plain wrong! One is
>> supposed to use real toplevel windows with the inhibit API.
>>
>
> Well, in fact that toplevel_xid is optionnal, and is used for
> eventually taking application screenshots (only if possible).

Oh, I was under the impression that the xid would be used to monitor the
window going away and automatically uninhibit when it is gone. Since that
is not the case I agree with you that just using a single inhibit with a
xid of 0 and ref-counting is much better. I'll rewrite my patch-set to
work this way tomorrow.

Regards,

Hans