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

[makeotf] Drop old MM in OpenType support #1144

Merged
merged 2 commits into from
May 29, 2020
Merged

Conversation

khaledhosny
Copy link
Collaborator

Fixes #995

@khaledhosny
Copy link
Collaborator Author

There is also code for reading MM from CFF fonts, but I’m not sure if I should remove it as well.

@khaledhosny
Copy link
Collaborator Author

There are some memory access issues I’m unable to track. I guess I should close the PR then, removing some unused and essentially harmless code is not worth the trouble.

@cjchapman
Copy link
Contributor

Also: OpebType -> OpenType. 😉

@cjchapman cjchapman changed the title [makeotf] Drop old MM in OpebType support [makeotf] Drop old MM in OpenType support May 27, 2020
Copy link
Contributor

@cjchapman cjchapman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@khaledhosny I think this PR is OK as is, so I'm going to go ahead and approve it, but I noticed some multi-master code that still remains, in case you want to clear these out as part of this PR:

  • cffread.h
    • remove mm struct — this will likely uncover more than what I’ve listed below
  • cffread.c
    • remove cff_MultipleMaster case from DICTRead()
  • parse.c
    • remove saveForceBold()
    • remove saveMM()
  • recode.c
    • in recodeAddNewGlyph(), remove these mm-related variables that are assigned values but not used:
      • metrics_gi
      • max_x
      • max_y
    • remove FillinMM_font_clampUDV()

@khaledhosny
Copy link
Collaborator Author

Yes, that is the MM reading code I was referring to in the commit message. I’ll work on it.

@cjchapman
Copy link
Contributor

Sorry, I spaced out on your comment about the MM reading code in my excitement over the memory issues. 😉

Thanks.

@khaledhosny
Copy link
Collaborator Author

It seems that recodeAddNewGlyph() is used for the -adds option which adds glyphs from a MM font, so I guess we will have to keep it as long as this option is supported.

@josh-hadley
Copy link
Collaborator

It seems that recodeAddNewGlyph() is used for the -adds option which adds glyphs from a MM font, so I guess we will have to keep it as long as this option is supported.

I suspect -adds does not get used very much, if at all, these days. But I don't think we should just remove it right now. I think we should discuss a bit, and if removing it is warranted, I propose we deprecate it (with warnings when triggered), and target for removal at some later date. After that we can revisit removal of recodeAddNewGlyph() and any related code.

@khaledhosny does that sound reasonable? Please let us know if you plan to make any more updates on this PR; if not, I think we ought to wrap it up and get it merged soon. I will open up an issue on the removal of -adds to give the community an opportunity to discuss.

@khaledhosny
Copy link
Collaborator Author

It seems that recodeAddNewGlyph() is used for the -adds option which adds glyphs from a MM font, so I guess we will have to keep it as long as this option is supported.

I suspect -adds does not get used very much, if at all, these days. But I don't think we should just remove it right now. I think we should discuss a bit, and if removing it is warranted, I propose we deprecate it (with warnings when triggered), and target for removal at some later date. After that we can revisit removal of recodeAddNewGlyph() and any related code.

Makes sense.

@khaledhosny does that sound reasonable? Please let us know if you plan to make any more updates on this PR; if not, I think we ought to wrap it up and get it merged soon.

No more changes from me.

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.

[makeotfexe] Is multiple master support usable?
3 participants