Skip to content

MNT: print fontname in missing glyph warning #26989

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 27, 2023
Merged

MNT: print fontname in missing glyph warning #26989

merged 2 commits into from
Oct 27, 2023

Conversation

story645
Copy link
Member

@story645 story645 commented Oct 3, 2023 •
edited
Loading

PR summary

Modified the "missing glyph" warning to also print out the name of the font the glyph is missing from. This is mostly to help me debug why #26854 is failing on CI but working locally but there have been times where I've gotten this error and would have found it helpful to have the added context.

second commit adds a :filter-warning: option to the ..plot directive spun out into #27076

PR checklist

@story645 story645 changed the title print fontname in missing glyph warning MNT: print fontname in missing glyph warning Oct 3, 2023
@tacaswell
Copy link
Member

How does this interact with the fallback list? Presumable we would want to show all of the fonts that were considered?

@story645
Copy link
Member Author

story645 commented Oct 4, 2023 •
edited
Loading

How does this interact with the fallback list? Presumable we would want to show all of the fonts that were considered?

Currently fallback suppresses the missing font and if fallback doesn't work only prints the first font

eta: I think b/c it's marked as a flag once and load_char_with_fallaback is kinda recursive.

matplotlib/src/ft2font.cpp

Lines 514 to 518 in 21ce592

bool was_found = load_char_with_fallback(ft_object_with_glyph, glyph_index, glyphs,
char_to_font, glyph_to_font, codepoints[n], flags,
charcode_error, glyph_error, false);
if (!was_found) {
ft_glyph_warn((FT_ULong)codepoints[n]);

@story645
Copy link
Member Author

story645 commented Oct 4, 2023

: I think b/c it's marked as a flag once and load_char_with_fallaback is kinda recursive.

Which also means I can't think of a clean way to only print the warnings if the fallback fails w/o maintaining a separate stack of warnings & I think that rabbit hole is out of scope for this PR.

@story645 story645 force-pushed the warning-fontname branch 2 times, most recently from 22e8b01 to e16b88d Compare October 4, 2023 18:56
@story645
Copy link
Member Author

story645 commented Oct 4, 2023 •
edited
Loading

Added a :filter-warning: to the plot directive so docs can build cleanly, but I'm really unsure that raising the warnings is all that helpful to someone using fallback-presumably they're using it b/c they expect that not all the fonts will work.

@tacaswell
Copy link
Member

When we exhaust the list of fallback fonts, do we have anyway to get back to the top of the list and walk it again collecting all of their names?

@story645
Copy link
Member Author

story645 commented Oct 4, 2023 •
edited
Loading

When we exhaust the list of fallback fonts, do we have anyway to get back to the top of the list and walk it again collecting all of their names?

Kinda maybe? if we exhaust the list, it means i==fallbacks.size() and if that works that's definitely a cleaner solution than the changes I've been making of passing around a cache object...but also currently pybind doesn't want to work for me so I can't build anything.

matplotlib/src/ft2font.cpp

Lines 672 to 680 in 6ba7d5f

for (size_t i = 0; i < fallbacks.size(); ++i) {
bool was_found = fallbacks[i]->load_char_with_fallback(
ft_object_with_glyph, final_glyph_index, parent_glyphs, parent_char_to_font,
parent_glyph_to_font, charcode, flags, charcode_error, glyph_error, override);
if (was_found) {
return true;
}
}
return false;

@story645
Copy link
Member Author

So I was wrong in that it'll miss the first glyph when there's an override.

@story645
Copy link
Member Author

Got this to work by adding an array to F2tFont to store the names of the fonts. Reason is because if you can't find anything, you need to print font + fallback_list

@story645 story645 requested a review from tacaswell October 12, 2023 21:28
@story645 story645 force-pushed the warning-fontname branch 3 times, most recently from c66bfd9 to f9274f3 Compare October 12, 2023 22:02
@story645 story645 mentioned this pull request Oct 12, 2023
Closed
5 tasks
@story645 story645 force-pushed the warning-fontname branch 3 times, most recently from df5d2ae to c719d89 Compare October 15, 2023 01:41
@story645 story645 mentioned this pull request Oct 15, 2023
Open
4 tasks
ksunden
ksunden reviewed Oct 16, 2023
View reviewed changes
ksunden
ksunden reviewed Oct 16, 2023
View reviewed changes
@story645 story645 force-pushed the warning-fontname branch 4 times, most recently from b3dd35d to 1ff3b42 Compare October 17, 2023 15:23
@story645 story645 force-pushed the warning-fontname branch 2 times, most recently from 0a39542 to 296977b Compare October 17, 2023 19:56
tacaswell
tacaswell reviewed Oct 18, 2023
View reviewed changes
tacaswell
tacaswell previously requested changes Oct 18, 2023
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

I think this can be done without adding any new state to the objects.

@story645
Copy link
Member Author

changed to parameter into function

@story645 story645 requested a review from tacaswell October 19, 2023 20:39
@story645 story645 added this to the v3.8.1 milestone Oct 22, 2023
tacaswell
tacaswell reviewed Oct 26, 2023
View reviewed changes
@tacaswell tacaswell dismissed their stale review October 26, 2023 20:30

passing cache through works.

@story645
Copy link
Member Author

story645 commented Oct 27, 2023 •
edited
Loading

one warning w/ all the fonts instead of n warnings w/ one font per @tacaswell's request - and I'm 99% sure that the set can be passed from c++ to python directly but since this is for a string I figure there's no harm in doing the strcat on the C++ side since passing the set over would I think involve copying the set to the python set object. I figure this can get cleaned up as part of the pybind port if it's important enough.

@story645 story645 force-pushed the warning-fontname branch 2 times, most recently from 22c8c97 to 6470890 Compare October 27, 2023 06:00
@story645 @QuLogic @tacaswell
prints font name in missing glyph warning
4ece8ab 
lists all fallback fonts if glyph missing in all fonts
removed ft_get_char_index_or_warn
added a test for otf fonts on pdf + some windows fonts to tests

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Thomas A Caswell <tcaswell@gmail.com>
@ksunden ksunden modified the milestones: v3.8.1, v3.9.0 Oct 27, 2023
@tacaswell
Copy link
Member

one warning w/ all the fonts instead of n warnings w/ one font per @tacaswell's request

It was not a request, I only asked (in a private channel) why many instead of one!

@tacaswell tacaswell merged commit dbebe96 into matplotlib:main Oct 27, 2023
@tacaswell
Copy link
Member

Thank you @story645 !

@story645 story645 deleted the warning-fontname branch October 27, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@ksunden ksunden ksunden approved these changes

@tacaswell tacaswell tacaswell approved these changes

Assignees
No one assigned
Labels
Maintenance topic: text/fonts
Projects
None yet
Milestone
v3.9.0
Development

Successfully merging this pull request may close these issues.
4 participants
@story645 @tacaswell @QuLogic @ksunden