[pulseaudio-discuss] [RFC] [PATCH] module-gsettings: new module to store configuration using gsettings

[Tanu Kaskinen]

On Fri, 2016-01-22 at 00:05 +0100, Sylvain Baubeau wrote:
> Hi all,
> 
> This simple module mimics the module-gconf behavior, using GSettings instead
> of GConf. The original intend for keeping the same hierarchy for the settings
> is to be able to migrate from GConf (using gsettings-data-convert for example).
> 
> This would allow utilities like paprefs to move from GConf, which is now
> deprecated.

Hi! Thanks for the patch, and sorry for the delayed reply.

I wish paprefs would be migrated to use our native protocol, because
neither GConf nor GSettings is usable over the network. That said, I
don't expect that to happen any time soon, since the required
functionality isn't currently available in the native protocol, so a
transition to GSettings is welcome. Some high level comments:

* There doesn't seem to be support for migrating existing settings from
GConf to GSettings. paprefs will initially keep using GConf, so in the
transition period I think both module-gconf and module-gsettings should
be loaded. Settings stored in the two databases should be synchronized,
so that when paprefs is updated to use GSettings, the old settings will
be remembered.

* Until the database syncing is implemented, I think it's best to
disable the GSettings support by default, because it's not really ready
for common use.

* Please separate stdin-util.h into .c and .h files as usual.

I didn't review most of the code in detail yet, but below are some
comments about the build system changes:

> --- a/configure.ac
> +++ b/configure.ac
> @@ -894,6 +894,22 @@ AS_IF([test "x$enable_gconf" = "xyes" && test "x$HAVE_GCONF" = "x0"],
>  
>  AM_CONDITIONAL([HAVE_GCONF], [test "x$HAVE_GCONF" = x1])
>  
> +#### GSettings support (optional) ####
> +
> +AC_ARG_ENABLE([gsettings],
> +    AS_HELP_STRING([--disable-gsettings],[Disable optional GSettings support]))
> +
> +AS_IF([test "x$enable_gsetttings" != "xno"],
> +    [PKG_CHECK_MODULES(GSETTINGS, [ gio-2.0 >= 2.26.0 ], HAVE_GSETTINGS=1, HAVE_GSETTINGS=0)],
> +    HAVE_GSETTINGS=0)
> +
> +AS_IF([test "x$enable_gsettings" = "xyes" && test "x$HAVE_GSETTINGS" = "x0"],
> +    [AC_MSG_ERROR([*** GSettings support not found])])
> +
> +AM_CONDITIONAL([HAVE_GSETTINGS], [test "x$HAVE_GSETTINGS" = x1])
> +
> +GLIB_GSETTINGS

This macro is provided by glib. If pulseaudio is built without
gsettings support, I think this will cause an error.

> +AC_CONFIG_FILES([src/modules/gsettings/org.freedesktop.pulseaudio.gschema.xml])

Why is this file included in AC_CONFIG_FILES? There don't seem to be
any substitutions in the file.

I also found this text on https://developer.gnome.org/GSettings/ :

"There are several examples in the wild of projects that use
.gschema.xml.in files. This is almost always wrong."

> @@ -2121,6 +2131,20 @@ gconf_helper_LDADD = $(AM_LDADD) libpulsecore- at PA_MAJORMINOR@.la libpulsecommon-
>  gconf_helper_CFLAGS = $(AM_CFLAGS) $(GCONF_CFLAGS)
>  gconf_helper_LDFLAGS = $(AM_LDFLAGS) $(BINLDFLAGS)
>  
> +# GSettings support
> +module_gsettings_la_SOURCES = modules/gsettings/module-gsettings.c
> +module_gsettings_la_LDFLAGS = $(MODULE_LDFLAGS)
> +module_gsettings_la_LIBADD = $(MODULE_LIBADD)
> +module_gsettings_la_CFLAGS = $(AM_CFLAGS) -DPA_GSETTINGS_HELPER=\"$(pulselibexecdir)/gsettings-helper\"
> +
> +gsettings_helper_SOURCES = modules/gsettings/gsettings-helper.c
> +gsettings_helper_LDADD = $(AM_LDADD) libpulsecore- at PA_MAJORMINOR@.la libpulsecommon- at PA_MAJORMINOR@.la libpulse.la $(GSETTINGS_LIBS)
> +gsettings_helper_CFLAGS = $(AM_CFLAGS) $(GSETTINGS_CFLAGS)
> +gsettings_helper_LDFLAGS = $(AM_LDFLAGS) $(BINLDFLAGS)
> +
> +gsettings_SCHEMAS = modules/gsettings/org.freedesktop.pulseaudio.gschema.xml
> + at GSETTINGS_RULES@

@GSETTINGS_RULES@ will probably cause an error if gsettings support is
disabled.

-- 
Tanu