linebender / piet

An abstraction for 2D graphics.
Apache License 2.0
1.24k stars 93 forks source link

Implement checking if the font familly exists in pango #455

Open JAicewizard opened 3 years ago

JAicewizard commented 3 years ago

fixes #430

JAicewizard commented 3 years ago

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.

JAicewizard commented 3 years ago

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 commented 3 years ago

actually weird that it works on other platforms

cmyr commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago
JAicewizard commented 3 years ago

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 commented 3 years ago

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

JAicewizard commented 3 years ago

This seems to have moved all the text...

JAicewizard commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

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 commented 3 years ago

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 commented 3 years ago

@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"?

JAicewizard commented 3 years ago

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.