OOps, sorry, this is of course a patch against g-s-d. Sorry for the confusion!

Created attachment 115516 [details] [review]
the patch

Why couldn't this be done in a Pulseaudio module instead?

Think of thin clients: PA and the X server run for a long time and on remote networked machine, do you really think it would be good design, if they would connect back to gconf/... of the client that logs in? I don't think so. If client configuration changes, then the client has to reconfigure the server. It shouldn't be the other way round, that the server hooks into the client and checks if its state changes. Also, if PA runs on a remote server it won't have access to the file system of the client and thus cannot monitor file system changes.

Also, running the glib mainloop inside of PA is still problematic, due to RT, lock-free, yadda, yadda. Can be fixed of course, but that actually turns out to be more complex than it appeared to me at first. I will do this eventually, however even if I have that i still think it would make more sense to have this on the client side, and hence in g-s-d, due to the aforementioned reasons.

Created attachment 115523 [details] [review]
updated patch, including gconf schema

Why do you listen to GConf, when the property is exported through X properties? Also, that code could be integrated in your x11 module for PA.

(In reply to comment #6)
> Why do you listen to GConf, when the property is exported through X properties?

But why? This code shall live in g-s-d, as does the code that binds gconf to the xsettings properties. So I see no problem in binding the cache flush directly to the gconf changes here, there is no need for further abstraction. Also, hooking into Gtksettings requires a patched gtk version for now, which makes things more fragile. And then, why have the notifications go through 3 layers of abstraction, if 1 is already more than enough?

> Also, that code could be integrated in your x11 module for PA.

Sure, but the file system monitoring couldn't be moved into pa. Hence, the cache flushing part would need to be duplicated in the pa module and in the g-s-d module. Which sucks. And then, the code in this patch is able to coalesce flushes due to file system changes and those due to gconf changes. This wouldn't be possible if we'd have the code for cashe flushing in two different pieces of code, that might live on different systems.

And finally: why should this be moved into PA?

Provided the outcome of your discussion is g-s-d (I have no opinion since I have no clue about PA) I'd prefer to see this integrated in the sound plugin instead of yet another new micro-plugin.

The reason I put this in a seperate module is that the entire sound plugin should be removed sooner or later.  The sound module starts esd and uploads samples. The former is now done from g-s via a normal autostart desktop file (for pa of course, not esd), the latter is now done on-demand from libcanberra, and doesn't need to be done on session login anymore. Hence the whole point in having the sound module is no more.

I'd like to keep it around however for one release or so, for those people who don't want to switch to PA just yet.

Yes, I do expect the sound module to go away or at least shrink considerably when esd is phased out. If the caching stuff is cleanly separated from what's there right now removing the cruft won't be any harder than removing an entire module, and a "sound" module just makes a whole lot more sense long-term than a "sound-sample-cache" plugin.

But, why again do you want to mix these two codebases although they do completely different things? They happen to share a word in the name ("sound"), but that alone is not reason enough I'd say to merge them, given that the old module is going to go away anyway.

The "sound" plugin handles sound settings. Unfortunately, right now that means supporting 2 (or one and a half) different implementations. I don't want a "sound-sample-cache" plugin because it's just too specialized and because renaming and migrating the stuff later (when esd is gone) would be great fun.

Anyway, has Bastien conceded?

I'd still have preferred for the code to go straight into PulseAudio, but I'm happy for it to be available as a g-s-d plugin. I don't see the point in trying to integrate it now when we'll need to de-integrate it later.

Humour me. All the "integration" is a couple lines in _start and _stop.

+       PKG_CHECK_MODULES(PULSE, libpulse >= 0.9.11,
+	       AC_DEFINE(HAVE_PULSE, 1, [Define if PULSE sound server should be used]),
+               AC_MSG_RESULT([disabled]))

The AC_MSG_RESULT is wrong. The else branch should just be empty.

+sample_info_cb(pa_context *c, const pa_sample_info *i, int eol, void *userdata)

Please change to using "function (..)".

+        g_return_val_if_fail(!error || !*error, FALSE);

Pretty much all the _if_fail () calls should be "if () return...;" instead. They are for indicating programmer errors, not valid runtime issues.

+        f = g_file_new_for_path(path);
+        g_return_val_if_fail(f, FALSE);

... except for cases which cannot actually fail.

+        g_object_unref(G_OBJECT(f));

Cast is unnecessary.

(In reply to comment #14)

> +        g_return_val_if_fail(!error || !*error, FALSE);
> 
> Pretty much all the _if_fail () calls should be "if () return...;" instead.
> They are for indicating programmer errors, not valid runtime issues.

Jesns, programmer errors should be caught using g_return_val_if_fail().  The patch is correct.  Why do you think it should be the other way?

Now that the sound plugin is gone, can we rename this to sound and commit please?  Fedora 10 is already shipping this.

(In reply to comment #15)
> (In reply to comment #14)
> Jesns, programmer errors should be caught using g_return_val_if_fail().  The
> patch is correct.  Why do you think it should be the other way?

Maybe you should look at it in context:

+        gconf_client_add_dir(client, GCONF_SOUND_DIR, GCONF_CLIENT_PRELOAD_NONE, error);
+        g_return_val_if_fail(!error || !*error, FALSE);

> Now that the sound plugin is gone, can we rename this to sound and commit
> please?  Fedora 10 is already shipping this.

Sure, fix the issues, and in it goes.

Created attachment 122541 [details] [review]
updated to 2.25.1

Still want it as "sound". Also, please look at the review in comment 14.

2009-01-08  Jens Granseuer  <...>

        Based on a patch by: Lennart Poettering <...>

        * configure.ac:
        * data/gnome-settings-daemon.schemas.in:
        * plugins/Makefile.am:
        * plugins/sound/Makefile.am:
        * plugins/sound/gsd-sound-manager.c:
        * plugins/sound/gsd-sound-plugin.h:
        * plugins/sound/sound.gnome-settings-plugin.in:
        Add a new sound plugin that tells PulseAudio to drop its sample
        cache when the sound theme changes (bug #545386).

Is this plugin be loaded on idle? Probably doesn't need to delay the GNOME login.

We can't load plugins on idle. And this one hardly does anything so I doubt starting would turn up high on a profile.