fontello / svg2ttf

SVG -> TTF font convertor
MIT License
518 stars 79 forks source link

Ligature Support #32

Closed sabberworm closed 9 years ago

sabberworm commented 9 years ago

I’ve modified the code to read ligature glyphs from the SVG font and write them to the GSUB table of the output file.

As a side-effect, glyphs are now de-duplicated as there isn’t a one-to-one mapping anymore between glyphs and unicode code points.

sabberworm commented 9 years ago

I’ve fixed the jshint errors. I’ve changed .jshintrc to allow “laxbreak”. However, the line breaks jshint complains about are not, in fact, “unsafe line breaks” and when jshint is fixed, I’d suggest turning this option off again.

puzrin commented 9 years ago

That's awesome. You are the first hero after long time, who can undestand mad ttf internals :)

I'll try to do careful review in nearest days.

puzrin commented 9 years ago

I'll continue tommorow. But in general, i'm very impressed with amount of efforts and general code quality.

puzrin commented 9 years ago

That's all. Please, do changes where you agree, and write where you are not agree. Then i'll merge PR and we will continue. Full acceptance + release will require some polish, because changeset is very serious. I appreciate you work - still remember, how many time was spent to make another tables work.

PS. can't find your email for some additional questions. Could you write me to vitaly@rcdesign.ru ?

puzrin commented 9 years ago

How are you? If you are busy, may be it worth to create a separate branch here fo "history" ? May be someone else wish to continue?

As i've written, i doubd about including code "as is" into release due possible compatibility problems, caused by tables change. But you did a big work, and result is valuable. It worth to keep it togeather in this repo somehow.

sabberworm commented 9 years ago

Hi Vitaly,

I’m a bit busy and I did procrastinate IE testing a bit but should get to it within the next few weeks.

I will put the subtable 0 back in if you like. But otherwise the code should be fine as-is. If one of your concerns (besides rewriting the Buffer class, which I’d rather leave to you) hasn’t been addressed, please tell me.

Sincerely, Raphael=

puzrin commented 9 years ago

I have no special requirements like "subtable 0 should exist". The only concern is backward compatibility, because package is used in fontello. If things work with old browsers & old devices - then no subtable0 needed.

Also it probably worth to squash some microcommits, to make changes more easy to understand.

PS. no probroblem, i'll rewrite buffer class

sabberworm commented 9 years ago

The problem I saw with the garbled characters in IE has nothing to do with either the GSUB header nor the subtable 0. I will need to investigate further.

puzrin commented 9 years ago

The problem I saw with the garbled characters in IE has nothing to do with either the GSUB header nor the subtable 0. I will need to investigate further.

Is IE ok without your patches? May be, something specific to your computer setup?

sabberworm commented 9 years ago

I’ve fixed the issues: there was an off-by-one error when writing subtable 4 (complicated beast!). And I’ve decided not to write subtable 6 as firefox can’t render it. Should be all good now.

sabberworm commented 9 years ago

Should I squash all the commits into one?

puzrin commented 9 years ago

Awesome!

Should I squash all the commits into one?

Yes, please. Now it's very difficult to track useful changes. I will do review again then.

puzrin commented 9 years ago

When we did svg2ttf the real time killer was to workaround things, not described in standards. I tried to share such knowledge via comments for others. Please, add comments why subtable 6 should not be added, and anything else you consider important and non trivial about ttf specification - on your choice.

sabberworm commented 9 years ago

I think most of the choices I made are documented (and follow the specs). Subtable 6 is just one format of a two-byte table I thought would be easy to write so I included it (in the unlikely case the resulting table would be smaller than if format 4 was used). However, it turned out that this format isn’t widely supported and the condition (only if it’s smaller than format 4) was rarely triggered, possibly leading to hard-to-track-down bugs. There are many other subtable formats described in the spec that I did not consider adding and don’t know if and how widely they are supported and I think adding a comment on each one of these would not be particularly helpful so I’m not sure commenting on format 6 is necessary.