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

Add getters for a font's name, style #19

Closed
wants to merge 24 commits into from

Conversation

stephenwithav
Copy link
Contributor

This PR aims to address #10 by adding Name and Style funcs to the public Font API.

Name and Style are retrieved from the first nameRecord that includes both.

The luxisr font is the only test case I added because I don't have the optional fonts referenced in TestIndex.

The tests passed when ran against a handful of those available in ttf-mscorefonts-installer.

The UTF-16 string check on line 346 is a hack. If you know of a better option, I'll happily update it. :)

@@ -95,11 +99,16 @@ type cm struct {
start, end, delta, offset uint32
}

// An nr holds a font's name, style
Copy link
Contributor

Choose a reason for hiding this comment

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

Doc comments should be complete sentences. Add a full stop.

@stephenwithav
Copy link
Contributor Author

The code has been updated.

Comments added. nr is now nameInfo. parseName is better written.

I haven't changed the eager reading yet.

}

const ( // The platform IDs in the name table.
PLATFORM_UNICODE = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Go names lookLikeThis, NOT_LIKE_THAT, even for constant names.

But it seems like you should be able to re-factor the constants like microsoftSymbolEncoding that are defined in parseCmap.

BTW it looks weird how you say that MICROSOFT_SYMBOL_CS has the value 1, but in parseCmap it says 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify the refactoring constants comment, do you mean move them to the global scope?

Thank you for the heads up on camelCasing constants in Go--and for catching the typo in MICROSOFT_SYMBOL_CS.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, move them to global scope.

I don't understand what you mean by a typo, though. Is the value 1 or 0?

@stephenwithav
Copy link
Contributor Author

parseName is still hairy.

Constants aren't camelCase yet.

NameIDFontFamily and NameIDFontSubfamily are currently the only supported NameIDs.

These'll be resolved later.

@stephenwithav
Copy link
Contributor Author

Just read parseCmap. Do you want me to add support for just the encodings used there?

type NameID uint16

const (
NameIDCopyright NameID = iota
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rather be explicit:
NameIDCopyright NameID = 0
NameIDFontFamily NameID = 1
etc
as the values are fixed in the file format, and I don't want adding or deleting constants here to change their values.

Below, delete NameIDReserved.

}

// parseSubtables wraps the commonality in Name and parseCmap.
func parseSubtables(b []byte, name string, firstSubtableOffset, subtableSize, valueOffset, valueLengthOffset int, offsets, values []int, isU16Offset bool) (int, int, int, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop the "first".

// Returns "" on error or not found.
func (f *Font) Name(id NameIDCode) string {
x, platformID, err := parseSubtables(f.name, "name", 6, 12,
func(b []byte) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to the previous line:

x, platformID, err := parseSubtables(f.name, "name", 6, 12, func(b []byte) bool {
return NameIDCode(u16(b, 6)) == id
})

@@ -78,6 +113,88 @@ func readTable(ttf []byte, offsetLength []byte) ([]byte, error) {
return ttf[offset:end], nil
}

// nameEntryInASCII converts b, which may be UTF-16 encoded, into an ACSII string.
func nameEntryInASCII(b []byte, utf16 bool) string {
Copy link
Contributor

Choose a reason for hiding this comment

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

Unused. Delete.

@stephenwithav
Copy link
Contributor Author

Updated.

@nigeltao
Copy link
Contributor

This has been merged.

Thanks for your patience.

@nigeltao nigeltao closed this Sep 21, 2015
@stephenwithav
Copy link
Contributor Author

You are very welcome. Thank you for working with me on this.

In CONTRIBUTORS, could you update my name and change my email to steven@stephenwithav.com?

@nigeltao
Copy link
Contributor

The only entry I can find in our Contributor License Agreement database (http://golang.org/doc/contribute.html#tmp_6) is for cureadvocate@gmail.com

If you want your CONTRIBUTORS entry to be under a different e-mail address, then I believe that you will have to sign the CLA again.

@stephenwithav
Copy link
Contributor Author

Is there a way to add an alternate email address to the CLA?

@nigeltao
Copy link
Contributor

I'm told: "He might be able to remove his GitHub username from his current CLA (Sign in with that email at https://cla.developers.google.com/clas) and then add his username to another CLA he signs with his new email."

@stephenwithav
Copy link
Contributor Author

Signed up for a new Google Account and submitted a new CLA. I didn't see an obvious way to delete the agreement from the current account, but I'll look into it more later.

nigeltao added a commit that referenced this pull request Sep 24, 2015
@nigeltao
Copy link
Contributor

I've updated the AUTHORS and CONTRIBUTORS files.

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.

2 participants