code review 1121045: Freetype-Go: new freetype package to provide a convenie...

Freetype-Go: new freetype package to provide a convenience API to
draw text onto an image.

This is a simple API that doesn't handle line breaking, ligatures,
right-to-left or vertical scripts, and other fancy features.

[nigeltao, 2010-05-12 07:46:57]

Hello r, rsc,

I'd like you to review this change.

[nigeltao, 2010-05-12 07:50:09]

CC golang-dev@googlegroups.com.

[r, 2010-05-12 20:08:56]

http://codereview.appspot.com/1121045/diff/9001/10005
File freetype/freetype.go (right):

http://codereview.appspot.com/1121045/diff/9001/10005#newcode6
freetype/freetype.go:6: // The freetype package provides a convenience API to
draw text onto an image.
s/convenience/convenient/

http://codereview.appspot.com/1121045/diff/9001/10005#newcode19
freetype/freetype.go:19: // co-ordinate pair measured in raster.Fixed units.
again this spelling.  i don't mind it (much); just be sure to be consistent

http://codereview.appspot.com/1121045/diff/9001/10005#newcode24
freetype/freetype.go:24: // An RGBAContext knows how to draw text from a given
font at a given size
it's inanimate. it doesn't know anything.

An RGBAContext holds the state for drawing text from a given font at a given
size.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode26
freetype/freetype.go:26: type RGBAContext struct {
i'm not thrilled with having a context. what does it buy you?
the libraries i've used with graphics contexts have encouraged some annoying
classes of bugs due to sharing. they're hard to use well even though the context
seems superficially to make things nicer.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode30
freetype/freetype.go:30: glyph    *truetype.Glyph
it feels wrong to me that the glyph should be part of the context

http://codereview.appspot.com/1121045/diff/9001/10005#newcode38
freetype/freetype.go:38: // rasterizer space.
this comment doesn't explain the sign of xmin and ymin. compare it to the next
comment.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode58
freetype/freetype.go:58: // FUnitToFixed converts the given number of FUnits
into fixed point units,
s/Fixed/FixedRD

http://codereview.appspot.com/1121045/diff/9001/10005#newcode64
freetype/freetype.go:64: // FUnitToFixed converts the given number of FUnits
into fixed point units,
s//FixedRU/

http://codereview.appspot.com/1121045/diff/9001/10005#newcode67
freetype/freetype.go:67: return raster.Fixed((x*c.scale + 255) >> 8)
guaranteed never negative?

http://codereview.appspot.com/1121045/diff/9001/10005#newcode73
freetype/freetype.go:73: return raster.Fixed(x * float(c.dpi) * 256.0 / 72.0)
is it worth parenthesizing the constant division? guarantee we avoid a division.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode123
freetype/freetype.go:123: // DrawText draws s with the top-left of the text at
pt.
you need to be much more explicit about what this means.  compare drawing "."
with "T"

http://codereview.appspot.com/1121045/diff/9001/10005#newcode185
freetype/freetype.go:185: ymax := int((c.FUnitToFixedRU(c.upe-int(b.YMin)) +
255) >> 8)
these lines argue that you may have the return type wrong

http://codereview.appspot.com/1121045/diff/9001/10005#newcode190
freetype/freetype.go:190: // SetColor sets the color to draw text with.
s/ with/

http://codereview.appspot.com/1121045/diff/9001/10005#newcode195
freetype/freetype.go:195: // SetDPI sets the screen resolution in Dots Per Inch.
caps unnecessary

http://codereview.appspot.com/1121045/diff/9001/10005#newcode201
freetype/freetype.go:201: // SetFont sets the font.
this and the next one need more explanation

http://codereview.appspot.com/1121045/diff/9001/10007
File freetype/raster/raster.go (right):

http://codereview.appspot.com/1121045/diff/9001/10007#newcode55
freetype/raster/raster.go:55: // A cell is part of a linked list (for a given yi
co-ordinate) of accumulated
why change the spelling?

[nigeltao, 2010-05-13 01:30:14]

http://codereview.appspot.com/1121045/diff/9001/10005
File freetype/freetype.go (right):

http://codereview.appspot.com/1121045/diff/9001/10005#newcode6
freetype/freetype.go:6: // The freetype package provides a convenience API to
draw text onto an image.
On 2010/05/12 20:08:56, r wrote:
> s/convenience/convenient/

Done.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode19
freetype/freetype.go:19: // co-ordinate pair measured in raster.Fixed units.
On 2010/05/12 20:08:56, r wrote:
> again this spelling.  i don't mind it (much); just be sure to be consistent

It is now consistently "co-ordinate".

http://codereview.appspot.com/1121045/diff/9001/10005#newcode24
freetype/freetype.go:24: // An RGBAContext knows how to draw text from a given
font at a given size
On 2010/05/12 20:08:56, r wrote:
> it's inanimate. it doesn't know anything.
> 
> An RGBAContext holds the state for drawing text from a given font at a given
> size.

Done.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode26
freetype/freetype.go:26: type RGBAContext struct {
On 2010/05/12 20:08:56, r wrote:
> i'm not thrilled with having a context. what does it buy you?

I think context isn't a great name, but I don't know what else to call it, and I
do want to have something that holds all this state. Are you suggesting instead
to do away with the struct and pass a bunch of extra args to DrawText?

http://codereview.appspot.com/1121045/diff/9001/10005#newcode30
freetype/freetype.go:30: glyph    *truetype.Glyph
On 2010/05/12 20:08:56, r wrote:
> it feels wrong to me that the glyph should be part of the context

Perhaps the Glyph type is poorly named. It's really some buffers that gets
re-used for each glyph to draw, rather than allocating a new Glyph each time.

How about s/truetype.Glyph/truetype.GlyphBuf/ ? The C Freetype library might
call this thing a GlyphSlot, but I might be confusing that with another concept,
and would have to double-check.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode38
freetype/freetype.go:38: // rasterizer space.
On 2010/05/12 20:08:56, r wrote:
> this comment doesn't explain the sign of xmin and ymin. compare it to the next
> comment.

I've added, "xmin and ymin are typically non-positive." Is that what you were
after?

http://codereview.appspot.com/1121045/diff/9001/10005#newcode58
freetype/freetype.go:58: // FUnitToFixed converts the given number of FUnits
into fixed point units,
On 2010/05/12 20:08:56, r wrote:
> s/Fixed/FixedRD

Done.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode64
freetype/freetype.go:64: // FUnitToFixed converts the given number of FUnits
into fixed point units,
On 2010/05/12 20:08:56, r wrote:
> s//FixedRU/

Done.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode67
freetype/freetype.go:67: return raster.Fixed((x*c.scale + 255) >> 8)
On 2010/05/12 20:08:56, r wrote:
> guaranteed never negative?

There is no guarantee if x itself is negative.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode73
freetype/freetype.go:73: return raster.Fixed(x * float(c.dpi) * 256.0 / 72.0)
On 2010/05/12 20:08:56, r wrote:
> is it worth parenthesizing the constant division? guarantee we avoid a
division.

Done.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode123
freetype/freetype.go:123: // DrawText draws s with the top-left of the text at
pt.
On 2010/05/12 20:08:56, r wrote:
> you need to be much more explicit about what this means.  compare drawing "."
> with "T"

Done.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode185
freetype/freetype.go:185: ymax := int((c.FUnitToFixedRU(c.upe-int(b.YMin)) +
255) >> 8)
On 2010/05/12 20:08:56, r wrote:
> these lines argue that you may have the return type wrong

You're right. I've changed them to convert to pixel units instead of Fixed
units. It's much nicer.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode190
freetype/freetype.go:190: // SetColor sets the color to draw text with.
On 2010/05/12 20:08:56, r wrote:
> s/ with/

Done.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode195
freetype/freetype.go:195: // SetDPI sets the screen resolution in Dots Per Inch.
On 2010/05/12 20:08:56, r wrote:
> caps unnecessary

Done.

http://codereview.appspot.com/1121045/diff/9001/10005#newcode201
freetype/freetype.go:201: // SetFont sets the font.
On 2010/05/12 20:08:56, r wrote:
> this and the next one need more explanation

I'm not sure what else to add.

I've also removed the SetFontBytes method and instead added a ParseFont function
at the top of this file.

http://codereview.appspot.com/1121045/diff/9001/10007
File freetype/raster/raster.go (right):

http://codereview.appspot.com/1121045/diff/9001/10007#newcode55
freetype/raster/raster.go:55: // A cell is part of a linked list (for a given yi
co-ordinate) of accumulated
On 2010/05/12 20:08:56, r wrote:
> why change the spelling?

It's for consistency: "co-ordinate" is spelled with a dash everywhere else in
freetype-go.

[r, 2010-05-13 21:21:49]

LGTM

i still have reservations about the context idea but without a counterproposal
we might as well move on

http://codereview.appspot.com/1121045/diff/15001/16005
File freetype/freetype.go (right):

http://codereview.appspot.com/1121045/diff/15001/16005#newcode37
freetype/freetype.go:37: glyph    *truetype.Glyph
glyphBuf is better.

http://codereview.appspot.com/1121045/diff/15001/16005#newcode45
freetype/freetype.go:45: // rasterizer space. xmin and ymin are typically
non-positive.
how about just saying  "<= 0"?

[nigeltao, 2010-05-14 03:30:34]

*** Submitted as
http://code.google.com/p/freetype-go/source/detail?r=f14d8dac603e ***

Freetype-Go: new freetype package to provide a convenience API to
draw text onto an image.

This is a simple API that doesn't handle line breaking, ligatures,
right-to-left or vertical scripts, and other fancy features.

R=r, rsc
CC=golang-dev
http://codereview.appspot.com/1121045

[nigeltao_gnome, 2010-05-18 07:53:10]

On 13 May 2010 14:21,  <r@golang.org> wrote:
> i still have reservations about the context idea but without a
> counterproposal we might as well move on

Just a thought: rename Context to Canvas.