Skip to content
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

Fixed two more bugs with SVG inheritance-diagram links #11634

Merged
merged 12 commits into from Aug 30, 2023

Conversation

ayshih
Copy link
Contributor

@ayshih ayshih commented Aug 22, 2023

Subject: Fixed two more bugs with SVG inheritance-diagram links

Feature or Bugfix

  • Bugfix

Purpose

This PR is a follow-on to fixes to inheritance-diagram links (#10614), because I discovered two bugs when trying to re-enable SVG inheritance diagrams on matplotlib (see matplotlib/matplotlib#26567). One of these bugs is actually in the graphviz writer, so affects use cases beyond just inheritance diagrams.

Detail

  • The bug in the graphviz writer (see Fix relative references in SVGs generated by sphinx.ext.graphviz #11078) is that it uses self.builder.outdir (e.g., build/html) as the base directory for relative URLs rather than the directory of the current document (e.g., build/html/api for build/html/api/doc1.rst). This happens to still work fine for many cases because the base directory can "cancel out" when determining the correct relative path from the image directory (e.g., build/html/_images). However, when the relative URL doesn't need to go all the way down to self.builder.outdir (e.g., build/html/api/doc1.rst pointing to build/html/api/doc2.rst means that the relative URL is simply doc2.rst), parts of the provided base directory get pulled into the generated relative path, and so passing in the wrong base directory results in an incorrect relative path (e.g., ../html/doc2.rst instead of ../api/doc2.rst).
  • When the inheritance diagram is in the same document as the class description, Fixes missing and broken links in inheritance diagrams #10614 accidentally kept the relative path from self.builder.outdir instead of using just the filename.

Relates

@AA-Turner
Copy link
Member

Looking at 7.2.5.

A

CHANGES Outdated Show resolved Hide resolved
@AA-Turner
Copy link
Member

@ayshih would you be able to look over the changes I've made here?

A

@ayshih
Copy link
Contributor Author

ayshih commented Aug 29, 2023

Hmm, one of your changes has caused this PR to no longer fix the matplotlib docs, so I'll track down why (and also expand the test accordingly).

@AA-Turner
Copy link
Member

AA-Turner commented Aug 30, 2023

6c10c76 probably fixes matplotlib, please would you be able to test?

A

@ayshih
Copy link
Contributor Author

ayshih commented Aug 30, 2023

Thanks for fixing this PR! I have verified that the matplotlib docs build correctly with the most recent changes. I have also expanded the unit test to cover what was being missed (linking between files in the same subdirectory), and verified that the unit test correctly fails when run on the earlier version (commit af8a1f4).

@AA-Turner AA-Turner merged commit 4692208 into sphinx-doc:master Aug 30, 2023
27 checks passed
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants