Post by suzuki toshiyaDear Bryce,
Post by Bryce HarringtonPost by suzuki toshiya+ 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!
Post by Bryce HarringtonI 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.
Post by suzuki toshiya* 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.
Post by suzuki toshiyathis 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
/* 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.
Post by suzuki toshiyaRegards,
mpsuzuki
Post by Bryce HarringtonPost by suzuki toshiyaDear 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.
Post by suzuki toshiya* 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
Post by suzuki toshiyaPost by suzuki toshiyaDear Bryce,
Thank you for prompt review!
Post by Bryce HarringtonPost by suzuki toshiya#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
Post by Bryce HarringtonPost by suzuki toshiyaHi,
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?
Post by suzuki toshiyait 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
--
cairo mailing list
***@cairographics.org
https://