Fix the URLs of SVG inheritance diagrams #152
Conversation
|
I have this PR as "draft" because I haven't yet tried to write a unit test. It works as intended when I build the SunPy docs. |
|
Maybe wanna address this warning too: Would be nice to have another draft PR out to astropy using this branch as proof-of-concept (what works for Sunpy might not work for astropy, you never know). |
Codecov Report
@@ Coverage Diff @@
## main #152 +/- ##
==========================================
- Coverage 89.43% 89.43% -0.01%
==========================================
Files 23 23
Lines 1098 1107 +9
==========================================
+ Hits 982 990 +8
- Misses 116 117 +1
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
|
I compared some URLs from the sunpy test build on RTD sunpy/sunpy#6267. It looks like the URL is being resolved relative to the SVG file and not relative to the HTML page where the SVG file is placed. So, assuming images are only ever placed in the RTD latest buildcoordinates diagramDiagram on HTML page: SVG file: href in SVG file: Resolves to: map diagramDiagram on HTML page: SVG file: href in SVG file: Resolves to: RTD build for sunpy/sunpy#6267coordinates diagramDiagram on HTML page: SVG file: href in SVG file: Resolves to: map diagramDiagram on HTML page: SVG file: href in SVG file: Resolves to: |
|
As discussed on Element, the root bug appears to actually be in |
| if env is not None: | ||
| graphviz_output_format = env.config.graphviz_output_format.upper() | ||
| if graphviz_output_format == 'SVG': | ||
| urls = {k: v[3:] for k, v in urls.items()} |
There was a problem hiding this comment.
The suggestion below works for me if I build the sunpy docs and test on a local web server. This way all the hrefs in SVG files only start with ../generated. If I understand the upstream issue correctly, the URLs for the PNG diagram need to be relative to the HTML file while the URLs inside the SVG diagram need to be relative to the SVG file itself? So in this line of the sphinx codebase, they should be able to apply the same fix as here (i.e., remove all "../" from child.get('refuri'))?
| urls = {k: v[3:] for k, v in urls.items()} | |
| urls = {k: "../" + v.split("../")[-1] for k, v in urls.items()} |
There was a problem hiding this comment.
This approach would work fine for the default configuration, but wouldn't be a general solution for all possible configurations of directory structure.
There was a problem hiding this comment.
Not sure I understand how a different directory structure could make this not work? (Unless people can change the default image directory.) Can we can assume that child.get('refuri') will always be rooted at the directory where the docs get generated to (yet prepended with any number of ../) and that the SVG files will always be in a directory _images?
I.e., docs are generated to a directory _build/html with files _build/html/user_guide/a/b.html _build/html/api_docs/x/y/z/module.html and a diagram at _build/html/_images/diag.svg.
Therefore child.get('refuri') will be ../../api_docs/x/y/z/module.html#test.class. So child.get('refuri').split("../")[-1] (api_docs/x/y/z/module.html#test.class) is always rooted at the directory containing _images. So, you can then do "../" + child.get('refuri').split("../")[-1] to get the path to module.html relative to diag.svg.
Also, the bug is definitely in sphinx as I was able to recreate it using only sphinx.
There was a problem hiding this comment.
My point is that we shouldn't assume that directory structure. The SVG files might get stored in a sub-sub-folder, or the relative path to the API docs might start with a '../'. Even if Sphinx itself doesn't support such a directory structure, it is so extensible that one just doesn't know. All of the information to re-path the URL should be available.
If this were a new bug, a hacky solution would probably be fine to get things working in most cases. Given that this bug is ancient, let's take the extra time to make a proper solution.
There was a problem hiding this comment.
Yes, you are correct! I was able to produce a directory structure where it didn't work (as the refuri only goes pack to the first directory in common with the page with the inheritance diagram).
Making the following modifications to ext.inheritance_diagram upstream I can get the links to work reliably on a minimal sphinx doc with lots of different directory hierarchies. I think this should be general solution that will work even if other plugins modify the default paths. What do you think?
404c404,412
< urls[child['reftitle']] = "../" + child.get('refuri')
---
> from pathlib import Path
> current_directory = Path(current_filename).parent
> url = "../" + str(
> current_directory /
> Path(self.builder.imgpath).parent /
> current_directory /
> Path(child.get('refuri'))
> )
> urls[child['reftitle']] = url
There was a problem hiding this comment.
Hmmm. The imgpath part is redundant in that code so maybe this isn't general...
There was a problem hiding this comment.
Your patch has some issues. My Sphinx modification is ayshih/sphinx@e711e26, and sunpy/sunpy#6267 shows that it fixes all SunPy inheritance diagrams. Can you try it on your test docs?
There was a problem hiding this comment.
I didn't notice that you updated the sunpy PR with a patched version of sphinx rather than this plugin! 🙃 😆 Yeah, your solution is much more robust across different systems etc and with variations in the location of the images! It works perfectly on my test docs with a big tree of directories with diagrams and API docs at all different levels. I think this bug is now finally solved! Well done! 🥳 There's an old issue upstream sphinx-doc/sphinx#3176 (as well as one opened just 8 hours ago! sphinx-doc/sphinx#10570)
There was a problem hiding this comment.
sphinx-doc/sphinx#10576 created!
|
I have put up a PR (sphinx-doc/sphinx#10576) that fixes the true bug in Sphinx, so I'm closing this PR |
Fixes #48
Currently, SVG inheritance diagrams do not link classes correctly because
sphinx.ext.inheritance_diagramprepends the URLs with'../'for SVG output, which conflicts with when the SVG is embedded in a page. See #48 (comment) and linked discussions therein. The'../'gets prepended here, right before the URLs are passed toInheritanceGraph.generate_dot()here.This PR defines a subclass of
InheritanceGraph(AutomodGraph) that overridesgenerate_dot()to removes the prepended'../'before generating the inheritance diagram. WhenAutomoddiagramusesInheritanceDiagramto create the document node, it casts the containedInheritanceGraphtoAutomodGraphso that the overriddengenerate_dot()will be used insphinx.ext.inhertiance_diagramcode.