Update KernFeatureWriter to write separate lookups without IgnoreMarks

[punchcutter]

To match Glyphs output for auto-generated lookups we need to determine if any kern pairs contain marks and then we don't want to use the IgnoreMarks flag on those lookups.

Also, some scripts get a dist feature by default, but it's entirely possible to have both kern and dist in the same font. There are no 100% clear rules about this, but if there is a contextual kern lookup in Glyphs then it will be added as kern.

[marekjez86]

LGTM

[anthrotype]

I would have liked to have the chance to review this...

[anthrotype]

It’s OK, I can do any fixups if needed before the next release. Thanks Zachary!

[marekjez86]

My bad I rushed it. Cosimo please do the right thing Marek

…

On Fri, Feb 22, 2019 at 11:10 Cosimo Lupo ***@***.***> wrote: It’s OK, I can do any fixups if needed before the next release. Thanks Zachary! — You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub <#314 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMtJfx3qSpsJsWPSzJOZfmLo-tizcVBAks5vP8ImgaJpZM4azhQE> .

[punchcutter]

@anthrotype Yes, please let me know what you think. I don't necessarily like everything about this, but it is an issue that needs to be addressed. This came up when we added the IgnoreMarks flag in the first place, but now I really ran into it while prepping Noto Siddham for delivery.

[davelab6]

I suggest to review the repo settings - I believe you can ensure that all PRs get a review now

[anthrotype]

we need to determine if any kern pairs contain marks and then we don't want to use the IgnoreMarks flag on those lookups.

That makes sense. Can you remind me which Noto font requires this patch?

I'm not quite sure about the dist vs kern distinction that you make (still trying to figure out what the code does and why)

[anthrotype]

@punchcutter ^

[punchcutter]

One Noto font that needs this is Siddham.

The whole dist vs kern thing is kind of ridiculous in the first place because we are assuming that these things are optional or not. I have a hard time seeing when and where either would or should be optional, but some implementations don't apply kern by default and some apply dist by default so we have a mix of how these are implemented everywhere. ufo2ft adds a dist feature instead of kern for scripts that harfbuzz expects to have dist. Mostly Indic scripts that are expected to use dist and have dist turned on by default. However, there are no real rules that say it must be one way or another. It's perfectly valid to have a kern feature as well as dist. Both are using the same lookup types and doing the same exact thing.

So one thing I was thinking about here is how in Glyphs you can add custom contextual kerning. If the user explicitly defines a kern feature there then I believe we should respect that and write that as a kern feature even if the script is otherwise expected to get a dist feature. If the user explicitly defines the contextual kerning in a dist feature then we write it as dist. I don't really like how this works either way because automatic feature generation can't always be expected to figure out what the user wants.

[anthrotype]

I don't get why you had to modify the _groupScriptsByTagAndDirection method.

Previously this method would simply parse the languagesystem statements and group the script/languages pairs by 1) whether they need "dist" or can use "kern" and 2) the script's dominant horizontal direction. These groupings were mutually exclusive (e.g. a script can either be dist-enabled or not, but not both).

With your patch, the method now also checks wether feature kern string is present in the features text (should rather use findFeatureTags function in ufo2ft.featureWriters.ast module), and if so it groups the dist-enabled languagesystems under both "kern" and "dist".

I don't understand why you need to do that.

[anthrotype]

what's wrong with outputting automatic dist feature even when user has some hand-coded kern feature?

[punchcutter]

It should output automatic dist even when user has hand-coded kern. Either way I would expect both features if there is code for both features. Or code for one and automatic code for another.

[anthrotype]

yeah, that's what I mean, the feature writer will add some automatically generated dist feature to the font, and whatever was there before (either 'kern' or none) stays untouched. That's the meaning of the mode "append".

[punchcutter]

Isn't that what's happening now? I don't remember exactly what's going on, but you can do whatever is right. Sorry if it's a mess. I was expecting to have this conversation before it got merged, but that didn't work out.

[anthrotype]

no problem, thanks!

I'll run this through Noto Siddham and make sure it does the right thing.