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
