jbms / sphinx-immaterial

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

Using without Google Fonts #222

Open hille721 opened 1 year ago

hille721 commented 1 year ago

Really like this project and would love to use it, as with it's dark theme it is so much better than sphinx-material. But the fact that it will download the Google Fonts from remote server is currently blocking me to use this project. In my CI I don't have internet access, thus downloading is not a option. Yes, I could place the fonts in a directory and pointing the sphinx_immaterial_external_resource_cache_dir, but in CI also this is not so easy. I could put them directly in my GIT repo, but then I would double the size.

Is there any possibility to disable the usage of Google fonts?

2bndy5 commented 1 year ago

This is the second time this theme's external resources extension has caused a problem concerning access to google's fonts.

I think this is worth looking into. The upstream mkdocs theme has support for manually specifying the font in a situation where downloading the fonts is not an option (see here), but I haven't tested this since the external resources extension was added to this theme.

2bndy5 commented 1 year ago

BTW, support for this would also cause problems with this theme's customized graphviz extension.

hille721 commented 1 year ago

I think this is worth looking into. The upstream mkdocs theme has support for manually specifying the font in a situation where downloading the fonts is not an option (see here), but I haven't tested this since the external resources extension was added to this theme.

Oh, just realized that this seems to work

html_theme_options = {
    # ...
    "font": False,
    # ...
}
2bndy5 commented 1 year ago

Yes, I see that too, but I'm looking into ways to compensate the graphviz custom extension... This patch seems to have a semi-acceptable result:

--- a/sphinx_immaterial/google_fonts.py
+++ b/sphinx_immaterial/google_fonts.py
@@ -181,7 +181,9 @@ def add_google_fonts(app: sphinx.application.Sphinx, fonts: List[str]):

 def get_ttf_font_paths(app: sphinx.application.Sphinx) -> Dict[Tuple[str, str], str]:
-    return getattr(app, _TTF_FONT_PATHS_KEY)
+    if hasattr(app, _TTF_FONT_PATHS_KEY):
+        return getattr(app, _TTF_FONT_PATHS_KEY)
+    return None

 def _builder_inited(app: sphinx.application.Sphinx):
diff --git a/sphinx_immaterial/graphviz.py b/sphinx_immaterial/graphviz.py
index d242e63..eb7faf9 100644
--- a/sphinx_immaterial/graphviz.py
+++ b/sphinx_immaterial/graphviz.py
@@ -235,10 +235,16 @@ def render_dot_html(
 ) -> Tuple[str, str]:

     theme_options = self.builder.config["html_theme_options"]
-    ttf_font_paths = google_fonts.get_ttf_font_paths(self.builder.app)
-    font = theme_options["font"]["text"]
+    if theme_options["font"]:
+        font = theme_options["font"]["text"]
+    else:
+        font = "Helvetica"

-    ttf_font = ttf_font_paths[(font, "regular")]
+    ttf_font_paths = google_fonts.get_ttf_font_paths(self.builder.app)
+    if ttf_font_paths:
+        ttf_font = ttf_font_paths[(font, "regular")]
+    else:
+        ttf_font = "Helvetica"  # use graphviz default fallback

     code = _replace_resolved_xrefs(node, code)
2bndy5 commented 1 year ago

It would also be nice to specify a custom font instead of using the system default as demonstrated in the upstream's "additional fonts" section. I have yet to test this approach though.

All of this should be noted in our docs as well.

2bndy5 commented 1 year ago

@Neojume https://github.com/jbms/sphinx-immaterial/issues/222#issuecomment-1446707021

jbms commented 1 year ago

There are various potential solutions that could be implemented:

  1. Use Google Fonts loaded by the browser directly from Google's server, as in mkdocs-material (without offline/privacy plugin) and previous versions of this theme. This doesn't allow the graphviz extension to use the correct font metrics.
  2. Use a system font. This will also mean that the graphviz extension won't use the correct font metrics.
  3. Store the cached font data inside your repository.
  4. Store the cached font data somewhere else that is accessible to your CI system
  5. Add the default Google fonts to this theme's package, as is done for the icons

Note that I actually face this same problem with tensorstore (https://github.com/google/tensorstore) --- we have an internal CI system that operates without internet access. I use option 3 / 4, where internally they are part of the repository but they are excluded from the open source tensorstore git repository.

I suppose the first question is what you are hoping to accomplish? Do you want to use the Google fonts, or just use system fonts? Do you care about graphviz?

2bndy5 commented 1 year ago

I think solution 1 would require reverting this PR change to base.html in a conditional way (ie when this theme's external resources ext is bypassed).

It is starting to feel like the external resources ext should be made optional and not auto-enabled by this theme. While I understand (& agree with) the security concern that it aims to solve, breaking builds to do so seems a bit opinionated. Alternatively, we could add a warning or info prompt when builds fail to execute the external resources ext, but that seems more like a rabbit hole to me.


[...] question is what you are hoping to accomplish? Do you want to use the Google fonts, or just use system fonts? Do you care about graphviz?

@hille721 Answers to these questions would be very valuable user feedback.

hille721 commented 1 year ago
  1. Use Google Fonts loaded by the browser directly from Google's server, as in mkdocs-material (without offline/privacy plugin) and previous versions of this theme. This doesn't allow the graphviz extension to use the correct font metrics.

Not optimimal imo. Prefer a bundled approach to ensure accessibility also on clients without internet access (there are a few in my company).

  1. Use a system font. This will also mean that the graphviz extension won't use the correct font metrics.

I guess this is what I currently achieved by setting "font": False (see https://github.com/jbms/sphinx-immaterial/issues/222#issuecomment-1446707021). It works for me and honestly I was not able to detect any difference on the font

  1. Store the cached font data inside your repository.

Already experimented with that. What I asked myself: Instead of specifying this cache setting wouldn't it be easier to allow it to store the font directly in _static\fonts?

  1. Store the cached font data somewhere else that is accessible to your CI system

Theoretically possible but imo to much effort for what I want to achieve.

  1. Add the default Google fonts to this theme's package, as is done for the icons

Imo this would be the best solution. It shouldn't be that much of additional size.

I suppose the first question is what you are hoping to accomplish? Do you want to use the Google fonts, or just use system fonts? Do you care about graphviz?

First of all I want to make it to run :) I'm not a font fetishist so I don't really care about what font I have as long as it is not directly visibly ugly. Currently I also don't have a use case with graphviz.

2bndy5 commented 1 year ago

Solution 5 may be laborious for us to ensure that the latest changes to the default fonts are available; I'm back to considering the fontsource npm pkg for automated updates. Judging from the CI artifacts, this will add around 2 MB to the theme's distro.

2bndy5 commented 1 year ago

225 includes a patch for the theme's graphviz ext (when using the system font) and docs additions about using the system's font or a self-hosted font. I'm keeping this open since it does not implement solution 5 (probably do that in a different PR).

jbms commented 1 year ago

The problem with solution 5 is that users can choose a different Google font in place of the default options, and we don't want to bundle all of the Google fonts. Though perhaps it is acceptable to just bundle the default fonts and still rely on the existing mechanism otherwise.

We could in principle allow the user to specify fonts in TTF format directly, rather than a Google font name. That would work with graphviz as well. However, there are some complications:

and it would quickly become annoying to specify all of that as part of the theme configuration.

2bndy5 commented 1 year ago

I was thinking only the default fonts available via solution 5, otherwise use the existing mechanism already in place.

I think those that want to use self-hosted fonts would be well versed enough to understand how to use CSS in the first place. I'd be slightly surprised (more likely impressed) if I was proven wrong about this.

TBH, the only thing I'm concerned with the graphviz ext is the uniformed coloring per theme settings. The inaccurate font metrics are not distracting from a reader's perspective. I consider it an absolute bonus to have the same font used in the generated graphs; its not a deal-breaker for me if we have to use system fonts for graphviz. Don't forget that our struggles with graphviz's font metrics is really a graphviz problem.

2bndy5 commented 1 year ago

I'm investigating the fontsource idea... The npm pkgs for @fontsource/roboto[-mono] ship with woff and woff2 font files. They also come with compiled CSS that use paths we'll have to adjust when copying over the CSS to a consolidated bundle. However, the pkgs also come with the necessary SCSS mixin to compile our own CSS based on the font variants deployed; I'll have to filter out the weights that we don't need.

Alternatively, we could download the default fonts from the google API in the build script and place the data in sphinx_immaterial folder where google_fonts.py can use it as if downloaded at docs' build time. I'm hesitant about this approach because I don't know JS that well. I think this might need an added dependency that can download the fonts.

Either way would require some pretty significant changes to the build script. For the second approach, perhaps an added .ts module specific to sphinx-immaterial. I'm only looking at the fontsource pkgs because Google recommends them in the repo's README.

jbms commented 1 year ago

I think we should use the same download method and source at both package build time (for the default fonts) and at doc build time (for user-specified fonts). Otherwise it would be confusing and more hassle to maintain. That would mean using Python rather than JavaScript. It also means not using scss since we don't process that at doc build tim le.

We could download fontsource npm packages instead of using the Google fonts server --- that might offer some advantages, like supporting more fonts and versioning the fonts properly.

It occurs to me that we could publish the fonts as separate PyPI packages, and do the same for the icons. That would mean users can choose whether to add a dependency on them or not.

2bndy5 commented 1 year ago

I like the idea to abstract the fonts as another pypi pkg. I think this would imply the repos belong to a sphinx-immaterial org, which would allow using org-specific re-usable CI workflows (& shared CI secrets if needed).

For now, I'll try to hack something together for google fonts' API instead of fontsource. Abstracting the fonts/icons into another pypi dep seems like a major deviation from upstream but still feasible.

jbms commented 1 year ago

It could make sense to create a GitHub org but what I had in mind for these font and icon packages is that we would still just have this one GitHub repo but would publish multiple packages from it.

2bndy5 commented 1 year ago

That's an interesting idea, something I haven't needed to do/try before.

As for switching to the fontsource CDN, I think we could use the fontsource API instead of google fonts API.