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

[Guillaume Emont <gemont igalia com>]

Hi Lionel!

Thanks for the new version.
As we discussed on IRC, can you add a configuration option to give the
opportunity to disable series recognition? For the record, the rationale
is that the information provided is only a guess and cannot be 100%
reliable. Some people/applications might want to disable it for that
reason. Obviously, _supported_keys() and _may_resolve() should take that
(de)activation into account.
Since it's still a cool feature ot have I feel the default configuration
should be to enable this.

I also have some comments on the code, inline in the patch below.

Cheers,

Guillaume


On 16/03/2011 18:48, 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>
> ---
>  configure.ac                                     |    3 +
>  src/metadata/local-metadata/grl-local-metadata.c |  393 ++++++++++++++++++++--
>  2 files changed, 373 insertions(+), 23 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 39c2955..54fe141 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -709,6 +709,9 @@ AC_ARG_ENABLE(localmetadata,
>                          if test "x$HAVE_GIO" = "xno"; then
>                             AC_MSG_ERROR([gio-2.0 not found, install it or use --disable-localmetadata])
>                          fi
> +                        if test "x$HAVE_GLIB_2_26" = "xno"; then
> +                           AC_MSG_ERROR([glib-2.26.0 or above not found, install it or use --disable-localmetadata])
> +                        fi
>                          ;;
>                  esac
>          ],
> diff --git a/src/metadata/local-metadata/grl-local-metadata.c b/src/metadata/local-metadata/grl-local-metadata.c
> index 62737ba..e1ae9e0 100644
> --- a/src/metadata/local-metadata/grl-local-metadata.c
> +++ b/src/metadata/local-metadata/grl-local-metadata.c
> (...)
> @@ -115,6 +155,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)
> +{

Like I asked in the first review: could you rename this function with a
name that describes more what it actually does?

> (...)
> @@ -233,13 +568,18 @@ grl_local_metadata_source_may_resolve (GrlMetadataSource *source,
>                                         GrlKeyID key_id,
>                                         GList **missing_keys)
>  {
> -  if (key_id != GRL_METADATA_KEY_THUMBNAIL
> -      || GRL_IS_MEDIA_AUDIO (media)
> -      || GRL_IS_MEDIA_BOX (media))
> -    return FALSE;
> -
> -  if (media && grl_data_key_is_known (GRL_DATA (media), GRL_METADATA_KEY_URL))
> -    return has_compatible_media_url (media);
> +  if (media) {
> +    if (grl_data_key_is_known (GRL_DATA (media), GRL_METADATA_KEY_URL) &&
> +        has_compatible_media_url (media)) {
> +      if (key_id == GRL_METADATA_KEY_THUMBNAIL ||
> +          key_id == GRL_METADATA_KEY_TITLE ||
> +          key_id == GRL_METADATA_KEY_SHOW ||
> +          key_id == GRL_METADATA_KEY_DATE ||
> +          key_id == GRL_METADATA_KEY_SEASON ||
> +          key_id == GRL_METADATA_KEY_EPISODE)
> +        return TRUE;
> +    }
> +  }
I think we can be more fine-grained here, and check for the media type
as well:
Box,Audio -> FALSE
Image -> TRUE if thumbnail was asked
Video -> TRUE if at least one of thumbnail, title, show, date, season,
episode was asked