Re: [PATCH 1/4] core: introduced grl_metadata_source_may_resolve() and corresponding vmethod.

["Juan A." Suárez Romero <jasuarez igalia com>]

On Thu, 2011-02-17 at 23:39 +0100, Guillaume Emont wrote:
>  
> +/**
> +  * Will intersect two lists, providing either the intersection in
> +  * @intersection, the difference (the list of elements in setA and not in
> +  * setB) in @difference or both, depending on whether any of them is @NULL.
> +  *
> +  * intersect_lists (A, B, I, D) is equivalent to:
> +  * I = g_list_copy (A);
> +  * D = filter_key_list (some_source, &I, TRUE, B)
> +  */


I find a bit weird that documentation makes reference to a deprecated
function.

I think it should be the other way around: filter_key_list() should
explain how to use intersect_lists() to get the same result.

> +
> +/**
> + * @data: a GrlData instance
> + * @deps: a list of GrlKeyID
> + *
> + * Returns: a list of all the keys that are in deps but are not defined in data
> + */
> +static GList *
> +missing_in_data (GrlData *data, const GList *deps)
> +{
> +  GList *present_keys, *iter, *known_keys, *result = NULL;
> +  GRL_DEBUG ("missing_in_data");
> +
> +  present_keys = grl_data_get_keys (data);
> +  known_keys = g_list_copy (present_keys);
> +
> +  for (iter = present_keys; iter; iter = g_list_next (iter)) {
> +    const GrlKeyID key = iter->data;
> +    if (!grl_data_key_is_known (data, key))
> +      known_keys = g_list_remove (known_keys, key);
> +  }
> +
> +  intersect_lists (deps, known_keys, NULL, &result);
> +
> +  g_list_free (present_keys);
> +  g_list_free (known_keys);
> +
> +  return result;
> +}


I find this function somewhat complicated, maybe I'm missing
something....

Shouldn't be easier to do

foreach key in deps;
  if grl_data_key_is_known(data, key) == FALSE then
      missing_list = g_list_append (missing_list, key);

return missing_list;


>  /* ================ API ================ */
>  
>  /**
> @@ -606,10 +667,13 @@ grl_metadata_source_slow_keys (GrlMetadataSource *source)
>   * a #GList with the keys, or @NULL if it can not resolve @key_id
>   *
>   * Since: 0.1.1
> + * Deprecated: 0.1.10: use grl_metadata_source_may_resolve() instead.
>   */
>  const GList *
>  grl_metadata_source_key_depends (GrlMetadataSource *source, GrlKeyID key_id)
>  {
> +  GRL_WARNING ("grl_metadata_source_key_depends() is deprecated, caller "
> +               "should use grl_metadata_source_may_resolve() instead.");
>    g_return_val_if_fail (GRL_IS_METADATA_SOURCE (source), NULL);
>  
>    if (GRL_METADATA_SOURCE_GET_CLASS (source)->key_depends) {
> @@ -644,6 +708,95 @@ grl_metadata_source_writable_keys (GrlMetadataSource *source)
>  }
>  
>  /**
> + * grl_metadata_source_may_resolve:
> + * @source: a metadata source
> + * @media: a media on which we want more metadata
> + * @key_id: the key corresponding to a metadata we might want
> + * @missing_keys: an optional originally empty list
> + *
> + * Checks whether @key_id may be resolved with @source for @media, so that the
> + * caller can avoid calling grl_metadata_source_resolve() if it can be known in
> + * advance it will fail.
> + *
> + * If the resolution is known to be impossible because more keys are needed in
> + * @media, and @missing_keys is not @NULL, it is populated with the list of
> + * GrlKeyID that would be needed.
> + *
> + * This function is synchronous and should not block.
> + *
> + * Returns: @TRUE if there's a possibility that @source resolves @key_id for
> + * @media, @FALSE otherwise.
> + */
> +gboolean
> +grl_metadata_source_may_resolve (GrlMetadataSource *source,
> +                                 GrlMedia *media,
> +                                 GrlKeyID key_id,
> +                                 GList **missing_keys)
> +{
> +  GrlMetadataSourceClass *klass;
> +  gboolean ret = TRUE;
> +
> +  GRL_DEBUG ("grl_metadata_source_may_resolve");
> +  g_return_val_if_fail (GRL_IS_METADATA_SOURCE (source), FALSE);
> +  g_return_val_if_fail (!missing_keys || !*missing_keys, FALSE);
> +
> +  klass = GRL_METADATA_SOURCE_GET_CLASS (source);
> +
> +  if (klass->may_resolve) {
> +    return klass->may_resolve (source, media, key_id, missing_keys);
> +  }
> +
> +  if (klass->key_depends) {
> +    /* compatibility code, to be removed when we get rid of key_depends() */
> +    const GList *deps;
> +    GList *missing;
> +
> +    GRL_WARNING ("Source %s should implement the may_resolve() vmethod, trying "
> +                 "with the deprecated key_depends() vmethod instead",
> +                 grl_metadata_source_get_name (source));
> +
> +    deps = klass->key_depends (source, key_id);
> +
> +    if (!deps)
> +      return FALSE;
> +
> +
> +    if (media)
> +      missing = missing_in_data (GRL_DATA (media), deps);
> +    else
> +      missing = g_list_copy ((GList *)deps);
> +
> +    if (missing) {
> +      ret = FALSE;
> +      if (missing_keys) {
> +        *missing_keys = missing;
> +        missing = NULL;
> +      }
> +    } else {
> +      ret = TRUE;
> +    }
> +
> +    if (missing)
> +      g_list_free (missing);
> +  } else if (GRL_IS_MEDIA_SOURCE (source)) {
> +    /* We're more forgiving to media source, as we should only ask them keys
> +     * during a media source operation, and we assume they are likely to return
> +     * all of their supported_keys() in that case. If a media source wants to
> +     * behave differently, it should implement may_resolve().*/
> +    const GList *supported_keys = grl_metadata_source_supported_keys (source);
> +    ret = NULL != g_list_find ((GList *)supported_keys, key_id);
> +  } else {
> +    GRL_WARNING ("Source %s does not implement may_resolve(), considering it "
> +                 "can't resolve %s",
> +                 grl_metadata_source_get_name (source),
> +                 GRL_METADATA_KEY_GET_NAME (key_id));
> +    ret = FALSE;
> +  }
> +
> +  return ret;
> +}
> +
> +/**
>   * grl_metadata_source_resolve:
>   * @source: a metadata source
>   * @keys: (element-type GObject.ParamSpec) (allow-none): the #GList
> diff --git a/src/grl-metadata-source.h b/src/grl-metadata-source.h
> index 51e0b4e..2f06667 100644
> --- a/src/grl-metadata-source.h
> +++ b/src/grl-metadata-source.h
> @@ -235,11 +235,15 @@ typedef struct _GrlMetadataSourceClass GrlMetadataSourceClass;
>   * @supported_operations: the operations that can be called
>   * @supported_keys: the list of keys that can be handled
>   * @slow_keys: the list of slow keys that can be fetched
> - * @key_depends: the list of keys which @key_id depends on
> + * @key_depends: a deprecated vmethod that will be removed in the future
>   * @writable_keys: the list of keys which value can be written
>   * @resolve: resolve the metadata of a given transfer object
>   * @set_metadata: update metadata values for a given object in a
>   * permanent fashion
> + * @may_resolve: return FALSE if it can be known without blocking that @key_id
> + * cannot be resolved for @media, TRUE otherwise. Optionally fill @missing_keys
> + * with a list of keys that would be needed to resolve. See
> + * grl_metadata_source_may_resolve().
>   *
>   * Grilo MetadataSource class. Override the vmethods to implement the
>   * element functionality.
> @@ -264,8 +268,11 @@ struct _GrlMetadataSourceClass {
>    void (*set_metadata) (GrlMetadataSource *source,
>  			GrlMetadataSourceSetMetadataSpec *sms);
>  
> +  gboolean (*may_resolve) (GrlMetadataSource *source, GrlMedia *media,
> +                           GrlKeyID key_id, GList **missing_keys);
> +
>    /*< private >*/
> -  gpointer _grl_reserved[GRL_PADDING];
> +  gpointer _grl_reserved[GRL_PADDING - 1];
>  };
>  
>  G_BEGIN_DECLS
> @@ -295,6 +302,11 @@ const GList *grl_metadata_source_key_depends (GrlMetadataSource *source,
>  
>  const GList *grl_metadata_source_writable_keys (GrlMetadataSource *source);
>  
> +gboolean grl_metadata_source_may_resolve (GrlMetadataSource *source,
> +                                          GrlMedia *media,
> +                                          GrlKeyID key_id,
> +                                          GList **missing_keys);
> +
>  void grl_metadata_source_resolve (GrlMetadataSource *source,
>                                    const GList *keys,
>                                    GrlMedia *media,