Re: [PATCH 1/3] local-metadata: added a new local source

[Iago Toral <itoral igalia com>]

Hi Guillaume,

a few comments:

- the patches did not build for me, you need to add the gthread-2.0 
dependency to your test, otherwise it fails to link.
- we cannot use copyrighted images for the tests, please provide new 
ones.

Some other comments below:

On Thu, 27 Jan 2011 19:26:06 +0100, Guillaume Emont <gemont igalia com> 
wrote:
(...)
> +/* ======================= Utilities ==================== */
> +static void
> +got_file_info (GFile *file, GAsyncResult *result,
> +               GrlMetadataSourceResolveSpec *rs)
> +{
> +  GFileInfo *info;
> +  GError *error = NULL;
> +  const gchar *thumbnail_path;
> +
> +  GRL_DEBUG ("got_file_info");
> +
> +  info = g_file_query_info_finish (file, result, &error);
> +  if (error)
> +    goto error;
> +
> +  thumbnail_path =
> +      g_file_info_get_attribute_byte_string (info,
> G_FILE_ATTRIBUTE_THUMBNAIL_PATH);
> +
> +
> +  if (thumbnail_path) {
> +    gchar * thumbnail_uri = g_filename_to_uri (thumbnail_path, NULL,
> &error);

       Code style:
       gchar *thumbnail_uri = ...

> +    if (error)
> +      goto error;
> +
> +    GRL_INFO ("Got thumbnail %s", thumbnail_uri);
> +    grl_media_set_thumbnail (rs->media, thumbnail_uri);
> +    g_free (thumbnail_uri);
> +
> +    rs->callback (rs->source, rs->media, rs->user_data, NULL);
> +    goto exit;
> +  }
> +
> +error:
> +    {
> +      GError *new_error;
> +      if (error)
> +        new_error = g_error_new (GRL_CORE_ERROR,
> GRL_CORE_ERROR_RESOLVE_FAILED,
> +                                 "Got error: %s", error->message);
> +      else
> +        new_error = g_error_new (GRL_CORE_ERROR,
> GRL_CORE_ERROR_RESOLVE_FAILED,
> +                                 "Could not find a thumbnail for 
> %s",
> +                                 g_file_get_path (file));

Not finding the thumbnail should not raise an error, it is a normal 
situation. At least other plugins do not raise an error if they cannot 
find the metadata they have been requested.

> +      rs->callback (rs->source, rs->media, rs->user_data, 
> new_error);
> +
> +      g_error_free (error);
> +      g_error_free (new_error);
> +    }
> +
> +exit:
> +  if (info)
> +    g_object_unref (info);
> +}
> +
> +static void
> +grl_local_metadata_source_resolve (GrlMetadataSource *source,
> +                                  GrlMetadataSourceResolveSpec *rs)
> +{
> +  const gchar *url;
> +  gchar *scheme;
> +  GError *error;

You forgot to initialize error to NULL...

> +  GRL_DEBUG ("grl_local_metadata_source_resolve");
> +
> +  url = grl_media_get_url (rs->media);
> +  scheme = g_uri_parse_scheme (url);
> +
> +  if (0 != g_strcmp0 (scheme, "file"))
> +    error = g_error_new (GRL_CORE_ERROR, 
> GRL_CORE_ERROR_RESOLVE_FAILED,
> +                         "local-metadata needs a url in the file://
> scheme");
> +  else if (!g_list_find (rs->keys, GRL_METADATA_KEY_THUMBNAIL))
> +    error = g_error_new (GRL_CORE_ERROR, 
> GRL_CORE_ERROR_RESOLVE_FAILED,
> +                         "local-metadata can only resolve the
> thumbnail key");
> +
> +  if (error) {

This is likely to be always TRUE because of what I mentioned above. 
Makes the test segfault when the test tries to clear the error...

> +    /* No can do! */
> +    rs->callback (source, rs->media, rs->user_data, error);
> +    g_error_free (error);
> +    goto exit;
> +  }
> +
> +  if (GRL_IS_MEDIA_VIDEO (rs->media)
> +      || GRL_IS_MEDIA_IMAGE (rs->media)) {
> +    resolve_image (rs);
> +  } else if (GRL_IS_MEDIA_AUDIO (rs->media)) {
> +    resolve_album_art (rs);
> +  } else {
> +    /* What's that media type? */
> +    rs->callback (source, rs->media, rs->user_data, NULL);
> +  }
> +
> +exit:
> +  g_free (scheme);
> +}
> +

Iago