[markFeatureWriter] disable grouping by default, build one lookup per mark class

[anthrotype]

this was motivated by discussions in #762 and #591

Also this is more similar to the way we decided to implement the mark feature generation in fontc.

I haven't added tests yet, am undecided whether we need to add an option like I did here or change the default behavior and do not group marks any more.

Also I simply sorted the lookups by the anchor name. I think this is easy enough to understand and work with and should cover the use case of "more specific anchors should override less specific ones". In general, the more specific anchors qualify the name of more general ones with some suffix so sorting alphabetically should make sure that e.g. 'top.alt' comes after 'top' and thus prevail in case of ambiguous attachments.

/cc @simoncozens @khaledhosny WDYT?

[anthrotype]

/cc @belluzj @madig

[khaledhosny]

I’m inclined to drop the mark grouping entirely. I don’t see any reason that warrants the extra code complication, and optimizations should not cause functional difference (as this optimization does).

Also it does not seem to be optimizing for something that matters, since it only decreases the number of lookup and increases the file size (in my tests), but there is only a handful of anchor names in any given font so the number of lookup does not matter in practice.

FWIW, Glyphs seems to be producing one lookup per anchor as well.

[khaledhosny]

Or at least make the default to False for now and see if anything breaks.

[belluzj]

If having many lookups doesn't matter then I don't have objections to simplify the code

[anthrotype]

another thing to note about this is that, currently we are somewhat arbitrarily giving priority to "top" and "bottom" anchors when we sort the lookups, I think this was done to match Glyphs.app however lots of time has passed and various changes occurred and this may no longer be necessary. In this PR, I am simply sorting alphabetically which means top and bottom will not be treated any special.
I think this is in theory the right thing to do and makes it easier to a font developer to understand what will happen and how to work within this and rename anchors accordingly if they desire a different order. But there might be existing font projects that may rely on the current behavior for one reason or another, which makes me wary of flipping the default...

[anthrotype]

I just tested with the latest Glyphs.app and it turns out it simply orders each lookup alphabetically and the lookups are named after the mark class they contain so it's not actually true that "top" and "bottom" always prevail. If a glyph has e.g. both "top" and "top.alt" and another mark has both "_top" and "_top.alt" anchors, which one do you expect to win?

Well, for current ufo2ft, "top" should win. But for Glyphs.app and for ufo2ft after this PR, it's "top.alt" that is placed last in the lookup order and thus is the one that sticks.

So we can argue that we are fixing a bug and aligning ourselves back to a major implementation like Glyphs.app and turn off the grouping by default and simply sort alphabetically.

We can keep it as an option just in case one wants the old behavior for compat reasons.

[anthrotype]

This will fail until I update the tests, but you get the idea. Should we then bump major version perhaps? Maybe do this at the same time as we ship the variable feature writers in #635..

[khaledhosny]

This will fail until I update the tests, but you get the idea. Should we then bump major version perhaps? Maybe do this at the same time as we ship the variable feature writers in #635..

For me this is a bug fix, so I don’t think it needs a major version.

[anthrotype]

For me this is a bug fix, so I don’t think it needs a major version.

I understand it doesn't suit your workflow but I don't think we can describe the current behavior as a 'bug', in fact we are keeping that behind a flag groupMarkClasses=True.

[khaledhosny]

I think #762 counts as a bug.

[anthrotype]

yes in some situations (with multiple ambiguous mark anchors) the grouping may produce undesired behavior, but the desired behavior is not well defined either. There are some benefits to the grouping (fewer mark lookups) and in most circumstances (except for the above mentioned) it works fine.
By switching the default to groupMarkClasses=False we will change all the newly built fonts even when they were working fine and this may be construed as an unwanted breaking change, hence my hesitation to simply bump patch version in this case.

[khaledhosny]

Ungrouping mark anchors is unlikely to cause regressions (and if it does it is more likely to be fixing undetected buggy behavior), so this counts like a bug fix.

There is a reason I have to patch the mark feature writer in every single font project I work on, the current behavior is broken.