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 parsing code for the Font Features, as well as new command #3

Merged
merged 7 commits into from Jan 23, 2018

Conversation

Projects
None yet
3 participants
@bramp
Collaborator

bramp commented Jan 21, 2018

No description provided.

bramp added some commits Jan 21, 2018

@bramp

This comment has been minimized.

Collaborator

bramp commented Jan 21, 2018

For example:

$ go run main.go features ~/path/to/font.woff 

Glyph Substitution Table (GSUB):
	Script "DFLT" (Default):
		Default Language:
			Feature "aalt" (Access All Alternates)
			Feature "c2sc" (Small Capitals From Capitals)
			Feature "case" (Case-Sensitive Forms)
			Feature "crcy" (Currency (Deprecated))
		Language "AZE " (Azerbaijani):
			Feature "smcp" (Small Capitals)
	Script "cyrl" (Cyrillic):
		Default Language:
			Feature "aalt" (Access All Alternates)
			Feature "c2sc" (Small Capitals From Capitals)
			Feature "case" (Case-Sensitive Forms)
			Feature "crcy" (Currency (Deprecated))

Glyph Positioning Table (GPOS):
	Script "DFLT" (Default):
		Default Language:
			Feature "kern" (Kerning)
	Script "cyrl" (Cyrillic):
		Default Language:
			Feature "kern" (Kerning)
	Script "latn" (Latin):
		Default Language:
			Feature "kern" (Kerning)
		Language "AZE " (Azerbaijani):
			Feature "kern" (Kerning)
"github.com/ConradIrwin/font/sfnt"
)
func layoutTable(font *sfnt.Font, tag sfnt.Tag, name string) {

This comment has been minimized.

@dmitshur

dmitshur Jan 21, 2018

Collaborator

I’d suggest moving layoutTable to the bottom, under Features, rather than on top.

In Go, it’s common to order code from higher level functions first to lower level helpers second. That way, when reading code, you first encounter higher level, easier to understand code that helps provide context for the helpers. Then, once you're familiar with how a helper is being used and why it’s needed, reading its code is easier.

This comment has been minimized.

@bramp

bramp Jan 21, 2018

Collaborator

Thanks for the feedback. Good point. Fixed.

@dmitshur

This comment has been minimized.

Collaborator

dmitshur commented Jan 21, 2018

(The rest of this PR is awesome, nice work!)

@dmitshur

Spotted a couple typos.

"github.com/ConradIrwin/font/sfnt"
)
// Features prints the gpos/gsub tables (contins font features).

This comment has been minimized.

@dmitshur

dmitshur Jan 21, 2018

Collaborator

s/contins/contains/

This comment has been minimized.

@bramp

bramp Jan 21, 2018

Collaborator

fixed

main.go Outdated
features: prints the gpos/gsub tables (contins font features)

This comment has been minimized.

@dmitshur

dmitshur Jan 21, 2018

Collaborator

s/contins/contains/

This comment has been minimized.

@bramp

bramp Jan 21, 2018

Collaborator

fixed

}
if len(tag) == 4 && tag[0] == 'c' && tag[1] == 'v' && tag[2] >= '0' && tag[2] <= '9' && tag[3] >= '0' && tag[3] <= '9' {
return "Character Variants"

This comment has been minimized.

@ConradIrwin

ConradIrwin Jan 22, 2018

Owner

@bramp might be worth preserving the number here (e.g. Character Variables 01)

This comment has been minimized.

@ConradIrwin

ConradIrwin Jan 22, 2018

Owner

(Also, should we do the same parsing for ss01...? )

This comment has been minimized.

@bramp

bramp Jan 22, 2018

Collaborator

Sure, will fix.

This comment has been minimized.

@bramp

bramp Jan 23, 2018

Collaborator

Added numeric suffix, plus added support for ss01... and added a test.

// Features prints the gpos/gsub tables (contains font features).
func Features() {
if len(os.Args) < 2 {
panic(fmt.Errorf("Specify a font file"))

This comment has been minimized.

@ConradIrwin

ConradIrwin Jan 22, 2018

Owner

Can we start to improve the code for handling these errors, we should make it clear how to fix the problem, and make the error message more legible. (I appreciate the other commands aren't yet very good).

fmt.Printf("Usage: font features <font-file>")
os.Exit(1)

This comment has been minimized.

@bramp

bramp Jan 23, 2018

Collaborator

I'll do that in a later PR.

if font.HasTable(tag) {
fmt.Printf("%s:\n", name)
t := font.Table(tag).(*sfnt.TableLayout)

This comment has been minimized.

@ConradIrwin

ConradIrwin Jan 22, 2018

Owner

@bramp This will panic if parsing fails. I'm not sure what to do about that except maybe change the library so that it parses tables on-demand instead of up-front so we can return a sensible error. Any better ideas?

This comment has been minimized.

@bramp

bramp Jan 22, 2018

Collaborator

Yes, panic'ng sucks. After this I'd be happy to change the whole library to pass back the errors.

This comment has been minimized.

@ConradIrwin

ConradIrwin Jan 22, 2018

Owner

That'd be incredible 😻

t := font.Table(tag).(*sfnt.TableLayout)
for _, script := range t.Scripts {
fmt.Printf("\tScript %q (%s):\n", script.Tag, script.String())

This comment has been minimized.

@ConradIrwin

ConradIrwin Jan 22, 2018

Owner

When I run this on a font I see a weird:

Script " " ():

Do you know why? Can we make it clearer what it means?

This comment has been minimized.

@bramp

bramp Jan 22, 2018

Collaborator

Would you email me that font?

This comment has been minimized.

@ConradIrwin

ConradIrwin Jan 22, 2018

Owner

it's here: https://gist.github.com/ConradIrwin/a85f8e432fedf2cd4ea400286a796b5e

If you have the gist tool installed (brew install gist) you can get the file with: gist -r a85f8e432fedf2cd4ea400286a796b5e | base64 --decode > adelle.ttf

This comment has been minimized.

@bramp

bramp Jan 23, 2018

Collaborator

So that font has a script who's tag is 0x20202020 (four space characters). The spec says nothing about what this means, so I suspect it is just some weird artefact.

I've changed the tool to not display the () if the name is unknown.

type scriptTable struct {
DefaultLangSys uint16 // Offset to default LangSys table, from beginning of Script table — may be NULL
LangSysCount uint16 // Number of LangSysRecords for this script — excluding the default LangSys
// langSysRecords[langSysCount] langSysRecord // Array of LangSysRecords, listed alphabetically by LangSys tag

This comment has been minimized.

@ConradIrwin

ConradIrwin Jan 22, 2018

Owner

@bramp Why is this here but commented out? Should we uncomment it or delete it?

This comment has been minimized.

@bramp

bramp Jan 23, 2018

Collaborator

Ah yes, perhaps unclear. The ondisk format has a variable length array as the last element. I explicitly deal with that in the code, but left the field commented to indicate it was there.

I can drop the comment, or make it clear why it's there.

return nil, io.ErrUnexpectedEOF
}
b = b[record.Offset:]

This comment has been minimized.

@ConradIrwin

ConradIrwin Jan 22, 2018

Owner

@bramp Is there a byte length on the record? I'm worried about the parsing code reading beyond the length of the record.

This comment has been minimized.

@bramp

bramp Jan 23, 2018

Collaborator

nope, there is sadly no length for any of these fields. I suspect they are all contained within the single table.

This comment has been minimized.

@bramp

bramp Jan 23, 2018

Collaborator

ah yes, all the records are within the same table, and that b array only contains the current table:

buffer := io.NewSectionReader(file, int64(entry.Offset), int64(entry.Length))
table, err := parseTable(entry.Tag, buffer)
...
    b, err := ioutil.ReadAll(buffer)
...
    b = b[record.Offset:]
@ConradIrwin

This comment has been minimized.

Owner

ConradIrwin commented Jan 22, 2018

Otherwise this looks great.

If you have the time, a test would be great, but happy to merge without.

@ConradIrwin

This comment has been minimized.

Owner

ConradIrwin commented Jan 22, 2018

(Thanks for the other view @shurcooL )

@ConradIrwin

This comment has been minimized.

Owner

ConradIrwin commented Jan 22, 2018

P.S I added you both as collaborators to this repository.

Please notify me for PRs with large changes or API changes — but happy if you want to merge your own PRs for tweaks to code-style / error messages.

bramp added some commits Jan 23, 2018

@bramp

This comment has been minimized.

Collaborator

bramp commented Jan 23, 2018

I've pushed my changes, if you want to make one more pass before merging?

@ConradIrwin

This comment has been minimized.

Owner

ConradIrwin commented Jan 23, 2018

Thanks!

@ConradIrwin ConradIrwin merged commit 03e4be4 into ConradIrwin:master Jan 23, 2018

@dmitshur

This comment has been minimized.

Collaborator

dmitshur commented on sfnt/table_feature.go in c7a3e4c Feb 2, 2018

It would be equivalent to write this more simply as:

featureIndices := make([]uint16, lang.FeatureIndexCount)

See S1019 (or https://golang.org/ref/spec#Making_slices_maps_and_channels):

for slices the capacity defaults to the length

This is minor, just wanted to share a simplification opportunity that gosimple found. /cc @bramp

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment