-
Notifications
You must be signed in to change notification settings - Fork 94
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
base: main
Are you sure you want to change the base?
Conversation
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. |
There was a problem hiding this 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
self.pango_context | ||
.list_families() | ||
.iter() | ||
.find(|family| family.name().unwrap().as_str() == family_name); |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(the better_match
function)
Co-authored-by: Colin Rofls <colin@cmyr.net>
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 |
actually weird that it works on other platforms |
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 |
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? |
|
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.
Certainly, especially since |
whoops, I think I got it. Filtering first by name case sensitively and then using the pango |
This seems to have moved all the text... |
|
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! |
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.
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 |
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. |
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: |
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. |
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. |
@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
this exactly matches the code over at We load the fontset using the font description ( The reason pango used to match another font is probably because of other |
fixes #430