-
Notifications
You must be signed in to change notification settings - Fork 45
Remove import side effect of creating font cache in font_manager #488
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
Changes from all commits
a7759e3
8a14e21
8317205
d0e3526
ae02e6a
41038b6
b9b7636
3296c83
4384ff8
2ed77e9
4d8e958
0cb905d
87ec45a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1390,17 +1390,74 @@ def is_opentype_cff_font(filename): | |
| return result | ||
| return False | ||
|
|
||
|
|
||
| # Global singleton of FontManager, cached at the module level. | ||
| fontManager = None | ||
|
|
||
| _fmcache = os.path.join(get_configdir(), 'fontList.cache') | ||
|
|
||
| def _get_font_cache_path(): | ||
| """ Return the file path for the font cache to be saved / loaded. | ||
|
|
||
| Returns | ||
| ------- | ||
| path : str | ||
| Path to the font cache file. | ||
| """ | ||
| return os.path.join(get_configdir(), 'fontList.cache') | ||
|
|
||
|
|
||
| def _rebuild(): | ||
| """ Rebuild the default font manager and cache its content. | ||
| """ | ||
| global fontManager | ||
| fontManager = _new_font_manager(_get_font_cache_path()) | ||
|
|
||
|
|
||
| def _new_font_manager(cache_file): | ||
| """ Create a new FontManager (which will reload font files) and immediately | ||
| cache its content with the given file path. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| cache_file : str | ||
| Path to the cache to be created. | ||
|
|
||
| Returns | ||
| ------- | ||
| font_manager : FontManager | ||
| """ | ||
| fontManager = FontManager() | ||
| pickle_dump(fontManager, _fmcache) | ||
| pickle_dump(fontManager, cache_file) | ||
| logger.debug("generated new fontManager") | ||
| return fontManager | ||
|
|
||
|
|
||
| def _load_from_cache_or_rebuild(cache_file): | ||
| """ Load the font manager from the cache and verify it is compatible. | ||
| If the cache is not compatible, rebuild the cache and return the new | ||
| font manager. | ||
|
|
||
| Parameters | ||
| ---------- | ||
| cache_file : str | ||
| Path to the cache to be created. | ||
|
|
||
| Returns | ||
| ------- | ||
| font_manager : FontManager | ||
| """ | ||
|
|
||
| try: | ||
| fontManager = pickle_load(cache_file) | ||
| if (not hasattr(fontManager, '_version') or | ||
| fontManager._version != FontManager.__version__): | ||
| fontManager = _new_font_manager(cache_file) | ||
| else: | ||
| fontManager.default_size = None | ||
| logger.debug("Using fontManager instance from %s", cache_file) | ||
| except Exception: | ||
| fontManager = _new_font_manager(cache_file) | ||
|
|
||
| return fontManager | ||
|
|
||
|
|
||
| # The experimental fontconfig-based backend. | ||
|
|
@@ -1440,18 +1497,21 @@ def findfont(prop, fontext='ttf'): | |
| return result | ||
|
|
||
| else: | ||
| try: | ||
| fontManager = pickle_load(_fmcache) | ||
| if (not hasattr(fontManager, '_version') or | ||
| fontManager._version != FontManager.__version__): | ||
| _rebuild() | ||
| else: | ||
| fontManager.default_size = None | ||
| logger.debug("Using fontManager instance from %s", _fmcache) | ||
| except Exception: | ||
| _rebuild() | ||
|
|
||
| def findfont(prop, **kw): | ||
| global fontManager | ||
| font = fontManager.findfont(prop, **kw) | ||
| font = default_font_manager().findfont(prop, **kw) | ||
| return font | ||
|
Comment on lines
1501
to
1503
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This function doesn't appear to be used anywhere outside of the tests. I checked
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Opened #497
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks. I noticed some confusion between
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well spotted. Will fix the spelling on the issue. |
||
|
|
||
|
|
||
| def default_font_manager(): | ||
| """ Return the default font manager, which is a singleton FontManager | ||
| cached in the module. | ||
|
|
||
| Returns | ||
| ------- | ||
| font_manager : FontManager | ||
| """ | ||
| global fontManager | ||
| if fontManager is None: | ||
| fontManager = _load_from_cache_or_rebuild(_get_font_cache_path()) | ||
| return fontManager | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| """ Tests for kiva.fonttools.font | ||
| """ | ||
| import os | ||
| import unittest | ||
|
|
||
| from kiva.fonttools import Font | ||
|
|
||
|
|
||
| class TestFont(unittest.TestCase): | ||
|
|
||
| def test_find_font_empty_name(self): | ||
| # This test relies on the fact there exists some fonts on the system | ||
| # that the font manager can load. Ideally we should be able to redirect | ||
| # the path from which the font manager loads font files, then this test | ||
| # can be less fragile. | ||
| font = Font(face_name="") | ||
| font_file_path = font.findfont() | ||
| self.assertTrue(os.path.exists(font_file_path)) | ||
|
|
||
| def test_find_font_some_face_name(self): | ||
| font = Font(face_name="ProbablyNotFound") | ||
|
|
||
| # There will be warnings as there will be no match for the requested | ||
| # face name. | ||
| with self.assertWarns(UserWarning): | ||
| font_file_path = font.findfont() | ||
| self.assertTrue(os.path.exists(font_file_path)) | ||
|
Comment on lines
+20
to
+27
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not excited that this is the established behavior but it's good to have it covered with a test before our changes get any more invasive. |
||
Uh oh!
There was an error while loading. Please reload this page.
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.
This undoes #368 because the import side effect being avoided is now removed. While this move is strictly speaking unnecessary, it reduces cognitive load for developers: Any imports that are not at the top always beg the question "why are they not at the top".