Avoid crash on null family name #128
Conversation
There was a problem hiding this comment.
This looks good with minor changes to avoid creating a glyph PBF when only a style_name exists, which I don't think makes much sense.
It looks like test/bin.test.js may need to be updated, as it appears to be erroring trying to read the invalid test fixture you added (picking it up in https://github.com/mapbox/node-fontnik/blob/master/bin/font-inspect#L29)
src/glyphs.cpp
Outdated
|
|
||
| std::vector<int> points_vec(points.begin(), points.end()); | ||
| if (ft_face->family_name || ft_face->style_name) { |
There was a problem hiding this comment.
Handling a case where only ft_face->style_name exists doesn't really make sense IMHO, we'd end up saving glyphs for fonts just named Regular or Bold
src/glyphs.cpp
Outdated
| } else if (ft_face->family_name) { | ||
| baton->faces.emplace_back(ft_face->family_name, std::move(points_vec)); | ||
| } else { | ||
|
|
There was a problem hiding this comment.
Empty else statement, per comment above handling only style_name doesn't make much sense.
There was a problem hiding this comment.
agree, fixed in 90ab6e8
src/glyphs.cpp
Outdated
| num_faces = ft_face->num_faces; | ||
| } | ||
|
|
||
| if (ft_face->family_name || ft_face->style_name) { |
There was a problem hiding this comment.
I think we should drop || ft_face->style_name here, as the else fallback will return an error if only style_name is present.
There was a problem hiding this comment.
agree, fixed in 90ab6e8
|
This is ready to go and green on travis, except that OS X machines are down right now due to some travis problem. |
src/glyphs.cpp
Outdated
| throw std::runtime_error("control is null"); | ||
| } | ||
| if (!to) { | ||
| throw std::runtime_error("control is null"); |
There was a problem hiding this comment.
Good catch. I just removed however (3e2915a) since these are uneeded.
|
This looks ready to merge and tag a release with the minor nit of the error for invalid |
Agreed.
This worked because in the currently deployed code (and in master and this previous to a2c47a1) the |
|
Next action on this:
|
|
Note: I'm not able to move on the next actions above for lack of bandwidth. /cc for help on pushing this through @jakepruitt @lbud @tmcw (folks who i can see have worked on the downstream fontmachine). |
Rough out extraction of sdf_glyph_foundry
|
Back to this. Merging now and will work on getting this into a release and published next. |
|
v0.5.0 release is now tagged and published with all these fixes, and including the refactoring work by @ChrisLoer 🙏.
Looking good at mapbox/fontmachine#15 |
Proposed solution for #124. Also addresses #123 and #125
/cc @xrwang @scothis