Re: [Multi-valued V2 (grilo) 11/12] core: Merge GrlDataMulti into GrlData

[Iago Toral <itoral igalia com>]

On Tue,  1 Mar 2011 10:50:30 +0100, "Juan A. Suarez Romero" 
<jasuarez igalia com> wrote:
(...)
> +/* ================ GrlData GObject ================ */
> +
> +G_DEFINE_TYPE (GrlData, grl_data, G_TYPE_OBJECT);
>
>  static void
>  grl_data_class_init (GrlDataClass *klass)
>  {
>    GObjectClass *gobject_class = (GObjectClass *)klass;
>
> -  gobject_class->set_property = grl_data_set_property;
> -  gobject_class->get_property = grl_data_get_property;
> +  gobject_class->set_property = grl_data_set_gobject_property;
> +  gobject_class->get_property = grl_data_get_gobject_property;

Maybe GrlProperty was not a good name after all, I'd rather keep 
grl_data_set/get_property like they were, it is confusing otheriwse.
I see two options:

a) Rename GrlProperty to something else. Maybe GrlRelated?
b) Use grl_set/get_related_property instead of grl_set/get_property for 
setting/getting GrlProperty instances.

Right now I think a) is the best choice.

>    gobject_class->finalize = grl_data_finalize;
>
>    g_type_class_add_private (klass, sizeof (GrlDataPrivate));
> @@ -98,21 +98,28 @@ grl_data_init (GrlData *self)
>    self->priv->data = g_hash_table_new_full (g_direct_hash,
>                                              g_direct_equal,
>                                              NULL,
> -                                            (GDestroyNotify) 
> free_val);
> +                                            NULL);
>  }

(...)

(...)

>  /**
> + * grl_data_add_property:
> + * @data: data to change
> + * @prop: a set of related properties with their values
> + *
> + * Adds a new set of values.
> + *
> + * All keys in prop must be related among them.
> + *
> + * @data will take the ownership of prop, so do not modify it.
> + **/
> +void
> +grl_data_add_property (GrlData *data,
> +                       GrlProperty *prop)
> +{
> +  GList *keys;
> +  GList *list_prop;
> +  GrlKeyID sample_key;
> +
> +  g_return_if_fail (GRL_IS_DATA (data));
> +  g_return_if_fail (GRL_IS_PROPERTY (prop));
> +
> +  keys = grl_property_get_keys (prop, TRUE);
> +  if (!keys) {
> +    /* Ignore empty properties */
> +    GRL_WARNING ("Empty set of values");
> +    g_object_unref (prop);
> +    return;
> +  }
> +
> +  sample_key = get_sample_key (keys->data);
> +  g_list_free (keys);
> +
> +  if (!sample_key) {
> +    g_object_unref (prop);
> +    return;
> +  }
> +
> +  list_prop = g_hash_table_lookup (data->priv->data, sample_key);
> +  list_prop = g_list_append (list_prop, prop);
> +  g_hash_table_insert (data->priv->data, sample_key, list_prop);
> +}

For all the "add" APIs below, I would be more explicit in the 
description of how they handle related keys. For example, for add_string 
I would say:

"Appends a new string value for @key in @data. If there are other keys 
that are related to @key, NULL values will be appended for each of them 
too.

> +/**
> + * grl_data_add_string:
> + * @data: data to append
> + * @key: (type Grl.KeyID): key to append
> + * @strvalue: the new value
> + *
> + * Appends a new string value for @key in @data. All related keys 
> are set to
> + * %NULL.
> + **/
> +void
> +grl_data_add_string (GrlData *data,
> +                     GrlKeyID key,
> +                     const gchar *strvalue)
> +{
> +  GrlProperty *prop;
> +
> +  prop = grl_property_new_for_key (key);
(...)

> +/**
> + * grl_data_get_all_single_property:
> + * @mdata: a data
> + * @key: a metadata key
> + *
> + * Returns all non-%NULL values for specified key. This ignores
> completely the
> + * related keys.

Returns all non-%NULL values for @key. This ignores related keys.

> + * Returns: (element-type GObject.Value) (transfer container): a 
> #GList with
> + * values. Do not change or free the values. Free the list with
> #g_list_free.
> + **/
> +GList *
> +grl_data_get_all_single_property (GrlData *data,
> +                                  GrlKeyID key)
> +{
> +  GrlKeyID sample_key;
> +
> +  g_return_val_if_fail (GRL_IS_DATA (data), NULL);
> +  g_return_val_if_fail (key, NULL);
> +
> +  sample_key = get_sample_key (key);
> +  if (!sample_key) {
> +    return NULL;
> +  }
> +
> +  return g_list_copy (g_hash_table_lookup (data->priv->data, 
> sample_key));
> +}
> +
> +/**
> + * grl_data_get_all_single_property_string:
> + * @mdata: a data
> + * @key: a metadata key
> + *
> + * Returns all non-%NULL values for specified key of type string.
> This ignores
> + * completely the related keys.

Returns all non-%NULL values for @key. Key must have been registered as 
a string-type key.

> + * Returns: (element-type utf8) (transfer container): a #GList with
> values. Do
> + * not change or free the strings. Free the list with #g_list_free.
> + **/
> +GList *
> +grl_data_get_all_single_property_string (GrlData *data,
> +                                         GrlKeyID key)
> +{
> +  GList *list_strings = NULL;
> +  GList *list_values;
> +  GList *value;
> +  const gchar *string_value;
> +
> +  g_return_val_if_fail (GRL_IS_DATA (data), NULL);
> +  g_return_val_if_fail (key, NULL);
> +
> +  /* Verify key is of type string */
> +  if (GRL_METADATA_KEY_GET_TYPE (key) != G_TYPE_STRING) {
> +    GRL_WARNING ("Wrong type (not string)");
> +    return NULL;
> +  }
> +
> +  list_values = grl_data_get_all_single_property (data, key);
> +  for (value = list_values; value; value = g_list_next (value)) {
> +    string_value = g_value_get_string (value->data);
> +    if (string_value) {
> +      list_strings = g_list_prepend (list_strings, (gpointer) 
> string_value);
> +    }
> +  }
> +
> +  g_list_free (list_values);
> +
> +  return g_list_reverse (list_strings);
> +}
> +
> +/**
> + * grl_data_remove_property:
> + * @data: a data
> + * @key: a metadata key
> + * @index: index of key to be removed, starting at 0
> + *
> + * Removes key and related keys from position in mdata.

I think this does not remove any key ro related keys, but their values 
for index, right? Please in that case change the description and use '@' 
to refer to parameters.

> + **/
> +void
> +grl_data_remove_property (GrlData *data,
> +                          GrlKeyID key,
> +                          guint index)
> +{
> +  GList *prop_element;
> +  GList *prop_list;
> +  GrlKeyID sample_key;
> +
> +  g_return_if_fail (GRL_IS_DATA (data));
> +  g_return_if_fail (key);
> +
> +  sample_key = get_sample_key (key);
> +  if (!sample_key) {
> +    return;
> +  }
> +
> +  prop_list = g_hash_table_lookup (data->priv->data, sample_key);
> +  prop_element = g_list_nth (prop_list, index);
> +  if (!prop_element) {
> +    GRL_WARNING ("%s: index %u out of range", __FUNCTION__, index);
> +    return;
> +  }
> +
> +  g_object_unref (prop_element->data);
> +  prop_list = g_list_delete_link (prop_list, prop_element);
> +  g_hash_table_insert (data->priv->data, sample_key, prop_list);
> +}
> +
> +/**
> + * grl_data_set_property:
> + * @data: a data
> + * @prop: a set of related keys
> + * @index: position to be updated, starting at 0
> + *
> + * Updates the values at position in @data with values in @prop.

... at position @index in @data....

Please, review all the API descriptions in this patch since there seem 
to various issues.

Iago