code review 14414044: freetype: add script to create more hinting tests.

freetype: add script to create more hinting tests.

[nigeltao, 2013-10-05 01:09:33]

Hello bsiegert@gmail.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/freetype-go

[nigeltao, 2013-10-05 01:12:54]

BTW, I'm not very good at bash script, so any advice there is welcome.

[r, 2013-10-05 15:50:00]

comments on sh only

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
File testdata/make-other-hinting-txts.sh (right):

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:6: # It has only been tested on an Ubuntu
12.04 system.

set -e

(exit on first error)

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:6: # It has only been tested on an Ubuntu
12.04 system.
shouldn't there be a cd command?

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:14: gcc ../cmd/print-glyph-points/main.c
-I/usr/include/freetype2 -lfreetype -o print-glyph-points
strange argument order. i would put the .c as the last argument. i'm surprised
it even works like this

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:17:
x-droid-sans-japanese-9pt-with-hinting.txt
i'm not convinced the line folding aids readability

[bsiegert, 2013-10-05 16:28:07]

https://codereview.appspot.com/14414044/diff/4001/cmd/print-glyph-points/main.c
File cmd/print-glyph-points/main.c (right):

https://codereview.appspot.com/14414044/diff/4001/cmd/print-glyph-points/main...
cmd/print-glyph-points/main.c:10: printf("usage: %s font_size font_file
[with_hinting|sans_hinting]\n", argv[0]);
no_hinting?

https://codereview.appspot.com/14414044/diff/4001/freetype/truetype/truetype_...
File freetype/truetype/truetype_test.go (right):

https://codereview.appspot.com/14414044/diff/4001/freetype/truetype/truetype_...
freetype/truetype/truetype_test.go:121: t.Errorf("%s: Open: %v", tc.name, err)
I do not remember offhand, but doesn't the error from Open include the file
name?

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
File testdata/make-other-hinting-txts.sh (right):

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:6: # It has only been tested on an Ubuntu
12.04 system.
On 2013/10/05 15:50:00, r wrote:
> shouldn't there be a cd command? 

Or mention in a comment where this script is supposed to be run.

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:8: ln -f -s
/usr/share/fonts/truetype/droid/DroidSansJapanese.ttf      
x-droid-sans-japanese.ttf
This is usually written as "ln -sf".

Also, make the fonts dir configurable, please:

${FONTDIR:=/usr/share/fonts/truetype}
Then:
ln -sf $FONTDIR/droid/yada yada

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:14: gcc ../cmd/print-glyph-points/main.c
-I/usr/include/freetype2 -lfreetype -o print-glyph-points
${CC:=gcc} $(pkg-config --cflags --libs freetype2) -o print-glyph-points
../cmd/print-glyph-points/main.c

[nigeltao, 2013-10-05 21:56:21]

https://codereview.appspot.com/14414044/diff/4001/cmd/print-glyph-points/main.c
File cmd/print-glyph-points/main.c (right):

https://codereview.appspot.com/14414044/diff/4001/cmd/print-glyph-points/main...
cmd/print-glyph-points/main.c:10: printf("usage: %s font_size font_file
[with_hinting|sans_hinting]\n", argv[0]);
On 2013/10/05 16:28:07, bsiegert wrote:
> no_hinting?

"sans" matches the filename and the truetype_test.go function name, and I also
like how "with" and "sans" are the same length.

https://codereview.appspot.com/14414044/diff/4001/freetype/truetype/truetype_...
File freetype/truetype/truetype_test.go (right):

https://codereview.appspot.com/14414044/diff/4001/freetype/truetype/truetype_...
freetype/truetype/truetype_test.go:121: t.Errorf("%s: Open: %v", tc.name, err)
On 2013/10/05 16:28:07, bsiegert wrote:
> I do not remember offhand, but doesn't the error from Open include the file
> name?

It does, but I prefer to be consistent if redundant.

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
File testdata/make-other-hinting-txts.sh (right):

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:6: # It has only been tested on an Ubuntu
12.04 system.
On 2013/10/05 16:28:07, bsiegert wrote:
> On 2013/10/05 15:50:00, r wrote:
> > shouldn't there be a cd command? 
> 
> Or mention in a comment where this script is supposed to be run.

Added a comment:
# Run it from this directory (testdata).

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:6: # It has only been tested on an Ubuntu
12.04 system.
On 2013/10/05 15:50:00, r wrote:
> 
> set -e
> 
> (exit on first error)

Done.

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:8: ln -f -s
/usr/share/fonts/truetype/droid/DroidSansJapanese.ttf      
x-droid-sans-japanese.ttf
On 2013/10/05 16:28:07, bsiegert wrote:
> This is usually written as "ln -sf".
> 
> Also, make the fonts dir configurable, please:
> 
> ${FONTDIR:=/usr/share/fonts/truetype}
> Then:
> ln -sf $FONTDIR/droid/yada yada

${FONTDIR:=/usr/share/fonts/truetype}
didn't work. It said:
./make-other-hinting-txts.sh: line 12: /usr/share/fonts/truetype: Is a directory

Instead, I did
FONTDIR="/usr/share/fonts/truetype"
Let me know if that's right.

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:14: gcc ../cmd/print-glyph-points/main.c
-I/usr/include/freetype2 -lfreetype -o print-glyph-points
On 2013/10/05 16:28:07, bsiegert wrote:
> ${CC:=gcc} $(pkg-config --cflags --libs freetype2) -o print-glyph-points
> ../cmd/print-glyph-points/main.c

Done.

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:14: gcc ../cmd/print-glyph-points/main.c
-I/usr/include/freetype2 -lfreetype -o print-glyph-points
On 2013/10/05 15:50:00, r wrote:
> strange argument order. i would put the .c as the last argument. i'm surprised
> it even works like this

The weird order works. The suggested order doesn't.
http://stackoverflow.com/questions/14110769/undefined-reference-with-freetype-2
suggests that it's a linker order thing.

$ gcc -I/usr/include/freetype2 -lfreetype -o print-glyph-points
../cmd/print-glyph-points/main.c
/tmp/ccatmY4z.o: In function `main':
main.c:(.text+0x131): undefined reference to `FT_Init_FreeType'
main.c:(.text+0x17e): undefined reference to `FT_New_Face'
main.c:(.text+0x1cd): undefined reference to `FT_Set_Char_Size'
main.c:(.text+0x226): undefined reference to `FT_Load_Glyph'
collect2: ld returned 1 exit status

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:17:
x-droid-sans-japanese-9pt-with-hinting.txt
On 2013/10/05 15:50:00, r wrote:
> i'm not convinced the line folding aids readability

Done.

[nigeltao, 2013-10-08 09:02:19]

Ping.

[bsiegert, 2013-10-08 09:04:33]

LGTM modulo the comment below.

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
File testdata/make-other-hinting-txts.sh (right):

https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting...
testdata/make-other-hinting-txts.sh:8: ln -f -s
/usr/share/fonts/truetype/droid/DroidSansJapanese.ttf      
x-droid-sans-japanese.ttf
Sorry, I meant

: ${FONTDIR:=/usr/share/fonts/truetype}

(forgot the colon)

[r, 2013-10-08 13:49:37]

LGTM

[nigeltao, 2013-10-09 09:07:47]

*** Submitted as
https://code.google.com/p/freetype-go/source/detail?r=00d9ad1a50a7 ***

freetype: add script to create more hinting tests.

R=bsiegert, r
CC=golang-dev
https://codereview.appspot.com/14414044