Skip to content

Correctly get weight & style hints from certain newer Microsoft fonts #12945

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

Merged
merged 1 commit into from
Dec 12, 2018

Conversation

Stephen-Chilcote
Copy link
Contributor

@Stephen-Chilcote Stephen-Chilcote commented Dec 6, 2018

PR Summary

We've been having some internationalization issues caused by MPL not loading our international font, Microsoft YaHei. Part of the problem is that YaHei got turned in to a .TTC by a Windows update, which we rectified by overlaying this PR. But we still weren't getting what we wanted because matplotlib was displaying using YaHei Light instead of regular YaHei, and it looked really bad.

We tracked the issue down to the fact that the font manager wasn't identifying the light and regular versions as having different weights, making them appear identical and leading to plots using whichever one was higher up in the list.

We rectified this issue by giving font_manager.ttfFontProperty the ability to read Microsoft's SFNT tables. This allows matplotlib to correctly interpret the properties of a bunch of Windows default fonts including the one we were specifically interested in.

We just did this:

    sfnt2 = sfnt.get((1, 0, 0, 2), b'').decode('latin-1').lower() or \
            sfnt.get((3, 1, 0x0409, 2), b'').decode('utf_16_be').lower()
    sfnt4 = sfnt.get((1, 0, 0, 4), b'').decode('latin-1').lower() or \
            sfnt.get((3, 1, 0x0409, 4), b'').decode('utf_16_be').lower()

and it started working (the utf_16_be lines are the new ones.)

I haven't written a test for this yet since, honestly, we're just wondering if there's some obvious reason that this is a bad idea and hasn't been done already. If the community think it's a worthwhile addition I can finish it up as a proper PR though.

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

Copy link
Contributor

@anntzer anntzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole thing should be rewritten (I have some plans for that) but in the meantime I think the PR's still a plus.

sfnt4 = sfnt.get((1, 0, 0, 4), b'').decode('latin-1').lower()
# to look for ASCII substrings, where any ASCII-compatible encoding works
# - or big-endian UTF-16, since important Microsoft fonts use that.
sfnt2 = sfnt.get((1, 0, 0, 2), b'').decode('latin-1').lower() or \
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use parentheses instead of backslashes for line continuations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

sfnt2 = (sfnt.get((1, 0, 0, 2), b'').decode('latin-1').lower() or
sfnt.get((3, 1, 0x0409, 2), b'').decode('utf_16_be').lower())
sfnt4 = (sfnt.get((1, 0, 0, 4), b'').decode('latin-1').lower() or
sfnt.get((3, 1, 0x0409, 4), b'').decode('utf_16_be').lower())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For reference:
https://www.freetype.org/freetype2/docs/reference/ft2-truetype_tables.html
3 = TT_PLATFORM_MICROSOFT
0x0409 = TT_MS_LANGID_ENGLISH_UNITED_STATES

utf-16-be encoding is specified at https://www.microsoft.com/typography/otspec/name.htm

Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The approach seems sensible to me. Can you add a test that at least on AppVeyor tests with a font that only has an MS table?

@Stephen-Chilcote
Copy link
Contributor Author

Stephen-Chilcote commented Dec 10, 2018

Well, I'm trying to, but to be honest I've spent about six or seven hours total at this point trying to get the tests to run on my machine so I can test my test...

The other problem is that I'm not sure where to legally get a font to test against other than having Windows installed, which I assume your test runners aren't running.

@anntzer
Copy link
Contributor

anntzer commented Dec 10, 2018

https://matplotlib.org/devel/testing.html does not work for you? If there are specific problems feel free to ask.
Note that you don't need to run the whole test suite (see the link for how to run a single test).

@Stephen-Chilcote
Copy link
Contributor Author

Well, I deleted everything and started over, following the directions here. So now I'm getting ImportError: cannot import name 'ft2font'. I get the same thing even if I use setup.cfg with local_freetype = True and rebuild.

@anntzer
Copy link
Contributor

anntzer commented Dec 10, 2018

For testing, you should install in developer mode (https://matplotlib.org/devel/contributing.html#installing-matplotlib-in-developer-mode); replace the python setup.py bdist_wheel line by python -mpip install -ve ..

@Stephen-Chilcote
Copy link
Contributor Author

It runs now, but every test errors out with:

__________________ ERROR at setup of test_bar_ticklabel_fail __________________

request = <SubRequest 'mpl_test_settings' for <Function 'test_bar_ticklabel_fail'>>

    @pytest.fixture(autouse=True)
    def mpl_test_settings(request):
        from matplotlib.testing.decorators import _cleanup_cm
    
        with _cleanup_cm():
    
            backend = None
>           backend_marker = request.node.get_closest_marker('backend')
E           AttributeError: 'Function' object has no attribute 'get_closest_marker'

lib\matplotlib\testing\conftest.py:27: AttributeError

@anntzer
Copy link
Contributor

anntzer commented Dec 10, 2018

You need a recent enough pytest (pip install --upgrade pytest should get you the last version).

@Stephen-Chilcote
Copy link
Contributor Author

The test as it stands will only work on a Windows system; if I get time tomorrow I'll try to look for (or create?) a favorably-licensed font that can be used for the test.

@anntzer
Copy link
Contributor

anntzer commented Dec 11, 2018

A windows-only test is fine (if you can find a free font it's even better but only if it's available online, we probably won't want to include it in the repository just for testing this feature). Looks like you need to rebase, though.

@Stephen-Chilcote
Copy link
Contributor Author

Alright, uh, I think I've done it. May have made a few mistakes along the way but it's there now.

Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pending CI.

@timhoffm timhoffm merged commit 0fec7f1 into matplotlib:master Dec 12, 2018
@tacaswell tacaswell added this to the v3.1 milestone Dec 12, 2018
raamana added a commit to raamana/matplotlib that referenced this pull request Dec 13, 2018
* upstream/master: (1723 commits)
  Correctly get weight & style hints from certain newer Microsoft fonts (matplotlib#12945)
  Remove some checks for Py<3.6 in the test suite. (matplotlib#12974)
  Fail-fast when trying to run tests with too-old pytest.
  Include scatter plots in Qt figure options editor. (matplotlib#12779)
  ENH: replace deprecated numpy header
  Minor simplifications.
  tickminorvisible-fix (matplotlib#12938)
  Remove animated=True from animation docs
  Update the documentation of Cursor
  Misc. cleanups.
  Add test for 3d conversion of empty PolyCollection
  Support ~ as nonbreaking space in mathtext.
  Deprecate public use of Formatter.pprint_val.
  MAINT: Unify calculation of normal vectors from polygons (matplotlib#12136)
  Fix the title of testing_api
  More table documentation
  Simplify bachelors degree example using new features.
  Avoid pyplot in showcase examples.
  Simplify argument checking in Table.__getitem__. (matplotlib#12932)
  Minor updates following bump to Py3.6+.
  ...

# Conflicts:
#	lib/matplotlib/widgets.py
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants