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

Support contextual anchors #921

Merged
merged 12 commits into from
Aug 2, 2023
Merged

Support contextual anchors #921

merged 12 commits into from
Aug 2, 2023

Conversation

simoncozens
Copy link
Collaborator

See #910. This implements the *top (etc.) anchors with an embedded custom context, causing them to replace standard anchor positions in certain contexts. Tested fairly extensively on an internal font project, but:

  • No test suite yet
  • No mkmk handling yet

@simoncozens simoncozens force-pushed the contextual-mark-writer branch from 76f92e0 to e3a4edf Compare June 28, 2023 06:20
@simoncozens
Copy link
Collaborator Author

I have to be honest here and say I've done what I need to make my font project work, and if anyone actually needs this in glyphsLib they're welcome to take it over.

@khaledhosny khaledhosny force-pushed the contextual-mark-writer branch 2 times, most recently from 8b6f580 to ec02004 Compare July 31, 2023 19:15
@khaledhosny khaledhosny requested a review from anthrotype July 31, 2023 19:17
@khaledhosny khaledhosny marked this pull request as ready for review July 31, 2023 19:17
@khaledhosny
Copy link
Collaborator

I added a simple test and I think this is good enough to merge.

@khaledhosny khaledhosny force-pushed the contextual-mark-writer branch 2 times, most recently from 0e8b76f to 7ef50f0 Compare July 31, 2023 22:05
@khaledhosny khaledhosny force-pushed the contextual-mark-writer branch from 7ef50f0 to 0a8daed Compare July 31, 2023 22:43
Also improve how the code is written by skipping lone semicolon when
there is no before part.
@khaledhosny khaledhosny force-pushed the contextual-mark-writer branch from c66175b to 5d3cdd8 Compare July 31, 2023 22:55
@anthrotype
Copy link
Member

I haven't looked a the logic of this proposed ContextualMarkFeatureWriter because I don't know how this contextual anchors are supposed to work, but I am a bit worried that this might make it more complicated to maintain and debug the MarkFeatureWriter, as we would then have code scattered across multiple repositories. I don't mean to block this, I understand there is a need to implement this, but I wonder, would it be possible to activate this special writer only when the .glyphs source actually contains such contextual anchors, and otherwise use the vanilla markFeatureWriter in ufo2ft? I suspect the majority of projects don't make use of this specialized writer and thus do not need to, so to speak, incur this added "risk" of higher debugging/maintenance cost introduced by the default use of this glyhps-specific writer. Does it make sense?

@khaledhosny
Copy link
Collaborator

I haven't looked a the logic of this proposed ContextualMarkFeatureWriter because I don't know how this contextual anchors are supposed to work, but I am a bit worried that this might make it more complicated to maintain and debug the MarkFeatureWriter, as we would then have code scattered across multiple repositories. I don't mean to block this, I understand there is a need to implement this, but I wonder, would it be possible to activate this special writer only when the .glyphs source actually contains such contextual anchors, and otherwise use the vanilla markFeatureWriter in ufo2ft? I suspect the majority of projects don't make use of this specialized writer and thus do not need to, so to speak, incur this added "risk" of higher debugging/maintenance cost introduced by the default use of this glyhps-specific writer. Does it make sense?

I want to extend this mark feature writer to also filter out anchor that start with non-alphanumeric character (googlefonts/ufo2ft#175 and googlefonts/ufo2ft#510), and possibly more in the future since it is not ideal to impose Glyphs-specific anchor naming conventions on UFO files (where anchor naming conventions are lightly specified).

Add missing GdefFeatureWriter, and remove unneeded mode option from
KernFeatureWriter since we now always add insertion marker comment to
always generate kern feature even if a manual feature exists.
@khaledhosny khaledhosny merged commit 9066b26 into main Aug 2, 2023
@khaledhosny khaledhosny deleted the contextual-mark-writer branch August 2, 2023 10:09
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

Successfully merging this pull request may close these issues.

3 participants