search

robotools / compositor

A basic OpenType GSUB and GPOS layout engine.
MIT License
31 stars 11 forks source link

test Coverage object if it has a Format attr #7

Closed typemytype closed 8 years ago

typemytype commented 9 years ago

a small patch to make compositor work with the github fontTools version

as far as I have tested it seems like the github fontTools doesn't add a Format attribute anymore, wondering why. It is not writing the Format attr to ttx. 

if hasattr(coverage, "Format"):
    self.CoverageFormat = coverage.Format

https://github.com/typesupply/compositor/blob/master/Lib/compositor/subTablesBase.py#L462

thanks

behdad commented 9 years ago

Correct.

(Didn't know anyone relies on it. I'm open to reconsidering that. But basically the reason was that the "Format" is not needed to use the coverage data, and best format is automatically chosen when compiling the font and the Format set on the object is ignored. The only use it can possibly have is informational.

typesupply commented 9 years ago

FontTools is very frequently used to inspect the data inside of a font, not just to modify it and recompile it. CoverageFormat is an official part of the spec.

Is there a change list somewhere of all the things that you've done since forking from the original FontTools? We're running into lots of things so it would be good to have something to review.

benkiel commented 9 years ago

+1 on @typesupply's request

There are several things that I use/have written over the past decade that rely on FontTools working in a stable way. A change list would be very helpful.

behdad commented 8 years ago

This can be closed now.

adrientetar commented 8 years ago

This can be closed now.

I am still experiencing this bug.

behdad commented 8 years ago

This can be closed now.

I am still experiencing this bug.

Humm. Even after this commit: https://github.com/behdad/fonttools/commit/6851f66d1810a4d3f484f2a82e39960e1a677f42

adrientetar commented 8 years ago

Humm. Even after this commit: behdad/fonttools@6851f66

Yea.

behdad commented 8 years ago

Humm. Even after this commit: behdad/fonttools@6851f66

Yea.

Can you please debug it then?

adrientetar commented 8 years ago

@behdad Should buildCoverage in otlLib set the Format attribute? Currently in feaLib neither preWrite() nor postRead() methods of Coverage are called so no Format attribute is set.

behdad commented 8 years ago

Ah, ok, so you are not actually loading from binary. Then that's a separate issue...

Tal, would be nice if you can merge the original suggestion. Or better, just remove the CoverageFormat member completely. There is no use for the Format of a Coverage table as the FontTools layer hides the different formats.

I'm hesitant to add a hack in otlLib to add Format, which would be wrong. Just told me today that he has updated his code that relied on the Coverage.Format to avoid it. Perhaps it's time to accept that my fork of fonttools is what everyone is using?

adrientetar commented 8 years ago

would be nice if you can merge the original suggestion.

I don't know what you are hinting at here but actually, yeah, CoverageFormat isn't internally used in the code so presumably there's no reason to keep it. I'm submitting a PR for that (I assume if it gets merged, then you are free to revert your patch that keeps the Format attribute around).

typesupply commented 8 years ago

I'll remove CoverageFormat. No big deal. Compositor abstracts the different formats as well.

adrientetar commented 8 years ago

@typesupply Put you a PR up for it.

typesupply commented 8 years ago

Done. Thanks!

behdad commented 8 years ago

Thanks Tal.