Re: [Multi-valued V2 (grilo) 04/12] core: Add API to handle multivalued data

[Iago Toral <itoral igalia com>]

On Tue,  1 Mar 2011 10:50:23 +0100, "Juan A. Suarez Romero" 
<jasuarez igalia com> wrote:
> Signed-off-by: Juan A. Suarez Romero <jasuarez igalia com>
> ---
>  src/data/grl-data-multi.c |  307
> +++++++++++++++++++++++++++++++++++++++++++--
>  src/data/grl-data-multi.h |   12 ++-
>  2 files changed, 308 insertions(+), 11 deletions(-)
>
> diff --git a/src/data/grl-data-multi.c b/src/data/grl-data-multi.c
> index 2047bc7..819e5e7 100644
> --- a/src/data/grl-data-multi.c
> +++ b/src/data/grl-data-multi.c
> @@ -34,6 +34,7 @@
>
>  #include "grl-data-multi.h"
>  #include "grl-log.h"
> +#include "grl-plugin-registry.h"
>
>  struct _GrlDataMultiPrivate {
>    GHashTable *extended_data;
> @@ -44,17 +45,11 @@ static void grl_data_multi_finalize (GObject 
> *object);
>  #define GRL_DATA_MULTI_GET_PRIVATE(o)                                
>   \
>    (G_TYPE_INSTANCE_GET_PRIVATE ((o), GRL_TYPE_DATA_MULTI,
> GrlDataMultiPrivate))
>
> -G_DEFINE_TYPE (GrlDataMulti, grl_data_multi, GRL_TYPE_DATA);
> +static void free_list_values (GrlKeyID key, GList *values, gpointer
> user_data);
>
> -static void
> -free_list_values (GrlKeyID key,
> -                  GList *values,
> -                  gpointer user_data)
> -{
> -  g_list_foreach (values, (GFunc) g_object_unref, NULL);
> -  g_list_free (values);
> -}
> +/* ================ GrlDataMulti GObject ================ */
>
> +G_DEFINE_TYPE (GrlDataMulti, grl_data_multi, GRL_TYPE_DATA);
>
>  static void
>  grl_data_multi_class_init (GrlDataMultiClass *klass)
> @@ -89,6 +84,17 @@ grl_data_multi_finalize (GObject *object)
>    G_OBJECT_CLASS (grl_data_multi_parent_class)->finalize (object);
>  }
>
> +/* ================ Utitilies ================ */
> +
> +static void
> +free_list_values (GrlKeyID key, GList *values, gpointer user_data)
> +{
> +  g_list_foreach (values, (GFunc) g_object_unref, NULL);
> +  g_list_free (values);
> +}
> +
> +/* ================ API ================ */
> +
>  /**
>   * grl_data_multi_new:
>   *
> @@ -102,3 +108,286 @@ grl_data_multi_new (void)
>    return g_object_new (GRL_TYPE_DATA_MULTI,
>  		       NULL);
>  }
> +
> +/**
> + * grl_data_multi_add:
> + * @mdata: a multivalued data
> + * @prop: a set of related properties with their values
> + *
> + * Adds a new set of values.
> + *
> + * All keys in prop must be related among them.
> + *
> + * mdata will take the ownership of prop, so do not modify it.
> + **/
> +void
> +grl_data_multi_add (GrlDataMulti *mdata,
> +                    GrlProperty *prop)

Well, since you decided to do incremental patches from the previous 
version I don't know

> +{
> +  GList *key;
> +  GList *keys;
> +  GList *values;
> +  GrlPluginRegistry *registry;
> +  const GList *related_keys;
> +  guint l;
> +
> +  g_return_if_fail (GRL_IS_DATA_MULTI (mdata));
> +  g_return_if_fail (GRL_IS_PROPERTY (prop));
> +
> +  keys = grl_data_get_keys (GRL_DATA (prop));
> +  if (!keys) {
> +    /* Ignore empty properties */
> +    GRL_WARNING ("Empty set of values");

As I mentioned in a previous review: let's try to be more descriptive 
with the warning messages. If I see this message in a log I won't have a 
clue of what is it all about. Why not: "Trying to set values on a 
GrlData instance from a GrlProperty without contents" instead?

Please review all your warning messages in this and following patches 
:)

> +    g_object_unref (prop);
> +    return;
> +  }
> +

Iago