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

adding captions to left sidebar #135

Closed
wants to merge 2 commits into from
Closed

Conversation

choldgraf
Copy link
Collaborator

@choldgraf choldgraf commented Mar 23, 2020

This is an attempt at making it possible to add the caption of a TocTree in the sidebar. The idea is that then people can break up their sidebars into natural sections like so:

image

I think this PR gets quite close, but for some reason the TocTree(app.env).get_toctree_for function isn't returning the captions of any sub-page toctrees. Any idea why that is? Am I missing a flag somewhere?

In our case, we already do define captions for sub-page toctrees, they just aren't being used at all in the rendered docs

Ah - I think the issue is that the pandas get_toctree_for page always gets the toctree for the master doc (e.g., index), and then renders the toctree for the specific sub-page from that:

https://github.com/sphinx-doc/sphinx/blob/af62fa61e6cbd88d0798963211e73e5ba0d55e6d/sphinx/environment/adapters/toctree.py#L320

This means that only captions for the master doc toctree will make it into the output.

So I think the ways around this would either be:

  • Upstream a change to sphinx that somehow lets the sub-toctree captions also make it into the output
  • Stop using the get_toctree_for function, and instead write a similar function that basically does the same thing, but only for the current page, so you get the current page's captions

I think option 2 sounds more reasonable, since it's also basically what we are doing by default.

Update

Demo the sidebar here:

https://217-130237506-gh.circle-artifacts.com/0/html/demo/index.html

@jorisvandenbossche
Copy link
Member

Cool, fully agree this is something we want to support (many docs on rtd eg also use this sphinx feature)

Can you add it somewhere to the demo docs, so we can see how it looks?

isn't returning the captions of any sub-page toctrees

I don't fully get the problem. What do you mean with "sub-page" here? Toctrees that are not defined on the top-level index.rst?

In our case, we already do define captions for sub-page toctrees, they just aren't being used at all in the rendered docs

Also don't fully understand this ;)

{% for nav_item in main_nav_item.children %}
{% if nav_item.children %}
{% if nav_item["type"] == "caption" %}
<li class="nav-caption">{{ nav_item.text }}</li>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want to start a new list after each caption? That's eg how rtd does it (eg http://xarray.pydata.org/en/stable/)

@choldgraf
Copy link
Collaborator Author

OK, I think the latest push gets us close to what we want. It subclasses the TocTree class to add a method that gets us the toctree for a sub-page of the site, rather than basing it off of the master_doc page.

However, when I build the site I am getting this error and for the life of me I cannot figure it out:

generating indices...  genindexfailed

Theme error:
An error happened in rendering the page genindex.
Reason: FileNotFoundError(2, 'No such file or directory')

It seems to be happening simply because we have called the get_doctree method on a page that is not the master_doc...but I have no idea why that should be affecting the site index, it's driving me nuts...

@choldgraf choldgraf force-pushed the caption branch 2 times, most recently from da274c7 to f7cf135 Compare March 23, 2020 23:34
@choldgraf
Copy link
Collaborator Author

choldgraf commented Mar 24, 2020

gah it gets even weirder, I found that I can replicate this error if I just put the following in my code:

        filename = Path(self.env.doctreedir).joinpath(pagename + '.doctree')
        doctree = pickle.loads(filename.read_bytes())

I think that it has something to do with the pickling that happens as well as genindex...I guess the question is whether it's possible to have access to the doctree outside of reading in the pickle

@choldgraf
Copy link
Collaborator Author

choldgraf commented Mar 24, 2020

Phewwww that took wayyy longer than I expected.

The problem from before was happening when Sphinx tried to build either the genindex or search page, in which case it was not able to find a TocTree for the page. The reason it took me so long to figure out was because Sphinx's error reporting when the error is inside of a Theme is terrible.

This version has this feature working...it is surprisingly difficult to get a TocTree with the second-level pages that have captions. What we have to do is:

  • Using the first TocTree call, figure out the first active page by parsing its items. This is the "top level" page that may have a caption. This URL will be relative to the current page.
  • Calculate the path of the active page relative to the root of the docs, so that we can look up its page-specific toctree
  • Look up the toctree for the top-level active page, this has the captions that we want
  • For this page in the original toctree, replace its children with the new children that contain captions

Also adds a simple CSS rule

@jorisvandenbossche I agree that ultimately it would be better to use a new <ul> for each toctree...but I would really prefer to tackle that in a different PR because this one has turned out to be far more work than I was expecting...

demo the sidebar here: https://217-130237506-gh.circle-artifacts.com/0/html/demo/index.html

note: I just realized this won't currently work for pages that are below the second level...need to figure that out.

toc_items = [item for child in toctree.children for item in child
if isinstance(item, docutils.nodes.list_item)]
toc_items = []
for child in toctree.children:
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a lot of this extra code is because we were making really strong assumptions about the structure of the toctree, so we need to handle some other edge cases

github_user = context['github_user']
github_repo = context['github_repo']
github_version = context['github_version']
raise ExtensionError(
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I may have accidentally black-ified this file in the process 😬 I am not sure how to only revert the black changes, so hoping it's OK

@choldgraf
Copy link
Collaborator Author

another round of trying to figure this out, and it is still more difficult than I imagined. I'm going to stop working on this for a while. Anybody else is welcome to pick it up...I think it is probably close, but I need to focus on other things.

@jorisvandenbossche
Copy link
Member

@choldgraf thanks for looking into it anyway!

Can you give a small pointer / summary of what is not yet working (or is a full sphinx build simply not yet finishing?)

@choldgraf
Copy link
Collaborator Author

Basically, the current approach tries the following:

  • On each page, we get the toctree from the master_doc page. However, this toctree only has captions for the master_doc, whereas we want the toctrees of the first-level pages.
  • So, it defines a new get_toctree_for_subpage method that allows us to say which page we want the toctree for (in our case, we want the first-level page toctrees). The function also takes a reference page (which is the page currently being build).
    • We loop through each first-level page, grab its toctree, and re-reference the links so they are relative to the current page.
  • We now have the toctree items with captions for each first-level page.
  • Finally, we want to then loop through the main toctree and replace each first-level page section with the new one we have just created, so that there are captions in there

I think that the general logic is correct in here, and basically there is just a bunch of making sure that edge-cases are dealt with (I ran into a ton of confusion because apparently there are two special pages created called genindex and search), and to make sure that the assumptions about the doctree are still correct (e.g. not every toctree item is a bullet_list now, so we need to change some of the hard-coded assumptions like toctree.children[0].children[0] etc.

@choldgraf
Copy link
Collaborator Author

choldgraf commented Apr 3, 2020

Ah I think I might have found a much easier way to do this. It turns out Sphinx has a list of all the toctrees on all pages, which we might be able to use to do this more easily, see here:

https://github.com/ExecutableBookProject/sphinx-book-theme/pull/39/files#diff-95c3c14f442154a7a77462f1e36e81e7R88-R95

This would let us skip over the monkey-patching etc

@jorisvandenbossche
Copy link
Member

@choldgraf I am planning to tomorrow (hopefully, finally) take a look at this again. But a question: I suppose you are already happily using this feature in sphinx-book-theme? Any things you discovered there in the meantime ?

@choldgraf
Copy link
Collaborator Author

choldgraf commented May 23, 2020

What I've ended up doing is parsing the nav object that is created by this theme with another that generates a full HTML list for the sidebar:

https://github.com/executablebooks/sphinx-book-theme/blob/master/sphinx_book_theme/__init__.py#L49

this is similar to what I had done in #110 (though that PR is probably stale enough now that it should be done from scratch)

I grab the "master toc" and then re-work the URLs so that they are relative to the current page instead of the master doc page:

https://github.com/executablebooks/sphinx-book-theme/blob/master/sphinx_book_theme/__init__.py#L106

here I grab the list of toctrees on the master page, and then store the first URL in each TOCtree for use later:

https://github.com/executablebooks/sphinx-book-theme/blob/master/sphinx_book_theme/__init__.py#L70

and finally here I add the caption if we're about to list the sidebar entry for an item that is the first in a toctree w/ a caption:

https://github.com/executablebooks/sphinx-book-theme/blob/master/sphinx_book_theme/__init__.py#L110

The challenge with this is how to generalize it so that it works on both the first level page (what the book theme uses) and in the second level pages (which pydata sphinx theme needs)

@jorisvandenbossche
Copy link
Member

OK, I think I start to understand the situation / problem .. ;) (saving the toctrees as xml files to look at helps a bit ..)

Now, an initial question then comes up: how do we want to put this caption information in the navigation list/dict objects?

Currently, for a certain level, we have a list of dicts, like:

[
    {"title": "Subpage 1", "url": "subpage1.html", "active": True, "children": []},
    {"title": "Subpage 2", "url": "subpage2.html", "active": False, "children": []},
    {"title": "Subpage 3", "url": "subpage3.html", "active": False, "children": []},
]

Assume we have captions on this level? How do we want the navigation object to look like?

I am trying to test what this PR does, but it doesn't seem to work (anymore? after my rebasing, maybe something went wrong here, or the sphinx version ..) on the demo docs, at least I don't see any caption in the output.

But from looking at the code, I assume this PR tries to make something like this:

[
    {"text": "Caption 1", type: "caption"}
    {"title": "Subpage 1", "url": "subpage1.html", "active": True, "children": [], type: "ref"},
    {"title": "Subpage 2", "url": "subpage2.html", "active": False, "children": [], type: "ref"},
    {"text": "Caption 2", type: "caption"}
    {"title": "Subpage 3", "url": "subpage3.html", "active": False, "children": [], type: "ref"},
]

Is that right?
And such structure would be sufficient for sphinx-book-theme to work with?

I am trying to think through some situations to see if this is the best structure. In principle, an alternative could be to actually add an additional nesting level.

Looking at how mkdocs does it (which was an inspiration for those lists of dicts), they seem to have a "section" concept which is a part of the navigation that itself has no page linked to it (https://www.mkdocs.org/user-guide/custom-themes/#navigation-objects, example how it is specififed in the configuration: https://www.mkdocs.org/user-guide/writing-your-docs/#configure-pages-and-navigation).

So that could be something like:

[
    {"text": "Caption 1", type: "caption", children: [
        {"title": "Subpage 1", "url": "subpage1.html", "active": True, "children": [], type: "ref"},
        {"title": "Subpage 2", "url": "subpage2.html", "active": False, "children": [], type: "ref"},
    ]},
    {"text": "Caption 2", type: "caption", children: [
        {"title": "Subpage 3", "url": "subpage3.html", "active": False, "children": [], type: "ref"}
    ]}
]

On the one hand: this might make it easier to loop through if you want to make separate html lists under each caption.
(right now in sphinx-book-theme it seems you make one large list, and make the captions a special list item ("navbar-spacial") to have it render differently?) So it might depend on how you want to layout the html which structure is easiest to work with.

On the other hand: this might give rise to corner cases, like: what to do if you have multiple .. toctree:: entries on one page where some have a caption and others not?

@jorisvandenbossche
Copy link
Member

Implementation wise random thought: it seems that all toctrees are available in the app.env.tocs (since you are using that now for finding the caption names), so I am wondering if we couldn't build up the full list of dicts navigation object for the full site once (instead of for every page when asked with get_nav_object).
Then we would only need to update the "active" keys for a certain page, and prune it for the levels that are not needed for that page, I assume (although this might be more complex than letting sphinx give the toctree for the current page with this pruning already done)

(that's a larger refactor, though)

@jorisvandenbossche
Copy link
Member

@choldgraf from the point of view of sphinx-book-theme's customizations, do you have a preference for the flat vs nested layout for the dicts/lists?

@choldgraf
Copy link
Collaborator Author

choldgraf commented Jun 30, 2020

Note that I think this will be superseded by #219 , I am happy to re-implement this once that is merged if we wish. I think it will be simpler with that implementation

@jorisvandenbossche
Copy link
Member

Done in #346

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants