[cairo] conditionals for older freetype2 without color font feature

[suzuki toshiya]

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.

* 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).

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?

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
> 
>