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

[ttLib] Add a convenience method to the cmap table to return the best available cmap #1092

Merged
merged 5 commits into from Nov 3, 2017

Conversation

Projects
None yet
3 participants
@justvanrossum
Collaborator

justvanrossum commented Nov 3, 2017

Often I just want to quickly access the best available cmap from a font, without being interested (or having to worry about) exactly which cmap subtables are in the font. I've used this functionality for while now in my own code as a separate function, and it's been so handy that I think it should just be in fonttools.

justvanrossum added some commits Nov 3, 2017

@justvanrossum justvanrossum changed the title from Add a convenience method to the cmap table to return the best available cmap to [ttLib] Add a convenience method to the cmap table to return the best available cmap Nov 3, 2017

@justvanrossum

This comment has been minimized.

Show comment
Hide comment
@justvanrossum

justvanrossum Nov 3, 2017

Collaborator

Btw. it could also be a method on TTFont, I use it often in conjunction with TTFont.getGlyphSet().

Collaborator

justvanrossum commented Nov 3, 2017

Btw. it could also be a method on TTFont, I use it often in conjunction with TTFont.getGlyphSet().

@anthrotype

This comment has been minimized.

Show comment
Hide comment
@anthrotype

anthrotype Nov 3, 2017

Member

it could also be a method on TTFont

sounds good to me

Member

anthrotype commented Nov 3, 2017

it could also be a method on TTFont

sounds good to me

@justvanrossum

This comment has been minimized.

Show comment
Hide comment
@justvanrossum

justvanrossum Nov 3, 2017

Collaborator

it could also be a method on TTFont

sounds good to me

  • if I go that route, shall I just commit to this pull request?
  • can the test code stay in test__c_m_a_p?
Collaborator

justvanrossum commented Nov 3, 2017

it could also be a method on TTFont

sounds good to me

  • if I go that route, shall I just commit to this pull request?
  • can the test code stay in test__c_m_a_p?
@anthrotype

This comment has been minimized.

Show comment
Hide comment
@anthrotype

anthrotype Nov 3, 2017

Member

you could have a same named getBestCmap method in both the cmap class and TTFont class, and the TTFont's method just calls the former.

Member

anthrotype commented Nov 3, 2017

you could have a same named getBestCmap method in both the cmap class and TTFont class, and the TTFont's method just calls the former.

@anthrotype

This comment has been minimized.

Show comment
Hide comment
@anthrotype

anthrotype Nov 3, 2017

Member

if you're going to keep the exception, then I'd rather have TTLibError than a catch-all ValueError (but I still prefer None)

Member

anthrotype commented Nov 3, 2017

if you're going to keep the exception, then I'd rather have TTLibError than a catch-all ValueError (but I still prefer None)

@anthrotype

This comment has been minimized.

Show comment
Hide comment
@anthrotype

anthrotype Nov 3, 2017

Member

... although TTLibError may also be misleading because it would seem to imply that a font without Unicode cmaps is somehow "faulty".

Member

anthrotype commented Nov 3, 2017

... although TTLibError may also be misleading because it would seem to imply that a font without Unicode cmaps is somehow "faulty".

cmapSubtable = self.getcmap(platformID, platEncID)
if cmapSubtable is not None:
return cmapSubtable.cmap
return None # None of the requested cmap subtables were found

This comment has been minimized.

@anthrotype

anthrotype Nov 3, 2017

Member

even without this line return None the function would have returned None. But explicit is better than implicit! :)

@anthrotype

anthrotype Nov 3, 2017

Member

even without this line return None the function would have returned None. But explicit is better than implicit! :)

This comment has been minimized.

@justvanrossum

justvanrossum Nov 3, 2017

Collaborator

I know, and yes, exactly! I always write return None if it has an explicit meaning other than "we're falling out of the method".

@justvanrossum

justvanrossum Nov 3, 2017

Collaborator

I know, and yes, exactly! I always write return None if it has an explicit meaning other than "we're falling out of the method".

@anthrotype

👍

@justvanrossum

This comment has been minimized.

Show comment
Hide comment
@justvanrossum

justvanrossum Nov 3, 2017

Collaborator

Thanks for the feedback, Cosimo!

Collaborator

justvanrossum commented Nov 3, 2017

Thanks for the feedback, Cosimo!

@anthrotype anthrotype merged commit 6040593 into master Nov 3, 2017

4 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@anthrotype anthrotype deleted the get-best-cmap branch Nov 3, 2017

@behdad

This comment has been minimized.

Show comment
Hide comment
@behdad

behdad Nov 3, 2017

Member

Can we make this match https://github.com/behdad/harfbuzz/blob/master/src/hb-ot-font.cc#L390

Also, would be good to make it return a base subtable and a Format14 subtable, if any.

Member

behdad commented Nov 3, 2017

Can we make this match https://github.com/behdad/harfbuzz/blob/master/src/hb-ot-font.cc#L390

Also, would be good to make it return a base subtable and a Format14 subtable, if any.

justvanrossum added a commit that referenced this pull request Nov 4, 2017

getBestCmap(): Expanded the list of cmap subtables to search for. Not…
… sure how to implement the rest of Behdad's suggestions from #1092
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment