FIX: skip sub directories when finding fonts on windows #22909

ghost · 2022-04-26T18:36:10Z

add win32 fonts directory check

PR Summary

PR Checklist

Tests and Styling

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

New features are documented, with examples if plot related.
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).
Documentation is sphinx and numpydoc compliant (the docs should build without error).

github-actions

Thank you for opening your first PR into Matplotlib!

If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.

You can also join us on gitter for real-time discussion.

For details on testing, writing docs, and our review process, please see the developer guide

We strive to be a welcoming and open project. Please follow our Code of Conduct.

tacaswell · 2022-04-26T19:11:39Z

Thank you for working on this @Croadden

The test failures are real and caused by these changes because win32FontDirectory uses some windows-only modules. This logic will have to be behind some platfrom gating. I suspect that 👍🏻

if sys.platfrom == 'win32' and ...:

would be enough because and will short circuit and not evaluate the second expression if the first is False.

Could you please also give the PR title and the commit messages more descriptive text (like "FIX: skip sub directories when finding fonts on windows") so that someone who is coming across this issue / commit in the future will have are very quick suggestion as to the motivation for the change. What files you changed and the actually changes is in the diff so the commit message is a chance for you to tell the future readers why you made this change!

ghost · 2022-04-26T19:48:04Z

Damn, was pretty sure I saw OS check :(
My bad, will fix. Sorry, I'm kinda new at github and whole this open source stuff

ghost · 2022-04-26T19:52:43Z

@tacaswell, I was pretty sure I used propper commit naming... Also I'm not able to rename commits without force pushing and rebasing, so maybe lets leave it as is for now? Lesson learnt.

ghost · 2022-04-26T19:53:33Z

Oh, found it! I'm using github desktop and I eddited commit description, but not commit summary. So that's why commit messages is Update font_manager.py instead of add win32 fonts directory check and FIX: skip sub directories when finding fonts on windows

tacaswell · 2022-04-27T17:59:51Z

lib/matplotlib/font_manager.py

-            for filename in filenames
-            if Path(filename).suffix.lower() in extensions]
+    if sys.platform == 'win32' and directory == win32FontDirectory():
+        return [os.path.join(directory, filename) for filename in os.listdir(directory) if os.path.isfile(filename)]


I am very confused why this is not failing the linter as too long of a line.....

Because we explicitly exclude font_manager.py from the line length check

matplotlib/.flake8

Line 60 in 1ab4a53

lib/matplotlib/font_manager.py: E501

I am very confused why this is not failing the linter as too long of a line.....

I'm running flake8 before each commit I make, so I think it's just ignored

Oh, wrote previous comment before you replied to me :)
So should I fix it or not?

tacaswell · 2022-04-27T18:18:23Z

Oh, found it!

I'm glad you sorted it out! The summary (e.g. the first line) is very helpful when skimming the history.

Unfortunately there is no way to update the commit message (which even though the UI shows it as two things is really just 1 block of text) is one of the values that git hashes to compute the SHA of the commit (which while annoying here, it means that someone can not change history without someone noticing!). As changing the message changes the SHA (and hence history) you have to force-push to publish the new commits. Again, git is trying to be helpful here and make sure that you do not accidentally delete history (think of the case where two people are working on the same branch. If both fetch the upstream branch, do some work and commit. The first person to push it works, the second person to push it fails (because if it did work it would effectively delete the commit from the first person). From git's point of view it can not tell "I change a commit and want to update it" from "two people are trying to push completely different work" so push says no, but git push --force-with-lease says "trust me git, I know what I am doing!".

Sorry for the wall of text, hopefully it is more helpful than harmful.

ghost · 2022-04-27T19:27:36Z

Your wall of the text help me so much, thanks! :)

But I made a lot of mistakes here. Sorry for being bad, my git experience is so low (was using SVN only for more than 5 years)
I fixes the issue with naming using rebase and everything seems fine in git and git gui (I have only 2 commits with proper naming here), but history seems broken here, at github. So what should I do? Maybe create another branch, delete this one and make another pull request?

lib/matplotlib/font_manager.py

tacaswell · 2022-04-27T22:51:23Z

@Croadden may I fix it and force-push to your branch?

ghost · 2022-04-28T07:18:13Z

@tacaswell Sure, no problem, it's just simple reformat

timhoffm · 2022-04-28T15:14:50Z

@tacaswell as you go, you can include my suggestion above.

Closes #22859 Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

tacaswell · 2022-04-28T17:36:20Z

Done!

Steps I took:

use the GH ui to commit @timhoffm 's suggestion
checked out the branch locally with the name fix_win32_fonts
git rebase -i back to where this forked from master (I used magit in emacs to set this up)
squashed everything down into one commit via the UI (could have maybe dropped duplicate commits, but squashing and fixing conflicts seemed like it would be faster)
fixed up a conflict as the rebase went
force pushed to your main branch: git push -v --force-with-lease Croadden fix_win32_fonts:refs/heads/main (again actually done through magit)
wrote this note

ghost · 2022-04-28T17:44:13Z

Thanks!

QuLogic · 2022-04-28T18:58:33Z

Thanks @Croadden! Congratulations on your first PR to Matplotlib 🎉 We hope to hear from you again.

ghost · 2022-04-28T19:12:57Z

Will do my best :)
Thanks!

github-actions bot reviewed Apr 26, 2022

View reviewed changes

tacaswell added this to the v3.6.0 milestone Apr 26, 2022

ghost changed the title ~~#22859 bug fix~~ FIX: skip sub directories when finding fonts on windows, bug #22859 Apr 26, 2022

ghost changed the title ~~FIX: skip sub directories when finding fonts on windows, bug #22859~~ FIX: skip sub directories when finding fonts on windows Apr 27, 2022

tacaswell reviewed Apr 27, 2022

View reviewed changes

timhoffm reviewed Apr 27, 2022

View reviewed changes

lib/matplotlib/font_manager.py Outdated Show resolved Hide resolved

tacaswell linked an issue Apr 28, 2022 that may be closed by this pull request

[Bug]: findSystemFonts should not look in subdirectories of C:\Windows\Fonts\ #22859

Closed

skip sub directories when finding fonts on windows

e800616

Closes #22859 Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>

tacaswell approved these changes Apr 28, 2022

View reviewed changes

QuLogic approved these changes Apr 28, 2022

View reviewed changes

QuLogic merged commit 4756fa2 into matplotlib:main Apr 28, 2022

anntzer mentioned this pull request Dec 7, 2022

Fix that font files never pass the test on Win #24641

Merged

almarklein mentioned this pull request Dec 7, 2022

Update font_manager to only use registry on Win #24655

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FIX: skip sub directories when finding fonts on windows #22909

FIX: skip sub directories when finding fonts on windows #22909

ghost commented Apr 26, 2022 •

edited by ghost

Loading

github-actions bot left a comment

tacaswell commented Apr 26, 2022

ghost commented Apr 26, 2022

ghost commented Apr 26, 2022

ghost commented Apr 26, 2022

tacaswell Apr 27, 2022

tacaswell Apr 27, 2022

ghost Apr 27, 2022

ghost Apr 27, 2022

tacaswell commented Apr 27, 2022

ghost commented Apr 27, 2022

tacaswell commented Apr 27, 2022

ghost commented Apr 28, 2022

timhoffm commented Apr 28, 2022

tacaswell commented Apr 28, 2022 •

edited

Loading

ghost commented Apr 28, 2022

QuLogic commented Apr 28, 2022

ghost commented Apr 28, 2022

FIX: skip sub directories when finding fonts on windows #22909

FIX: skip sub directories when finding fonts on windows #22909

Conversation

ghost commented Apr 26, 2022 • edited by ghost Loading

PR Summary

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

tacaswell commented Apr 26, 2022

ghost commented Apr 26, 2022

ghost commented Apr 26, 2022

ghost commented Apr 26, 2022

tacaswell Apr 27, 2022

Choose a reason for hiding this comment

tacaswell Apr 27, 2022

Choose a reason for hiding this comment

ghost Apr 27, 2022

Choose a reason for hiding this comment

ghost Apr 27, 2022

Choose a reason for hiding this comment

tacaswell commented Apr 27, 2022

ghost commented Apr 27, 2022

tacaswell commented Apr 27, 2022

ghost commented Apr 28, 2022

timhoffm commented Apr 28, 2022

tacaswell commented Apr 28, 2022 • edited Loading

ghost commented Apr 28, 2022

QuLogic commented Apr 28, 2022

ghost commented Apr 28, 2022

ghost commented Apr 26, 2022 •

edited by ghost

Loading

tacaswell commented Apr 28, 2022 •

edited

Loading