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

ttc fonts supported #5210

Open
wants to merge 5 commits into
base: develop
Choose a base branch
from
Open

ttc fonts supported #5210

wants to merge 5 commits into from

Conversation

bon-ami
Copy link

@bon-ami bon-ami commented Oct 18, 2024

theme.CheckFontParsable added
font.ParseTTC replaces font.ParseTTF. The first font of the collection is taken.

Description:

Fixes #4053,#3799,#2761

Checklist:

  • Tests included.
  • Lint and formatter run with no errors.
  • Tests all pass.

Where applicable:

  • Public APIs match existing style and have Since: .

Allen Tse added 2 commits October 18, 2024 11:34
theme.CheckFontParsable added
font.ParseTTC replaces font.ParseTTF. The first font of the collection is taken.
theme.CheckFontParsable added
font.ParseTTC replaces font.ParseTTF. The first font of the collection is taken.
@andydotxyz
Copy link
Member

You do realise that this will parse the entire font collection and then return only the first font? Could be quite a bad performance drop.

Also, why all the nil checks added? Having font rendering need to worry about nil seems bad - because we cannot possible render a string if the font is nil. Unless I misunderstood the change this seems like a bad thing.

@coveralls
Copy link

coveralls commented Oct 18, 2024

Coverage Status

coverage: 59.953% (-0.01%) from 59.964%
when pulling de68f13 on bon-ami:develop
into e88cc1f on fyne-io:develop.

@dweymouth
Copy link
Contributor

Yes, this certainly isn't really solving TTC support. TTC support means loading all the fonts in the collection when needed to display specific glyphs. The point of TTC is usually achieving larger glyph coverage than any single TTF can support on its own

@bon-ami
Copy link
Author

bon-ami commented Oct 18, 2024

@andydotxyz , @dweymouth ,

  1. this is not a final solution. I cannot use current API to fully support ttc. Since I remember seeing the font mechanism/lib is to be switched, I only want to make ttc partially supported.
  2. I want to protect the whole app being crashed by the rendering issue, by not showing the texts. Because I am using a customized theme with fonts, if the font is invalid, I have no API to check that and it will crash my whole app.
  3. So I also added an API to check the validity of a font resource.

as I commented in #4053 (comment),
I think we should provide a temporary change according to #2761 (comment)
as a quick, though not full, support to #3799

@andydotxyz
Copy link
Member

Thanks for this, some thoughts:

  1. this is not a final solution. I cannot use current API to fully support ttc. Since I remember seeing the font mechanism/lib is to be switched, I only want to make ttc partially supported.

The font lib was switched out a couple of releases ago, there is no future plans to change font loading.
If it is a temporary solution it should certainly not add new public APIs - we have to support them forever.

  1. I want to protect the whole app being crashed by the rendering issue, by not showing the texts. Because I am using a customized theme with fonts, if the font is invalid, I have no API to check that and it will crash my whole app.

If there is a crash we should fix that directly - no Fyne API call or render should ever panic.
For the case where you are worried a font is not supported it is possible to query the go-text/typesetting loader directly in your app.

  1. So I also added an API to check the validity of a font resource.

As noted above APIs should not be part of a temporary fix but have clear utility and purpose. I get the feeling that this has been added to avoid a crash - which is not how we do things as the panic itself should be resolved.

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.

4 participants