Re: supporting notifications from backends

[Cyrille Moureaux <Cyrille Moureaux Sun COM>]

Hi Mark,

> 	So, I spent some time mulling over your patch for bug #107692[1],
> reminding myself of GConf's weird internal semantics and trying to
> figure out whether we were missing any obvious corner cases.
>  
Thanks for looking into it.

> 	Your patch generally looks good. I've made some changes:
>
>  + Extended the backend vtable rather than adding a new one
>  
Fine by me.

>  + Removed the init_listeners function as we don't have an initial
>    listeners list (as you say) and its more usual to allow different 
>    callbacks per-listener, even if we don't use them.
>
>  + Added an identifier for each listener which we can use in order to 
>    remove the listeners. Its just re-using the client listener id. 
>    Since the backend can be used multiple times in a single stack, the 
>    listener id will only be unique within the context of a single 
>    GConfSource.
>  
I initially thought about using a similar scheme, allowing multiple 
listeners to be added/removed, but then couldn't really see the benefit 
of it. The only client of the backends is the daemon itself, so it 
seemed to make sense to have one function for setting up the change 
detection system (the init function) and then another to keep the 
backends informed of the only relevant information, which is the list of 
locations of interest.

From the point of view of the relationship between the backend and the 
daemon, the addition of yet another client listener for 
/apps/metacity/general when there's already a client listening for 
/apps/metacity shouldn't change the state of the backend, it should 
still only report changes to anything below /apps/metacity to the daemon 
for further action.

If there's an add_listener called for each client listener, all backends 
have to do some comparisons to realise that the same callback/user data 
pair is being passed each time so that they can ignore most of the 
requests, otherwise if they just assume all listeners passed are somehow 
different, everytime something changes in the store the daemon is going 
to be called once per client listening to that point.

Anyway, I'm not really opposed to the add/remove pair you've introduced, 
but I'm not sure it's that useful. At any rate, these were the reasons I 
didn't use that scheme in the first place (but I agree it leaves more 
flexibility for possible evolutions).

>  + Fixed a leak of the ConfigValue in source_notify_cb()
>  
Doh! Thanks for that.

>  + Added a FIXME to source_notify_cb(). Basically, we should be able to
>    prevent notifying the client of a change if the change won't 
>    actually cause the client to see a different value. I'm going to try
>    and get the patch in #88829 merged soon too and we should be able to
>    re-use some of that to fix this. See gconf_sources_is_affected() in
>    the patch with that bug.
>  
If I'm correct there's already a mechanism higher up the chain to trap 
multiple notifications about the same change and notifications which 
actually do not reflect a change, so I don't think the potential problem 
could go all the way up to the client. But it's definitely a good idea 
to prevent the situation as soon as possible.

>  + Added documentation and cleaned up the coding style a bit
>  
And to think I had made the effort of writing all in lower case and 
underscores and funny spacing to match the rest of the code... ;-)

> 	That patch is attached below. Let me know if you see any problems with
> it.
>  
I don't have major issues, just a small comment on both 
gconf_sources_add_listener and gconf_sources_remove_listeners which 
start with:

+ GList *tmp;
+
+  tmp = sources->sources;
+
+  while (sources != NULL)

I think it should be "while (tmp != NULL)" since sources is unlikely to 
change (hopefully) and it's a bit late to check its nullity when it's 
already been dereferenced.

Thanks for the review,

Cyrille