jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
191 stars 29 forks source link

Client Error: Bad Request for url when fetching some Google Fonts. #193

Closed iSOLveIT closed 1 year ago

iSOLveIT commented 1 year ago

Description I would like to use the Recursive Google Fonts but when I configure the html_theme_options, fonts, I get a Sphinx-Immaterial extension error.

What's it all about? I want to use the Recursive font as the text font in my project but I keep getting a 400 Client Error: Bad Request url error when I build my Sphinx project. From my research, I realised that the way Sphinx-Immaterial requests fonts from Google Fonts won't always work because some fonts do not provide other font styles like italics.

For example:

In my project, I want to use the Recursive font which does not provide italic versions of the font. As a result, if Sphinx-Immaterial tries fetching this URL: https://fonts.googleapis.com/css?family=Recursive:700i, it will get a Bad Request error (status-code: 400).

What should happen: Sphinx-Immaterial downloads the Google Fonts specified by the user, caches them (using the cache specified by sphinx_immaterial_external_resource_cache_dir), and includes the fonts in the built documentation.

What happened instead: I get the following Sphinx-Immaterial extension error:

Extension error (sphinx_immaterial.external_resource_cache):
Handler <function _builder_inited at 0x7fd07afc7370> for event 'builder-inited' threw an exception (exception: 400 Client Error: Bad Request for url: https://fonts.googleapis.com/css?family=Recursive:700i)

How to reproduce this bug:

  1. Create projectA:
    mkdir projectA
    cd projectA
    sphinx-quickstart 
  2. Install Sphinx-Immaterial and configure projectA by adding the following lines to projectA/source/conf.py:
    extensions = ["sphinx_immaterial"]
    html_theme = "sphinx_immaterial"
    html_theme_options = {
    # Other Sphinx-Immaterial configuration
    "font": {
        "text": "Recursive"
    },
    }
  3. Build documentation using make html or sphinx-build -b html . ./_build/html

Browser(s) and Device(s) observed on:

I hope the description helps to reproduce the issue :)

jbms commented 1 year ago

Do you have a proposal for how to fix it? Just don't include missing variants?

iSOLveIT commented 1 year ago

Hi @jbms, I read through the codes and realised that under external_resource_cache.py from lines 199-200, you specify which font weights to fetch when downloading a font.

 for style in ["300", "300i", "400", "400i", "700", "700i"]:
        css_future_keys.append((font, style))
        css_futures.append(fetch_font(font, style))

I think the user should be given the option to specify which font weights they want to use in conf.py then you could use this list: ["300", "300i", "400", "400i", "700", "700i"] as default if they don't specify.

So under conf.py we could have something like this:

html_theme_options = {
 # Other Sphinx-Immaterial configuration
 "font": {
    "text": {
             "name": "Recursive",
             "styles": ["300", "400", "700"]
         }
 },
}

I don't know if this might affect any other part of the extension but this is an idea I will propose. Thank you.

2bndy5 commented 1 year ago

For clarity, I would prefer the field name be weights instead of styles.

jbms commented 1 year ago

It seems reasonable to allow it to be customized. --- the current set is what mkdocs-material uses, presumably because that's what the theme uses. But it would also be nice for it to just work by default.

2bndy5 commented 1 year ago

But it would also be nice for it to just work by default.

You mean like warn instead of fail for unresolved resources?

https://github.com/jbms/sphinx-immaterial/blob/f9d9f5b4cf875ae13f94b0139dbf02ad23e5f9e9/sphinx_immaterial/external_resource_cache.py#L200-L202

 for style in styles: # styles: ["300", "300i", "400", "400i", "700", "700i"] 
     font_fetched = fetch_font(font, style)
     if font_fetched:
         css_future_keys.append((font, style)) 
         css_futures.append(font_fetched)
     else:
         logger.warn("Could not fetch font %s %s", style, font)
jbms commented 1 year ago

We could check for http 400 status and not warn at all as long as at least one variant is found.

jbms commented 1 year ago

We also will need to somehow cache the fact that the file is missing so that we don't repeat the requests on every build.

2bndy5 commented 1 year ago

Just to summarize the current solution: The new weights key was not added. Instead all the default weights/variants are fetched from google's font API using the requested font's metadata. And then fonts are downloaded accordingly. https://github.com/jbms/sphinx-immaterial/blob/1920f5718c2473c73dec9846074428722b227ebf/sphinx_immaterial/google_fonts.py#L147-L167