Re: [Tracker] Request for review, jolla upstreaming

[Martyn Russell <martyn lanedo com>]

On 12/03/14 09:37, Philip Van Hoof wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
>
> Hi guys, here are the commits from Jolla. Ready for review:
>
> https://git.gnome.org/browse/tracker/log/?h=jolla-upstreaming

Thanks Philip, comments below:

> +#else /* GIFLIB_MAJOR >= 5 */
> +static inline void print_gif_error()
> +{
> +#if defined(GIFLIB_MAJOR) && defined(GIFLIB_MINOR) && ((GIFLIB_MAJOR == 4 && GIFLIB_MINOR >= 2) || GIFLIB_MAJOR 
> > 4)
> +       const char *str = GifErrorString ();
> +       if (str != NULL) {
> +               g_message ("GIF, error: '%s'", str);
> +       } else {
> +               g_message ("GIF, undefined error");
> +       }
> +#else
> +       PrintGifError();
> +#endif

Apparently, GIFLIB_MAJOR is only defined in version 4.2, wouldn't it 
make more sense to have:

  #ifdef GLIB_MAJOR
  ...
  #else
  PrintGifError()
  #endif

?

> -AC_ARG_WITH(enca,
> -            AS_HELP_STRING([--with-enca],
> +AC_ARG_ENABLE(enca,
> +            AS_HELP_STRING([--enable-enca],

Nice.

> -if test "x$have_enca" = "xyes" || test "x$have_libicu" = "xyes"; then
> +AC_ARG_ENABLE(icu-charset-detection,
> +            AS_HELP_STRING([--enable-icu-charset-detection],
> +                           [enable libicu for charset detection in MP3s [[default=auto]]]),,
> +            [enable_icu_charset_detection=auto])
> +

...

OK, so it looks like this is just about disabling enca AND icu for MP3 
encoding detection. The existing code already does the same thing when 
we have enca and/or icu. Is that right or? If so, it looks good to me.

> +extractmodules_LTLIBRARIES += libextract-bmp.la
> +rules_DATA += 10-bmp.rule

...

Cool to have a BMP extractor. Though you leak filename in the extractor 
and that needs fixing first.

The fallbacks for BMP on master should do the basics of what your patch 
does here (i.e. fill in Photo/Image ontology details) - detailed in the 
xml files.

---

All other patches/changes look ok to me.

Thanks Philip,

--
Regards,
Martyn

Founder & Director @ Lanedo GmbH.
http://www.linkedin.com/in/martynrussell