Thanks for this work. Glyph is written with 'ph', not with 'f'.

Hi Simon,
Yeah, Glyf refers to the table within a TrueType font, not a glyph. I've discerned the difference between the two by spelling the table-name as it is represented in the font, as "glyf".

Hope that makes sense

Mehdi

Thanks for your explanation.

The title suggests that the patch cleans up something; it does not; it implements a redesign.

Without knowing much detail about Glyf tables, I was a bit surprised about the method names GlyfFlags.offsetToNextComposedGlyf(int flags) and GlyfFlags.moreComposites(int flags). At first sight it suggests that there is some hidden iterator. After some more thought, I assume that flags contains pointers to data in the table, and that these allow one to calculate the extent of this glyf; if that is so, then offsetToNextComposedGlyf may be better called extentOfComposedGlyf.

Otherwise the design seems fine, but I am not able to judge about the correctness of the code.

Hi Simon,
I guess it's best to address each of these, individually:

>The title suggests that the patch cleans up something; it does not; it
>implements a redesign.

Well, I guess so, the reason I call it clean up is because previously there was a lot of duplicated code, magic numbers, poorly named variables... all the big names. I started it with the intention of not changing the working code, but as I got into it, it seemed that there the old code was iterating over the whole of the subset glyph list several times... A job that could have been made simpler through recursion. I didn't redesign it, because if I were to do so, I would have changed FontFileReader by removing the write method, and remapping the glyphs when the glyph data is extracted from "glyf". (see the TODO)

>Without knowing much detail about Glyf tables, I was a bit surprised about the
>method names GlyfFlags.offsetToNextComposedGlyf(int flags)...

Ok, for this you have to read the TTF spec. But, the flags represent the properties and parameters of the composed glyph and, depending on which flags are set, the number of bytes until the next glyph data.. That method returns the offset to the next composed by reading which flags are set, and which aren't. Since none of the glyph parameters are changed, we just want the offset to the next composed glyph.

> GlyfFlags.moreComposites(int flags)

This analyses the flags, checks whether the current data is the last glyph in the "glyf" data we are reading. It is simply used to control the do-while loop.

Hope that helps, the glyf table spec can be found at: http://www.microsoft.com/typography/otspec/glyf.htm

I think this change more is more consistent with the spec, since composite glyphs can be recursive, it seemed prudent to me to clean-up/redesign the code to match that.

Created attachment 27345 [details]
Glyf Table

Added javadocs to GlyfFlags and changed the method name to make it less ambiguous.

Thanks for your explanations. Note that I will not commit this patch. I will leave that to committers who are more knowledgable about fonts.

So a test was possible after all, this is great news. I've applied the patch with minor modifications to make the test run consistently on (hopefully) all JVMs. (I replaced two HashMap with TreeMap.)

Thanks!
Vincent