Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ligature Support #32

Merged
merged 1 commit into from Apr 16, 2015
Merged

Ligature Support #32

merged 1 commit into from Apr 16, 2015

Conversation

sabberworm
Copy link
Contributor

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
Copy link
Contributor Author

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.

This was referenced Dec 3, 2014
@puzrin
Copy link
Member

puzrin commented Dec 4, 2014

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
Copy link
Member

puzrin commented Dec 4, 2014

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

@puzrin
Copy link
Member

puzrin commented Dec 6, 2014

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
Copy link
Member

puzrin commented Jan 23, 2015

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
Copy link
Contributor Author

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
Copy link
Member

puzrin commented Jan 24, 2015

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
Copy link
Contributor Author

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
Copy link
Member

puzrin commented Mar 11, 2015

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
Copy link
Contributor Author

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
Copy link
Contributor Author

Should I squash all the commits into one?

@puzrin
Copy link
Member

puzrin commented Mar 11, 2015

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
Copy link
Member

puzrin commented Mar 11, 2015

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
Copy link
Contributor Author

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.

* Write GSUB table with ligatures
* Rewrite of the cmap code so code points and glyphs are independant of one another
* Refactor cmap code so no offsets have to be changed when commenting out subtables and their headers
* Make sure the missing glyph is added only once and is at the beginning
puzrin pushed a commit that referenced this pull request Apr 16, 2015
@puzrin puzrin merged commit 71bc1ba into fontello:master Apr 16, 2015
@tgfjt tgfjt mentioned this pull request Aug 5, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants