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

[Iago Toral <itoral igalia com>]

Give me some time to review this feature.

Some comments about this particular patch follow below:

On Tue,  1 Feb 2011 09:07:42 +0100, "Juan A. Suarez Romero" 
<jasuarez igalia com> wrote:
(...)
> +  /**
> +   * GrlMediaSource::content-changed:
> +   * @source: source that has changed
> +   * @media: the media that has changed, or contains the children 
> that have
> +   * changed

I would use this wording instead:
   * @media: the media that changed or one of its ancestors.


> +   * @change_type: what has happened

I would use this wording instead:
  * @change_type: the kind of change that occurred

> +   * @location_unknown: @TRUE if the change can be in the @media or 
> in any

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.


> +   * descendant of it. @FALSE means that either the @media has 
> changed, or a
> +   * direct child has changed
> +   *
> +   * Signals that the content in the source has changed. Usually 
> @media is a
> +   * #GrlBox, meaning that the content of that box has changed. if
> +   * @location_unknown is @TRUE it means the source can establish 
> where the
                                                       ^^^ cannot
> +   * change happened: could be either in the box, in any child, or 
> in any
> +   * 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
> happened, but
> +   * not where, it would use the root box (@NULL id) and 
> location_unknown as
                                                             
^^^^^^^^^^^^^^^
                                                          and set 
location_unkown to
> +   * #TRUE.
> +   */
> +  registry_signals[SIG_CONTENT_CHANGED] =
> +    g_signal_new("content-changed",
> +                 G_TYPE_FROM_CLASS (gobject_class),
> +                 G_SIGNAL_RUN_FIRST | G_SIGNAL_ACTION,
> +                 G_STRUCT_OFFSET(GrlMediaSourceClass, 
> content_changed),
> +                 NULL,
> +                 NULL,
> +                 grl_marshal_VOID__OBJECT_ENUM_BOOLEAN,
> +                 G_TYPE_NONE,
> +                 3,
> +                 GRL_TYPE_MEDIA,
> +                 GRL_TYPE_MEDIA_SOURCE_CHANGE_TYPE,
> +                 G_TYPE_BOOLEAN);
>  }
>
>  static void
> diff --git a/src/grl-media-source.h b/src/grl-media-source.h
> index 6975004..83a5360 100644
> --- a/src/grl-media-source.h
> +++ b/src/grl-media-source.h
> @@ -63,6 +63,19 @@
>    (G_TYPE_INSTANCE_GET_CLASS ((obj),                            \
>                                GRL_TYPE_MEDIA_SOURCE,            \
>                                GrlMediaSourceClass))
> +/**
> + * GrlMediaSourceChangeType:
> + * @GRL_CONTENT_CHANGED: content has changed.
> + * @GRL_CONTENT_ADDED: new content has been added.
> + * @GRL_CONTENT_REMOVED: content has been removed
> + *
> + * Specifies which kind of change has happened in the plugin
> + */
> +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?

>  /* 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.

>  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.

>  #endif /* _GRL_MEDIA_SOURCE_H_ */
> diff --git a/src/grl-type-builtins.c.template
> b/src/grl-type-builtins.c.template
> new file mode 100644
> index 0000000..734d23a
> --- /dev/null
> +++ b/src/grl-type-builtins.c.template
> @@ -0,0 +1,35 @@
> +/*** BEGIN file-header ***/
> +#include "grl-type-builtins.h"
> +#include "@filename@"
> +/*** END file-header ***/
> +
> +/*** BEGIN file-production ***/
> +/* enumerations from "@filename@" */
> +/*** END file-production ***/
> +
> +/*** BEGIN value-header ***/
> +GType
> +@enum_name@_get_type (void)
> +{
> +    static GType etype = 0;
> +    if (G_UNLIKELY(etype == 0)) {
> +        static const G@Type@Value values[] = {
> +/*** END value-header ***/
> +
> +/*** BEGIN value-production ***/
> +            { @VALUENAME@, "@VALUENAME@", "@valuenick@" },
> +/*** END value-production ***/
> +
> +/*** BEGIN value-tail ***/
> +            { 0, NULL, NULL }
> +        };
> +        etype = g_@type@_register_static (g_intern_static_string
> ("@EnumName@"), values);
> +    }
> +    return etype;
> +}
> +
> +/*** END value-tail ***/
> +
> +/*** BEGIN file-tail ***/
> +
> +/*** END file-tail ***/
> diff --git a/src/grl-type-builtins.h.template
> b/src/grl-type-builtins.h.template
> new file mode 100644
> index 0000000..6714f84
> --- /dev/null
> +++ b/src/grl-type-builtins.h.template
> @@ -0,0 +1,24 @@
> +/*** BEGIN file-header ***/
> +#ifndef _GRL_TYPE_BUILTINS_H_
> +#define _GRL_TYPE_BUILTINS_H_
> +
> +#include <glib-object.h>
> +
> +G_BEGIN_DECLS
> +/*** END file-header ***/
> +
> +/*** BEGIN file-production ***/
> +
> +/* enumerations from "@filename@" */
> +/*** END file-production ***/
> +
> +/*** BEGIN value-header ***/
> +GType @enum_name@_get_type (void) G_GNUC_CONST;
> +#define @ENUMPREFIX@_TYPE_@ENUMSHORT@ (@enum_name@_get_type())
> +/*** END value-header ***/
> +
> +/*** BEGIN file-tail ***/
> +G_END_DECLS
> +
> +#endif /* _GRL_TYPE_BUILTINS_H_ */
> +/*** END file-tail ***/