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

Use local copy of fonts #4606

Merged
merged 5 commits into from
Aug 3, 2022
Merged

Use local copy of fonts #4606

merged 5 commits into from
Aug 3, 2022

Conversation

pnorman
Copy link
Collaborator

@pnorman pnorman commented Jul 16, 2022

Fixes #4602
Fixes #4490
Fixes #4520
Fixes #4610

This moves to downloading a copy of the fonts locally, allowing more recent versions to be used, guarantee coverage, and eliminate fallback fonts for old systems. In addition the CJK font is re-ordered with the Arabic font to stop overwriting the design of commonly used signs in other fonts.

By having a more recent copy, it allows the number of scripts with bold support to be significantly improved.

The total download size is 98MB

image

pnorman added 3 commits July 15, 2022 20:56
By downloading the fonts, we can use more recent versions of the fonts,
and fonts that are not currently packaged.
Like Arabic, we don't want this font to redefine the rendering
of common symbols.
This was a fallback font, and is no longer needed.
@pnorman
Copy link
Collaborator Author

pnorman commented Jul 16, 2022

I don't have a docker setup on my system, so the docker change could use testing.

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

Not yet tested but wondering:

  • should we introduce a dependency on running shell scripts and curl when we use python for all other scripting so far and the necessary means to do this in python are already used in get-external-data.py so this would not introduce any new dependencies?
  • do we need to maintain two separate font lists (in mss and for the download) or could we combine that into one?

INSTALL.md Show resolved Hide resolved
@pnorman
Copy link
Collaborator Author

pnorman commented Jul 16, 2022

should we introduce a dependency on running shell scripts and curl when we use python for all other scripting so far and the necessary means to do this in python are already used in get-external-data.py so this would not introduce any new dependencies?

I looked at this but it was a lot of work to reimplement curl -z properly.

do we need to maintain two separate font lists (in mss and for the download) or could we combine that into one?

I considered this, but ruled it out. We have a specific ordering for some fonts, exceptions, and mapping between file name and font name isn't easy.

@mmd-osm
Copy link

mmd-osm commented Jul 18, 2022

https://github.com/notofonts/noto-fonts mentions:

This repository is in the process of being migrated.

New versions of Noto fonts will soon be available at https://notofonts.github.io/

Has this been considered already? My impressionwas that the script is still using the old location, but I may be misreading things here.

@pnorman
Copy link
Collaborator Author

pnorman commented Jul 18, 2022

https://github.com/notofonts/noto-fonts mentions:

This repository is in the process of being migrated.

New versions of Noto fonts will soon be available at https://notofonts.github.io/

The move is still in progress. If you look at their commit log they're adding more fonts to the new site each day. It shouldn't be much work to change the URL when all the fonts are at the new location.

@pnorman pnorman dismissed imagico’s stale review July 27, 2022 21:43

Based on mis-reading of diff

@pnorman pnorman mentioned this pull request Jul 27, 2022
Copy link
Collaborator

@sommerluk sommerluk left a comment

Choose a reason for hiding this comment

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

Looks good.

While this still does not guarantee that all users have exactly the same font version (because of different moments when doing the font download), it is a big step forward.

Also, the better support for bold fonts is great, and of course that we do not have any longer the warnings for missing fonts.

Moreover, removing the fallback fonts is good. Especially, DejaVu appeared in the regular font list and the bold font list, which made things complicate.

I've made a PR correcting a bit the documentation about Arabic.

@pnorman pnorman merged commit d60d4b0 into gravitystorm:master Aug 3, 2022
@pnorman
Copy link
Collaborator Author

pnorman commented Aug 3, 2022

I've made a PR correcting a bit the documentation about Arabic.

Note: I don't see this PR

@sommerluk
Copy link
Collaborator

Note: I don't see this PR

See here: pnorman#8

I've rebased it and made a PR directly to this repository now: #4634

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants