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

Issue 14414044: code review 14414044: freetype: add script to create more hinting tests. (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 2 months ago by nigeltao

Modified:
11 years, 2 months ago

Reviewers:
r, bsiegert

CC:
bsiegert, r, golang-dev

Visibility:
Public.

Description

freetype: add script to create more hinting tests.

Patch Set 2 : diff -r 140544b4efd0 https://code.google.com/p/freetype-go #

Patch Set 3 : diff -r 140544b4efd0 https://code.google.com/p/freetype-go #

Total comments: 18

Patch Set 4 : diff -r 140544b4efd0 https://code.google.com/p/freetype-go #

Patch Set 5 : diff -r 140544b4efd0 https://code.google.com/p/freetype-go #

Created: 11 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+147 lines, -53 lines)			Patch
M	cmd/print-glyph-points/main.c	View	1 2 3	2 chunks	+26 lines, -5 lines	0 comments	Download
M	freetype/truetype/truetype_test.go	View	1	1 chunk	+89 lines, -46 lines	0 comments	Download
M	testdata/README	View	1	1 chunk	+2 lines, -2 lines	0 comments	Download
A	testdata/make-other-hinting-txts.sh	View	1 2 3 4	1 chunk	+30 lines, -0 lines	0 comments	Download

Messages

Total messages: 9

Expand All Messages | Collapse All Messages

nigeltao

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

11 years, 2 months ago (2013-10-05 01:09:33 UTC) #1

nigeltao

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

11 years, 2 months ago (2013-10-05 01:12:54 UTC) #2

comments on sh only https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting-txts.sh File testdata/make-other-hinting-txts.sh (right): https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting-txts.sh#newcode6 testdata/make-other-hinting-txts.sh:6: # It has only been ...

11 years, 2 months ago (2013-10-05 15:50:00 UTC) #3

bsiegert

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.c#newcode10 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_test.go File ...

11 years, 2 months ago (2013-10-05 16:28:07 UTC) #4

nigeltao

11 years, 2 months ago (2013-10-05 21:56:21 UTC) #5

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.

bsiegert

LGTM modulo the comment below. https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting-txts.sh File testdata/make-other-hinting-txts.sh (right): https://codereview.appspot.com/14414044/diff/4001/testdata/make-other-hinting-txts.sh#newcode8 testdata/make-other-hinting-txts.sh:8: ln -f -s /usr/share/fonts/truetype/droid/DroidSansJapanese.ttf ...

11 years, 2 months ago (2013-10-08 09:04:33 UTC) #7

nigeltao

11 years, 2 months ago (2013-10-09 09:07:47 UTC) #9

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

Expand All Messages | Collapse All Messages