Skip to content

Update font_manager to only use registry on Win #24655

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

Merged
merged 4 commits into from
Jan 11, 2023
Merged

Update font_manager to only use registry on Win #24655

merged 4 commits into from
Jan 11, 2023

Conversation

almarklein
Copy link
Contributor

@almarklein almarklein commented Dec 7, 2022

anntzer
anntzer previously approved these changes Dec 7, 2022
@tacaswell
Copy link
Member

xref to #24001

@anntzer anntzer requested review from anntzer and removed request for anntzer December 7, 2022 19:12
@anntzer anntzer dismissed their stale review December 7, 2022 19:13

Actually, a test needs to be updated too.

@anntzer anntzer added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Dec 15, 2022
@anntzer
Copy link
Contributor

anntzer commented Dec 15, 2022

I think that this or something similar needs to make it into 3.7 to fix #24641 being partially incorrect; or #24641 needs to be temporarily reverted for 3.7.

@anntzer anntzer added this to the v3.7.0 milestone Dec 15, 2022
@tacaswell
Copy link
Member

____________________________ test_user_fonts_win32 ____________________________
[gw1] win32 -- Python 3.10.8 C:\hostedtoolcache\windows\Python\3.10.8\x64\python.exe

    @pytest.mark.skipif(sys.platform != 'win32', reason='Windows only')
    def test_user_fonts_win32():
        if not (os.environ.get('APPVEYOR') or os.environ.get('TF_BUILD')):
            pytest.xfail("This test should only run on CI (appveyor or azure) "
                         "as the developer's font directory should remain "
                         "unchanged.")
    
        font_test_file = 'mpltest.ttf'
    
        # Precondition: the test font should not be available
        fonts = findSystemFonts()
        if any(font_test_file in font for font in fonts):
            pytest.skip(f'{font_test_file} already exists in system fonts')
    
        user_fonts_dir = MSUserFontDirectories[0]
    
        # Make sure that the user font directory exists (this is probably not the
        # case on Windows versions < 1809)
        os.makedirs(user_fonts_dir)
    
        # Copy the test font to the user font directory
        shutil.copy(Path(__file__).parent / font_test_file, user_fonts_dir)
    
        # Now, the font should be available
        fonts = findSystemFonts()
>       assert any(font_test_file in font for font in fonts)
E       assert False
E        +  where False = any(<generator object test_user_fonts_win32.<locals>.<genexpr> at 0x000002364CB2A8F0>)

https://dev.azure.com/matplotlib/matplotlib/_build/results?buildId=30184&view=logs&j=595c9a33-3ac5-58f0-6ce6-44c63ffdfe86&t=6405dc77-f20a-51d5-22e4-a611ccc41f2e&l=182

The failure is definitely caused by this change

story645
story645 previously approved these changes Dec 29, 2022
Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

both loops return the same fonts1 with extensions [".afm", ".ttf", ".otf", ".ttc" ] and the hkey points to C:\WINDOWS\Fonts

ETA: I'm now confused how that test failed since both of the keys in the fontpath are in the windows installed search function, unless it's a permissions thing?

for domain, base_dirs in [
(winreg.HKEY_LOCAL_MACHINE, [win32FontDirectory()]), # System.
(winreg.HKEY_CURRENT_USER, MSUserFontDirectories), # User.
]:

Footnotes

  1. the old loop returns {'', '.CompositeFont', '.TTF', '.dat', '.fon', '.ini', '.ttc', '.ttf', '.xml'} because there's no extension filtering.

@story645
Copy link
Member

story645 commented Dec 30, 2022

Also, I dunno if this is sensible but I modified the test to run it on my local machine, and I'm now getting the same failure, w/ the same message:

@pytest.mark.skipif(sys.platform != 'win32', reason='Windows only')
def test_user_fonts_win32():
    font_test_file = 'mpltest.ttf'

    # Precondition: the test font should not be available
    fonts = findSystemFonts()
    if any(font_test_file in font for font in fonts):
        pytest.skip(f'{font_test_file} already exists in system fonts')

    user_fonts_dir = Path(MSUserFontDirectories[0])

    # Make sure that the user font directory exists (this is probably not the
    # case on Windows versions < 1809)
    user_fonts_dir.mkdir(exist_ok=True)

    # Copy the test font to the user font directory
    shutil.copy(Path(__file__).parent / font_test_file, user_fonts_dir)

    assert (user_fonts_dir/font_test_file).exists()

    # Now, the font should be available
    fonts = findSystemFonts()

    assert any(font_test_file in font for font in fonts)
    (Path(user_fonts_dir)/font_test_file).unlink()
    assert not (Path(user_fonts_dir)/font_test_file).exists()

ETA: if this is sensible, then I'll wrap up "temp copy" in a context manager.

ETA2: I have no idea why the test is failing b/c the directory for writing the tempfile is identical when fontpaths is set or when it's empty

C:\Users\story\AppData\Local\Microsoft\Windows\Fonts\mpltest.ttf
C:\Users\story\AppData\Local\Microsoft\Windows\Fonts\mpltest.ttf

while this is flipped:

findSystemFonts folders: {WindowsPath('C:/Windows/Fonts'), WindowsPath('C:/Users/story/AppData/Local/Microsoft/Windows/Fonts')}
findSystemFonts folders: {WindowsPath('C:/Users/story/AppData/Local/Microsoft/Windows/Fonts'), WindowsPath('C:/Windows/Fonts')}

and I checked and yeah the file is in AppData

@QuLogic
Copy link
Member

QuLogic commented Dec 31, 2022

Also, I dunno if this is sensible but I modified the test to run it on my local machine, and I'm now getting the same failure, w/ the same message:

Ah, are you saying the test is wrong? I think we maybe should be calling AddFontResourceEx for that test instead of directly copying the file.

@story645
Copy link
Member

Ah, are you saying the test is wrong?

No, just that I'm not sure why I can't run it locally if I create and clean-up the file during the test?

@QuLogic
Copy link
Member

QuLogic commented Jan 4, 2023

I guess what I would like to confirm is; if you use Explorer to copy a new font there, does it get added to the registry? Does it only do so if you use the Install button on the font viewer? What about if you have a Python script that copies the font there; does the registry update automatically, or do you have to open the directory in Explorer to refresh it, or does nothing update the registry?

@story645
Copy link
Member

story645 commented Jan 4, 2023

if you use Explorer to copy a new font there, does it get added to the registry?

No
Also "'SOFTWARE\Microsoft\Windows\CurrentVersion\Fonts'" doesn't exist at least on my machine

Does it only do so if you use the Install button on the font viewer?

Yup

What about if you have a Python script that copies the font there; does the registry update automatically,

nope

or do you have to open the directory in Explorer to refresh it

that doesn't seem to work either

ETA: as far as I can tell, that test works because it's checking if a file put into a folder is found in the folder, completely bypassing the registry.

@tacaswell
Copy link
Member

Per discussion on this weeks dev call:

  • go with windows registry only
  • xfail the test
    • defer questions of how to test this to later
    • do we want to change the registry as part of the test?
    • opt in to running locally?
    • flag to only run an CI + windows?
  • need a manual test that this works
    • download a new font
    • use windows way to install it
    • remove mpl's font cache
    • verify the new font can be found
  • add behavior change note

1. we are moving to registry only for finding fonts on windows
2. our test just drops the font in the correct directory
3. this is not enough to get the registry to agree it is there

We will xfail this test for now.
@tacaswell tacaswell requested a review from story645 January 5, 2023 22:05
@tacaswell tacaswell dismissed story645’s stale review January 5, 2023 22:06

per need for changes as discussed on the call.

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.

but I wrote the behavior change note so this review should be taken with a grain of salt.

@tacaswell
Copy link
Member

@story645 Can you please confirm that this works as expected for fonts installed as Windows expects and merge if you are satisfied?

@story645
Copy link
Member

story645 commented Jan 6, 2023

heads up that I won't be able to get to this til tomorrow night/sunday

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

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

tiny what's new nit. I don't think the failing tests are related to this PR, but I'm not comfortable merging while so many tests are failing.

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: hannah <story645@gmail.com>
@QuLogic QuLogic merged commit 21663b5 into matplotlib:main Jan 11, 2023
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jan 11, 2023
oscargus added a commit that referenced this pull request Jan 11, 2023
…655-on-v3.7.x

Backport PR #24655 on branch v3.7.x (Update font_manager to only use registry on Win)
@ksunden ksunden mentioned this pull request Feb 20, 2023
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: findSystemFonts should not look in subdirectories of C:\Windows\Fonts\
7 participants