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

Implement checking if the font familly exists in pango #455

Open
wants to merge 19 commits into
base: master
Choose a base branch
from

Conversation

JAicewizard
Copy link
Contributor

fixes #430

@JAicewizard
Copy link
Contributor Author

I am not sure if the snapshots test this function as well. Testing on druid is a bit of an issue atm since the cairo versioning mismatch mismatch.

Copy link
Member

@cmyr cmyr left a comment

Choose a reason for hiding this comment

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

Cool, this is a welcome fixup. I have a few notes inline. :)

piet-cairo/src/text.rs Outdated Show resolved Hide resolved
self.pango_context
.list_families()
.iter()
.find(|family| family.name().unwrap().as_str() == family_name);
Copy link
Member

Choose a reason for hiding this comment

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

As a note: a fancier version of this might do things like return Helvetica Neue for Helvetica if Helvetica isn't present, and it might also want to be case-insensitive?

Additionally, it looks like we aren't using this check? If we don't find the font we should return None.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Additionally, it looks like we aren't using this check?

What check do you mean? I can add the Helvetica -> Helvetica Neue check if thats what you mean.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean is that we're looking at the families to see if family_name exists, but we're still always returning Some from this function.

We may also want to keep around a HashSet of family names so that we aren't doing a linear search each time, but that can be future work.

Copy link
Member

Choose a reason for hiding this comment

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

Just for fun here are some useful guidelines on font matching: https://drafts.csswg.org/css-fonts-3/#font-matching-algorithm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What I mean is that we're looking at the families to see if family_name exists, but we're still always returning Some from this function.

I feel really stupid now. I just noticed this as well while looking at the code again.

As for the matching, I am honestly not sure how I would go about implementing that. Stripping away all the stuff we dont have/need, im left with the only restriction being "a case insensitive match". it should be easy to match case insensitively, but that doesnt solve the Helvetica -> Helvetica Neue match you wanted. I could check for substrings, I think thats easiest. It wont match the other way around, but we could check vise-versa as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There exists a function in pango for matching, I just use that.

Copy link
Member

Choose a reason for hiding this comment

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

Is this a change you're going to make, or is it in the current version? I didn't see it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I currently made a change so that it uses the pango matching function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(the better_match function)

@JAicewizard
Copy link
Contributor Author

The panic is due to the font not being present. Before if the font was invalid pango would just ignore the attribute, but the example doesnt handle this. Should I fix the example with a .unwrap_or(FontFamily::MONOSPACE) like in picture 5?

@JAicewizard
Copy link
Contributor Author

actually weird that it works on other platforms

@cmyr
Copy link
Member

cmyr commented Jul 27, 2021

This is an included font on the other platforms. What is confusing to me is that if you look at the current cairo snapshot, it does seem to be using some font that looks like courier? https://github.com/linebender/piet-snapshots/blob/master/cairo/cairo-test-12.png

@JAicewizard
Copy link
Contributor Author

its a bit hard to debug since I dont have the exact environment of the CI. Maybe pango does a better/different matching internally that we are not aware of?

I can add some print statements and just run them through CI every time, see where it goes wrong?

@cmyr
Copy link
Member

cmyr commented Jul 27, 2021

  • there is certainly something weird going on; perhaps when it is given a font it doesn't recognize, pango falls back to trying to find the best match?

  • have you tried using https://docs.gtk.org/Pango/vfunc.FontMap.get_family.html? That may be better than doing this linear scan on our own.

  • let's not worry about the "Helvetica" vs "Helvetica Neue" problem for now, but we should be matching lowercase names.

  • if get_family does not work, then I would suggest the following: when we create the PangoContext, we should call list_families, and then create a HashSet of lowercased family names. we can store this (behind a shared reference) in the CairoText object; and then in font_family we check if the lowercased version of the provided string is in this set. When we load additional fonts, we will have to make sure that we also add them to this set.

@JAicewizard
Copy link
Contributor Author

have you tried using https://docs.gtk.org/Pango/vfunc.FontMap.get_family.html? That may be better than doing this linear scan on our own.

I looked at that, however it is described as "Gets a font family by name." I assumed this would have to be a direct match.

there is certainly something weird going on; perhaps when it is given a font it doesn't recognize, pango falls back to trying to find the best match?

Certainly, especially since best_match does exactly that, it finds the best match. It cant be the case that it selects none, so the list from pango must be none, after we apply filters.

@JAicewizard
Copy link
Contributor Author

whoops, I think I got it. Filtering first by name case sensitively and then using the pango best_match wont work well together :)

@JAicewizard
Copy link
Contributor Author

This seems to have moved all the text...

@JAicewizard
Copy link
Contributor Author

JAicewizard commented Jul 27, 2021

fontmap.family is too new for the latest ubuntu. Now does ubuntu like to stay behind a bit, but that does mean we really cant force that on devs. (or test it in CI)

@JAicewizard
Copy link
Contributor Author

I really dont know how pango gits to its font before. It doesnt use its default, it cant use the invalid font we specified, what does it use?? who knows!

@JAicewizard
Copy link
Contributor Author

JAicewizard commented Jul 28, 2021

I have come to the conclusion that we cant properly match with pango. So the current implementation is the best I think it will ever be. If no font is found that matches the by name (case insensitively, non-exact) it returns none, and thus the test fails. The test is wrong, it should handle None values properly.

if get_family does not work, then I would suggest the following: when we create the PangoContext, we should call list_families, and then create a HashSet of lowercased family names. we can store this (behind a shared reference) in the CairoText object; and then in font_family we check if the lowercased version of the provided string is in this set. When we load additional fonts, we will have to make sure that we also add them to this set.
I think it might be better to store a matches, just like on macos.

Sorry for the previous messages, it was really frustrating because it doesnt really work how you expect it to work, and getting it to reproduce the pango internal behaviour seemed impossible.

edit: (@cmyr) I accidentally edited this instead of replying, I think I have restored the original version

@cmyr
Copy link
Member

cmyr commented Jul 28, 2021

Sorry for the previous messages, it was really frustrating because it doesnt really work how you expect it to work, and getting it to reproduce the pango internal behaviour seemed impossible.

Nothing here is magic. Pango is open source. If you are motivated, you can figure this out, although it may be challenging, and it certainly isn't necessary for this PR.

@JAicewizard
Copy link
Contributor Author

JAicewizard commented Jul 28, 2021

This is how pango does it, using the default context font as fallback. Still doesnt work however. I also cant query what pango ended up selecting, which is even more frustrating.

There is 2 options rn:
1: When no suitable font is found, use the context default as a fallback when available, and return that. Accept image changes, and update them.
2: Return none, and fix the test. This might also involves updating the images with the fonts.

@cmyr
Copy link
Member

cmyr commented Aug 16, 2021

I don't have the bandwidth to really mentor this issue right now, but for posterity I think the ideal solution:

  • allocates a single lookup table once of lowercase family names
  • updates this table if custom fonts are loaded (if this is supported)
  • does a case insensitive match of the incoming family name

I am curious about pango's current fallback behaviour. In particular, it seems like the font that we get back when we ask for Courier is a monospace font, and I don't believe this is the case if we ask for the system default.

If this is true (and I may be totally wrong) then I would be interested in knowing why this happens. That will involve either using a debugger or going and reading the pango source.

I would be happy to merge a patch that ticks these boxes, and separately I would be happy to hear an explanation for the pango behaviour.

I'm going to release 0.5 soon, but this is not a breaking change and can be added in future patch release.

@JAicewizard
Copy link
Contributor Author

I would be happy to hear an explanation for the pango behaviour.

And so would I :)

It is required for this patch to match the fallback font with pango?

I stared at the pango source code for hours, and sadly the current fallback in this patch matches what I could find in the source. I will see if I might be able to debug the C source code somehow.

@cmyr
Copy link
Member

cmyr commented Aug 16, 2021

@JAicewizard a good first step for thinking about the pango behaviour: what is the behaviour? If you ask for "Times New Roman" (assuming this font is not present) do you get a different font from pango than if you ask for "Courier"?

This ignored char matching, language, and other attributes
@JAicewizard
Copy link
Contributor Author

this exactly matches the code over at pango_context.c:itemize_state_process_run which is used to select fonts for the characters.

We load the fontset using the font description (itemize_state_update_for_new_run: state->current_fonts = pango_font_map_load_fontset (state->context->font_map, state->context, is_emoji ? state->emoji_font_desc : state->font_desc, state->derived_lang);) (this is done earlier, not in itemize_state_process_run)
And then call do what get_font does but without the character matching: pango_fontset_foreach (state->current_fonts, get_font_foreach, &info) (get_font_foreach is just a function that sets the font to the the first font with the required char, but we dont need to match a char per-see).

The reason pango used to match another font is probably because of other FontDescription propperties that we cant set because we only have the name.

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.

impl Text::font_family for cairo
2 participants