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

[Iago Toral <itoral igalia com>]

Ok, I see you fixed this and other things I had mentioned, like the ABI 
issues etc in later patches in the series.
The commit history looks a bit weird like this IMHO. For example, first 
you have a commit that breaks the ABI, and later on a commit to fix that 
and this procedure is repeated for almost all the things I mentioned... 
Please , check if you can reorder/squash some of the commits. Other than 
this and a minor typo I sent before I think it is ready to be pushed.

Iago

On Wed, 02 Feb 2011 07:14:40 +0000, Iago Toral <itoral igalia com> 
wrote:
> On Tue,  1 Feb 2011 18:09:07 +0100, "Juan A. Suarez Romero"
> <jasuarez igalia com> wrote:
> (...)
> > +gboolean
> > +grl_media_source_notify_changed_start (GrlMediaSource *source,
> > +                                       GError **error)
> > +{
> > +  g_return_val_if_fail (GRL_IS_MEDIA_SOURCE (source), FALSE);
> > +  g_return_val_if_fail (grl_media_source_supported_operations
> > (GRL_METADATA_SOURCE (source)) &
> > +                        GRL_OP_NOTIFY_CHANGED, FALSE);
> > +
> > +  return GRL_MEDIA_SOURCE_GET_CLASS (source)->notify_changed_start 
> > (source,
> > +                                                                    
> > error);
> > +}
> > +
> > +/**
> > + * grl_media_source_notify_changed_stop:
> > + * @source: a media source
> > + *
> > + * 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.
> > + */
> > +void grl_media_source_notify_changed_stop (GrlMediaSource *source)
> > +{
> > +  g_return_if_fail (GRL_IS_MEDIA_SOURCE (source));
> > +  GrlMediaSourceClass *source_class = GRL_MEDIA_SOURCE_GET_CLASS 
> > (source);
> > +
> > +  if (source_class->notify_changed_stop) {
> > +    source_class->notify_changed_stop (source);
> > +  }
> > +}
>
> For consistency, I would rather have:
>
> g_return_val_if_fail (grl_media_source_supported_operations
> (GRL_METADATA_SOURCE (source)) &
>                         GRL_OP_NOTIFY_CHANGED, FALSE);
>
> and remove the if() checking for the presence of notify_changed_stop
> below, just like in notify_start.
>
> Iago
> _______________________________________________
> grilo-list mailing list
> grilo-list gnome org
> http://mail.gnome.org/mailman/listinfo/grilo-list