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

test Coverage object if it has a Format attr #7

Closed
typemytype opened this issue Apr 28, 2015 · 15 comments
Closed

test Coverage object if it has a Format attr #7

typemytype opened this issue Apr 28, 2015 · 15 comments

Comments

@typemytype
Copy link
Member

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

behdad commented Apr 28, 2015

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

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

benkiel commented Apr 29, 2015

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

behdad commented Jan 14, 2016

This can be closed now.

@adrientetar
Copy link
Contributor

This can be closed now.

I am still experiencing this bug.

@behdad
Copy link

behdad commented Jan 20, 2016

This can be closed now.

I am still experiencing this bug.

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

@adrientetar
Copy link
Contributor

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

Yea.

@behdad
Copy link

behdad commented Jan 20, 2016

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

Yea.

Can you please debug it then?

@adrientetar
Copy link
Contributor

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

behdad commented Jan 20, 2016

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

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

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

@adrientetar
Copy link
Contributor

@typesupply Put you a PR up for it.

@typesupply
Copy link
Member

Done. Thanks!

@behdad
Copy link

behdad commented Jan 20, 2016

Thanks Tal.

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

No branches or pull requests

5 participants