Skip to content

Change win32InstalledFonts return value #12213

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 1 commit into from
Sep 23, 2018
Merged

Change win32InstalledFonts return value #12213

merged 1 commit into from
Sep 23, 2018

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Sep 22, 2018 •
edited
Loading

PR Summary

In the context of #12173 it was noted that matplotlib.font_manager.win32InstalledFonts() returned None if no fonts are found. This is inconsistent with the docs which claims that a list of fonts is returned. This PR changes the return value to an empty list, which is a more natural choice and is more convenient than the None value. If the value can be None, we have to explicitly check before doing any list-operations on the return value. An empty list can be handled more gracefully.

Returning an empty list is also the behavior of OSXInstalledFonts().

This is in principle a breaking API change. However, I think it's ok to document it and apply it right away.

Note: this fixes one of the exceptions in #12173:

  File "C:\Python\Python35\lib\site-packages\matplotlib\font_manager.py", line 264, in findSystemFonts
    fontfiles.update(win32InstalledFonts(fontext=fontext))
TypeError: 'NoneType' object is not iterable

PR Checklist

  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@timhoffm timhoffm added this to the v3.0.x milestone Sep 22, 2018
@Kojoley
Copy link
Member

Kojoley commented Sep 22, 2018

There is already #12174, however it did not add an api change entry.

@timhoffm
Copy link
Member Author

Oh, didn't see #12174. The discussion there has gone a bit off-topic as well.

Anyway, we should add an API change note. While it's unlikely that someone will actually use that function and have a problem with the change, it's still a function that appears in the official docs.

@Kojoley Kojoley mentioned this pull request Sep 22, 2018
Closed
@timhoffm
Copy link
Member Author

Taking up the solution proposed by @Kojoley in #12173 (comment)

@tacaswell tacaswell merged commit 927c920 into matplotlib:master Sep 23, 2018
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Sep 23, 2018
@tacaswell
Backport PR matplotlib#12213: Change win32InstalledFonts return value
600dc27
@meeseeksmachine meeseeksmachine mentioned this pull request Sep 23, 2018
Merged
@timhoffm timhoffm deleted the win32InstalledFonts branch September 23, 2018 13:51
@Kojoley Kojoley mentioned this pull request Sep 23, 2018
Closed
6 tasks
timhoffm pushed a commit that referenced this pull request Sep 23, 2018
@meeseeksmachine @timhoffm
Backport PR #12213: Change win32InstalledFonts return value (#12230)
c8c6311
@Lyghtning
Copy link

I don't know if this is allowed or not, I'm a complete novice when it comes to Python, and libraries, imports. What I can say is I had this problem, ended up here after having to do an environment reload which updated my matplotlib.

You guys are amazing, checked out the file update, made the modifications, back up and running.
Thank you for being such contributor to newbies like me. Thank you seriously.

@sebbosafened
Copy link

sebbosafened commented Sep 26, 2018 •
edited
Loading

The solution proposed by @Kojoley in #12173 did not solve the issue for me, I still get

 File "C:\Users\thisuser\AppData\Local\Programs\Python\Python35\lib\site-packages\matplotlib\font_manager.py", line 264, in findSystemFonts
    fontfiles.update(win32InstalledFonts(fontext=fontext))
TypeError: 'NoneType' object is not iterable

@Kojoley
Copy link
Member

Kojoley commented Sep 26, 2018

@sebbosafened Hm, I do not know how it is even possible to get a None value after the PR.

line 264

Please, check twice, the line number should be different.

matplotlib/lib/matplotlib/font_manager.py

Line 261 in deae70a

fontfiles.update(win32InstalledFonts(fontext=fontext))

@sebbosafened
Copy link

sebbosafened commented Sep 27, 2018 •
edited
Loading

I copied everything from your version of font_manager.py (

matplotlib/lib/matplotlib/font_manager.py

Line 261 in deae70a

fontfiles.update(win32InstalledFonts(fontext=fontext))
) to my local font_manager.py (after backing up my local version that I had already edited - with obvious mistakes - according to yourt post @Kojoley on 23rd september, 00:16 CEST) and now it works. Thank you a lot.

@zunction
Copy link

I copied everything from your version of font_manager.py (

matplotlib/lib/matplotlib/font_manager.py

Line 261 in deae70a

fontfiles.update(win32InstalledFonts(fontext=fontext))
) to my local font_manager.py (after backing up my local version that I had already edited - with obvious mistakes - according to yourt post @Kojoley on 23rd september, 00:16 CEST) and now it works. Thank you a lot.

Somehow the contents of font_manager.py have to be manually updated like what @sebbosafened did. I also managed to get it to work after updating my local version of font_manager.py to the version here. Thanks a lot for solving the problem 👍

@AndrewPanB
Copy link

I copied everything from your version of font_manager.py (
matplotlib/lib/matplotlib/font_manager.py
Line 261 in deae70a
fontfiles.update(win32InstalledFonts(fontext=fontext))
) to my local font_manager.py (after backing up my local version that I had already edited - with obvious mistakes - according to yourt post @Kojoley on 23rd september, 00:16 CEST) and now it works. Thank you a lot.

Somehow the contents of font_manager.py have to be manually updated like what @sebbosafened did. I also managed to get it to work after updating my local version of font_manager.py to the version here. Thanks a lot for solving the problem

I change "if sys.platform == 'win32:" to if sys.platform == 'win64': in matplotlib/lib/matplotlib/font_manager.py
It works.

@Kojoley
Copy link
Member

Kojoley commented Oct 9, 2018

I change "if sys.platform == 'win32:" to if sys.platform == 'win64': in matplotlib/lib/matplotlib/font_manager.py

It cannot has that value https://mail.python.org/pipermail/patches/2000-May/000648.html
https://github.com/python/cpython/blob/c510c6b8b60f211793e0b84c317ea6974e8a6153/PC/pyconfig.h#L297-L301

@jckling
Copy link

jckling commented Oct 10, 2018

I copied everything from your version of font_manager.py (
matplotlib/lib/matplotlib/font_manager.py
Line 261 in deae70a
fontfiles.update(win32InstalledFonts(fontext=fontext))
) to my local font_manager.py (after backing up my local version that I had already edited - with obvious mistakes - according to yourt post @Kojoley on 23rd september, 00:16 CEST) and now it works. Thank you a lot.

Somehow the contents of font_manager.py have to be manually updated like what @sebbosafened did. I also managed to get it to work after updating my local version of font_manager.py to the version here. Thanks a lot for solving the problem

I change "if sys.platform == 'win32:" to if sys.platform == 'win64': in matplotlib/lib/matplotlib/font_manager.py
It works.

It works for me! Thanks a lot 🎉🎉🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Kojoley Kojoley Kojoley approved these changes

Assignees
No one assigned
Labels
API: changes
Projects
None yet
Milestone
v3.0.1
Development

Successfully merging this pull request may close these issues.
8 participants
@timhoffm @Kojoley @Lyghtning @sebbosafened @zunction @AndrewPanB @jckling @tacaswell