Re: [PATCH 1/5] core: Add "content-changed" signal

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

On Tue, 2011-02-01 at 10:59 +0000, Iago Toral wrote:
> Give me some time to review this feature.

Thanks.

>  Some comments about this particular patch follow below:
> 

>  I would use this wording instead:
>     * @media: the media that changed or one of its ancestors.
> 
>  I would use this wording instead:
>    * @change_type: the kind of change that occurred
> 
>  I would use this wording instead:
>    * @location_unknown: @TRUE if the change happened in @media itself or 
>  in one of its direct children (when @media is a box). @FALSE otherwise.
> 
> > +   * undirect descendant.
>         ^^^^^^^^^^^^^^^^^^^^ I would use this wording instead:
>        * other descendant of the box in the hierarchy.
> 
> > +   * For the cases where the source only can signal that a change
>                                         ^^^^^^^^
>                                         can only
> > location_unknown as
>                                                               
>  ^^^^^^^^^^^^^^^
>                                                            and set 
>  location_unkown to


Nice. I'll change it.

> > +typedef enum {
> > +  GRL_CONTENT_CHANGED,
> > +  GRL_CONTENT_ADDED,
> > +  GRL_CONTENT_REMOVED,
> > +} GrlMediaSourceChangeType;
> >

>  I think we should be mre specific here, at least for the _CHANGE case. 
>  When exactly are we going to use _CHANGE instead of _ADDED/_REMOVED?
> 

In the case of GrlMediaBox, I would use _CHANGED either if some property
has changed (for instance, the folder has been renamed), or if several
children has been added/removed.

I see _CHANGED as the general way of telling something changed, without
being able to tell if it was due new content or removed content.

> >  /* GrlMediaSource object */
> >
> > @@ -382,6 +395,12 @@ struct _GrlMediaSourceClass {
> >
> >    /*< private >*/
> >    gpointer _grl_reserved[GRL_PADDING];
> > +
> > +  /* signals */
> > +  void (*content_changed) (GrlMediaSource *source,
> > +                           GrlMedia *media,
> > +                           GrlMediaSourceChangeType change_type,
> > +                           gboolean location_unknown);
> >  };
> 
> 
>  Hey! you are breaking the ABI here :)
>  you have to remove one slot from the _grl_reserved[] array to keep the 
>  size of the structure constant when you added the signal:
> 
>  gpointer _grl_reserved[GRL_PADDING - 1];
> 
>  Always keep that in mind when adding/removing APIs from now on.
> 

Oh, yes. I already knew it, but I forgot to fix it.

> >  G_BEGIN_DECLS
> > @@ -502,6 +521,7 @@ GrlMedia
> > *grl_media_source_get_media_from_uri_sync (GrlMediaSource *source,
> >                                                      const GList 
> > *keys,
> >
> > GrlMetadataResolutionFlags flags,
> >                                                      GError **error);
> > +
> >  G_END_DECLS
> 
>  Aren't this grl-type-builtin* files auto-generated? In that case we 
>  should not have them stored.

grl-type-builtin.?.template are not autogenerated. grl-type-builtin.?
are generated from the templates.


	J.A.