8000 Add tests for most of FT2Font, and fix some bugs by QuLogic · Pull Request #28765 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]
Skip to content

Add tests for most of FT2Font, and fix some bugs #28765

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
Merged
merged 11 commits into from
Sep 6, 2024
Merged

Add tests for most of FT2Font, and fix some bugs #28765

merged 11 commits into from
Sep 6, 2024

Conversation

QuLogic
Copy link
Member
@QuLogic QuLogic commented Aug 29, 2024

PR summary

I tried to cross-compare these against something external, like FontForge or fonttools, though a lot are just "make sure it keeps working the same way".

I left out FT2Font.get_ps_font_info because it doesn't appear that any of shipped fonts would return anything but None.

Additionally, this fixes a few bugs I've found:

  • The return value of FT2Font.set_text is a NumPy array with a data pointer that was freed, meaning any reading of it would read an invalid pointer. Apparently, we never use this return value internally, so it's never been a problem, but it has existed since about forever.
  • The created/modified timestamps in the head table from FT2Font.get_sfnt_table were signed, which is incorrect, as they should still be positive now, given that the epoch is some time in 1901. I'm not sure if we want to consider changing these to datetimes instead, but that would be an API change, so I didn't do that.
  • Fix a few errors in the docs.

I want to also add some more tests for fallbacks, but haven't finished that yet.

PR checklist

QuLogic added 11 commits August 24, 2024 03:32
@QuLogic
TST: Add a test for FT2Image
2c3cae2 
The only externally-available API here is `draw_rect_filled` and
conversion to NumPy arrays via the buffer protocol.
@QuLogic
TST: Add tests for FT2Font attributes
720aed0
@QuLogic
TST: Add tests for invalid FT2Font arguments
a1b4498
@QuLogic
TST: Add tests for FT2Font.clear
2387233 
Also, ensure some internals are initialized/cleared.
@QuLogic
TST: Add tests for FT2Font charmaps
6bad7f0
@QuLogic
TST: Add tests for FT2Font.get_sfnt{,_table}
2754aaa 
Also, correct the reading of creation and modified dates.
@QuLogic
TST: Add tests for FT2Font.get_kerning
3748c99
@QuLogic
Fix return value of FT2Font.set_text and add tests for it
833a259 
`PyArray_SimpleNewFromData` does not copy its input data, and the
`std::vector` is a local variable that disappears after the C++ method
returns.
@QuLogic
TST: Add test for FT2Font.set_size
f618fc2
@QuLogic
TST: Add tests for FT2Font.load_{char,glyph}
7c390de
@QuLogic
TST: Add tests for FT2Font drawing and path generation
06df253
@QuLogic QuLogic mentioned this pull request Aug 30, 2024
Open
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 6, 2024
@QuLogic
Improve fallback font export tests
9c5136f 
Working on matplotlib#28765, I realized that we don't need an external font to
test font fallback, as `cmr10`, and related, fonts have fairly low
coverage compared to `DejaVu Sans`. Thus font fallback can easily be
tested by starting with `cmr10`, and pulling additional characters from
`DejaVu Sans`.

In addition, this change increases unique characters in the figure to
587, which triggers some code paths for tables that are limited to 256
characters.
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Sep 6, 2024
@QuLogic
Improve fallback font export tests
0129d60 
Working on matplotlib#28765, I realized that we don't need an external font to
test font fallback, as `cmr10`, and related, fonts have fairly low
cov
8000
erage compared to `DejaVu Sans`. Thus font fallback can easily be
tested by starting with `cmr10`, and pulling additional characters from
`DejaVu Sans`.

In addition, this change increases unique characters in the figure to
491, which triggers some code paths for tables that are normally limited
to 256 characters.
@QuLogic QuLogic mentioned this pull request Sep 6, 2024
Merged
1 task
@tacaswell tacaswell added this to the v3.10.0 milestone Sep 6, 2024
@tacaswell tacaswell merged commit cb9cf3b into matplotlib:main Sep 6, 2024
48 checks passed
@QuLogic QuLogic deleted the ft2font-tests branch September 6, 2024 01:55
@tacaswell
Copy link
Member

It looks like this PR is causing the code in 5f2a89a to fail to compile on the older OSX targets we use with errors that look like

        FAILED: src/ft2font.cpython-313-darwin.so.p/ft2font.cpp.o
451
        c++ -Isrc/ft2font.cpython-313-darwin.so.p -Isrc -I../src -I../subprojects/freetype-2.6.1/include -I../../../pip-build-env-4ch2e0bi/overlay/lib/python3.13/site-packages/numpy/_core/include -I../extern/agg24-svn/include -I/Library/Frameworks/Python.framework/Versions/3.13/include/python3.13 -fvisibility=hidden -fvisibility-inlines-hidden -flto -fdiagnostics-color=always -DNDEBUG -Wall -Winvalid-pch -std=c++17 -O3 -DNPY_NO_DEPRECATED_API=NPY_1_7_API_VERSION -D__STDC_FORMAT_MACROS=1 -DPY_ARRAY_UNIQUE_SYMBOL=MPL_ft2font_ARRAY_API '-DFREETYPE_BUILD_TYPE="local"' -MD -MQ src/ft2font.cpython-313-darwin.so.p/ft2font.cpp.o -MF src/ft2font.cpython-313-darwin.so.p/ft2font.cpp.o.d -o src/ft2font.cpython-313-darwin.so.p/ft2font.cpp.o -c ../src/ft2font.cpp
452
        ../src/ft2font.cpp:731:14: error: 'to_chars<unsigned int, 0>' is unavailable: introduced in macOS 10.15
453
                std::to_chars(buffer.data() + 3, buffer.data() + buffer.size(),
454
                     ^
455
        /Applications/Xcode_14.2.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/charconv:458:1: note: 'to_chars<unsigned int, 0>' has been explicitly marked unavailable here
456
        to_chars(char* __first, char* __last, _Tp __value, int __base)
457
        ^

@QuLogic
Copy link
Member Author
QuLogic commented Sep 6, 2024

No, that's been failing since it was merged; it's not related to this PR specifically.

QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Oct 16, 2024
@QuLogic
Improve fallback font export tests
4f6cf0e 
Working on matplotlib#28765, I realized that we don't need an external font to
test font fallback, as `cmr10`, and related, fonts have fairly low
coverage compared to `DejaVu Sans`. Thus font fallback can easily be
tested by starting with `cmr10`, and pulling additional characters from
`DejaVu Sans`.

In addition, this change increases unique characters in the figure to
491, which triggers some code paths for tables that are normally limited
to 256 characters.
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Dec 3, 2024
@QuLogic
Improve fallback font export tests
e17491d 
Working on matplotlib#28765, I realized that we don't need an external font to
test font fallback, as `cmr10`, and related, fonts have fairly low
coverage compared to `DejaVu Sans`. Thus font fallback can easily be
tested by starting with `cmr10`, and pulling additional characters from
`DejaVu Sans`.

In addition, this change increases unique characters in the figure to
491, which triggers some code paths for tables that are normally limited
to 256 characters.
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Dec 11, 2024
@QuLogic
Improve fallback font export tests
73dbb38 
Working on matplotlib#28765, I realized that we don't need an external font to
test font fallback, as `cmr10`, and related, fonts have fairly low
coverage compared to `DejaVu Sans`. Thus font fallback can easily be
tested by starting with `cmr10`, and pulling additional characters from
`DejaVu Sans`.

In addition, this change increases unique characters in the figure to
491, which triggers some code paths for tables that are normally limited
to 256 characters.
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Dec 18, 2024
@QuLogic
Improve fallback font export tests
0636690 
Working on matplotlib#28765, I realized that we don't need an external font to
test font fallback, as `cmr10`, and related, fonts have fairly low
coverage compared to `DejaVu Sans`. Thus font fallback can easily be
tested by starting with `cmr10`, and pulling additional characters from
`DejaVu Sans`.

In addition, this change increases unique characters in the figure to
491, which triggers some code paths for tables that are normally limited
to 256 characters.
QuLogic added a commit to QuLogic/matplotlib that referenced this pull request Dec 19, 2024
@QuLogic
Improve fallback font export tests
d48cc1c 
Working on matplotlib#28765, I realized that we don't need an external font to
test font fallback, as `cmr10`, and related, fonts have fairly low
coverage compared to `DejaVu Sans`. Thus font fallback can easily be
tested by starting with `cmr10`, and pulling additional characters from
`DejaVu Sans`.

In addition, this change increases unique characters in the figure to
491, which triggers some code paths for tables that are normally limited
to 256 characters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oscargus oscargus oscargus approved these changes

Assignees
No one assigned
Labels
topic: text/fonts
Projects
None yet
Milestone
v3.10.0
Development

Successfully merging this pull request may close these issues.
3 participants
@QuLogic @tacaswell @oscargus
0