-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Deprecate the TTFPATH & AFMPATH environment variables. #15943
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
Conversation
Thanks for letting me know -- works for me!
I'll add a patch to proplot.register_fonts that tries to use FontManager.addfont, then populate TTFPATH only if addfont does not exist.
7a885bb to
a5fa73a
Compare
Actually, just realized this might be a significant slowdown since proplot's fonts (and indeed all user-added fonts) would no longer be cached. I see a couple options here:
- Allow
_rebuildto accept positional*args, then pass each of these arguments tofontManager.addfontbefore caching the font manager. - Separate the "rebuilding" and "caching" as follows:
This way proplot can add its fonts then manually update the cache.
class FontManager(...): ... def _cache(self): """Cache the font manager.""" with cbook._lock_path(_fmcache): json_dump(self, _fmcache) def _rebuild(): global fontManager fontManager = FontManager() fontManager._cache() _log.info("generated new fontManager")
Would you consider adding one of these to the branch? I am partial to # 2 :)
a5fa73a to
98685c6
Compare
But dumping is actually already public API (json_dump)? (Well, sure, perhaps we can move _lock_path into json_dump.) So you can just add your fonts and call json_dump?
Ok, makes sense. Yeah my reason for suggesting the _cache function was preferring a one-liner call with one private API reference, rather than two lines with two private API refs (_lock_path and _fmcache). With _lock_path inside json_dump however a _cache function won't add much.
Yup, I wrote #16111 hoping this would help your use case :)
It does help, thanks!
fdc5044 to
52e3125
Compare
Maybe instead store TTFPATH and AFMPATH values in cache and rebuild if they are not the same, like it is done with the cache version?
This doesn't really help -- this would only be useful if the envvars are set before importing mpl anyways, as we can't (and don't want to...) register a listener for changes in the variables (rebuilding is private API), but it's really hard for anyone to be sure matplotlib wasn't imported before them. Also I think addfont() provides more than enough functionality to replace them.
52e3125 to
e8559db
Compare
this would only be useful if the envvars are set before importing mpl anyways
That's the purpose of environment variables, is not it?
Also I think addfont() provides more than enough functionality to replace them.
It does not look like it is, as @lukelbd has to rewrite the cache file manually, what is a hack in my opinion, and #16111 just makes the hack smaller.
Probably we are missing something. Why @lukelbd wants to add fonts at runtime, while at the same time needs them to be in the cache at startup?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As of PR itself, it is indeed better to deprecate those envvars if a cache is not regenerated according to them.
TTFPATH and AFMPATH were not sufficient previously anyways, @lukelbd needed additionally to call the private _rebuild() to make them work (as he will always need when using environment variables), which means that technically we're completely within our rights to break that.
Even if we settle on a public API that allows others to add fonts to the cache (not just to the current fm instance), that's unlikely to be one based on envvars, because they will always suffer from the issue that mpl isn't notified when an envvar is modified, so the API would look like os.environ["..."] = ...; rebuild() in which case we may as well make the API be rebuild(...).
e8559db to
473b92a
Compare
TTFPATH and AFMPATH were not sufficient previously anyways
It depends on a purpose. Like if something Conda-like has it's own font directory and that virtual environment activation changes TTFPATH/AFMPATH and Matplotlib home/cache directory it would be sufficient and understandable usage. And actually, something like this probably already exists.
Fair enough, but that's clearly an terrible and brittle API (needing to set a bunch of env vars for something that should just be a simple API call; moreover this doesn't even pick up new added fonts).
moreover this doesn't even pick up new added fonts).
That's already an issue of the current font manager on OSX/Windows too, is not it?
That's already an issue of the current font manager on OSX/Windows too, is not it?
Yes, and it should be fixed -- but in a sense there's no API for users to add fonts; it seems worse to have an API which is "you can do this to add fonts to the path -- unless someone already did it some time ago in which case it won't work".
473b92a to
b9f8195
Compare
b9f8195 to
c1289b8
Compare
These undocumented environment variables allow one to selectively add paths to the font cache... but only if the font cache doesn't exist yet (they don't force cache regen). The font-cache regen API (_rebuild()) is also private (even though one can trigger it "publically" e.g. by calling `findfont` with a nonexisting font...). Instead, we now expose `addfont()` which is a documented API for registering fonts with the font manager.
c1289b8 to
ec7a731
Compare
These undocumented environment variables allow one to selectively add
paths to the font cache... but only if the font cache doesn't exist yet
(they don't force cache regen). The font-cache regen API (_rebuild())
is also private (even though one can trigger it "publically" e.g. by
calling
findfontwith a nonexisting font...).Instead, we now expose
addfont()which is a documented API forregistering fonts with the font manager.
attn @lukelbd Your proplot package is the only one I am aware of that uses TTFPATH; how does addfont() look to you?
PR Summary
PR Checklist