Re: [Multi-valued V2 (grilo) 07/12] core: Create property with an initial set of keys

[Iago Toral <itoral igalia com>]

On Tue,  1 Mar 2011 10:50:26 +0100, "Juan A. Suarez Romero" 
<jasuarez igalia com> wrote:
> Signed-off-by: Juan A. Suarez Romero <jasuarez igalia com>
> ---
>  src/data/grl-property.c |   23 +++++++++++++++++++++++
>  src/data/grl-property.h |    2 ++
>  2 files changed, 25 insertions(+), 0 deletions(-)
>
> diff --git a/src/data/grl-property.c b/src/data/grl-property.c
> index f3d21bd..7605697 100644
> --- a/src/data/grl-property.c
> +++ b/src/data/grl-property.c
> @@ -59,3 +59,26 @@ grl_property_new (void)
>
>    return grlprop;
>  }

I miss some gtk-doc in this class explaining in more detail its purpose 
and when it should be used.

> +/**
> + * grl_property_new_with_keys:
> + * @related_keys: (element-type Grl.KeyID): list of keys.
> + *
> + * Creates a new place to store related keys and values.

Creates a new property instance that can be used to store related keys 
and values.

> + * Initially it will contain the specified set of keys with no 
> values.

This constructors looks like something that is half done. I think we 
should provide users with a constructor with no arguments that just 
creates an empty property and then we let users set independent 
key/value pairs on it or we provide them with a constructor that can set 
both the keys and the values in one go. But this version does not look 
right to me (and yes, I know you can just set the keys now and set the 
values later, but still).

> + * Returns: a new #GrlProperty
> + **/
> +GrlProperty *
> +grl_property_new_with_keys (const GList *keys)
> +{
> +  GrlProperty *prop = grl_property_new ();
> +
> +  while (keys) {
> +    grl_data_add (GRL_DATA (prop), keys->data);

Do we have API in GrlData for adding a key with no value like this? 
Please let's not do strange things. grl_data_add should receive a key 
and a value.

> +    keys = g_list_next (keys);
> +  }
> +
> +  return prop;
> +}