Remove import side effect of creating font cache in font_manager by kitchoi · Pull Request #488 · enthought/enable

kitchoi · 2020-12-14T10:42:03Z

Closes #362

This PR removes the import side effect of building font cache in the font_manager module. The action of loading fonts and building the cache is deferred until the global font manager singleton is requested. All code external to font_manager should use the default_font_manager callable to access this font manager singleton.

Benefits of this change:

Make it harder to resurrect the import side-effect in other modules that depend on font_manager (it is too easy to introduce an import and not noticing that brings about additional side effects.)
Make it easier to test font_manager without the import side effect. (The global singleton still makes testing harder.)

It might be easier to review this PR by commits. As an overview:

Add the default_font_manager function and add tests for modules (outside of font_manager that uses the fontManager singleton
Refactor code to make removing the import side effects easier
Remove the import side effect
Rewrite tests after the import side effect is gone: Its absence makes testing easier

Note:
If downstream code has only ever access the FontManager via kiva.font.Font.findfont, then this PR does not change the behaviour perceived externally because #368 already defers the import side effect to where kiva.font.Font.findfont is called.

…obal singleton

They take some time to run

- Extract function - Make load function not to modify the global variable - Evaluate cache path in a function - Remove _fmcache definition - Add a test for findfont before changing it - Remove one global - Use default_font_manager in tests

- Rename rebuild_or_load_from_cache -> load_from_cache_or_rebuild to reflect actual order - Expand documentation and comments - Rename functions to make them more obviously private

kitchoi · 2020-12-14T10:55:12Z

CI is failing while trying to build the EDM environment: it could not access a repository. It is orthogonal but is nonetheless blocking.
For the record, log from appveyor:
log (18).txt

kitchoi · 2020-12-14T14:03:08Z

Rebuilding CI following the fix in #489

kitchoi · 2020-12-14T14:06:59Z

Ah... I need to merge master in because this requires changes to the CI config...

codecov-io · 2020-12-14T14:20:48Z

Codecov Report

Merging #488 (4d8e958) into master (77cb194) will increase coverage by 0.10%.
The diff coverage is 78.57%.

@@            Coverage Diff             @@
##           master     #488      +/-   ##
==========================================
+ Coverage   30.51%   30.61%   +0.10%     
==========================================
  Files         206      206              
  Lines       17853    17865      +12     
  Branches     2461     2462       +1     
==========================================
+ Hits         5447     5469      +22     
+ Misses      12068    12062       -6     
+ Partials      338      334       -4

Impacted Files	Coverage Δ
kiva/trait_defs/ui/wx/kiva_font_editor.py	`37.14% <40.00%> (-0.36%)`	⬇️
kiva/fonttools/font_manager.py	`54.26% <85.71%> (+3.81%)`	⬆️
kiva/fonttools/font.py	`46.05% <100.00%> (-0.71%)`	⬇️
enable/container.py	`55.18% <0.00%> (-1.68%)`	⬇️
enable/abstract_window.py	`53.43% <0.00%> (-0.77%)`	⬇️
enable/qt4/base_window.py	`51.07% <0.00%> (-0.62%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 77cb194...4d8e958. Read the comment docs.

kitchoi · 2020-12-14T14:24:16Z

 import copy
 from kiva.constants import (DEFAULT, DECORATIVE, ROMAN, SCRIPT, SWISS, MODERN,
                            TELETYPE, NORMAL, ITALIC, BOLD, BOLD_ITALIC)
+from .font_manager import default_font_manager, FontProperties


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

jwiggins

Tests look good. I can't see anything that this would break but I'm still a bit wary of Hyrum's Law...

jwiggins · 2020-12-14T20:52:51Z

    def findfont(prop, **kw):
-        global fontManager
-        font = fontManager.findfont(prop, **kw)
+        font = default_font_manager().findfont(prop, **kw)
        return font


This function doesn't appear to be used anywhere outside of the tests. I checked chaco and two proprietary projects...

Opened #497
(Part of me wonders if this needs a deprecation warning or if we are confident no one uses this enough it can just die.)

Thanks. I noticed some confusion between findfont and findFont in that issue, but the matter is settled here.

Well spotted. Will fix the spelling on the issue.

jwiggins · 2020-12-14T20:58:34Z

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


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.

The extraction for testability wasn't necessary. It is fine to instantiate the KivaFontEditor

kitchoi · 2020-12-15T11:59:18Z

Thank you @jwiggins

Done switching to use tempfile.TemporaryDirectory and moving all_facenames back. Merging...

kitchoi added 10 commits December 11, 2020 18:26

Use a callable to get the default font manager, which is still the gl…

a7759e3

…obal singleton

Add tests for the import side effect and cache building

8a14e21

They take some time to run

Remove import side effect

d0e3526

Rewrite test now that import side effect is gone, and other cleanups

ae02e6a

Move imports back up now the side effect is gone

41038b6

Trivial renaming and cleanups

b9b7636

- Rename rebuild_or_load_from_cache -> load_from_cache_or_rebuild to reflect actual order - Expand documentation and comments - Rename functions to make them more obviously private

Improve test readability and patch reusability

3296c83

Local flake8 for the test module

4384ff8

Expand docstring in tests

2ed77e9

Merge branch 'master' into 362-avoid-import-side-effect-squashed-1

4d8e958

kitchoi commented Dec 14, 2020

View reviewed changes

kitchoi requested a review from jwiggins December 14, 2020 14:27

jwiggins approved these changes Dec 14, 2020

View reviewed changes

kitchoi added 2 commits December 15, 2020 11:25

Move all_facenames implementation back into the editor

0cb905d

The extraction for testability wasn't necessary. It is fine to instantiate the KivaFontEditor

Use tempfile.TemporaryDirectory instead

87ec45a

kitchoi mentioned this pull request Dec 15, 2020

Remove findfont module-level function in kiva.fonttools.font_manager #497

Closed

kitchoi merged commit 7b56e52 into master Dec 15, 2020

kitchoi deleted the 362-avoid-import-side-effect-squashed-1 branch December 15, 2020 11:59

jwiggins approved these changes Dec 15, 2020

View reviewed changes

jwiggins mentioned this pull request Dec 24, 2020

Split out the GL backend #392

Merged

2 tasks

kitchoi mentioned this pull request Jan 4, 2021

Fix Font.findfontname AttributeError when the default font manager has not been initialized #531

Merged

Conversation

kitchoi commented Dec 14, 2020

Uh oh!

kitchoi commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kitchoi commented Dec 14, 2020

Uh oh!

kitchoi commented Dec 14, 2020

Uh oh!

codecov-io commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

kitchoi Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jwiggins left a comment

Choose a reason for hiding this comment

Uh oh!

jwiggins Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

kitchoi Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

jwiggins Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

kitchoi Dec 15, 2020

Choose a reason for hiding this comment

Uh oh!

jwiggins Dec 14, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kitchoi commented Dec 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kitchoi commented Dec 14, 2020 •

edited

Loading

codecov-io commented Dec 14, 2020 •

edited

Loading

kitchoi Dec 14, 2020 •

edited

Loading