Use CoreText to find macOS fonts - #31977
Conversation
| @@ -1,5 +1,6 @@ | |||
| def display_is_valid() -> bool: ... | |||
| def xdisplay_is_valid() -> bool: ... | |||
| def find_available_fonts() -> set[str] | None: ... | |||
There was a problem hiding this comment.
| def find_available_fonts() -> set[str] | None: ... | |
| def get_available_fonts() -> set[str] | None: ... |
| } | ||
| return fonts; | ||
| #else | ||
| return py::none(); |
There was a problem hiding this comment.
should this be a py::set() call so that you can iterate it on the returning item even on a non macos platform? It is private, so probably not a big deal either way, but you are calling it as for x in None in the _get_macos helper above which would raise instead of returning empty.
There was a problem hiding this comment.
I was trying to mimic the "on other platforms, returns None" in Win32_GetCurrentProcessExplicitAppUserModelID and Win32_GetForegroundWindow.
I think _get_macos_fonts() should be changed to check for None rather than returning an empty set. To me, an empty set would indicate that the platform-specific code ran but found no fonts.
I think raising NotImplementedError might be the proper solution if the other functions in that file would do the same? I'm not sure (still learning!) :)
Co-authored-by: Greg Lucas <greg.m.lucas@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
|
I happened across foliojs/font-manager#19 while looking at these functions. Is that also a problem in this implementation (and was it something that we supported before)? |
I'm not sure what is going on in the linked bug. My suspicion is one of the following:
I'm leaning towards 1. In any case, our previous implemented ran |
|
I did some more digging last night.
The new code calls This is a subtle change of behavior, but I'm not sure if the old behavior is correct. There are situations where a user may have multiple user-level fonts installed with the same name and wants to deactivate one of them. Assuming we want to change this behavior, should the version number of the font cache be bumped in font_manager.py? It's currently: |
|
Hmm, yes, I think that is correct, as we did the same thing for Windows: #22859 You can bump the cache key to |
Closes #28249
Closes #31965
Why is this change necessary?
Prior to this change,
findSystemFonts()in font_manager.py calledsystem_profiler -xml SPFontsDataType. This subprocess would load 1000+ font files in order to parse localized names. As we only need font paths, we can instead query CoreText directly. This finishes in ~63ms on my machine instead of ~7 seconds.Differences in found fonts
system_profileruses the privatekCTFontCollectionIncludeInvisibleFontsOptionoption to find "invisible" fonts. These are fonts with names starting with '.'. These fonts are typically unavailable using the standard macOS font APIs. On my machine (macOS 14.8), the following fonts are "invisible":My change uses the
CTFontCollectionCreateFromAvailableFonts()without this private option; hence, these fonts will be missing from the list returned by_get_macos_fonts().However, font_manager.py later re-scans the "/System/Library/Fonts" path, so it should discover these invisible fonts at that time.
I tested this change on macOS Mojave (10.14) and macOS Sonoma (14.8). I was unable to test on 10.12 or 10.13 as I updated my 10.12 test device to 10.14. The CoreText API used has been available since 10.5, so I do not foresee any compatibility issues.
AI Disclosure
The original C-based implementation in #31965 is my own without any usage of AI. I used AI to help me convert it to C++ and return a
py::set(my original implementation printed the paths). I used AI to check the resulting files and verify that I was following the correct formatting guidelines.PR checklist