From a7759e30b89c4c4b85583e208b4d817eadeaebec Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Fri, 11 Dec 2020 18:26:10 +0000
Subject: [PATCH 01/12] Use a callable to get the default font manager, which
 is still the global singleton

---
 kiva/fonttools/font.py                        |  4 +--
 kiva/fonttools/font_manager.py                |  7 +++++
 kiva/fonttools/tests/test_font.py             | 27 +++++++++++++++++++
 kiva/fonttools/tests/test_font_manager.py     | 16 ++++++++++-
 kiva/trait_defs/ui/wx/kiva_font_editor.py     | 10 +++++--
 .../ui/wx/tests/test_kiva_font_editor.py      | 18 +++++++++++++
 6 files changed, 77 insertions(+), 5 deletions(-)
 create mode 100644 kiva/fonttools/tests/test_font.py
 create mode 100644 kiva/trait_defs/ui/wx/tests/test_kiva_font_editor.py

diff --git a/kiva/fonttools/font.py b/kiva/fonttools/font.py
index f47de81e5..043bb61f4 100644
--- a/kiva/fonttools/font.py
+++ b/kiva/fonttools/font.py
@@ -93,10 +93,10 @@ def findfont(self):
         """ Returns the file name containing the font that most closely matches
         our font properties.
         """
-        from .font_manager import fontManager
+        from .font_manager import default_font_manager
 
         fp = self._make_font_props()
-        return str(fontManager.findfont(fp))
+        return str(default_font_manager().findfont(fp))
 
     def findfontname(self):
         """ Returns the name of the font that most closely matches our font
diff --git a/kiva/fonttools/font_manager.py b/kiva/fonttools/font_manager.py
index c6c916a27..e3523c70e 100644
--- a/kiva/fonttools/font_manager.py
+++ b/kiva/fonttools/font_manager.py
@@ -1455,3 +1455,10 @@ def findfont(prop, **kw):
         global fontManager
         font = fontManager.findfont(prop, **kw)
         return font
+
+
+def default_font_manager():
+    """ Return the default font manager.
+    """
+    global fontManager
+    return fontManager
diff --git a/kiva/fonttools/tests/test_font.py b/kiva/fonttools/tests/test_font.py
new file mode 100644
index 000000000..234f5cd52
--- /dev/null
+++ b/kiva/fonttools/tests/test_font.py
@@ -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))
diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index 1c370bc6c..aef166820 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -5,7 +5,13 @@
 from pkg_resources import resource_filename
 from fontTools.ttLib import TTFont
 
-from ..font_manager import FontEntry, createFontList, ttfFontProperty, getPropDict
+from ..font_manager import (
+    createFontList,
+    default_font_manager,
+    FontEntry,
+    FontManager,
+    ttfFontProperty,
+)
 
 data_dir = resource_filename('kiva.fonttools.tests', 'data')
 
@@ -91,3 +97,11 @@ def test_font_with_italic_style(self):
         self.assertEqual(entry.weight, exp_weight)
         self.assertEqual(entry.stretch, exp_stretch)
         self.assertEqual(entry.size, exp_size)
+
+
+class TestFontManager(unittest.TestCase):
+    """ Test API of the font manager module."""
+
+    def test_default_font_manager(self):
+        font_manager = default_font_manager()
+        self.assertIsInstance(font_manager, FontManager)
diff --git a/kiva/trait_defs/ui/wx/kiva_font_editor.py b/kiva/trait_defs/ui/wx/kiva_font_editor.py
index 33ec416be..9428787eb 100644
--- a/kiva/trait_defs/ui/wx/kiva_font_editor.py
+++ b/kiva/trait_defs/ui/wx/kiva_font_editor.py
@@ -27,7 +27,7 @@
 from traitsui.wx.font_editor \
     import ToolkitEditorFactory as EditorFactory
 
-from kiva.fonttools.font_manager import fontManager
+from kiva.fonttools.font_manager import default_font_manager
 
 
 #-------------------------------------------------------------------------------
@@ -119,7 +119,13 @@ def str_font ( self, font ):
     def all_facenames(self):
         """ Returns a list of all available font typeface names.
         """
-        return sorted({f.name for f in fontManager.ttflist})
+        return _all_facenames()
+
+
+def _all_facenames():
+    """ Return a list of all available (TTF) font typeface names."""
+    font_manager = default_font_manager()
+    return sorted({f.name for f in font_manager.ttflist})
 
 
 def KivaFontEditor(*args, **traits):
diff --git a/kiva/trait_defs/ui/wx/tests/test_kiva_font_editor.py b/kiva/trait_defs/ui/wx/tests/test_kiva_font_editor.py
new file mode 100644
index 000000000..c885c629d
--- /dev/null
+++ b/kiva/trait_defs/ui/wx/tests/test_kiva_font_editor.py
@@ -0,0 +1,18 @@
+""" Tests for ui.wx.kiva_font_editor
+"""
+
+import unittest
+
+from kiva.tests._testing import is_wx, skip_if_not_wx
+
+if is_wx():
+    from kiva.trait_defs.ui.wx import kiva_font_editor
+
+
+@skip_if_not_wx
+class TestFacename(unittest.TestCase):
+
+    def test_all_facenames(self):
+        # Test loading of available face names does not fail.
+        # The available face names depend on the system
+        self.assertGreaterEqual(len(kiva_font_editor._all_facenames()), 0)

From 8a14e21915b794043aa1bd726b32f82144657af6 Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Fri, 11 Dec 2020 19:08:49 +0000
Subject: [PATCH 02/12] Add tests for the import side effect and cache building

They take some time to run
---
 kiva/fonttools/tests/test_font_manager.py | 99 +++++++++++++++++++++++
 1 file changed, 99 insertions(+)

diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index aef166820..6020ac914 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -1,15 +1,24 @@
+import glob
+import importlib
 import os
+import shutil
+import sys
+import tempfile
 import unittest
 from unittest import mock
 
 from pkg_resources import resource_filename
 from fontTools.ttLib import TTFont
 
+from traits.etsconfig.api import ETSConfig
+
 from ..font_manager import (
     createFontList,
     default_font_manager,
     FontEntry,
     FontManager,
+    pickle_dump,
+    pickle_load,
     ttfFontProperty,
 )
 
@@ -99,9 +108,99 @@ def test_font_with_italic_style(self):
         self.assertEqual(entry.size, exp_size)
 
 
+class TestFontCache(unittest.TestCase):
+    """ Test internal font cache building mechanism."""
+
+    def setUp(self):
+        self.ttf_files = [
+            os.path.abspath(os.path.join(data_dir, "TestTTF.ttf"))
+        ]
+
+        self.temp_dir = tempfile.mkdtemp()
+        self.addCleanup(shutil.rmtree, self.temp_dir)
+
+        self.cache_file = os.path.join(self.temp_dir, "font.cache")
+
+        with patch_system_fonts(self.ttf_files):
+            pickle_dump(FontManager(), self.cache_file)
+
+    def test_load_font_cached_to_file(self):
+        # patch import... fight import side-effect
+        module_name = "kiva.fonttools.font_manager"
+        modules = sys.modules
+        original_data_dir = ETSConfig.application_data
+
+        ETSConfig.application_data = self.temp_dir
+        original_module = modules.pop(module_name)
+        try:
+            cache_dir = os.path.join(self.temp_dir, "kiva")
+            os.makedirs(cache_dir)
+            shutil.copyfile(
+                self.cache_file,
+                os.path.join(cache_dir, "fontList.cache")
+            )
+            new_module = importlib.import_module(module_name)
+        except Exception:
+            raise
+        else:
+            expected_manager = pickle_load(
+                os.path.join(cache_dir, "fontList.cache")
+            )
+            self.assertEqual(
+                new_module.fontManager.ttffiles,
+                expected_manager.ttffiles,
+            )
+        finally:
+            ETSConfig.application_data = original_data_dir
+            modules[module_name] = original_module
+
+    def test_rebuild_if_cache_not_found(self):
+        module_name = "kiva.fonttools.font_manager"
+        modules = sys.modules
+        original_data_dir = ETSConfig.application_data
+
+        ETSConfig.application_data = self.temp_dir
+        original_module = modules.pop(module_name)
+        try:
+            new_module = importlib.import_module(module_name)
+        except Exception:
+            raise
+        else:
+            # A cache is created
+            self.assertTrue(
+                os.path.join(self.temp_dir, "kiva", "fontList.cache")
+            )
+        finally:
+            ETSConfig.application_data = original_data_dir
+            modules[module_name] = original_module
+
+
 class TestFontManager(unittest.TestCase):
     """ Test API of the font manager module."""
 
     def test_default_font_manager(self):
         font_manager = default_font_manager()
         self.assertIsInstance(font_manager, FontManager)
+
+
+def patch_system_fonts(ttf_files):
+    """ Patch findSystemFonts with the given list of font file paths.
+
+    This speeds up tests by avoiding having to parse a lot of font files
+    on a system.
+
+    Parameters
+    ----------
+    ttf_files : list of str
+        List of file paths for TTF fonts
+    """
+
+    def fake_find_system_fonts(fontpaths=None, fontext='ttf'):
+        if fontext == "ttf":
+            return ttf_files
+        return []
+
+    return mock.patch(
+        "kiva.fonttools.font_manager.findSystemFonts",
+        fake_find_system_fonts,
+    )

From 8317205e547cfffe2aff24e0cf592d60a87f057c Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Fri, 11 Dec 2020 19:13:22 +0000
Subject: [PATCH 03/12] Refactor to make removing import side effect easier

- 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
---
 kiva/fonttools/font_manager.py            | 57 +++++++++++++++++------
 kiva/fonttools/tests/test_font_manager.py | 18 ++++++-
 2 files changed, 60 insertions(+), 15 deletions(-)

diff --git a/kiva/fonttools/font_manager.py b/kiva/fonttools/font_manager.py
index e3523c70e..2200d2207 100644
--- a/kiva/fonttools/font_manager.py
+++ b/kiva/fonttools/font_manager.py
@@ -1393,14 +1393,53 @@ def is_opentype_cff_font(filename):
 
 fontManager = None
 
-_fmcache = os.path.join(get_configdir(), 'fontList.cache')
+def get_font_cache_path():
+    return os.path.join(get_configdir(), 'fontList.cache')
 
 
 def _rebuild():
     global fontManager
+    fontManager = new_with_cache(get_font_cache_path())
+
+
+def new_with_cache(cache_file):
+    """ Create a new FontManager and immediately cache its content with the
+    given file path.
+
+    Parameters
+    ----------
+    cache_file : str
+        Path to the cache to be created.
+    """
     fontManager = FontManager()
-    pickle_dump(fontManager, _fmcache)
+    pickle_dump(fontManager, cache_file)
     logger.debug("generated new fontManager")
+    return fontManager
+
+
+def rebuild_or_load_from_cache(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.
+    """
+
+    try:
+        fontManager = pickle_load(cache_file)
+        if (not hasattr(fontManager, '_version') or
+                fontManager._version != FontManager.__version__):
+            fontManager = new_with_cache(cache_file)
+        else:
+            fontManager.default_size = None
+            logger.debug("Using fontManager instance from %s", cache_file)
+    except Exception:
+        fontManager = new_with_cache(cache_file)
+
+    return fontManager
 
 
 # The experimental fontconfig-based backend.
@@ -1440,20 +1479,10 @@ 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()
+    fontManager = rebuild_or_load_from_cache(get_font_cache_path())
 
     def findfont(prop, **kw):
-        global fontManager
-        font = fontManager.findfont(prop, **kw)
+        font = default_font_manager().findfont(prop, **kw)
         return font
 
 
diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index 6020ac914..50464dfd4 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -15,7 +15,9 @@
 from ..font_manager import (
     createFontList,
     default_font_manager,
+    findfont,
     FontEntry,
+    FontProperties,
     FontManager,
     pickle_dump,
     pickle_load,
@@ -147,7 +149,7 @@ def test_load_font_cached_to_file(self):
                 os.path.join(cache_dir, "fontList.cache")
             )
             self.assertEqual(
-                new_module.fontManager.ttffiles,
+                new_module.default_font_manager().ttffiles,
                 expected_manager.ttffiles,
             )
         finally:
@@ -182,6 +184,20 @@ def test_default_font_manager(self):
         font_manager = default_font_manager()
         self.assertIsInstance(font_manager, FontManager)
 
+    def test_findFont(self):
+        # Warning because there are no families defined.
+        with self.assertWarns(UserWarning):
+            font = findfont(
+                FontProperties(
+                    family=[],
+                    weight=500,
+                )
+            )
+        # The returned value is a file path
+        # This assumes there exists fonts on the system that can be loaded
+        # by the font manager while the test is run.
+        self.assertTrue(os.path.exists(font))
+
 
 def patch_system_fonts(ttf_files):
     """ Patch findSystemFonts with the given list of font file paths.

From d0e3526f7f7a58a4c6087780ea336cbc81aa6a5b Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Fri, 11 Dec 2020 19:56:20 +0000
Subject: [PATCH 04/12] Remove import side effect

---
 kiva/fonttools/font_manager.py            | 3 ++-
 kiva/fonttools/tests/test_font_manager.py | 9 ++++-----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/kiva/fonttools/font_manager.py b/kiva/fonttools/font_manager.py
index 2200d2207..41f222f61 100644
--- a/kiva/fonttools/font_manager.py
+++ b/kiva/fonttools/font_manager.py
@@ -1479,7 +1479,6 @@ def findfont(prop, fontext='ttf'):
         return result
 
 else:
-    fontManager = rebuild_or_load_from_cache(get_font_cache_path())
 
     def findfont(prop, **kw):
         font = default_font_manager().findfont(prop, **kw)
@@ -1490,4 +1489,6 @@ def default_font_manager():
     """ Return the default font manager.
     """
     global fontManager
+    if fontManager is None:
+        fontManager = rebuild_or_load_from_cache(get_font_cache_path())
     return fontManager
diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index 50464dfd4..5c474e766 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -156,7 +156,7 @@ def test_load_font_cached_to_file(self):
             ETSConfig.application_data = original_data_dir
             modules[module_name] = original_module
 
-    def test_rebuild_if_cache_not_found(self):
+    def test_no_import_side_effect(self):
         module_name = "kiva.fonttools.font_manager"
         modules = sys.modules
         original_data_dir = ETSConfig.application_data
@@ -168,10 +168,9 @@ def test_rebuild_if_cache_not_found(self):
         except Exception:
             raise
         else:
-            # A cache is created
-            self.assertTrue(
-                os.path.join(self.temp_dir, "kiva", "fontList.cache")
-            )
+            # A cache is not created
+            cache_path = os.path.join(self.temp_dir, "kiva", "fontList.cache")
+            self.assertFalse(os.path.exists(cache_path))
         finally:
             ETSConfig.application_data = original_data_dir
             modules[module_name] = original_module

From ae02e6af77539daa3d4c82561f3877cc091cbbde Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Fri, 11 Dec 2020 20:05:34 +0000
Subject: [PATCH 05/12] Rewrite test now that import side effect is gone, and
 other cleanups

---
 kiva/fonttools/font_manager.py            | 27 +++++++++----
 kiva/fonttools/tests/test_font_manager.py | 46 +++++++++--------------
 2 files changed, 38 insertions(+), 35 deletions(-)

diff --git a/kiva/fonttools/font_manager.py b/kiva/fonttools/font_manager.py
index 41f222f61..09323db75 100644
--- a/kiva/fonttools/font_manager.py
+++ b/kiva/fonttools/font_manager.py
@@ -1399,17 +1399,21 @@ def get_font_cache_path():
 
 def _rebuild():
     global fontManager
-    fontManager = new_with_cache(get_font_cache_path())
+    fontManager = new_font_manager(get_font_cache_path())
 
 
-def new_with_cache(cache_file):
-    """ Create a new FontManager and immediately cache its content with the
-    given file 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, cache_file)
@@ -1426,18 +1430,22 @@ def rebuild_or_load_from_cache(cache_file):
     ----------
     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_with_cache(cache_file)
+            fontManager = new_font_manager(cache_file)
         else:
             fontManager.default_size = None
             logger.debug("Using fontManager instance from %s", cache_file)
     except Exception:
-        fontManager = new_with_cache(cache_file)
+        fontManager = new_font_manager(cache_file)
 
     return fontManager
 
@@ -1486,7 +1494,12 @@ def findfont(prop, **kw):
 
 
 def default_font_manager():
-    """ Return the 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:
diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index 5c474e766..5f53e3345 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -12,6 +12,7 @@
 
 from traits.etsconfig.api import ETSConfig
 
+from .. import font_manager as font_manager_module
 from ..font_manager import (
     createFontList,
     default_font_manager,
@@ -127,34 +128,23 @@ def setUp(self):
             pickle_dump(FontManager(), self.cache_file)
 
     def test_load_font_cached_to_file(self):
-        # patch import... fight import side-effect
-        module_name = "kiva.fonttools.font_manager"
-        modules = sys.modules
-        original_data_dir = ETSConfig.application_data
-
-        ETSConfig.application_data = self.temp_dir
-        original_module = modules.pop(module_name)
-        try:
-            cache_dir = os.path.join(self.temp_dir, "kiva")
-            os.makedirs(cache_dir)
-            shutil.copyfile(
-                self.cache_file,
-                os.path.join(cache_dir, "fontList.cache")
-            )
-            new_module = importlib.import_module(module_name)
-        except Exception:
-            raise
-        else:
-            expected_manager = pickle_load(
-                os.path.join(cache_dir, "fontList.cache")
-            )
-            self.assertEqual(
-                new_module.default_font_manager().ttffiles,
-                expected_manager.ttffiles,
-            )
-        finally:
-            ETSConfig.application_data = original_data_dir
-            modules[module_name] = original_module
+        original_singleton = font_manager_module.fontManager
+        self.addCleanup(
+            setattr,
+            font_manager_module, "fontManager", original_singleton
+        )
+
+        font_manager_module.fontManager = None
+        with mock.patch.object(
+                font_manager_module, "get_font_cache_path",
+                return_value=self.cache_file
+        ):
+            default_manager = font_manager_module.default_font_manager()
+
+        expected_manager = pickle_load(self.cache_file)
+        self.assertEqual(default_manager.ttffiles, expected_manager.ttffiles)
+        # The global singleton is now set.
+        self.assertIsInstance(font_manager_module.fontManager, FontManager)
 
     def test_no_import_side_effect(self):
         module_name = "kiva.fonttools.font_manager"

From 41038b6127b3f81c42bef1690957fe732c997f3d Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Fri, 11 Dec 2020 20:20:25 +0000
Subject: [PATCH 06/12] Move imports back up now the side effect is gone

---
 kiva/fonttools/font.py | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/kiva/fonttools/font.py b/kiva/fonttools/font.py
index 043bb61f4..933865424 100644
--- a/kiva/fonttools/font.py
+++ b/kiva/fonttools/font.py
@@ -5,6 +5,7 @@
 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
 
 # Various maps used by str_to_font
 font_families = {
@@ -93,8 +94,6 @@ def findfont(self):
         """ Returns the file name containing the font that most closely matches
         our font properties.
         """
-        from .font_manager import default_font_manager
-
         fp = self._make_font_props()
         return str(default_font_manager().findfont(fp))
 
@@ -109,8 +108,6 @@ def _make_font_props(self):
         """ Returns a font_manager.FontProperties object that encapsulates our
         font properties
         """
-        from .font_manager import FontProperties
-
         # XXX: change the weight to a numerical value
         if self.style == BOLD or self.style == BOLD_ITALIC:
             weight = "bold"

From b9b763643b76846ceee01259c2580e5613ed2148 Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Mon, 14 Dec 2020 09:36:28 +0000
Subject: [PATCH 07/12] Trivial renaming and cleanups

- 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
---
 kiva/fonttools/font_manager.py            | 26 ++++++++++++++++-------
 kiva/fonttools/tests/test_font_manager.py |  2 +-
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/kiva/fonttools/font_manager.py b/kiva/fonttools/font_manager.py
index 09323db75..a54a7449a 100644
--- a/kiva/fonttools/font_manager.py
+++ b/kiva/fonttools/font_manager.py
@@ -1390,19 +1390,29 @@ def is_opentype_cff_font(filename):
         return result
     return False
 
-
+# Global singleton of FontManager, cached at the module level.
 fontManager = None
 
-def get_font_cache_path():
+
+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())
+    fontManager = _new_font_manager(_get_font_cache_path())
 
 
-def new_font_manager(cache_file):
+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.
 
@@ -1421,7 +1431,7 @@ def new_font_manager(cache_file):
     return fontManager
 
 
-def rebuild_or_load_from_cache(cache_file):
+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.
@@ -1440,12 +1450,12 @@ def rebuild_or_load_from_cache(cache_file):
         fontManager = pickle_load(cache_file)
         if (not hasattr(fontManager, '_version') or
                 fontManager._version != FontManager.__version__):
-            fontManager = new_font_manager(cache_file)
+            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)
+        fontManager = _new_font_manager(cache_file)
 
     return fontManager
 
@@ -1503,5 +1513,5 @@ def default_font_manager():
     """
     global fontManager
     if fontManager is None:
-        fontManager = rebuild_or_load_from_cache(get_font_cache_path())
+        fontManager = _load_from_cache_or_rebuild(_get_font_cache_path())
     return fontManager
diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index 5f53e3345..f0c19270b 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -136,7 +136,7 @@ def test_load_font_cached_to_file(self):
 
         font_manager_module.fontManager = None
         with mock.patch.object(
-                font_manager_module, "get_font_cache_path",
+                font_manager_module, "_get_font_cache_path",
                 return_value=self.cache_file
         ):
             default_manager = font_manager_module.default_font_manager()

From 3296c83a3547264f52bb1e352c10201287dadd08 Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Mon, 14 Dec 2020 09:52:44 +0000
Subject: [PATCH 08/12] Improve test readability and patch reusability

---
 kiva/fonttools/tests/test_font_manager.py | 117 ++++++++++++++++------
 1 file changed, 84 insertions(+), 33 deletions(-)

diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index f0c19270b..bb972693b 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -1,3 +1,4 @@
+import contextlib
 import glob
 import importlib
 import os
@@ -122,48 +123,47 @@ def setUp(self):
         self.temp_dir = tempfile.mkdtemp()
         self.addCleanup(shutil.rmtree, self.temp_dir)
 
-        self.cache_file = os.path.join(self.temp_dir, "font.cache")
+    def test_load_font_from_cache(self):
+        # Test loading fonts from cache file.
+        with patch_global_font_manager(None):
+            with patch_font_cache(self.temp_dir, self.ttf_files):
+                default_manager = font_manager_module.default_font_manager()
 
-        with patch_system_fonts(self.ttf_files):
-            pickle_dump(FontManager(), self.cache_file)
+            # For some reasons, there are duplications in the list of files
+            self.assertEqual(
+                set(default_manager.ttffiles), set(self.ttf_files)
+            )
+            # The global singleton is now set.
+            self.assertIsInstance(font_manager_module.fontManager, FontManager)
 
-    def test_load_font_cached_to_file(self):
-        original_singleton = font_manager_module.fontManager
-        self.addCleanup(
-            setattr,
-            font_manager_module, "fontManager", original_singleton
-        )
+    def test_build_font_if_no_cache(self):
+        # Calling default_font_manager will build the font cache
+        # The temporary directory does not have a font cache file.
+        with change_ets_app_dir(self.temp_dir) as cache_file:
 
-        font_manager_module.fontManager = None
-        with mock.patch.object(
-                font_manager_module, "_get_font_cache_path",
-                return_value=self.cache_file
-        ):
-            default_manager = font_manager_module.default_font_manager()
+            with patch_global_font_manager(None), \
+                    patch_system_fonts(self.ttf_files):  # patch for speed
+                default_manager = font_manager_module.default_font_manager()
 
-        expected_manager = pickle_load(self.cache_file)
-        self.assertEqual(default_manager.ttffiles, expected_manager.ttffiles)
-        # The global singleton is now set.
-        self.assertIsInstance(font_manager_module.fontManager, FontManager)
+            # The cache file is created
+            self.assertTrue(os.path.exists(cache_file))
 
     def test_no_import_side_effect(self):
+        # Importing font_manager should have no side effect of creating
+        # the font cache. Regression test for enthought/enable#362
         module_name = "kiva.fonttools.font_manager"
         modules = sys.modules
-        original_data_dir = ETSConfig.application_data
-
-        ETSConfig.application_data = self.temp_dir
         original_module = modules.pop(module_name)
-        try:
-            new_module = importlib.import_module(module_name)
-        except Exception:
-            raise
-        else:
-            # A cache is not created
-            cache_path = os.path.join(self.temp_dir, "kiva", "fontList.cache")
-            self.assertFalse(os.path.exists(cache_path))
-        finally:
-            ETSConfig.application_data = original_data_dir
-            modules[module_name] = original_module
+        with change_ets_app_dir(self.temp_dir) as cache_path:
+            try:
+                new_module = importlib.import_module(module_name)
+            except Exception:
+                raise
+            else:
+                # A cache is not created
+                self.assertFalse(os.path.exists(cache_path))
+            finally:
+                modules[module_name] = original_module
 
 
 class TestFontManager(unittest.TestCase):
@@ -188,6 +188,57 @@ def test_findFont(self):
         self.assertTrue(os.path.exists(font))
 
 
+@contextlib.contextmanager
+def change_ets_app_dir(dirpath):
+    """ Temporarily change the application data directory in ETSConfig.
+
+    Returns
+    -------
+    font_cache_file_path : str
+        Path to the font cache in the given data directory.
+        Returned for convenience.
+    """
+    original_data_dir = ETSConfig.application_data
+    ETSConfig.application_data = dirpath
+    try:
+        yield font_manager_module._get_font_cache_path()
+    finally:
+        ETSConfig.application_data = original_data_dir
+
+
+def patch_global_font_manager(new_value):
+    """ Patch the global FontManager instance at the module level.
+
+    Parameters
+    ----------
+    new_value : FontManager or None
+        Temporary value to be used as the global font manager.
+    """
+    return mock.patch.object(font_manager_module, "fontManager", new_value)
+
+
+@contextlib.contextmanager
+def patch_font_cache(dirpath, ttf_files):
+    """ Patch the font cache content with the given list of FFT fonts
+    and application data directory.
+
+    Parameters
+    ----------
+    ttf_files : list of str
+        List of file paths to TTF files.
+
+    Returns
+    -------
+    font_cache_file_path : str
+        Path to the font cache in the given data directory.
+        Returned for convenience.
+    """
+    with change_ets_app_dir(dirpath) as cache_file:
+        with patch_system_fonts(ttf_files):
+            font_manager_module._new_font_manager(cache_file)
+        yield cache_file
+
+
 def patch_system_fonts(ttf_files):
     """ Patch findSystemFonts with the given list of font file paths.
 

From 4384ff8c8d0592c2276da8ac50d5c8f0b832456b Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Mon, 14 Dec 2020 10:29:51 +0000
Subject: [PATCH 09/12] Local flake8 for the test module

---
 kiva/fonttools/tests/test_font_manager.py | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index bb972693b..b889d8d6a 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -1,5 +1,4 @@
 import contextlib
-import glob
 import importlib
 import os
 import shutil
@@ -21,8 +20,6 @@
     FontEntry,
     FontProperties,
     FontManager,
-    pickle_dump,
-    pickle_load,
     ttfFontProperty,
 )
 
@@ -143,7 +140,7 @@ def test_build_font_if_no_cache(self):
 
             with patch_global_font_manager(None), \
                     patch_system_fonts(self.ttf_files):  # patch for speed
-                default_manager = font_manager_module.default_font_manager()
+                font_manager_module.default_font_manager()
 
             # The cache file is created
             self.assertTrue(os.path.exists(cache_file))
@@ -156,7 +153,7 @@ def test_no_import_side_effect(self):
         original_module = modules.pop(module_name)
         with change_ets_app_dir(self.temp_dir) as cache_path:
             try:
-                new_module = importlib.import_module(module_name)
+                importlib.import_module(module_name)
             except Exception:
                 raise
             else:

From 2ed77e9f5a1e4a6741f911acb8e084ff096e7ade Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Mon, 14 Dec 2020 10:30:59 +0000
Subject: [PATCH 10/12] Expand docstring in tests

---
 kiva/fonttools/tests/test_font_manager.py | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index b889d8d6a..fd2a5e469 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -189,6 +189,12 @@ def test_findFont(self):
 def change_ets_app_dir(dirpath):
     """ Temporarily change the application data directory in ETSConfig.
 
+    Parameters
+    ----------
+    dirpath : str
+        Path to be temporarily set to the ETSConfig.application_data
+        so that it gets used for computing the font cache file path.
+
     Returns
     -------
     font_cache_file_path : str
@@ -221,6 +227,9 @@ def patch_font_cache(dirpath, ttf_files):
 
     Parameters
     ----------
+    dirpath : str
+        Path to be temporarily set to the ETSConfig.application_data
+        so that it gets used for computing the font cache file path.
     ttf_files : list of str
         List of file paths to TTF files.
 

From 0cb905dadbe51c9fa80b284e618dadd5941cfd98 Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Tue, 15 Dec 2020 11:25:28 +0000
Subject: [PATCH 11/12] Move all_facenames implementation back into the editor

The extraction for testability wasn't necessary. It is fine to instantiate
the KivaFontEditor
---
 kiva/trait_defs/ui/wx/kiva_font_editor.py            | 9 ++-------
 kiva/trait_defs/ui/wx/tests/test_kiva_font_editor.py | 3 ++-
 2 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/kiva/trait_defs/ui/wx/kiva_font_editor.py b/kiva/trait_defs/ui/wx/kiva_font_editor.py
index 9428787eb..5bd0de7df 100644
--- a/kiva/trait_defs/ui/wx/kiva_font_editor.py
+++ b/kiva/trait_defs/ui/wx/kiva_font_editor.py
@@ -119,13 +119,8 @@ def str_font ( self, font ):
     def all_facenames(self):
         """ Returns a list of all available font typeface names.
         """
-        return _all_facenames()
-
-
-def _all_facenames():
-    """ Return a list of all available (TTF) font typeface names."""
-    font_manager = default_font_manager()
-    return sorted({f.name for f in font_manager.ttflist})
+        font_manager = default_font_manager()
+        return sorted({f.name for f in font_manager.ttflist})
 
 
 def KivaFontEditor(*args, **traits):
diff --git a/kiva/trait_defs/ui/wx/tests/test_kiva_font_editor.py b/kiva/trait_defs/ui/wx/tests/test_kiva_font_editor.py
index c885c629d..934273560 100644
--- a/kiva/trait_defs/ui/wx/tests/test_kiva_font_editor.py
+++ b/kiva/trait_defs/ui/wx/tests/test_kiva_font_editor.py
@@ -15,4 +15,5 @@ class TestFacename(unittest.TestCase):
     def test_all_facenames(self):
         # Test loading of available face names does not fail.
         # The available face names depend on the system
-        self.assertGreaterEqual(len(kiva_font_editor._all_facenames()), 0)
+        editor_factory = kiva_font_editor.KivaFontEditor()
+        self.assertGreaterEqual(len(editor_factory.all_facenames()), 0)

From 87ec45af0d287aff94d53f72d1266d88524bc900 Mon Sep 17 00:00:00 2001
From: Kit Yan Choi <kchoi@enthought.com>
Date: Tue, 15 Dec 2020 11:42:13 +0000
Subject: [PATCH 12/12] Use tempfile.TemporaryDirectory instead

---
 kiva/fonttools/tests/test_font_manager.py | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/kiva/fonttools/tests/test_font_manager.py b/kiva/fonttools/tests/test_font_manager.py
index ba7511d65..37f001d52 100644
--- a/kiva/fonttools/tests/test_font_manager.py
+++ b/kiva/fonttools/tests/test_font_manager.py
@@ -119,8 +119,9 @@ def setUp(self):
             os.path.abspath(os.path.join(data_dir, "TestTTF.ttf"))
         ]
 
-        self.temp_dir = tempfile.mkdtemp()
-        self.addCleanup(shutil.rmtree, self.temp_dir)
+        temp_dir_obj = tempfile.TemporaryDirectory()
+        self.temp_dir = temp_dir_obj.name
+        self.addCleanup(temp_dir_obj.cleanup)
 
     def test_load_font_from_cache(self):
         # Test loading fonts from cache file.