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

remove bundled fonts #67

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

exaexa
Copy link
Contributor

@exaexa exaexa commented Jan 14, 2025

This might be a pretty drastic change for some setups but I believe it's generally for the good of it;

in particular:

  • most setups capable of displaying or drawing graphics will have the base fonts available
  • bundling helvetica.ttf from mscorefonts is questionable and I'd say Julia registries might object

Additionally this adds an early assert that catches the NaN values (which may come from defunct fonts as I unfortunately observed in #66 ) early and provides a somewhat useful error message.

If the plain removal of the font bundle is too much, I think we can bundle a different one with clear license terms.

…issing

(most people should be able to load the default font)
@davibarreira
Copy link
Owner

I did not explain in the issue, but the envelope with NaN is not actually an error. If a diagram is empty, its envelope should be NaN. Similarly, if a text is the empty string "". If no font is found, we should try to use the one provided in the assets.

@exaexa
Copy link
Contributor Author

exaexa commented Jan 14, 2025

Ah wait I thought these are handled by Nothing? (As done in envelope(::TextGeom,...) a few lines below the patch.)

(Generally we were taught that NaN should generally not be used to communicate useful values, only errors that require attention.)

@davibarreira
Copy link
Owner

You are correct. Its Nothing not NaN. I was confusing the two. This NaN should not be appearing anywhere.

Copy link
Owner

@davibarreira davibarreira left a comment

Choose a reason for hiding this comment

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

The asserts looks good! But I'd like to keep a bundled font. Probably the one you suggested.
Perhaps join #68 here and do a single PR?

@exaexa
Copy link
Contributor Author

exaexa commented Jan 15, 2025

OK I'll reorganize a little.

@exaexa exaexa marked this pull request as draft January 15, 2025 11:30
@exaexa
Copy link
Contributor Author

exaexa commented Jan 15, 2025

I moved the assert to #68, I guess these are the changes that should be generally "ok to go". That leaves the bundle alone, and should be hopefully mergeable.

I'll read up on the TexGyre licensing and see how to move forward here. For now let's have this as a draft.

@exaexa exaexa force-pushed the mk-rm-bundled-fonts branch from 80bcd4e to 7b330e0 Compare January 15, 2025 13:04
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.

2 participants