Skip to content

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

Merged
merged 1 commit into from
Jan 14, 2020

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented Dec 15, 2019

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.

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

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@lukelbd
Copy link
Contributor

lukelbd commented Dec 15, 2019

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.

@lukelbd
Copy link
Contributor

lukelbd commented Jan 5, 2020

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:

  1. Allow _rebuild to accept positional *args, then pass each of these arguments to fontManager.addfont before caching the font manager.
  2. Separate the "rebuilding" and "caching" as follows:
    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")
    This way proplot can add its fonts then manually update the cache.

Would you consider adding one of these to the branch? I am partial to # 2 :)

@anntzer
Copy link
Contributor Author

anntzer commented Jan 5, 2020

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?

@lukelbd
Copy link
Contributor

lukelbd commented Jan 5, 2020

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.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 5, 2020

Yup, I wrote #16111 hoping this would help your use case :)

@lukelbd
Copy link
Contributor

lukelbd commented Jan 5, 2020

It does help, thanks!

@anntzer anntzer force-pushed the ttfafmpath branch 2 times, most recently from fdc5044 to 52e3125 Compare January 7, 2020 09:40
@Kojoley
Copy link
Member

Kojoley commented Jan 9, 2020

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?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 9, 2020

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.

@Kojoley
Copy link
Member

Kojoley commented Jan 9, 2020

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?

Copy link
Member

@Kojoley Kojoley left a 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.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 9, 2020

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(...).

@Kojoley
Copy link
Member

Kojoley commented Jan 9, 2020

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.

@anntzer
Copy link
Contributor Author

anntzer commented Jan 9, 2020

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).

@Kojoley
Copy link
Member

Kojoley commented Jan 9, 2020

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?

@anntzer
Copy link
Contributor Author

anntzer commented Jan 9, 2020

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".

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.
@Kojoley Kojoley merged commit 5914990 into matplotlib:master Jan 14, 2020
@anntzer anntzer deleted the ttfafmpath branch January 14, 2020 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants