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

Fix variable font instantiation, part two (#1981) #1990

Closed
wants to merge 4 commits into from

Conversation

pmjdebruijn
Copy link
Contributor

(1) Prevents identical wght variations from being doubly embedded
under different named weights (typically normal and bold)

(2) Assign good approximate named weights, which prevents
metadata from being actively misleading.

(1) Prevents identical wght variations from being doubly embedded
    under different named weights (typically normal and bold)

(2) Assign good approximate named weights, which prevents
    metadata from being actively misleading.
Adds italic naming to font metadata where appropriate
Adds stretch naming to font metadata where appropriate
@liZe
Copy link
Member

liZe commented Dec 5, 2023

Hi!

Thanks for this pull request.

The overall logic seems to be good, I’ll just try to simplify a couple of things.

Just one question: how did you define the values used to set stretch?

@pmjdebruijn
Copy link
Contributor Author

Do you mean the range boundary numbers like 56.25?

I took this as a guide:

https://developer.mozilla.org/en-US/docs/Web/CSS/font-stretch#keyword_to_numeric_mapping

And those should be the midway points between them.

@liZe
Copy link
Member

liZe commented Dec 6, 2023

@pmjdebruijn I’ve updated this PR, feedback is welcome!

@pmjdebruijn
Copy link
Contributor Author

You broke something, now I'm getting 'Roboto-Flex-weight=925' in the PDF font list, for example

@liZe
Copy link
Member

liZe commented Dec 28, 2023

You broke something, now I'm getting 'Roboto-Flex-weight=925' in the PDF font list, for example

It happens when you use numbers that don’t correspond to exact weight values.

@pmjdebruijn
Copy link
Contributor Author

I get that, but that sortof defeats the point, my code purposefully matched ranges, so would always get an approximated weight value.

@liZe
Copy link
Member

liZe commented Dec 28, 2023

I get that, but that sortof defeats the point, my code purposefully matched ranges, so would always get an approximated weight value.

Yes, there are pros and cons. We always get different names for different weight values (that’s cool), we get the common names when the value exactly match the weight value (that’s cool), we let Pango name the font for us (that’s cool, that’s their job), but we have numbers instead of approximate weight names (that’s not cool).

(1) Prevents identical wght variations from being doubly embedded under different named weights (typically normal and bold)

That’s still OK.

(2) Assign good approximate named weights, which prevents metadata from being actively misleading.

I prefer to get exact names, that are readable when we use exact values, and are explicit when we don’t. Moreover the code is shorter.

@pmjdebruijn
Copy link
Contributor Author

But, for 'wdth' you are matching ranges aren't you? So this is inconsistent behavior...

@liZe
Copy link
Member

liZe commented Dec 28, 2023

But, for 'wdth' you are matching ranges aren't you? So this is inconsistent behavior...

Yes, it is, because set_stretch only allows constants while set_weight allows intermediate values.

I’ll be happy to remove manual ranges for stretch when the feature is available in Pango. You can open an issue and ask them why their API is inconsistent (there’s probably a good reason) … or just live with it.

@pmjdebruijn
Copy link
Contributor Author

My point was more the other way around though, we can have WeasyPrint be consistent, by approximating weight too...

Anyhow, if you plan to merge the code like this, please remove my name from the related commits, I don't want to be associated with this.

@liZe
Copy link
Member

liZe commented Dec 28, 2023

My point was more the other way around though, we can have WeasyPrint be consistent, by approximating weight too...

I’m sorry, I get your point, but I prefer to use the feature provided by Pango for weight, even if it’s not available for stretch (and for some other properties, where we can only use constants or even booleans).

Anyhow, if you plan to merge the code like this, please remove my name from the related commits, I don't want to be associated with this.

Your name is only in the commits you wrote, I’ve just added commits on top of them. That’s pretty common in open source projects to get your code rewritten at some point, it doesn’t mean that you are associated with awful modifications people will apply on your code 😄.

Do you really want me to take your code and pretend that you never wrote it, because we disagree on details?

@liZe
Copy link
Member

liZe commented Jan 7, 2024

Thanks for the fix and for taking the time to discuss on this issue.

Anyhow, if you plan to merge the code like this, please remove my name from the related commits, I don't want to be associated with this.

I’ve rebased the commits in c3f0307 so that your name doesn’t appear.

@liZe liZe closed this Jan 7, 2024
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.

2 participants