Re: [PATCH 2/5] core: Add API to handle "content-changed" signal

["Juan A." Suárez Romero <jasuarez igalia com>]

On Tue, 2011-02-01 at 11:16 +0000, Iago Toral wrote:
> > diff --git a/src/grl-media-source.c b/src/grl-media-source.c
> > index fd2e804..215b282 100644
> > --- a/src/grl-media-source.c
> > +++ b/src/grl-media-source.c
> > @@ -2045,6 +2045,8 @@ grl_media_source_supported_operations
> > (GrlMetadataSource *metadata_source)
> >    if (media_source_class->test_media_from_uri &&
> >        media_source_class->media_from_uri)
> >      caps |= GRL_OP_MEDIA_FROM_URI;
> > +  if (media_source_class->notify_changed_start)
> > +    caps |= GRL_OP_NOTIFY_CHANGED;
> 
>  I think we need to demand plugins to implement both, start and stop. 
>  does not make sense to have one without the other I think.
> 

Fair enough. This will simplify code.


> > +
> > +/**
> > + * grl_media_source_notify_changed_start:
> > + * @source: a media source
> > + * @error: a #GError, or @NULL
> > + *
> > + * Starts signal "content-changed" emission when source finds
>  
>  I would use this wording instead:
> 
>  Starts emitting "content-changed" signals when @source discovers 
>  changes in the content. This instructs @source to setup the machinery 
>  needed to be aware of changes in the content.
> 
> > changes in the content.

Good.

> > + * grl_media_source_notify_changed_stop:
> > + * @source: a media source
> > + *
> > + * Stops emitting "content-changed" signal.
> 
>  I would use this wording instead:
> 
>  This will drop emission of content-changed signals from @source. When 
>  this is done @source should stop the machinery required for it to track 
>  changes in the content.
> 

Good.

> > +{
> > +  g_return_if_fail (GRL_IS_MEDIA_SOURCE (source));
> > +  g_return_if_fail (!media || GRL_IS_MEDIA (media));
> > +
> > +  g_signal_emit (source,
> > +                 registry_signals[SIG_CONTENT_CHANGED],
> > +                 0,
> > +                 media,
> > +                 change_type,
> > +                 location_unknown);
> > +}
> 
>  didn't you say that this helper would take care of the NULL media use 
>  case?
> 
>  I think we should also add in the documentation that this particular 
>  API is intended only for plugin developers...
> 

Yes. I forgot to add it in the documentation.

And now that you mention it, either if you are writing a plugin or an
application, you always include the same grilo.h header. There is no
splitting of functions.

I wonder if it is worth to keep to headers, one for application
developers and another for plugins. Thus grilo-plugins.h would include
grilo.h plus all functions specifically designed to write plugins (like
this change_notify() one), and not intended to be used by clients.


> >    /*< private >*/
> >    gpointer _grl_reserved[GRL_PADDING];
> 
>  Again, you are breaking the ABI here, you have to remove two extra 
>  slots from _grl_reserved for these.


Sure. As I said previously, I forgot to fix this issue.