Re: [PATCH 1/9] core: Create relations between keys

[Iago Toral <itoral igalia com>]

On Mon, 21 Feb 2011 10:36:27 +0100, "Juan A. Suarez Romero" 
<jasuarez igalia com> wrote:
> A couple of keys are related when getting a value for one of the 
> keys, it is
> expected that the other key has also a value.
>
> It is intended to be used with keys providing multivalued data: for
> each single
> value in one key, there must be a value matching that single value. 
> This
> matched value can be NULL, meaning that it doesn't have actually a 
> value, but
> at least it is possible to create a relation between values in both 
> keys.
>
> Signed-off-by: Juan A. Suarez Romero <jasuarez igalia com>
> ---
>  src/grl-plugin-registry.c |   89
> +++++++++++++++++++++++++++++++++++++++++++++
>  src/grl-plugin-registry.h |    7 ++++
>  2 files changed, 96 insertions(+), 0 deletions(-)
>
> diff --git a/src/grl-plugin-registry.c b/src/grl-plugin-registry.c
> index 20ee3a8..c5944c5 100644
> --- a/src/grl-plugin-registry.c
> +++ b/src/grl-plugin-registry.c
> @@ -61,6 +61,7 @@ struct _GrlPluginRegistryPrivate {
>    GHashTable *configs;
>    GHashTable *plugins;
>    GHashTable *sources;
> +  GHashTable *relation_keys;

I would rename this to 'related_keys'.

>    GParamSpecPool *system_keys;
>    GHashTable *ranks;
>    GSList *plugins_dir;
> @@ -130,6 +131,8 @@ grl_plugin_registry_init (GrlPluginRegistry 
> *registry)
>      g_hash_table_new_full (g_str_hash, g_str_equal, NULL, NULL);
>    registry->priv->sources =
>      g_hash_table_new_full (g_str_hash, g_str_equal, g_free, NULL);
> +  registry->priv->relation_keys =
> +    g_hash_table_new_full (g_direct_hash, g_direct_equal, NULL, 
> NULL);
>    registry->priv->system_keys =
>      g_param_spec_pool_new (FALSE);
>
> @@ -798,11 +801,76 @@ grl_plugin_registry_register_metadata_key
> (GrlPluginRegistry *registry,
>      g_param_spec_pool_insert (registry->priv->system_keys,
>                                key,
>                                GRL_TYPE_MEDIA);
> +    /* Each key is related with himself */
                                   ^^^^^^^ itself :)

> +    g_hash_table_insert (registry->priv->relation_keys,
> +                         key,
> +                         g_list_prepend (NULL, key));
>      return key;
>    }
>  }
>
>  /**
> + * grl_plugin_registry_register_metadata_relation:
> + * @registry: the plugin registry
> + * @key1: key involved in relationship
> + * @key2: key involved in relationship
> + *
> + * Creates a relation between @key1 and @key2.
> + *
> + * It make sense when using multiple values: means that for each
         ^^^^^^^^^^
         makes sense
> value in @key1
> + * there must be also a value in @key2 (which can be NULL).
> + **/
> +void
> +grl_plugin_registry_register_metadata_relation (GrlPluginRegistry 
> *registry,
> +                                                GrlKeyID key1,
> +                                                GrlKeyID key2)
> +{

I would rename this API to register_metadata_key_relation. That is, I 
would add _key_. The same for the rest of this API set.

> +  GList *key1_partners, *key1_peer;
> +  GList *key2_partners, *key2_peer;
> +
> +  g_return_if_fail (GRL_IS_PLUGIN_REGISTRY (registry));
> +  g_return_if_fail (key1);
> +  g_return_if_fail (key2);
> +
> +  if (key1 == key2) {
> +    return;
> +  }
> +
> +  /* Search for keys related with each key */
> +  key1_partners = g_hash_table_lookup 
> (registry->priv->relation_keys, key1);
> +  key2_partners = g_hash_table_lookup 
> (registry->priv->relation_keys, key2);
> +
> +  /* Check if they are already related */
> +  if (!key1_partners || !key2_partners || key1_partners == 
> key2_partners) {
> +    return;
> +  }


Why do you exit if key1_partners *or* key2_partners are NULL?
Also, can that even happen given that you relate each key with itself?

> +  /* Merge both relations */
> +  for (key1_peer = key1_partners;
> +       key1_peer;
> +       key1_peer = g_list_next (key1_peer)) {
> +    /* Search key1 peer in key2 partners */
> +    for (key2_peer = key2_partners;
> +         key2_peer && key2_peer != key1_peer;
> +         key2_peer = g_list_next (key2_peer));
> +    if (!key2_peer) {
> +      /* Didn't found; add it to key2 partners */
> +      key2_partners = g_list_prepend (key2_partners,
> +                                      key1_peer->data);
> +    }
> +  }

In the inner loop you check key2_peer != key1_peer. Is this even 
possible? I mean if there is a key in key1_partners that is already in 
key2_partners shouldn't key1_partners and key2_partners be the same (and 
thus never reach these loops)?

Iago