Fix rebuilding font cache #13852
Fix rebuilding font cache #13852
Conversation
|
looks fine, left some minor comments. for testing (depending on how much effort you want to put into it... not sure it's worth it) you could (ab)use the recent #13766 which adds $XDG_DATA_HOME/fonts to the font search path on linux (run a subprocess with $XDG_DATA_HOME pointing to a tmpdir and drop mpltest.ttf (which we have for testing purposes) there, build a font cache, then delete mpltest.ttf and check that the right thing happens if you then make a call to findfont that should find mpltest.ttf and trigger a rebuild. |
db77e33 to
565915c
Compare
| 'findfont: Found a missing font file. Rebuilding cache.') | ||
| _rebuild() | ||
| return self.findfont(prop, fontext, directory, | ||
| fallback_to_default=True, |
There was a problem hiding this comment.
Shouldn't that be fallback_to_default=fallback_to_default? Otherwise we overwrite the user's choice in the recursive call.
There was a problem hiding this comment.
sure, sounds reasonable... but why was it not failing before?
| prop.get_family(), self.defaultFamily[fontext]) | ||
| default_prop = prop.copy() | ||
| default_prop.set_family(self.defaultFamily[fontext]) | ||
| return self.findfont(default_prop, fontext, directory, |
There was a problem hiding this comment.
Similarly, shouldn't that contain rebuild_if_missing=rebuild_if_missung?
|
Ok, testing is a bit complicated. For now, I wouldn't dive into crafting that unit test unless somebody really wishes that. |
565915c to
4cd263d
Compare
| # No sufficently good font found. | ||
| if rebuild_if_missing: | ||
| _log.info( | ||
| 'findfont: Found a missing font file. Rebuilding cache.') |
There was a problem hiding this comment.
the log message no longer corresponds to the condition (previously this was only triggered if not os.path.isfile(result.fname) -- perhaps(?) the logic should be changed (dunno), but then the log message should be changed too)
|
Given #13810 (comment)
I'm wondering if we need a smarter rebuild method that e.g. does only try to rebuild once per session for a specific font. Otherwise, when doing 20 plots with a non-existing font, I'd rebuild the cache 20 times to just find out that the font is still not available. |
|
rebuild_if_missing should only trigger if we find a font that's listed in the cache but does not actually exist in the filesystem; in that case, after rebuilding, the font should no longer be listed in the cache so it won't trigger further rebuilds. |
|
Since this Pull Request has not been updated in 60 days, it has been marked "inactive." This does not mean that it will be closed, though it may be moved to a "Draft" state. This helps maintainers prioritize their reviewing efforts. You can pick the PR back up anytime - please ping us if you need a review or guidance to move the PR forward! If you do not plan on continuing the work, please let us know so that we can either find someone to take the PR over, or close it. |
|
I'm not going to continue on this. |
PR Summary
Fixes #13810.
The current implementation did
fallback_to_defaultprior torebuild_if_missing. Therefore rebuild was only performed if also the default font was not found, rendering the parameterrebuild_if_missingessentially meaningless.This fix reorders the logic.
Anybody an idea how to unit-test this?