googlefonts / ufo2ft

A bridge from UFOs to FontTools objects (and therefore, OTFs and TTFs).
MIT License
151 stars 43 forks source link

Feature insert code eating features? #724

Closed simoncozens closed 1 year ago

simoncozens commented 1 year ago

I just started trying to fix #506 (at least the part where abvm/blwm/mark are having classes placed in the wrong order), and I noticed something odd in my test failures:

E           @kern2.Default.foo = [period];
E           @kern2.Latn.foo = [b];
E         +
E         + feature ss01 {
E         +     sub a by period;
E         +     # Make period be both Latn and Zyyy.
E         + } ss01;
E
E           lookup kern_Latn {

Wait, what, is the kern feature writer hallucinating entire features? No, it turns out that we add that code to the feature file when testing:

https://github.com/googlefonts/ufo2ft/blob/87c688180f983dd2dd9a77f2d3d6b1784340ab8c/tests/featureWriters/kernFeatureWriter_test.py#L1380-L1392

but when we set our test expectations, it's not there:

https://github.com/googlefonts/ufo2ft/blob/87c688180f983dd2dd9a77f2d3d6b1784340ab8c/tests/featureWriters/kernFeatureWriter_test.py#L1326-L1335

I don't understand this. To me it looks like the kern feature writer is eating the ss01 feature, and our tests ensure that it does get eaten. Am I reading this wrong?

anthrotype commented 1 year ago

Only the new features are returned from writeFeatures method

simoncozens commented 1 year ago

Ah, OK, so the tests have an assumption that new things are added to the end of the file. My thought was to place all the definitions right at the top. Anyway, that makes more sense; thank you!

anthrotype commented 1 year ago

it's defined in https://github.com/googlefonts/ufo2ft/blob/87c688180f983dd2dd9a77f2d3d6b1784340ab8c/tests/featureWriters/__init__.py#L9-L20

however, now I'm thinking that assuming that only the lines following len(feaFile) are new might not actually be true all the time esp. with insertion markers and such

simoncozens commented 1 year ago

I'm rewriting it to look for "really new" statements (i.e. not in the file before), regardless of where they were. This will allow me to then put definitions at the top.