[cairo] conditionals for older freetype2 without color font feature

[Bryce Harrington]

On Wed, Apr 04, 2018 at 11:30:18AM +0900, suzuki toshiya wrote:
> Dear Bryce,
> 
> Bryce Harrington wrote:
> >> +  AC_LINK_IFELSE([AC_LANG_PROGRAM([
> >> +#include <ft2build.h>
> >> +#include FT_FREETYPE_H
> >> +],[
> >> +FT_Long has_color = FT_HAS_COLOR( ((FT_Face)NULL) );
> >> +])],[AC_MSG_RESULT([yes])],[
> >> +  AC_DEFINE([FT_HAS_COLOR(x)], [(0)], [define nothing if freetype2 supports
> >> color font])
> >> +  AC_MSG_RESULT([no, disable color font (freetype2 >= 2.5.1 is required)])
> >> +])
> >> +
> > Your patch applies cleanly and configure passes for me with no issue,
> > thank you.
> 
> Thank you very much for spending your time for this!
> 
> >  I noticed a small error in the comment, shouldn't it be
> > "define nothing if freetype2 does not support color fonts"?  I can fix
> > that locally.
> 
> I'm sorry for making you confused. Please let me try to clarify.

Thanks for your more detailed explanation, and I apologize if I am
indeed confused.  I never was an expert on autotools, and use it less
and less these days.

However, looking at the AC_DEFINE() parameter descriptions the third
parameter is essentially just a comment inserted to explain the first
and second parameter.

> * if newer freetype2 with FT_HAS_COLOR() macro is found,
> config.h defines nothing about it. the original FT_HAS_COLOR()
> macro in freetype2 header file is used.
> 
> * if older freetype2 without FT_HAS_COLOR() macro is found,
> config.h defines a macro converting "FT_HAS_COLOR(x)" to "(0)".
> by including config.h, the cairo sources can use as if FT_HAS_COLOR()
> macro function is available (although it fails always).

Right, so in this latter case, I am imaginging that in config.h it
writes something like:

/* Define to nothing if freetype2 supports color fonts */
#define FT_HAS_COLOR(x) (0)

But actually we only want to define to (0) if freetype2 does *not*
support color fonts, right?  "Define to nothing" might be a bit
ambiguous though (does it mean undefine?  define to blank?  define to
zero?)  So, my suggestion is for it to maybe look more like this:

/* Define to (0) if freetype2 does not support color fonts */
#define FT_HAS_COLOR(x) (0)

What do you think?  I apologize for being nitpicky on a mere comment,
but thank you for taking the extra time on this.

> this is slightly tricky utilization of config.h (define nothing
> in "with-" case, define "HAS_xxx" in "without-" case), because
> usually config.h defines something if available and defines
> nothing if unavailable.
> 
> if this tricky usage is a pitfall for the maintainers, I would
> revise it to more conventional style, by the insertion of postamble
> content by AH_BOTTOM(), like:
> 
> /* conventional style of config.h */
> #define HAVE_FT_HAS_COLOR
> 
> ...
> 
> /* following is added by AH_BOTTOM() */
> #ifndef HAVE_FT_HAS_COLOR
> # define FT_HAS_COLOR(x) (0)
> #endif
> 
> It would be slightly lengthy, but not so hard to understand.
> which do you prefer?

I think as long as the description is correct, having the test done in
configure.ac as you've proposed makes the most sense.  That is where
most version discrepancies appear to be dealt with, and thus the place
one would look if odd behavior turned up from this change.

> Regards,
> mpsuzuki
> 
> 
> Bryce Harrington wrote:
> > On Wed, Apr 04, 2018 at 01:15:41AM +0900, suzuki toshiya wrote:
> >> Dear Bryce,
> >>
> >> Here is revised patch. Please let me explain for your review
> > 
> >> diff --git a/configure.ac b/configure.ac
> >> index d78b2ed..ebc31d9 100644
> >> --- a/configure.ac
> >> +++ b/configure.ac
> >> @@ -580,6 +580,17 @@ if test "x$use_ft" = "xyes"; then
> >>
> >>    AC_CHECK_FUNCS(FT_Get_X11_Font_Format FT_GlyphSlot_Embolden
> >> FT_GlyphSlot_Oblique FT_Load_Sfnt_Table FT_Library_SetLcdFilter
> >> FT_Get_Var_Design_Coordinates FT_Done_MM_Var)
> >>
> >> +  AC_MSG_CHECKING(for FT_HAS_COLOR)
> >> +  AC_LINK_IFELSE([AC_LANG_PROGRAM([
> >> +#include <ft2build.h>
> >> +#include FT_FREETYPE_H
> >> +],[
> >> +FT_Long has_color = FT_HAS_COLOR( ((FT_Face)NULL) );
> >> +])],[AC_MSG_RESULT([yes])],[
> >> +  AC_DEFINE([FT_HAS_COLOR(x)], [(0)], [define nothing if freetype2 supports
> >> color font])
> >> +  AC_MSG_RESULT([no, disable color font (freetype2 >= 2.5.1 is required)])
> >> +])
> >> +
> >>    LIBS="$_save_libs"
> >>    CFLAGS="$_save_cflags"
> >>  fi
> > 
> > Hi Toshiyasan,
> > 
> > Your patch applies cleanly and configure passes for me with no issue,
> > thank you.  I noticed a small error in the comment, shouldn't it be
> > "define nothing if freetype2 does not support color fonts"?  I can fix
> > that locally.
> > 
> >> * the patched part tries to compile & link a source including FT_HAS_COLOR()
> >> which is defined as a macro function. basically, the functions whose names
> >> are in all upper cases in FreeType2 are macro functions, so the availability
> >> check could be simplified to the check of C preprocessor macro, but I did
> >> like this, to minimize the assumption.
> >>
> >> * for newer FreeType2 with FT_HAS_COLOR(), config.h defines nothing about it.
> >> (note: config.h.in looks like as if "#undef FT_HAS_COLOR" is executed
> >> for newer FreeType2, but it is not - please check the result on the
> >> platform with newer FreeType2, it would be commented out)
> >>
> >> * for older FreeType2 without FT_HAS_COLOR(), config.h defines as
> >> #define FT_HAS_COLOR(x) (0)
> > 
> > Yes, makes sense.  I've written a commit message incorporating the above
> > details, and crediting you for the work.
> > 
> > Thanks again,
> > Bryce
> > 
> >> suzuki toshiya wrote:
> >>> Dear Bryce,
> >>>
> >>> Thank you for prompt review!
> >>>
> >>>  >> #ifndef FT_HAS_COLOR
> >>>  >> # define FT_HAS_COLOR(x) ( 0 )
> >>>  >> #endif
> >>>  >>
> >>>  >> in config header is better?
> >>>  >
> >>>  > Offhand I prefer this latter approach, just feels a bit cleaner.  Would
> >>>  > you mind respinning your patch with this approach?
> >>>
> >>> OK! I would revise and resubmit.
> >>>
> >>> Regards,
> >>> mpsuzuki
> >>>
> >>> On 4/3/2018 12:30 PM, Bryce Harrington wrote:
> >>>> On Tue, Apr 03, 2018 at 10:34:20AM +0900, suzuki toshiya wrote:
> >>>>> Hi,
> >>>>>
> >>>>> FT_HAS_COLOR() macro is unavailable in older freetype2
> >>>>> without color font feature. attached is a quick fix.
> >>>>>
> >>>>> or, something like
> >>>>>
> >>>>> #ifndef FT_HAS_COLOR
> >>>>> # define FT_HAS_COLOR(x) ( 0 )
> >>>>> #endif
> >>>>>
> >>>>> in config header is better?
> >>>> Offhand I prefer this latter approach, just feels a bit cleaner.  Would
> >>>> you mind respinning your patch with this approach?
> >>>>
> >>>>> it seems that fontconfig decided to drop old freetype2,
> >>>>> but I wish if cairo can provide (limited) support for
> >>>>> older freetype2.
> >>>> Sounds ok, I don't see a problem with doing this.
> >>>>
> >>>> Thanks,
> >>>> Bryce
> >>>>
> > 
> >> sh: 1: highlight: not found
> > 
> >