Skip to content
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

._glyph does not exists anymore, go for the new _getGlyphAndOffset() … #52

Merged
merged 3 commits into from
Oct 8, 2022

Conversation

typemytype
Copy link
Member

…callback

this will not be backwards compatible with older fontTools...

@jenskutilek

…callback

this will not be backwards compatible with older fontTools...
@justvanrossum
Copy link
Contributor

You should not use those internal attrs/methods at all. It was previously wrong, and this PR doesn't fix it. Do not use internal stuff.

If you need access to a glyph object from the glyf table, you should get it from the glyf table.

Copy link
Contributor

@jenskutilek jenskutilek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @typemytype, I have been bitten by this change in a couple of my own modules ...

@@ -146,7 +146,7 @@ def extractGlyphPrograms(source, destination):
return
glyph_set = source.getGlyphSet()
for name in glyph_set.keys():
glyph = glyph_set[name]._glyph
glyph, offset = glyph_set[name]._getGlyphAndOffset()
Copy link
Contributor

@jenskutilek jenskutilek Oct 8, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps better like this?

glyph = glyph_set.glyfTable[name]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just get the glyf table from the ttfont. Why even use getGlyphSet at all?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

glyph_set.glyfTable should also be private.

@jenskutilek
Copy link
Contributor

And probably you should raise the fonttools dependency in setup to fontTools>=4.37.3

@justvanrossum
Copy link
Contributor

justvanrossum commented Oct 8, 2022

Just do this:

def extractGlyphPrograms(source, destination):
    """
    Extract the TrueType pre-program to the font lib.
    """
    if "glyf" not in source:
        return
    glyph_table = source["glyf"]
    for name in glyph_table.keys():
        glyph = glyph_table[name]
        dest_glyph = destination[name]
        if glyph.isComposite():
            # Extract composite flags
            _extractCompositeFlags(glyph, dest_glyph)
        if not hasattr(glyph, "program"):
            continue

        hash_pen = HashPointPen(dest_glyph.width, destination)
        dest_glyph.drawPoints(hash_pen)
        lib = dest_glyph.lib[TRUETYPE_INSTRUCTIONS_KEY] = {
            "formatVersion": "1",
            "id": hash_pen.hash,
        }
        lib["assembly"] = _byteCodeToTtxAssembly(glyph.program)

font.getGlyphSet() is an abstraction to extract outline data, not a way to get at raw glyf glyphs.

@jenskutilek
Copy link
Contributor

I was probably under the impression that the glyphset was the preferred way of getting at the glyphs because getGlyphSet() is so high up in the module ...

@jenskutilek
Copy link
Contributor

And probably you should raise the fonttools dependency in setup to fontTools>=4.37.3

With @justvanrossum’s suggestion, the dependency can of course stay as it was.

@typemytype
Copy link
Member Author

thanks @justvanrossum

@typemytype typemytype merged commit c310904 into master Oct 8, 2022
@justvanrossum
Copy link
Contributor

I was probably under the impression that the glyphset was the preferred way of getting at the glyphs because getGlyphSet() is so high up in the module ...

But using attrs starting with underscores should trigger all alarm bells...

font.getGlyphSet() is high up because it implements drawable glyphs for both glyf and CFF (*), and abstracts away their differences where possible. If you need to access the glyf table, you should access the glyf table, and not try to sneak your way through a higher level abstraction.

*) And since recently (related to the refactoring that triggered this PR) it supports glyf + gvar and CFF2, via the location argument of font.getGlyphSet().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants