-
-
Notifications
You must be signed in to change notification settings - Fork 8.1k
Make PyFT2Font a subclass of FT2Font #30324
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
Conversation
|
The goal here is to avoid having to this whole |
b3d0b19 to
d725cf9
Compare
tacaswell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modulo clarifying a comment and moving two parts of the codes closer together again.
This makes it easier to do later refactors.
d725cf9 to
b1e1375
Compare
|
Modulo clarifying the possibly duplicate close #30324 (review) |
b1e1375 to
db17baf
Compare
tacaswell
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@QuLogic can self-merge when CI passes again.
I thought this was because this branch was missing d2d969e, but it was actually a change introduced in matplotlib#30324 as a separate (but similar-looking) issue.
PR summary
This avoids a level of indirection, and also means that a
FT2Fontis aPyFT2Font(since there are no other subclasses), which helps with some future work. The only downside is that the constructor/destructor order is fixed and slightly different, so we need to split the open/close stages for theFT_Faceand explicitly call them inPyFT2Font.There are currently still two copies of the fallback list, one on each level. The one on
FT2Fontis what's actually used, but the one onPyFT2Fontis the actual owner (because pybind11 owns the objects, so we need a Python object to own them). I think it may be possible to drop that extra copy by switching tostd::shared_ptr, but that will likely require the smart holders that are new in pybind11 v3, and I haven't fully tested that out yet.This PR is based on/waiting for #30322.PR checklist