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

[Hans de Goede]

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 at 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