Hi Sylvain, Here's my belated review of this patch. Changes are needed, but I plan to implement them myself, because you quite likely busy with something else nowadays and I think getting this patch merged is quite urgent. However, if you're eager to fix the patch yourself, let me know and I'll happily do something else :) The commit message needs to be rewritten, because currently the commit message is your reply to me. module-gsettings needs to be added to default.pa. gsettings-helper and org.freedesktop.pulseaudio.gschema.valid should be added to src/.gitignore. The org.freedesktop.pulseaudio.modules schema is installed by paprefs, but it's needed by module-gsettings too. The schema should be installed by module-gsettings, because it's fine to make paprefs depend on module-gsettings, but module-gsettings shouldn't depend on paprefs. If the org.freedesktop.pulseaudio.modules schema installed by module- gsettings, the schema shouldn't define what children the modules object has, because that's controlled by paprefs. Removing the children from the schema should be fine. If I understood the GSettings documentation correctly, children can be added dynamically with g_settings_new_with_path(). stdin-util.h doesn't follow the usual conventions regarding how interfaces are defined in PulseAudio, but I think I'll let that pass, because the header is only used by code that will hopefully be removed some day (as I've written earlier, paprefs should be changed to use the native protocol for communication). config.h should be included by all .c files, and none of the .h files. More comments inline: On Mon, 2016-07-25 at 21:58 +0200, Sylvain Baubeau wrote: > --- > configure.ac | 20 ++ > src/Makefile.am | 32 ++- > src/modules/gconf/module-gconf.c | 295 +-------------------- > src/modules/gsettings/gsettings-helper.c | 119 +++++++++ > src/modules/gsettings/module-gsettings.c | 111 ++++++++ > .../org.freedesktop.pulseaudio.gschema.xml | 98 +++++++ > src/modules/stdin-util.c | 281 ++++++++++++++++++++ > src/modules/stdin-util.h | 89 +++++++ > 8 files changed, 752 insertions(+), 293 deletions(-) > create mode 100644 src/modules/gsettings/gsettings-helper.c > create mode 100644 src/modules/gsettings/module-gsettings.c > create mode 100644 src/modules/gsettings/org.freedesktop.pulseaudio.gschema.xml > create mode 100644 src/modules/stdin-util.c > create mode 100644 src/modules/stdin-util.h > > diff --git a/configure.ac b/configure.ac > index 7fbecab..7845a31 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -926,6 +926,24 @@ 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([--enable-gsettings],[Enable optional GSettings support])) > + > +AS_IF([test "x$enable_gsettings" = "xyes"], We probably should enable gsettings by default. However, if it turns out that the configuration migration doesn't work smoothly if both module-gconf and module-gsettings are loaded, then we should make sure that only one of those modules is enabled by default. The reason why it might not work smoothly is that module-gsettings might start loading modules before module-gconf has unloaded its modules. And I'm not sure that the gconf configuration is cleared during the migration, in which case module-gconf won't unload anything. > + [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]) > + > +if test "x$enable_gsettings" = "xyes" && test "x$HAVE_GSETTINGS" = "x1" ; then Testing $enable_gsettings is redundant, we know that it's "yes" if $HAVE_GSETTINGS is 1. > + GLIB_GSETTINGS > +fi > diff --git a/src/modules/gsettings/gsettings-helper.c b/src/modules/gsettings/gsettings-helper.c > new file mode 100644 > index 0000000..e0a00a6 > --- /dev/null > +++ b/src/modules/gsettings/gsettings-helper.c > @@ -0,0 +1,119 @@ > +/*** > + This file is part of PulseAudio. > + > + Copyright 2006 Lennart Poettering > + > + PulseAudio 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. > + > + PulseAudio 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 > + General Public License for more details. > + > + You should have received a copy of the GNU Lesser General Public License > + along with PulseAudio; if not, see <http://www.gnu.org/licenses/>. > +***/ > + > +#ifdef HAVE_CONFIG_H > +#include <config.h> > +#endif > + > +#include <string.h> > +#include <stdlib.h> > +#include <stdio.h> > + > +#include <gio/gio.h> > +#include <glib.h> > + > +#include <pulsecore/core-util.h> > + > +#define PA_GSETTINGS_MODULE_SCHEMA "org.freedesktop.pulseaudio.module" > +#define PA_GSETTINGS_MODULES_SCHEMA "org.freedesktop.pulseaudio.modules" > +#define PA_GSETTINGS_MODULES_PATH "/org/freedesktop/pulseaudio/modules/" > + > +static void modules_callback(GSettings *settings, gchar *key, gpointer user_data); > + > +static void handle_module(gchar *name) { > + GSettings *settings; > + gchar p[1024]; > + gboolean enabled; > + int i; > + > + pa_snprintf(p, sizeof(p), PA_GSETTINGS_MODULES_PATH"%s/", name); > + > + if (!(settings = g_settings_new_with_path(PA_GSETTINGS_MODULE_SCHEMA, > + p))) > + return; > + > + enabled = g_settings_get_boolean(settings, "enabled"); > + > + printf("%c%s%c", enabled ? '+' : '-', name, 0); > + > + if (enabled) { > + for (i = 0; i < 10; i++) { > + gchar *n, *a; > + > + pa_snprintf(p, sizeof(p), "name%d", i); > + n = g_settings_get_string(settings, p); > + > + pa_snprintf(p, sizeof(p), "args%i", i); > + a = g_settings_get_string(settings, p); > + > + printf("%s%c%s%c", n, 0, a, 0); > + > + g_free(n); > + g_free(a); > + } > + > + printf("%c", 0); > + } > + > + fflush(stdout); > + > + g_object_unref(G_OBJECT(settings)); > +} > + > +static void modules_callback(GSettings *settings, gchar *key, gpointer user_data) { > + handle_module(user_data); > +} > + > +int main(int argc, char *argv[]) { > + GMainLoop *g; > + GSettings *settings; > + gchar **modules, **m; > + > +#if !GLIB_CHECK_VERSION(2,36,0) > + g_type_init(); > +#endif > + > + if (!(settings = g_settings_new(PA_GSETTINGS_MODULES_SCHEMA))) > + goto fail; > + > + g_signal_connect(settings, "changed", (GCallback) modules_callback, NULL); The "changed" signal on the modules object can't use modules_callback(), because modules_callback() expects to get the module object name as the user_data argument, but you're passing NULL here. We need to deal with new module objects appearing and disappearing, but it's not clear to me how that should be done. The "changed" signal is documented as follows: The "changed" signal is emitted when a key has potentially changed. The modules object doesn't have any keys, however. We're interested in children, not keys. The documentation for g_settings_list_children() mentions the "children-changed" signal: For GSettings objects that are lists, this value can change at any time and you should connect to the "children-changed" signal to watch for those changes. The "children-changed" signal isn't documented, however, so I'm not sure if it actually exists. Also, how can the modules schema declare that it's a list? Or does it not have to declare that? Since g_settings_new_with_path() can be used to dynamically create children, I'm pretty sure that dynamic childern are possible, but the GSettings documentation fails to tell how the dynamic children are supposed to be tracked (or I'm just bad at reading the documentation). > + > + modules = g_settings_list_children (settings); > + > + for (m = modules; *m; m++) { > + g_signal_connect(g_settings_get_child (settings, *m), "changed", > + (GCallback) modules_callback, *m); g_settings_get_child() may return NULL if the child was removed after the g_settings_list_children() call. That needs to be handled. Also, g_settings_get_child() has the "transfer full" tag in the documentation, which I think means that we have to unref the returned object. > + handle_module(*m); > + } The modules variable needs to be freed with g_strfreev(). -- Tanu https://liberapay.com/tanuk https://www.patreon.com/tanuk
