[pulseaudio-discuss] [PATCH] module-gsettings: new module to store configuration using gsettings
Tanu Kaskinen
tanuk at iki.fi
Wed Apr 4 07:45:43 UTC 2018
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
More information about the pulseaudio-discuss
mailing list