{{ message }}
Support not embedding glyphs in svg mathtests. - #22881
Merged
Merged
Conversation
3 tasks
oscargus
approved these changes
May 7, 2022
jklymak
reviewed
May 18, 2022
jklymak
left a comment
jklymak
left a comment
Member
There was a problem hiding this comment.
I guess I'm no clear on this - how do we normally embed fonts in SVGs? If as shapes, then shouldn't we test that?
Contributor
Author
Member
|
OK, but I don't understand the motivation, other than a moderate optimization of the repo size. If we embed paths, that seems to be what we should test, even for new tests. |
Member
|
I think most of the time,we're not interested in testing the shape of the glyph path (and the algorithm of that). This glyph should be in that position is enough, and potentially more stable. If the path generation changes all tests would be broken even when the are testing primarily other aspects. |
Contributor
Author
timhoffm
reviewed
Jun 11, 2022
anntzer
force-pushed
the
mathtextsvgtests
branch
from
June 11, 2022 23:18
6478015 to
681e264
Compare
timhoffm
approved these changes
Jun 12, 2022
Member
|
Can self-merge after squashing. |
Contributor
Author
anntzer
force-pushed
the
mathtextsvgtests
branch
from
June 12, 2022 08:20
681e264 to
7402350
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
Redo of #19201 (decrease size of mathtext svg baselines, by switching to embedding text as text, not as paths), but without removing the other formats or restricting fonts. Suggested as a preliminary to #22852 (which would just need to move the affected tests to the new
svgastext_math_testslist.Intentionally contains an example test in a second commit, which is reverted by the third commit: checkout the second commit to test locally. I will remove these two commits from history (and hence get rid of the "unclean PR" lint) if this PR gets approved.
PR Checklist
Tests and Styling
pytestpasses).flake8-docstringsand runflake8 --docstring-convention=all).Documentation
doc/users/next_whats_new/(follow instructions in README.rst there).doc/api/next_api_changes/(follow instructions in README.rst there).This reverts commit ce14a1b.