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

Release v0.18 breaks codepoints #72

Closed
Finii opened this issue Jan 25, 2022 · 9 comments
Closed

Release v0.18 breaks codepoints #72

Finii opened this issue Jan 25, 2022 · 9 comments

Comments

@Finii
Copy link
Collaborator

Finii commented Jan 25, 2022

With release v0.18 the codepoints of the logos in the ttf font have changed. That means, that a person who uses a symbol with v0.17 needs to adapt after updating.

image

This commit f161cf6 removes the file .fontcustom-manifest.json (and prevents re-adding by putting it into .gitignore, so it is not by accident?!). But that file defines which glyph gets which codepoint.
If the file is not existing the glyphs are added in alphabetical filename order.
That will shuffle the logos' codepoints whenever new logos are added.

This has happened in the past already sometimes, but the question is if this is what users of the font file would want.
If you have any document or commandline prompt or other use of the font file and update the font file the contents changes.

Suggestion: Keep codepoints of a certain logo constant and just add new logos in the end (or in vacant spots).

In the whole history of font-logos there has been only 3 of this breaking codepoint re-enumerations:

  • v0.1 -> v0.2 (February 2016, v0.1 was a tagged pre-release, so this might be ok)
  • v0.9 -> v0.10 (Jan 2018, renaming from linux to logos, so this might be ok)
  • v0.17 -> v0.18 (July 2021, ... reason unknown)
@Finii
Copy link
Collaborator Author

Finii commented Jan 25, 2022

Here is the full history. The last glyph is always colored blue.

v0.1 v0.5
v0.2 (shuffle behind Fedora) v0.6
v0.3 v0.7
v0.4

image

v0.8 v0.11
v0.9 v0.12
v0.10 (shuffle to fill gaps? Renaming) v0.13

image

v0.14 v0.16
v0.15 v0.17

image

v0.17 v0.18

image

Finii added a commit to Finii/font-logos that referenced this issue Jan 26, 2022
[why]
When codepoints are changed for the individual logos that should be a
deliberate decision.

[how]
We have now a dedicated file for the codepoint to logo mapping. That
will be used for the font generation: `glyphs.json`.

If new logos show up or exisiting ones are deleted this will change that
dedicated file when `compile.sh` is called and what happened is clearly
visible in the changed git status.

People who add / remove a logo have to run `compile.sh` and afterwards
review the changes in the `glyphs.json` file and include that changes in
their commit.

[how]
This is only slightly different to the formerly used solution: Keep the
`.fontcustom-manifest.json` in git. But that has been removed with
commit f161cf6 for some reason. Maybe because that file is hidden, which
is not so nice.

This PR just preserves the "glyphs" section of the manifest and extracts
it into a dedicated file, that can be added and reviewed.

[note]
Glyph codepoints reverted back to f161cf6^ i.e. v0.17.

Fixes lukas-w#33
Fixes lukas-w#72

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Finii added a commit to Finii/font-logos that referenced this issue Jan 26, 2022
[why]
When codepoints are changed for the individual logos that should be a
deliberate decision.

[how]
We have now a dedicated file for the codepoint to logo mapping. That
will be used for the font generation: `glyphs.json`.

If new logos show up or exisiting ones are deleted this will change that
dedicated file when `compile.sh` is called and what happened is clearly
visible in the changed git status.

People who add / remove a logo have to run `compile.sh` and afterwards
review the changes in the `glyphs.json` file and include that changes in
their commit.

[note]
This is only slightly different to the formerly used solution: Keep the
`.fontcustom-manifest.json` in git. But that has been removed with
commit f161cf6 for some reason. Maybe because that file is hidden, which
is not so nice.

This PR just preserves the "glyphs" section of the manifest and extracts
it into a dedicated file, that can be added and reviewed.

[note]
Glyph codepoints reverted back to `f161cf6^` i.e. 12f3783 (a.k.a. v0.17)

Fixes lukas-w#33
Fixes lukas-w#72

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@lukas-w
Copy link
Owner

lukas-w commented Jul 3, 2022

@Finii Sorry again for taking so long to respond to this. .fontcustom-manifest.json was indeed not removed by accident. Originally this icon font was made for web use so unlike CSS classes, I haven't considered code points a documented interface, partly because as far as I know Font Custom offers no stability guarantee or ability to control the code points allocated.

I agree though that code points should be made persistent and optimally allocated in a specified range for Nerd Fonts (#33). Because of the lack of support for this in Font Custom and because it hasn't seen a commit in almost five years I'd rather look for alternatives to Font Custom. Plus it's becoming increasingly difficult to get it running on modern systems (I switched to building in Docker because of this). With a quick search I found gulp-iconfont, fantasticon and itgalaxy/webfont but I don't know what else is out there. Suggestions welcome :)

@lukas-w lukas-w closed this as completed in 34043a8 Jul 5, 2022
@lukas-w
Copy link
Owner

lukas-w commented Jul 5, 2022

@Finii I noticed that FontCustom is really just a thin wrapper around FontForge so I changed the build to use FontForge directly via 34043a8 which allows us to set code points directly (saved as offsets in icons.tsv) instead of working around FontCustom's missing functionality for this. I also tried to fantasticon & webfont but the output was slightly lower quality. I used code points starting from 0xf300 to account for #33 so this breaks code point compatibility with all previous releases, but to be fair neither code points nor CSS classes were ever guaranteed to be stable as all releases so far have been pre-release (0.x.y) going by Semantic Versioning.

From now on code points will be persistent (except maybe for major version releases) and to indicate this change the new version is released as v1.0.0.

I do feel bad about having to close #75 now because that is an excellent PR, but I've wanted to get rid of FontCustom for a while now. Sorry about the wasted work but more importantly, thank you for the exemplary bug reports and pull request! 🍻

@Finii
Copy link
Collaborator Author

Finii commented Jul 6, 2022

Thank you for fixing this issue 🎉

No need to feel bad to close #75. That was just a workaround for the assumption that you had important reasons to remove the 'config' file. And it was fun to implement. And it always looked bad (i.e. too complicated) - but I was hesitant to suggest fontforge directly, because that is a higher maintenance burden on you. I'm using ff directly for everything ;)

Thank you for the effort and the determination to understand ff scripting.

I will see that we update font-logos in Nerd Fonts asap - which might mean a while. I can generate the PR over there, but Ryan seems to be in a hiatus right now, and the PRs pile up right now. Not too happy with that situation, but what can one do.

Just one note about the codepoints. It still changes the codepoints, additionally to shifting them all up by 0x0200.

Left 0.17, right 1.0.0
image

The order changed 'right/higher-codepoint' of 'Ubuntu invers'. Before it was 'docker', now it is 'Alma', and so on, shuffled. Maybe there was a reason, maybe not 😁

What is missing from the repo is your f'{outputdir}/{fontname}.json' i.e. assets/font-logos.json I assume.
You generate it 👍

Edit: Reword to avoid possible misunderstanding regarding 'right of Ubuntu'
Edit: Remove comment on fontname.json

@Finii
Copy link
Collaborator Author

Finii commented Jul 6, 2022

Just one note about the codepoints. It still changes the codepoints, additionally to shifting them all up by 0x0200.
...
The order changed 'right/higher-codepoint' of 'Ubuntu invers'. Before it was 'docker', now it is 'Alma', and so on, shuffled. Maybe there was a reason, maybe not grin

Those who can read ... :-)

All new icons not included in Nerd Fonts yet are allocated upward of 0xf300.

Sorry, please ignore that comment. That explains it. (Obviously meant is NF 2.1.0 and not 2.2.0RC, which is ok!)

Edit: Add parenthesized comment about NF versions

@Finii
Copy link
Collaborator Author

Finii commented Jul 6, 2022

Maybe you want to add some metadata to the font file.

Like for example the version number (at least).

Here is a code snippet to do that, too lazy for a PR:

version = "1.0.0"
font.sfntRevision = None # Auto-set (refreshed) by fontforge
font.appendSFNTName("English (US)", "Version", version)
font.version = version
font.generate(...)

But then, I also do not see where you set the copyright string 🤔

Here you can see the various version numbers:

image

image


(My problem is also that this Javascript stuff drives me crazy ;) Installing nodejs seems to be not enough (but more dependencies are not given in the README):
image

@Finii
Copy link
Collaborator Author

Finii commented Jul 7, 2022

Incorporated the new release into Nerd Fonts (see MR above). Thank you again!

@lukas-w
Copy link
Owner

lukas-w commented Jul 8, 2022

Like for example the version number (at least).

Here is a code snippet to do that [...]

Done via c3955d3, thanks. 👍

But then, I also do not see where you set the copyright string 🤔

I don't. I don't feel comfortable claiming copyright on something that consists entirely of other people's work 😉

Installing nodejs seems to be not enough

Right, npm install was missing from the README. Fixed via 83c0317.

@Finii
Copy link
Collaborator Author

Finii commented Jul 8, 2022

I'm too stupid...:

image

Ah, I'm on Ubuntu 22.04, that comes with ages old node.js 12.x ... manually upgrade to 14.20 and it works 👍

Ok... this is the result:

image

You see, in your code you did not add the line

font.sfntRevision = None # Auto-set (refreshed) by fontforge

Which is needed to have that information auto-updated. You could of course also set it manually (I guess, I never did).

And the copyright seems to be autogenerated if there is not entry.

So maybe

font.copyright = None

Well, maybe this

 for iconId, icon in fontData['icons'].items():
        addIcon(iconId, icon)
 
+font.sfntRevision = None # Auto-set (refreshed) by fontforge
 font.appendSFNTName("English (US)", "Version", fontData['version']['string'])
+font.appendSFNTName("English (US)", "Vendor URL", "https://github.com/lukas-w/font-logos")
 font.version = fontData['version']['string']
+font.copyright = None
 
 font.generate(os.path.join(outputdir, fontname + '.ttf'))
 font.generate(os.path.join(outputdir, fontname + '.woff'))

That's about it.
Fonts are really badly interestingly designed and have the same information in different places. You can see all the beauty here https://fontforge.org/docs/scripting/python/fontforge.html#font (which sometimes does not help if you do not know what you're doing). The SFNTNames are a complete universe on their own with lots of additional optional fields...

I need to stop :-)

Finii added a commit that referenced this issue Nov 20, 2023
[why]
The copyright in the font files are unspecified at the moment and
Fontforge fills in something deduced from the current user.

[how]
Specify a concrete copyright value that matches the values the previous
releases had.

[note]
#72 (comment)

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Finii added a commit that referenced this issue Nov 20, 2023
[why]
The copyright in the font files are unspecified at the moment and
Fontforge fills in something deduced from the current user.

[how]
Specify a concrete copyright value that matches the values the previous
releases had.

[note]
#72 (comment)

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
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 a pull request may close this issue.

2 participants