Re: [PATCH 1/2] local-metadata: add series support

[Guillaume Emont <gemont igalia com>]

Lionel,

Thanks for the patches, it will be awesome to have that feature! It's
not 100% perfect (I don't really believe that's even possible), but
already works quite well, at least for my TV Series. Improvements can be
added in the future on this base anyway, I think I might have some ideas
in the back of my head[1].

Nevertheless, I would like a few changes to be made to your patch before
accepting it.

First, local-metadata was initially written to resolve only thumbnails,
and its original author being a lazy a*se, he just assumed the only key
that could be asked by resolve() is the thumbnail one, because that's
what _supported_keys() and _may_resolve() say the source supports. If
you want to add support for other keys, you need to:
 - update _may_resolve() and _supported_keys()
 - ensure we don't waste cpu cycles looking for and filling in metadata
that was not asked.

Right now, in a typical use case (such as what we have in
grilo-test-ui), things work fine e.g. with the filesystem media source,
at least if you don't have a thumbnail in ~/.thumbnails/ for the file (I
didn't test with thumbails), because local-metadata is called to resolve
thumbnails. Since there is no check on what is asked, it feeds the
series metadata anyway, which is why we get that info displayed in
grilo-test-ui, but that's kind of "by accident".

Secondly, there are various improvement suggestions and comments that
you can find in-line below.

Cheers,

Guillaume

PS: I did not reply to the configure.ac patch, but know that it looks
good to me.

[1]
http://bazaar.launchpad.net/~guijemont/+junk/ms-trunk/files/head:/plugins/moovida_name_parser/
yeah, that's where the back of my head is.

FOn 14/03/2011 12:28, lionel g landwerlin linux intel com wrote:
> From: Lionel Landwerlin <lionel g landwerlin linux intel com>
> 
> Based on a patch for Tracker by Iain Holmes :
> 
> http://build.meego.com/package/view_file?file=0002-Tracker-extract-Parse-the-video-filename-to-obtain-e.patch&package=tracker&project=devel%3Acontentfw&srcmd5=c91b724488ee9ae476a9ce65755d8152
> 
> Signed-off-by: Lionel Landwerlin <lionel g landwerlin linux intel com>
> ---
>  src/metadata/local-metadata/grl-local-metadata.c |  300 +++++++++++++++++++++-
>  1 files changed, 295 insertions(+), 5 deletions(-)
> 
> diff --git a/src/metadata/local-metadata/grl-local-metadata.c b/src/metadata/local-metadata/grl-local-metadata.c
> index 62737ba..c9a8e96 100644
> --- a/src/metadata/local-metadata/grl-local-metadata.c
> +++ b/src/metadata/local-metadata/grl-local-metadata.c
> @@ -24,6 +24,9 @@
>  #include "config.h"
>  #endif
>  
> +#include <string.h>
> +#include <stdlib.h>
> +
>  #include <grilo.h>
>  #include <gio/gio.h>
>  
> @@ -42,6 +45,37 @@ GRL_LOG_DOMAIN_STATIC(local_metadata_log_domain);
>  #define LICENSE     "LGPL"
>  #define SITE        "http://www.igalia.com";
>  
> +/**/
> +
> +#define TV_REGEX \
> +  "(?<showname>.*)\\.(?<season>(?:\\d{1,2})|(?:[sS]\\K\\d{1,2}))(?<episode>(?:\\d{2})|(?:[eE]\\K\\d{1,2}))\\.?(?<name>.*)?"

Since it's a long regex, could it be split across two lines (or more)?

> +#define MOVIE_REGEX "(?<name>.*)\\.?[\\(\\[](?<year>[12][90]\\d{2})[\\)\\]]"
> +
> +/**/
> +
> (...)
> @@ -115,6 +149,191 @@ G_DEFINE_TYPE (GrlLocalMetadataSource,
>                 GRL_TYPE_METADATA_SOURCE);
> (...)
> +
> +/* tidies strings before we run them through the regexes */
> +static gchar *
> +video_uri_to_metadata (const gchar *uri)

I think this function could have a name that describes better what it
does, get_sanitised_basename() or something like that.

> (...)
> @@ -130,7 +349,7 @@ got_file_info (GFile *file, GAsyncResult *result,
>      goto error;
>  
>    thumbnail_path =
> -      g_file_info_get_attribute_byte_string (info, G_FILE_ATTRIBUTE_THUMBNAIL_PATH);
> +    g_file_info_get_attribute_byte_string (info, G_FILE_ATTRIBUTE_THUMBNAIL_PATH);

I don't know which indentation is more correct, but this is off-topic in
this commit.

>  
>  
>    if (thumbnail_path) {
> @@ -168,6 +387,73 @@ exit:
>  }
>  
>  static void
> +resolve_video (GrlMetadataSourceResolveSpec *rs)
> +{
> +  gchar *title, *showname;
> +  GDateTime *date;
> +  gint season, episode;
> +  GrlData *data = GRL_DATA (rs->media);
> +  guint flags = 0;
> +
> +  GRL_DEBUG ("%s", __FUNCTION__);

Just a note for later discussion: most code uses the function name
"written by hand" for this, some uses __FUNCTION__ and some uses
__func__. I think we should try to be more consistent on that. GCC's
documentation recommends[2] the use of __func__ with a #define if we're
not using C99, I don't know how other compilers handle
__func__/__FUNCTION__ or friends.

[2] http://gcc.gnu.org/onlinedocs/gcc/Function-Names.html

> (...)
> @@ -249,11 +535,11 @@ grl_local_metadata_source_may_resolve (GrlMetadataSource *source,
>  
>  static void
>  grl_local_metadata_source_resolve (GrlMetadataSource *source,
> -                                  GrlMetadataSourceResolveSpec *rs)
> +                                   GrlMetadataSourceResolveSpec *rs)
>  {
>    GError *error = NULL;
>  
> -  GRL_DEBUG ("grl_local_metadata_source_resolve");
> +  GRL_DEBUG ("%s", __FUNCTION__);
>  
>    if (!has_compatible_media_url (rs->media))
>      error = g_error_new (GRL_CORE_ERROR, GRL_CORE_ERROR_RESOLVE_FAILED,


The change above is unrelated to this commit. If you want it in, please
send it in a separate patch.

> @@ -269,8 +555,12 @@ grl_local_metadata_source_resolve (GrlMetadataSource *source,
>      return;
>    }
>  
> -  if (GRL_IS_MEDIA_VIDEO (rs->media)
> -      || GRL_IS_MEDIA_IMAGE (rs->media)) {
> +  GRL_DEBUG ("\ttrying to resolve for: %s", grl_media_get_url (rs->media));
> +
> +  if (GRL_IS_MEDIA_VIDEO (rs->media)) {
> +    resolve_video (rs);
> +    resolve_image (rs);
> +  } else if (GRL_IS_MEDIA_IMAGE (rs->media)) {
>      resolve_image (rs);
>    } else if (GRL_IS_MEDIA_AUDIO (rs->media)) {
>      resolve_album_art (rs);