Add face_index support to celiagg and kiva.agg backends.#605
Conversation
kitchoi
left a comment
kitchoi
left a comment
There was a problem hiding this comment.
The little bit of exposure I had with font manager does not seem enough to qualify me as that second pair of 👀 . I would want to sit down to give this a harder look if I am to review this, but realistically speaking, I am not sure I have the bandwidth to do that. I am sorry.
One comment after a quick glance.
|
We can wrap this up once #633 is merged. |
jwiggins
force-pushed
the
feature/font-face-index
branch
from
48ac3da to
27e3991
Compare
|
OK. I've tested this on Windows and macOS and am happy where it's at. I'm ready for a full review! |
|
I have not reviewed the code changes at all, but here is an update of what I see when I run the benchmark suite on this branch: Detailsshow_text: show_text_2x: show_text_radial_gradient: |
|
After a git clean and a rebuild, I no longer see any italicized text 👍 |
rahulporuri
left a comment
rahulporuri
left a comment
There was a problem hiding this comment.
LGTM with a couple of comments. Given the current refactor of font code that is happening in parallel, I guess we need to redo some of these changes?
| spec = font.findfont() | ||
| self.assertTrue(os.path.exists(spec.filename)) | ||
| self.assertEqual(spec.face_index, 0) | ||
|
|
There was a problem hiding this comment.
Do you think its worth adding a test for face_index or will that be easier after the ongoing refactor of the font manager code?
There was a problem hiding this comment.
A test for face_index? What do you mean? And yes, it'll probably be easier after integrating these changes with the new font manager code.
| { | ||
| unsigned i; | ||
|
|
||
| if ((face_name_len + 4 + 1) > m_face_lookup_scratch_len) |
There was a problem hiding this comment.
i'm not entirely sure what the magical 4 and 1 here are.
There was a problem hiding this comment.
4 for the face index (this is the %04u in the format string) and 1 for the NULL.
|
Thanks for the feedback and testing. I'll integrate this into my larger font manager refactor and then we'll go from there. |
5 participants
Fixes #391
Leaving this as a draft for now, because it depends on the latest release of celiagg which is not yet available via EDM.Aside from that this is a pretty scary change due to how twisty the font manager code is [and uncertainty about 3rd-party callers of said code]. It's be great to get some extra eyes on this before we move forward.