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

font-patcher: Check if glyph source is available (--glyphdownload) #741

Closed
wants to merge 1 commit into from

Conversation

Finii
Copy link
Collaborator

@Finii Finii commented Dec 27, 2021

[why]
When users just download the script (and not the source glyphs) the
script fails with an obscure error message.

[how]
Check if the glyphdir exists at all. If not give a hint to download the
glyphs.

Check if the individual glyph font exists and is readable. Bail out if
not.

Add option --glyphdownload that tries to download missing glyph fonts.

Requirements / Checklist

What does this Pull Request (PR) do?

Add checks before we open a symbol font and give hints to the users if we can not.

How should this be manually tested?

  • Patch a font (shall work as before)
  • Remove one (needed) symbol font, patch a font again (errors out)
  • Remove complete src/glyphs/ (or use --glyphdir), patch a font again (errors out)

The two bottom cases a meaningful message should be printed.

  • Repeat the 3 tests but specify --glyphdownload. (all shall work)

Any background context you can provide?

For example #659 #662 #495

What are the relevant tickets (if any)?

Screenshots (if appropriate or helpful)

image

Edit: Add --glyphdownload as that has been added as 2nd commit to the PR

@Finii
Copy link
Collaborator Author

Finii commented Dec 29, 2021

Not sure if we want to go that route, but it is requested a lot it seems...:

Autodownload...

image

@Finii
Copy link
Collaborator Author

Finii commented Dec 29, 2021

Two notes:

  • Of course the first commit should be merged regardless
  • The second commit is kind of optional

If we go the route with the second commit there might be problems of font-patcher version versus actual repo version - when the glyph paths change. So in reality we should download the repo-glyph stuff from the version (and not MASTER) that the font-patcher actually is. But this is not possible at the moment because the versions are not increased even when the directory structure is changed. This might need more thinking and/or ideas. At least maybe a warning in the code when a download fails, that one needs to use the font-patcher HEAD.

@Finii Finii changed the title font-patcher: Check if glyph source is available font-patcher: Check if glyph source is available (--glyphdownload) Jan 13, 2022
Finii added a commit that referenced this pull request Feb 6, 2022
[why]
People might want to use the font-patcher with just the one script file.
The error message does not help them to understand the problem.

[how]
Require the modules only if the user wants to use it (i.e. --makegroups).
Give the expected path in the error message.

We could also download the missing files instead, similar to #741
But that PR did not get any feedback yet, so I do not know if this is
something we want.

Anyhow, the fetching of missing parts should then be unified for both
usecases (i.e. Fontname* and src/glyphs).

And then, there is font-patcher.zip (which needs to be adapted), maybe
that is the way to go.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii mentioned this pull request Apr 9, 2022
2 tasks
Finii added a commit that referenced this pull request May 13, 2022
[why]
People might want to use the font-patcher with just the one script file.
The error message does not help them to understand the problem.

[how]
Require the modules only if the user wants to use it (i.e. --makegroups).
Give the expected path in the error message.

We could also download the missing files instead, similar to #741
But that PR did not get any feedback yet, so I do not know if this is
something we want.

Anyhow, the fetching of missing parts should then be unified for both
usecases (i.e. Fontname* and src/glyphs).

And then, there is font-patcher.zip (which needs to be adapted), maybe
that is the way to go.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii mentioned this pull request Jul 13, 2022
3 tasks
Finii added a commit that referenced this pull request Aug 20, 2022
[why]
People might want to use the font-patcher with just the one script file.
The error message does not help them to understand the problem.

[how]
Require the modules only if the user wants to use it (i.e. --makegroups).
Give the expected path in the error message.

We could also download the missing files instead, similar to #741
But that PR did not get any feedback yet, so I do not know if this is
something we want.

Anyhow, the fetching of missing parts should then be unified for both
usecases (i.e. Fontname* and src/glyphs).

And then, there is font-patcher.zip (which needs to be adapted), maybe
that is the way to go.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
Finii added a commit that referenced this pull request Aug 22, 2022
[why]
People might want to use the font-patcher with just the one script file.
The error message does not help them to understand the problem.

[how]
Require the modules only if the user wants to use it (i.e. --makegroups).
Give the expected path in the error message.

We could also download the missing files instead, similar to #741
But that PR did not get any feedback yet, so I do not know if this is
something we want.

Anyhow, the fetching of missing parts should then be unified for both
usecases (i.e. Fontname* and src/glyphs).

And then, there is font-patcher.zip (which needs to be adapted), maybe
that is the way to go.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii added this to the v2.3.0 milestone Aug 22, 2022
Finii added a commit that referenced this pull request Sep 7, 2022
[why]
When users just download the script (and not the source glyphs) the
script fails with an obscure error message.

[how]
Check if the glyphdir exists at all. If not give a hint to download the
glyphs.

Check if the individual glyph font exists and is readable. Bail out if
not.

[note]
Cherry picked, was part of #741

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii force-pushed the feature/check-glyph-file-existing branch from d09e346 to 3dad8c1 Compare September 7, 2022 07:55
@Finii
Copy link
Collaborator Author

Finii commented Sep 7, 2022

Rebase on master, force push.

One of the original commits is already in master.

@Finii
Copy link
Collaborator Author

Finii commented Sep 7, 2022

All readme files need to be corrected 🛑

@Finii
Copy link
Collaborator Author

Finii commented Sep 30, 2022

Does this download the --makegroups stuff also? I guess not. Would that be beneficial, or just disable the feature? Who uses that feature?

[why]
People often just download the font-patcher script and then the patching
fails because the symbol glyphs are missing.

A typical comment is that a complete git clone is needed in that case,
which is rediculous bandwidth. Just downloading the needed files seems
rather complicated.

[how]
Add option --glyphdownload that (tries to) download just the needed
glyph files. This is just bare basic stuff.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii force-pushed the feature/check-glyph-file-existing branch from 3dad8c1 to 8c16442 Compare September 30, 2022 10:48
@Finii
Copy link
Collaborator Author

Finii commented Jan 4, 2023

I believe we should promote the FontPatcher.zip instead.
This would need to keep a parallel set of sub-files as for the zip generation.
A lot of maintenance burden.

@Finii Finii closed this Jan 4, 2023
LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
[why]
People might want to use the font-patcher with just the one script file.
The error message does not help them to understand the problem.

[how]
Require the modules only if the user wants to use it (i.e. --makegroups).
Give the expected path in the error message.

We could also download the missing files instead, similar to ryanoasis#741
But that PR did not get any feedback yet, so I do not know if this is
something we want.

Anyhow, the fetching of missing parts should then be unified for both
usecases (i.e. Fontname* and src/glyphs).

And then, there is font-patcher.zip (which needs to be adapted), maybe
that is the way to go.

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
[why]
When users just download the script (and not the source glyphs) the
script fails with an obscure error message.

[how]
Check if the glyphdir exists at all. If not give a hint to download the
glyphs.

Check if the individual glyph font exists and is readable. Bail out if
not.

[note]
Cherry picked, was part of ryanoasis#741

Signed-off-by: Fini Jastrow <ulf.fini.jastrow@desy.de>
@Finii Finii deleted the feature/check-glyph-file-existing branch March 16, 2024 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant