Skip to content
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.

Sign up for GitHub

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

Jump to bottom

FIX: correctly handle generic font families in svg text-as-text mode #23638

Merged
merged 3 commits into from Sep 30, 2022
Merged

FIX: correctly handle generic font families in svg text-as-text mode #23638

merged 3 commits into from Sep 30, 2022

Conversation

tacaswell
Copy link
Member

PR Summary

This:

  • expands the generic fonts families to the user configured specific fonts
    in the svg output
  • ensures that each font family only appears once per text element in the svg output
  • ensures that generic families are unquoted and specific families are

closes #22528
closes #23492

I still need to write a test for this, but interactively this gives things like

 <text style="font: 10px 'DejaVu Sans', 'WenQuanYi Zen Hei', 'DejaVu Sans', 'Bitstream Vera Sans', 'Computer Modern Sans Serif', 'Lucida Grande', 'Verdana', 'Geneva', 'Lucid', 'Arial', 'Helvetica', 'Avant Garde', sans-serif; text-anchor: start" x="236.16" y="174.528" transform="rotate(-0 236.16 174.528)">There are 几个汉字 in between!</text>

PR Checklist

Tests and Styling

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

@tacaswell tacaswell added this to the v3.6.0 milestone Aug 16, 2022
@tacaswell tacaswell mentioned this pull request Aug 16, 2022
Closed
@tacaswell tacaswell marked this pull request as ready for review August 17, 2022 02:40
@tacaswell
Copy link
Member Author

This might require some changes to sort out if CI has the fonts (or just just the "wrong" kind of font to exercise the code).

@tacaswell
Copy link
Member Author

I appear to have introduced a memory leak as the runners are getting OOM'd and running the tests with a single worker is using ~2G of ram.

@tacaswell
Copy link
Member Author

This seems to cause an 8x increase in memory usage and a 10x slow down in the test suite.

@tacaswell tacaswell marked this pull request as draft August 17, 2022 14:23
anntzer
anntzer reviewed Aug 18, 2022
View reviewed changes
lib/matplotlib/font_manager.py Outdated
family,
", ".join(self._expand_aliases(family))
if family in font_family_aliases:
# deal with the sans/san-serif/sans serif issue
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the following line (the start of the for loop) can be just

for fam in self._expand_aliases(family):
    if fam in font_family_aliases: continue  # is that even possible?
    ...

Copy link
Member Author

Choose a reason for hiding this comment

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

This will go into another PR, this change seems to be causing a 10x memory increase and I do not understand why yet.

@tacaswell tacaswell modified the milestones: v3.6.0, v3.7.0 Aug 19, 2022
@tacaswell
Copy link
Member Author

This is not ready yet and I do not want to hold 3.6 on it. It is possible that the changes will be small enough to backport to a 3.6.x, but I am not optimistic.

@tacaswell tacaswell force-pushed the svg_generic_fonts branch 2 times, most recently from 36725ed to af2cc4f Compare September 1, 2022 01:51
@tacaswell tacaswell modified the milestones: v3.7.0, v3.6.1 Sep 1, 2022
@tacaswell
Copy link
Member Author

I think this may now be small enough to backport to v3.6.x but I do not think we should block 3.6.0 over it.

@tacaswell tacaswell marked this pull request as ready for review September 1, 2022 02:07
anntzer
anntzer reviewed Sep 1, 2022
View reviewed changes
lib/matplotlib/backends/backend_svg.py Outdated
@@ -1151,10 +1151,56 @@ def _draw_text_as_text(self, gc, x, y, s, prop, angle, ismath, mtext=None):
weight = fm.weight_dict[prop.get_weight()]
if weight != 400:
font_parts.append(f'{weight}')

def unique_iter(inp):
Copy link
Contributor

@anntzer anntzer Sep 1, 2022 •
edited

Choose a reason for hiding this comment

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

That's actually [*dict.fromkeys(inp)] (or even without the conversion to list which is not needed in this case, iterating over the keys is fine); this is so short that it could even be inlined with a comment, something like dict.fromkeys(s for fam in prop.get_family() for s in _format_font_name(fam)) (or dict.fromkeys(itertools.chain(*map(_format_font_name, prop.get_family()))) for itertools aficionados)

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, nice!

Copy link
Member

Choose a reason for hiding this comment

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

I replaced unique_iter, but didn't do anything fancier.

@tacaswell @QuLogic
TST: add a test of font families in svg text-as-text mode
a6f15f2 
This tests:

1. all of the fonts from the generic lists Matplotlib maintains ends up in the
   svg
2. the generic family is included and un-escaped
3. the specific fonts are escaped
4. glyph fallback will happen in fonts found via generic family names
@tacaswell @QuLogic
FIX: correctly handle generic font families in svg text-as-text mode
a0fcf79 
This:

- expands the generic fonts families to the user configured specific fonts
  in the svg output
- ensures that each font family only appears once per text element in the svg output
- ensures that generic families are unquoted and specific families are

closes matplotlib#22528
closes matplotlib#23492
@tacaswell @QuLogic
FIX: do not raise in lru_cached function
6e99a52 
If the cached function raises it will not be cached and we will continuously
pay for cache misses.
@QuLogic
Copy link
Member

QuLogic commented Sep 30, 2022

I pushed some minor corrections to the comments directly.

@tacaswell
Copy link
Member Author

I'm going to merge this on @QuLogic 's review via working on it and @anntzer 's review.

@tacaswell tacaswell merged commit 8f6616c into matplotlib:main Sep 30, 2022
@tacaswell tacaswell deleted the svg_generic_fonts branch September 30, 2022 22:27
@meeseeksmachine meeseeksmachine mentioned this pull request Sep 30, 2022
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 30, 2022
@tacaswell @meeseeksmachine
Backport PR matplotlib#23638: FIX: correctly handle generic font fami…
bf16cb6 
…lies in svg text-as-text mode
oscargus added a commit that referenced this pull request Oct 1, 2022
@oscargus
Merge pull request #24059 from meeseeksmachine/auto-backport-of-pr-23…
147b5d4 
…638-on-v3.6.x

Backport PR #23638 on branch v3.6.x (FIX: correctly handle generic font families in svg text-as-text mode)
@ksunden ksunden mentioned this pull request Feb 20, 2023
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@oscargus oscargus oscargus left review comments

@anntzer anntzer anntzer approved these changes

Assignees
No one assigned
Labels
backend: svg
Projects
None yet
Milestone
v3.6.1
4 participants
@tacaswell @QuLogic @anntzer @oscargus