From: Tony Cook <tony@develop=help.com>
Date: Mon, 18 Oct 2004 05:34:03 +0000 (+0000)
Subject: - the FT2 glyph_names() method didn't do correct error handling
X-Git-Tag: Imager-0.48^2~308
X-Git-Url: http://git.imager.perl.org/imager.git/commitdiff_plain/a4168beab92e4f73e4e86f4777ee7dcedb9881dd

- the FT2 glyph_names() method didn't do correct error handling
  when the string parameter wasn't supplied
- i_ft2_glyph_name() accepted only an unsigned char as the
  character code to get the name for, which meant it
  didn't work for unicode characters \x{100} or above
- the XS for i_ft2_glyph_name() had a similar problem
- added NameTest.ttf to be used in checking unicode glyph
  names
- added reliable_only optional parameter to the glyph_names()
  method so you can ignore theresult of FT_Has_PS_Glyph_Names()
- partly resolves https://rt.cpan.org/Ticket/Display.html?id=7949
- handle errors given by i_ft2_glyph_name() a bit more
  correctly
- the FT1 glyph_names() method didn't do correct error handling
  when the string parameter wasn't supplied
- some memory allocated when creating a callback IO object (io_new_cb)
  wasn't being released (detected with valgrind)
- the testtools.pl match[nx]() functions escapes the test string on
  test failure a bit better
---

diff --git a/Changes b/Changes
index 24edb66b..967dbd13 100644
--- a/Changes
+++ b/Changes
@@ -848,11 +848,30 @@ Revision history for Perl extension Imager.
 
 - the changes to scale() had some problems with integer vs floating point
   calculations (only caught in tests under perl 5.8.5 <sigh>)
+- the FT2 glyph_names() method didn't do correct error handling
+  when the string parameter wasn't supplied
+- i_ft2_glyph_name() accepted only an unsigned char as the
+  character code to get the name for, which meant it
+  didn't work for unicode characters \x{100} or above
+- the XS for i_ft2_glyph_name() had a similar problem
+- added NameTest.ttf to be used in checking unicode glyph
+  names
+- added reliable_only optional parameter to the glyph_names()
+  method so you can ignore theresult of FT_Has_PS_Glyph_Names()
+- partly resolves https://rt.cpan.org/Ticket/Display.html?id=7949
+- handle errors given by i_ft2_glyph_name() a bit more
+  correctly
+- the FT1 glyph_names() method didn't do correct error handling 
+  when the string parameter wasn't supplied
+- some memory allocated when creating a callback IO object (io_new_cb)
+  wasn't being released (detected with valgrind)
+- the testtools.pl match[nx]() functions escapes the test string on 
+  test failure a bit better
 
 =================================================================
 
         For latest versions check the Imager-devel pages:
-        http://imager.perl.org/~addi/perl/Imager/
+        http://imager.perl.org/
 
 =================================================================
 
diff --git a/Imager.xs b/Imager.xs
index 2fad7b08..2e5baba4 100644
--- a/Imager.xs
+++ b/Imager.xs
@@ -493,6 +493,7 @@ static void io_destroyer(void *p) {
   SvREFCNT_dec(cbd->readcb);
   SvREFCNT_dec(cbd->seekcb);
   SvREFCNT_dec(cbd->closecb);
+  myfree(cbd);
 }
 
 struct value_name {
@@ -4265,10 +4266,11 @@ undef_int
 i_ft2_can_face_name()
 
 void
-i_ft2_glyph_name(handle, text_sv, utf8 = 0)
+i_ft2_glyph_name(handle, text_sv, utf8 = 0, reliable_only = 1)
         Imager::Font::FT2 handle
         SV *text_sv
         int utf8
+        int reliable_only
       PREINIT:
         char const *text;
         STRLEN work_len;
@@ -4283,7 +4285,7 @@ i_ft2_glyph_name(handle, text_sv, utf8 = 0)
         text = SvPV(text_sv, work_len);
         len = work_len;
         while (len) {
-          unsigned char ch;
+          unsigned long ch;
           if (utf8) {
             ch = i_utf8_advance(&text, &len);
             if (ch == ~0UL) {
@@ -4296,7 +4298,8 @@ i_ft2_glyph_name(handle, text_sv, utf8 = 0)
             --len;
           }
           EXTEND(SP, 1);
-          if (outsize = i_ft2_glyph_name(handle, ch, name, sizeof(name))) {
+          if (outsize = i_ft2_glyph_name(handle, ch, name, sizeof(name), 
+                                         reliable_only)) {
             PUSHs(sv_2mortal(newSVpv(name, 0)));
           }
           else {
diff --git a/MANIFEST b/MANIFEST
index c26cfdf9..741fa4d7 100644
--- a/MANIFEST
+++ b/MANIFEST
@@ -56,6 +56,7 @@ fontfiles/ExistenceTest.afm     please edit ExistenceTest.sfd in CVS
 fontfiles/ExistenceTest.pfb     to change these files, edited and
 fontfiles/ExistenceTest.ttf     generated using pfaedit
 fontfiles/ImUgly.ttf
+fontfiles/NameTest.ttf          test glyph_names() - see t38ft2font.t
 fontfiles/dcr10.afm
 fontfiles/dcr10.pfb
 fontfiles/dodge.ttf
diff --git a/fontfiles/NameTest.sfd b/fontfiles/NameTest.sfd
new file mode 100644
index 00000000..a9ff6c78
--- /dev/null
+++ b/fontfiles/NameTest.sfd
@@ -0,0 +1,72 @@
+SplineFontDB: 1.0
+FontName: NameTest
+FullName: NameTest
+FamilyName: NameTest
+Weight: Medium
+Copyright: Created by Tony Cook - test font for the glyph_names() method
+Comments: 2004-10-13: Created.
+Version: 001.000
+ItalicAngle: 0
+UnderlinePosition: -100
+UnderlineWidth: 50
+Ascent: 800
+Descent: 200
+XUID: [1021 123 2012976297 155577]
+FSType: 0
+OS2WinAscent: 0
+OS2WinAOffset: 1
+OS2WinDescent: 0
+OS2WinDOffset: 1
+HheadAscent: 0
+HheadAOffset: 0
+HheadDescent: 0
+HheadDOffset: 0
+ScriptLang: 1
+ 1 latn 1 dflt 
+TtfTable: cvt  4
+!$MDh
+EndTtf
+Encoding: Custom
+UnicodeInterp: none
+DisplaySize: -24
+AntiAlias: 1
+FitToEm: 1
+WinInfo: 0 21 12
+BeginChars: 65536 2
+StartChar: exclam
+Encoding: 33 33 65535
+Width: 295
+Flags: W
+HStem: 30 60<141 143>
+Fore
+111 60 m 0
+ 111 76.5596 124.44 90 141 90 c 0
+ 157.56 90 171 76.5596 171 60 c 0
+ 171 43.4404 157.56 30 141 30 c 0
+ 124.44 30 111 43.4404 111 60 c 0
+165 171 m 2
+ 165 157.2 153.8 146 140 146 c 0
+ 126.2 146 115 157.2 115 171 c 0
+ 115 228.6 115 728.006 115 747 c 0
+ 115 760.8 126.2 772 140 772 c 0
+ 153.8 772 165 760.8 165 747 c 2
+ 165 171 l 2
+EndSplineSet
+EndChar
+StartChar: hyphentwo
+Encoding: 8208 8208 65535
+Width: 487
+Flags: W
+HStem: 308 50<114 378>
+Fore
+372 358 m 2
+ 385.8 358 397 346.8 397 333 c 0
+ 397 319.2 385.8 308 372 308 c 0
+ 346.2 308 122.395 308 114 308 c 0
+ 100.2 308 89 319.2 89 333 c 0
+ 89 346.8 100.2 358 114 358 c 2
+ 372 358 l 2
+EndSplineSet
+EndChar
+EndChars
+EndSplineFont
diff --git a/fontfiles/NameTest.ttf b/fontfiles/NameTest.ttf
new file mode 100644
index 00000000..a28723ae
Binary files /dev/null and b/fontfiles/NameTest.ttf differ
diff --git a/freetyp2.c b/freetyp2.c
index 8d202d55..268c1333 100644
--- a/freetyp2.c
+++ b/freetyp2.c
@@ -944,8 +944,8 @@ i_ft2_can_face_name(void) {
 #endif
 
 int
-i_ft2_glyph_name(FT2_Fonthandle *handle, unsigned char ch, char *name_buf, 
-                 size_t name_buf_size) {
+i_ft2_glyph_name(FT2_Fonthandle *handle, unsigned long ch, char *name_buf, 
+                 size_t name_buf_size, int reliable_only) {
 #ifdef FT_CONFIG_OPTION_NO_GLYPH_NAMES
   i_clear_error();
   *name_buf = '\0';
@@ -953,35 +953,41 @@ i_ft2_glyph_name(FT2_Fonthandle *handle, unsigned char ch, char *name_buf,
 
   return 0;
 #else
+  FT_UInt index;
+
   i_clear_error();
 
-  if (FT_Has_PS_Glyph_Names(handle->face)) {
-    FT_UInt index = FT_Get_Char_Index(handle->face, ch);
+  if (!FT_HAS_GLYPH_NAMES(handle->face)) {
+    i_push_error(0, "no glyph names in font");
+    *name_buf = '\0';
+    return 0;
+  }
+  if (reliable_only && !FT_Has_PS_Glyph_Names(handle->face)) {
+    i_push_error(0, "no reliable glyph names in font - set reliable_only to 0 to try anyway");
+    *name_buf = '\0';
+    return 0;
+  }
 
-    if (index) {
-      FT_Error error = FT_Get_Glyph_Name(handle->face, index, name_buf, 
-                                         name_buf_size);
-      if (error) {
-        ft2_push_message(error);
-        *name_buf = '\0';
-        return;
-      }
-      if (*name_buf) {
-        return strlen(name_buf) + 1;
-      }
-      else {
-        return 0;
-      }
+  index = FT_Get_Char_Index(handle->face, ch);
+  
+  if (index) {
+    FT_Error error = FT_Get_Glyph_Name(handle->face, index, name_buf, 
+                                       name_buf_size);
+    if (error) {
+      ft2_push_message(error);
+      *name_buf = '\0';
+      return;
+    }
+    if (*name_buf) {
+      return strlen(name_buf) + 1;
     }
     else {
-      i_push_error(0, "no glyph for that character");
-      *name_buf = 0;
       return 0;
     }
   }
   else {
-    i_push_error(0, "no glyph names in font");
-    *name_buf = '\0';
+    i_push_error(0, "no glyph for that character");
+    *name_buf = 0;
     return 0;
   }
 #endif
diff --git a/image.h b/image.h
index 97628cb0..87912b2d 100644
--- a/image.h
+++ b/image.h
@@ -308,8 +308,9 @@ extern int i_ft2_has_chars(FT2_Fonthandle *handle, char const *text, int len,
 extern int i_ft2_face_name(FT2_Fonthandle *handle, char *name_buf, 
                            size_t name_buf_size);
 extern int i_ft2_can_face_name(void);
-extern int i_ft2_glyph_name(FT2_Fonthandle *handle, unsigned char ch, 
-                            char *name_buf, size_t name_buf_size);
+extern int i_ft2_glyph_name(FT2_Fonthandle *handle, unsigned long ch, 
+                            char *name_buf, size_t name_buf_size,
+                            int reliable_only);
 extern int i_ft2_can_do_glyph_names(void);
 extern int i_ft2_face_has_glyph_names(FT2_Fonthandle *handle);
 
diff --git a/lib/Imager/Font.pm b/lib/Imager/Font.pm
index b21ae81f..710063fb 100644
--- a/lib/Imager/Font.pm
+++ b/lib/Imager/Font.pm
@@ -700,7 +700,7 @@ Imager::Font->new(file=>"arial.ttf", color=>$blue, aa=>1)
 Returns the internal name of the face.  Not all font types support
 this method yet.
 
-=item glyph_names(string=>$string [, utf8=>$utf8 ] );
+=item glyph_names(string=>$string [, utf8=>$utf8 ][, reliable_only=>0 ] );
 
 Returns a list of glyph names for each of the characters in the
 string.  If the character has no name then C<undef> is returned for
@@ -710,6 +710,13 @@ Some font files do not include glyph names, in this case Freetype 2
 will not return any names.  Freetype 1 can return standard names even
 if there are no glyph names in the font.
 
+Freetype 2 has an API function that returns true only if the font has
+"reliable glyph names", unfortunately this always returns false for
+TTF fonts.  This can avoid the check of this API by supplying
+C<reliable_only> as 0.  The consequences of using this on an unknown
+font may be unpredictable, since the Freetype documentation doesn't
+say how those name tables are unreliable, or how FT2 handles them.
+
 Both Freetype 1.x and 2.x allow support for glyph names to not be
 included.
 
diff --git a/lib/Imager/Font/FreeType2.pm b/lib/Imager/Font/FreeType2.pm
index 9b1bbb47..9471e504 100644
--- a/lib/Imager/Font/FreeType2.pm
+++ b/lib/Imager/Font/FreeType2.pm
@@ -132,10 +132,15 @@ sub glyph_names {
 
   my $string = $input{string};
   defined $string
-    or return Imager->_seterror("no string parameter passed to glyph_names");
-  my $utf8 = _first($input{utf8} || 0);
+    or return Imager->_set_error("no string parameter passed to glyph_names");
+  my $utf8 = _first($input{utf8}, 0);
+  my $reliable_only = _first($input{reliable_only}, 1);
 
-  i_ft2_glyph_name($self->{id}, $string, $utf8);
+  my @names = i_ft2_glyph_name($self->{id}, $string, $utf8, $reliable_only);
+  @names or return Imager->_set_error(Imager->_error_as_msg);
+
+  return @names if wantarray;
+  return pop @names;
 }
 
 1;
diff --git a/lib/Imager/Font/Truetype.pm b/lib/Imager/Font/Truetype.pm
index ebb5ce61..ca147c4c 100644
--- a/lib/Imager/Font/Truetype.pm
+++ b/lib/Imager/Font/Truetype.pm
@@ -92,7 +92,7 @@ sub glyph_names {
 
   my $string = $input{string};
   defined $string
-    or return Imager->_seterror("no string parameter passed to glyph_names");
+    or return Imager->_set_error("no string parameter passed to glyph_names");
   my $utf8 = _first($input{utf8} || 0);
 
   Imager::i_tt_glyph_name($self->{id}, $string, $utf8);
diff --git a/t/t38ft2font.t b/t/t38ft2font.t
index 3f3e5101..067d5d9e 100644
--- a/t/t38ft2font.t
+++ b/t/t38ft2font.t
@@ -7,7 +7,7 @@
 # Change 1..1 below to 1..last_test_to_print .
 # (It may become useful if the test is moved to ./t subdirectory.)
 
-BEGIN { $| = 1; print "1..116\n"; }
+BEGIN { $| = 1; print "1..120\n"; }
 END {print "not ok 1\n" unless $loaded;}
 use Imager qw(:all);
 
@@ -264,7 +264,8 @@ else {
 }
 
 if (Imager::Font::FreeType2->can_glyph_names) {
-  # pfaedit doesn't seem to save glyph names into TTF files
+  # FT2 considers POST tables in TTF fonts unreliable, so use
+  # a type 1 font, see below for TTF test 
   my $exfont = Imager::Font->new(file=>'fontfiles/ExistenceTest.pfb',
                                type=>'ft2');
   if (okx($exfont, "load Type 1 via FT2")) {
@@ -272,22 +273,51 @@ if (Imager::Font::FreeType2->can_glyph_names) {
       Imager::Font::FreeType2::i_ft2_glyph_name($exfont->{id}, "!J/");
     #use Data::Dumper;
     #print Dumper \@glyph_names;
-    okx($glyph_names[0] eq 'exclam', "check exclam name");
+    isx($glyph_names[0], 'exclam', "check exclam name");
     okx(!defined($glyph_names[1]), "check for no J name");
-    okx($glyph_names[2] eq 'slash', "check slash name");
+    isx($glyph_names[2], 'slash', "check slash name");
 
     # oo interfaces
     @glyph_names = $exfont->glyph_names(string=>"!J/");
-    okx($glyph_names[0] eq 'exclam', "check exclam name OO");
+    isx($glyph_names[0], 'exclam', "check exclam name OO");
     okx(!defined($glyph_names[1]), "check for no J name OO");
-    okx($glyph_names[2] eq 'slash', "check slash name OO");
+    isx($glyph_names[2], 'slash', "check slash name OO");
+
+    # make sure a missing string parameter is handled correctly
+    eval {
+      $exfont->glyph_names();
+    };
+    isx($@, "", "correct error handling");
+    matchx(Imager->errstr, qr/no string parameter/, "error message");
   }
   else {
-    skipx(6, "couldn't load type 1 with FT2");
+    skipx(8, "couldn't load type 1 with FT2");
+  }
+
+  # freetype 2 considers truetype glyph name tables unreliable
+  # due to some specific fonts, supplying reliable_only=>0 bypasses
+  # that check and lets us get the glyph names even for truetype fonts
+  # so we can test this stuff <sigh>
+  # we can't use ExistenceTest.ttf since that's generated with 
+  # AppleStandardEncoding since the same .sfd needs to generate
+  # a .pfb file, NameTest.ttf uses a Unicode encoding
+
+  # we were using an unsigned char to store a unicode character
+  # https://rt.cpan.org/Ticket/Display.html?id=7949
+  $exfont = Imager::Font->new(file=>'fontfiles/NameTest.ttf',
+                               type=>'ft2');
+  if (okx($exfont, "load TTF via FT2")) {
+    my $text = pack("C*", 0xE2, 0x80, 0x90); # "\x{2010}" as utf-8
+    my @names = $exfont->glyph_names(string=>$text,
+                                     utf8=>1, reliable_only=>0);
+    isx($names[0], "hyphentwo", "check utf8 glyph name");
+  }
+  else {
+    skipx(1, "could not load TTF with FT2");
   }
 }
 else {
-  skipx(7, "FT2 compiled without glyph names support");
+  skipx(9, "FT2 compiled without glyph names support");
 }
 
 sub align_test {
diff --git a/t/testtools.pl b/t/testtools.pl
index 9821c1a5..d5b2a242 100644
--- a/t/testtools.pl
+++ b/t/testtools.pl
@@ -85,13 +85,11 @@ sub requireokx {
 sub matchn($$$$) {
   my ($num, $str, $re, $comment) = @_;
 
-  my $match = $str =~ $re;
+  my $match = defined($str) && $str =~ $re;
   okn($num, $match, $comment);
   unless ($match) {
-    $str =~ s/\\/\\\\/g;
-    $str =~ s/[^\x20-\x7E]/"\\x".sprintf("%02X", ord($1))/ge;
-    print "# The string '$str'\n";
-    print "# did not match '$re'\n";
+    print "# The value: ",_sv_str($str),"\n";
+    print "# did not match: qr/$re/\n";
   }
   return $match;
 }