Standardize logo image behavior between Sphinx and this theme

[12rambau]

related to #1130

I don't manage to reproduce the current behavior on my local computer, I want to check if it's related to the distant build.

[12rambau]

I have 0 clue what changed but it's back to normal.

superceed #1131
Fix #1130

[choldgraf]

Does this mean that people in general need to add the _static in front of their logos now?

Fwiw the logo looks broken in the RTD build, but only for the light theme

https://pydata-sphinx-theme--1132.org.readthedocs.build/en/1132/

[choldgraf]

I think we may need to add a conditional check in our template. so that if it is sphinx 6 we do not add _static and if < 6 we do?

[12rambau]

arf, my computer uses the dark theme I forgot that they were piloted from 2 different images.

[choldgraf]

Ah interesting - yeah we should make sure that the link behavior is the same between html_logo and the logo theme configuration

I think it's going to confuse people if there is an implicit "_static" if the config is nested under "logo" but if we require the "_static" if using html_logo

[12rambau]

Ok, I think I find a way to be compatible with everything:

• Move logic away from jinja
• Create logo_light and logo_dark from the logo/image_light/image_dark
• Use the same logic as in sphinx so compatible with sphinx 4-5-6
• if by any chance they modify their jinja template again, we won't be impacted

[12rambau]

I'm stuck. The only configuration that continues to fail is:
html_theme_options["logo"] is not set at all in the conf.py. If that's the case, the update of theme_options is not persisted to the jinja template and the logo images are not displayed. Does anybody have an idea why ?

[choldgraf]

OK so I think this might be what is happening:

We are updating various configuration options in the builder-inited phase:

pydata-sphinx-theme/src/pydata_sphinx_theme/__init__.py

Lines 33 to 41 in a6329b6

	def update_config(app):
	theme_options = app.config.html_theme_options
	# DEPRECATE >= v0.10
	if theme_options.get("search_bar_position") == "navbar":
	logger.warning(
	"Deprecated config `search_bar_position` used."
	"Use `search-field.html` in `navbar_end` template list instead."
	)

However, when builder-inited has triggered, this by definition means that the builder has already been defined and set. So the options that were in html_theme_options are now already baked into the context variables and such. At this point, changing html_theme_options will have no effect because the builder is already inited.

So if I'm right, we'd need to either:

1. Trigger the "update the html_theme_options values" function before the builder is inited (but we cannot use config-inited unless we force users to add this theme as an extension as well).
2. Figure out what inside the builder we need to update since it has already been set up, and make the changes there instead of in html_theme_options

Option 3 is that I'm totally wrong and it's a different problem :-)

[12rambau]

hmmm, then why is working when I set 2 absolute url or 1 html_logo and a html_theme_options["logo"]["dark_image"] ?
It is as if I cannot add more keys but I can still modify them. OR (and then it's worse) they are linked and not copied #mutableobject and that's why it's working in all the other config

[12rambau]

Thanks @choldgraf for the emotional support, that was a amazing catch. So context is reading html_theme_option but not copying them: thus the weird behavior.

Calling everything from context works like a charm.

I need #1136 to validate windows build. As it seems very unlikely that I generated any bugs there, I think it's ready to merge.

[12rambau]

I finally got something that works in all our test. It required a lot of special cases, I made my best to make it seemless, it wil not block moving forward as "logo_url" will always be set.

[12rambau]

• I updated the link in the documentation to point to Sphinx doc
• I refactored the code to add breath between the 2 logo block
• I use an image_relative dict so that it will be callable using a jinja variable (trust me it will be useful very soon)
• ✅ test
• ✅ docs

I don't care about #1065 as it is not blocking anything. Let's do the 0.13rc1 release once merged

[choldgraf]

LGTM - thanks for putting so much thought into getting this one right!

[drammock]

Super good work @12rambau !!

[jklymak]

This broke Matplotlib's doc build with 0.13. The Changelog is quite terse and this thread is pretty long, and the PR description oblique. What is the TLDR summary of the change here?

[12rambau]

we deal now with logos the same way Sphinx is doing it: https://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_logo

• it will work whatever the version (there are internal changes between 5 and 6 that we were unable to handle)
• the path need to be relative to the configuration directory

[12rambau]

@jklymak in the case of matplotlib I don't understand why it was working before, where are the file you point to as logos ? https://github.com/matplotlib/matplotlib/blob/217f8f70875279709f6d34f98d5c1447d6a6d822/doc/conf.py#L457

[jklymak]

We supply the logos as part of mpl-sphinx-theme, which is a thin wrapper around pydata-sphinx-theme. https://github.com/jklymak/mpl-sphinx-theme/tree/main/mpl_sphinx_theme. Placing the logos in static/images/ of the theme used to copy them to build/html/_static/images, and referring to them in the theme as images/logo.svg used to work. We abstract the logos to mpl-sphinx-theme so that a number of sites can have consistent logos and navbars.

I'm sure we can get this all to work; I haven't spent any time trying to debug, but was hoping for a little more clarity on what happened here that broke that flow. Thanks!

[12rambau]

I think changing "images/logo.svg" to "_static/images/logo.svg" will do the trick

[jklymak]

mpl-sphinx-theme distributes static/images/logo2.svg (https://github.com/jklymak/mpl-sphinx-theme/blob/main/mpl_sphinx_theme/static/images/logo2.svg).

In 0.12.0, during the build this is copied to _build/html/_static/images and linked at /_static/images/logo2.svg, using a conf.py that looked like:

html_theme_options = {
    "logo": {"link": "https://matplotlib.org/stable/",
             "image_light": "images/logo2.svg",
             "image_dark": "images/logo_dark.svg"},
...}

In 0.13.0, the file is still copied to _build/html/_static/images, however, there is a warning:

WARNING: Path to light image logo does not exist: images/logo2.svg

and the build logo image source is now looking at: _static/logo2.svg.

If (after make clean) I change the conf.py to have "image_light":"_static/images/logo2.svg"
the files again are at _build/html/_static/images, but we get the similar warning:
WARNING: Path to light image logo does not exist: _static/images/logo2.svg
and the image source still says _static/logo2.svg.

Next debugging step was to move, in the theme, logo2.svg to static/logo2.svg. This copies over fine, and the site builds fine with "image_light":"_static/logo2.svg", but it still returns a Warning: WARNING: Path to light image logo does not exist: _static/logo2.svg. If I change to "image_light":"logo2.svg" it still complains that file doesn't exist though the site builds properly.

I assume what has happened here is that you are assuming that the local static is all that there is to static, but the theme can carry static info (https://www.sphinx-doc.org/en/master/development/theming.html#creating-themes) and you are making a Warning that is not warranted. Since many of us fail doc builds with warnings, maybe the Warning needs to go away, or be more robust.

Second you are stripping information from the theme options so that the subdirectory structure is being lost. I'm not sure if that is because of the Warning or if you are doing that on-purpose, however it seems we should be allowed to have subdirectories in the the static info of themes.

[jklymak]

I think this is too restrictive, in that it doesn't allow paths relative to static. Is stripping the name needed at all?

[12rambau]

strip the name is essential if you use files from outside the doc folder (i.e. the root of the repo) which would have path like "../../../my_hidden_logo.png". This file needs to end up in the build _static directory eventually

[jklymak]

There probably is not a good way to check if the path exists in a theme-supplied static directory? If not, then maybe just skip this warning or demote to INFO or DEBUG?

[jklymak]

Also, should this not continue at this point?

[12rambau]

There probably is not a good way to check if the path exists in a theme-supplied static directory? If not, then maybe just skip this warning or demote to INFO or DEBUG?

I'm -1 on changing the warning, as you mentioned people are listening to warnings in their CI/CD it would mean that nobody would realize that the logos are not displayed anymore until someone check visually the logs/website.

Also, should this not continue at this point?

yes we should