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

Handle TTCs gracefully #752

Merged
merged 1 commit into from
Jan 9, 2022
Merged

Handle TTCs gracefully #752

merged 1 commit into from
Jan 9, 2022

Conversation

Finii
Copy link
Collaborator

@Finii Finii commented Jan 5, 2022

[why]
When a True Type Collection file (.ttc) is used as font source this is
not handled and just the first file in the collection is processed and
saved. But the user is not informed.

When the target file format is True Type Collection, no file at all is
written.

These are two distinct cases, because you can in fact open a .ttc and
save the first font (patched) when specifying a different extension via
-ext. Or open a normal font and specify ttc as extension i.e. target
file format.

[how]
Check if a collection is to be opened. As we currently have no code to
loop through all fonts (and just the first font is processed) a message
is issued and we exit. Typically a user would want all the fonts and
would have to 'explode' the collection into multiple single font files
beforehand.

Prevent the target to be ttc, as that is not handled in fontforge at
all. To save TTCs a different API function is to be used. Unfortunately
fontforge does not care and just does nothing.

font.generateTtc() would have to be used with ttc extensions...

Anyhow. As the looping through all fonts is missing anyhow, and I feel
the usefulness is very slim, we just prevent silent failures with this
commit.

Requirements / Checklist

What does this Pull Request (PR) do?

How should this be manually tested?

  • Normal patching should not be changed.
  • try fontforge --script font-patcher [...] -ext ttc (shall fail with meaningful message)
  • try fontforge --script font-patcher somefont.tcc (shall fail with meaningful message)
  • try fontforge --script font-patcher somefont.tcc -ext ttf (WOULD work for the first embedded font, now prevented)

Any background context you can provide?

#735

What are the relevant tickets (if any)?

#175
#664 (!!!?)
powerline/fontpatcher#6

We could also wield #6 ... in a next step ;) If you think its needed. The change today would be much smaller than back then I guess. But there are so many PRs waiting ;)

Screenshots (if appropriate or helpful)

[why]
When a True Type Collection file (.ttc) is used as font source this is
not handled and just the first file in the collection is processed and
saved. But the user is not informed.

When the target file format is True Type Collection, no file at all is
written.

These are two distinct cases, because you can in fact open a .ttc and
save the first font (patched) when specifying a different extension via
`-ext`. Or open a normal font and specify `ttc` as extension i.e. target
file format.

[how]
Check if a collection is to be opened. As we currently have no code to
loop through all fonts (and just the first font is processed) a message
is issued and we exit. Typically a user would want all the fonts and
would have to 'explode' the collection into multiple single font files
beforehand.

Prevent the target to be ttc, as that is not handled in fontforge at
all. To save TTCs a different API function is to be used. Unfortunately
fontforge does not care and just does nothing.

font.generateTtc() would have to be used with ttc extensions...

Anyhow. As the looping through all fonts is missing anyhow, and I feel
the usefulness is very slim, we just prevent silent failures with this
commit.

Signed-off-by: Fini Jastrow <[email protected]>
@ryanoasis ryanoasis merged commit 877887c into master Jan 9, 2022
@ryanoasis ryanoasis deleted the bugfix/try-to-save-ttc branch January 9, 2022 19:19
@lilyball
Copy link
Collaborator

lilyball commented Feb 8, 2022

FWIW powerline/fontpatcher#6 is mostly just whitespace. If you ask GitHub to hide whitespace in the diff it just becomes +11 -2. The change itself is fairly simple and can probably be ported to the current script without much difficulty.

@Finii
Copy link
Collaborator Author

Finii commented Feb 9, 2022

But is there interest? Code that is never used will break sooner or later.
I have a lot bought (commercial) fonts and some free ones, but never stumbled over ttc. Maybe because I prefer otf.
Any instances of ttcs?

@Finii
Copy link
Collaborator Author

Finii commented Feb 9, 2022

If you ask GitHub to hide whitespace

Did not know that feature. Nice. (Python with its bloody indent-levels is bad for diffs 😬)
Looks really simple, will give a shoot.

@Finii Finii mentioned this pull request Feb 9, 2022
2 tasks
@Finii
Copy link
Collaborator Author

Finii commented Feb 9, 2022

It's a bit more envolved, ... need to clean up code first 😒
Envy your nicer/more modern naming convention 😸

See #783

LNKLEO pushed a commit to LNKLEO/Nerd that referenced this pull request Nov 24, 2023
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.

3 participants