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

Improve and extend Namelist file format documentation. #642

Closed
wants to merge 12 commits into from

Conversation

graphicore
Copy link
Collaborator

This is a living PR for discussion. Don't merge it until we agree that it is good.

@davelab6
Copy link
Member

Great work! Please assign to me when ready to merge

@davelab6
Copy link
Member

@graphicore is this ready to merge?

@graphicore
Copy link
Collaborator Author

Great work! Please assign to me when ready to merge

Uh, I missed this. Thanks.

I'm rather happy with this. The biggest three issues for me right now are:

  1. The tools/encodings/README.md has a passage at the top "TODO: clarify: Typically all the Unicodes in each file are in each font." That sentence is hard for me to understand, it seems that it's just not true. Do I miss something?
  2. Since you seem to agree with what I did to the Namelist format, especially the #$ include headers. I should remove all parts in tools/encodings/README.md that say it's a "proposal". Also, I'd clean it up once more in general, in case I see something more
  3. Wrong includes could also be fixed later, but in general, are the dependencies via the #$ include headers declared correctly? At the moment, the newly added Greek encodings don't have these headers, so that should be fixed. And the "optional" encodings don't have these headers. The latter usually don't have unicode codepoints, so it's not important for the CLDR/language detection related stuff.

Other things can be done in a subsequent PR, like tasks derrived from #660 and #624

@graphicore
Copy link
Collaborator Author

I'll tackle the remarks above later today, as far as I can.

@graphicore
Copy link
Collaborator Author

graphicore commented Feb 21, 2017

What I did:

  1. just removed the "TODO: clarify:". This must have made sense before, so maybe I'm reading it wrongly. Feel free to comment/ask for a change on this one before you merge.
  2. I had another go on that README.md file, I think it's OK now.
  3. I added include rules to Greek core, plus and expert. The rest can be dealt with later, I'll have some questions anyways.

@graphicore
Copy link
Collaborator Author

I just removed including GF-latin-plus_unique-glyphs.nam into GF-greek-core.nam because of: graphicore/specimenTools#19

Note that we should probably create GF-latin-core.nam because that should be included by GF-greek-core.nam.

I'd rather do this and other changes on the encodings in a separate PR which would however depend on this PR. Or I can just add to this PR. What should I do?

@graphicore
Copy link
Collaborator Author

closed by merging #685

@graphicore graphicore closed this Mar 16, 2017
@davelab6
Copy link
Member

Thanks!

"TODO: clarify: Typically all the Unicodes in each file are in each font." That sentence is hard for me to understand, it seems that it's just not true. Do I miss something?

I guess that there are a couple of things to take from that.

  1. all the unicode characters specified in a glyph set definition should be available as drawn glyphs in each font that meets that definition, and

  2. all the fonts in a family should have the same unicode encoded glyphs. There is a FB issue to track that.

@graphicore
Copy link
Collaborator Author

  1. all the unicode characters specified in a glyph set definition should be available as drawn glyphs in each font that meets that definition, and

I think the key here is saying should, we'll have to update fonts from time to time to keep the glyph set definitions and the fonts in sync.

@davelab6, somehow related, could you have look at this please? graphicore/specimenTools#20

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