-
-
Notifications
You must be signed in to change notification settings - Fork 8k
fix(font manager): Allow Rebuilding Cache #30406
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
base: main
Are you sure you want to change the base?
Conversation
One could never rebuild the cache given the existing conditional logic (premature return to exception based solely on fallback_to_default). Now, if a user disables fallback and enables rebuild, the rebuild logic will execute.
Failed tests include the garbage collector holding on to locals following production of a graphic involving text with a non-existent font family, and warnings in usetex regarding unavailable serif. The new logic pathway opened up by this change could be old. It includes a self-reference which may be when failure 2 is produced. Not sure how locals could be held onto by way of this code, however (failure 1) |
@@ -1504,6 +1504,9 @@ def _findfont_cached(self, prop, fontext, directory, fallback_to_default, | |||
default_prop.set_family(self.defaultFamily[fontext]) | |||
return self.findfont(default_prop, fontext, directory, | |||
fallback_to_default=False) | |||
elif rebuild_if_missing: | |||
# Continue to rebuild condition | |||
result = '' |
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.
Can we use None
hear instead (which might require adjusting the check below) setting the result to empty string is a bit odd.
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.
I chose a value that is "not-filey" because I don't know what isfile does with a None, and modifying the check below has the potential to fundamentally change the logic pathway, so I didn't feel comfortable changing that. Open to alternatives, though.
I don't quite understand what you mean by that. Are you saying the the text related tests that are failing should not have been failing (as a rebuild of the fontcache discovers a font that we should select)? |
@tacaswell The tests that are failing are:
In short, I don't see how this change is a direct cause of these tests, though I recognize my understanding is limited. I made this pull request to solve a very simple issue: in no way can a user - using public methods - tell matplotlib to rebuild its font cache, and instead the default MO has been to tell users to manually delete the cache (c.f., I don't consider adding a font to be a viable workaround for rebuilding the cache). |
One could never rebuild the cache given the existing conditional logic (premature return to exception based solely on fallback_to_default). Now, if a user disables fallback and enables rebuild, the rebuild logic will execute.
This allows a user to install a font and run:
to force a rebuild of the font cache.
It was a simple fix to the conditional logic: if fallback is False, don't return with error if rebuild is True, but instead set result to empty string (used later in the rebuild code) so that the rebuild code can actually be proceeded to.
PR summary
PR checklist
Tested on my font cache locally in my normal non-dev env.
Unless other ideas exist, a unit test would require: