Remove redundant code from kiva.fonttools.font_manager

[jwiggins]

Resolves #391

At last we've arrived. This PR removes a ton of code from kiva.fonttools.font_manager, leaving only FontManager, fontManager and default_font_manager as the things we're [sort of] OK with people importing. Really, everyone should just use Font and don't peek under the covers (unless writing a Kiva backend).

Additionally, kiva.fonttools.afm is being removed completely now that #700 has been merged.

This PR will break any code which explicitly tries to muck about with the creation of the system font cache. A nice API for this will arrive with the resolution of #555. This PR will also break any code which was calling functions that have been moved to the new private kiva.fonttools modules. Searching all of GitHub didn't turn up a lot of external code which has these behaviors, so I'm not too worried (though I might reach out to some of those projects to give them a heads up).

Test coverage is a bit better in this new world.
master branch:

Name                             Stmts   Miss Branch BrPart  Cover
------------------------------------------------------------------
kiva/fonttools/__init__.py           1      0      0      0   100%
kiva/fonttools/_constants.py         5      0      0      0   100%
kiva/fonttools/_score.py            60      0     20      0   100%
kiva/fonttools/_util.py             31     31     12      0     0%
kiva/fonttools/afm.py              213    184     74      0    10%
kiva/fonttools/font.py              77      9     22      5    86%
kiva/fonttools/font_manager.py     582    256    232     38    49%
------------------------------------------------------------------
TOTAL                              969    480    360     43    45%

This branch: [Note that kiva.fonttools._scan_sys actually has better coverage, but it's hard to capture that on a single OS]

Name                                 Stmts   Miss Branch BrPart  Cover
----------------------------------------------------------------------
kiva/fonttools/__init__.py               1      0      0      0   100%
kiva/fonttools/_constants.py             5      0      0      0   100%
kiva/fonttools/_font_properties.py     110      8     34      3    91%
kiva/fonttools/_scan_parse.py          127      9     50      3    93%
kiva/fonttools/_scan_sys.py            151     69     76      7    53%
kiva/fonttools/_score.py                60      0     20      0   100%
kiva/fonttools/_util.py                 31      3     12      1    91%
kiva/fonttools/font.py                  76     10     20      4    85%
kiva/fonttools/font_manager.py         159     33     50     16    74%
----------------------------------------------------------------------
TOTAL                                  720    132    262     34    79%

[jwiggins]

Better yet, the coverage report from 5.0.1:

Name                             Stmts   Miss Branch BrPart  Cover
------------------------------------------------------------------
kiva/fonttools/__init__.py           1      0      0      0   100%
kiva/fonttools/afm.py              213    184     74      0    10%
kiva/fonttools/font.py              77     36     22      3    46%
kiva/fonttools/font_manager.py     634    219    252     43    61%
kiva/fonttools/sstruct.py           80      2     28      1    97%
------------------------------------------------------------------
TOTAL                             1005    441    376     47    52%

[rahulporuri]

[jwiggins]

Nothing? Really?

[rahulporuri]

Nothing? Really?

:D Not really. I was going to ask if we could flake8 kiva.fonttools now that it's all new and fancy - but i checked and it's already flake8 clean (except for two imports in the __init__). I thought I needed to reiterate my stance that it should be safe to ship this code as we don't have any internal (enthought) users of the removed code - but you already mentioned it. Me saying it would have been redundant after having already said it on other PRs. I browsed through the code locally and it looks good to me.

I guess the only thing i can say is - given the amount of time you've spend digging into kiva.fonttools, is there information in your head that's not already in the code/docstrings/existing docs that's worth documenting? i didn't think there was any apart from esoteric details about fonts/font handling/fontTools, which is why i didn't bring it up earlier

I guess we could ask internally if anyone has qualms with the refactor/cleanup. It couldn't hurt. I guess?

so, tl; dr 😄

[corranwebster]

You might want to mess around a bit more with the font_manager.py module docstring - possibly move the historical copyright and author info to block comments and change it to be a bit more oriented towards inclusion in autogenerated API docs.

But also not a big issue if you have more changes in the pipe for the font manager.

[jwiggins]

Is there information in your head that's not already in the code/docstrings/existing docs that's worth documenting?

A good question. I would say "Yes", but only because that's how things normally go. That said, I think with the tests and logically separating the parts out into modules probably means that I could understand this code in six months when I've lost the context. That's my hope at least. In the meantime, it might be nice to see if Kit has the time to take a look and give her opinion on the state of things.

There is still one big fix I'd like to address. That's the bad fallback font selection. "vera.ttf" doesn't exist on macOS, so it would be nice to have a slightly more sane fallback font selection process.

[aaronayres35]

LGTM as well. Just had a couple very minor questions

[aaronayres35]

why not set size=12.0 in the method signature?
Also why size and default_size? it seems in the rest of the code for the class default_size is used

[jwiggins]

I'm glad you brought this up because I forgot about it. It's because of this code: https://github.com/enthought/enable/pull/704/files#diff-07c82150ae0bd465451ecaaaf02e959e718f46ea5e494d84dfd2ed81e6194316L1216-L1217.

That line of code in score_size gets run if you pass a a FontProperties instance to FontManager.findfont with its size attribute explicitly set to one of the strings in font_scalings. That's basically never. I know, because nobody complained about it in the 11 years that it had a bug that would raise an exception.

I fixed that bug, but now score_size must be called with a valid default size. That means that FontManager needs to have a non-None value set to .default_size. But it gets worse. _load_from_cache_or_rebuild explicitly sets FontManager.default_size back to None after loading it from the cache! We should probably stop doing that, but the fact that it's done implies that messing with the default_size was somehow the expected interface?

I'm not sure what to do.

[aaronayres35]

I see you've opened #709, thank you! This can be addressed separately from this PR

[jwiggins]

#709 opened to track

[jwiggins]

@corranwebster kiva.fonttools.font_manager is explicitly ignored by the documentation build (as of #703)

[jwiggins]

#710 opened to track the default font issue.

[jwiggins]

Thanks for the feedback everyone. I'm going to merge now, but there's still some scattered work to do which should be covered by open issues.

[rahulporuri]

I was wondering if we should document what all previously public stuff from font_manager got removed - or moved out to private modules - but it should be straightforward to someone to figure that out for themselves.