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

Fix: more efficient determination of when to hide primary sidebar #1609

Merged
merged 6 commits into from
Jan 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,15 @@
<nav class="bd-docs-nav bd-links"
aria-label="{{ _('Section Navigation') }}">
<p class="bd-links__title" role="heading" aria-level="1">{{ _("Section Navigation") }}</p>
<div class="bd-toc-item navbar-nav">{{ sidebar_nav_html }}</div>
<div class="bd-toc-item navbar-nav">
{{- generate_toctree_html(
"sidebar",
show_nav_level=theme_show_nav_level|int,
maxdepth=theme_navigation_depth|int,
collapse=theme_collapse_navigation|tobool,
includehidden=True,
titles_only=True
)
-}}
</div>
</nav>
21 changes: 5 additions & 16 deletions src/pydata_sphinx_theme/theme/pydata_sphinx_theme/layout.html
Original file line number Diff line number Diff line change
Expand Up @@ -6,23 +6,8 @@
{% endset %}
{%- extends "basic/layout.html" %}
{%- import "static/webpack-macros.html" as _webpack with context %}
{# Metadata and asset linking #}
{# Create the sidebar links HTML here to re-use in a few places #}
{# If we have no sidebar links, pop the links component from the sidebar list #}
{%- set sidebar_nav_html = generate_toctree_html("sidebar",
show_nav_level=theme_show_nav_level|int,
maxdepth=theme_navigation_depth|int,
collapse=theme_collapse_navigation|tobool,
includehidden=True,
titles_only=True)
-%}
{% if sidebar_nav_html | length == 0 %}
{% set sidebars = sidebars | reject("in", "sidebar-nav-bs.html") | list %}
{% endif %}
Comment on lines -19 to -21
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this conditional was moved later, to just before its results are actually needed.

{# A flag for whether we include a secondary sidebar based on the page metadata #}
{% set remove_sidebar_secondary = (meta is defined and meta is not none
and 'html_theme.sidebar_secondary.remove' in meta)
or not theme_secondary_sidebar_items %}
{% set remove_sidebar_secondary = (meta is defined and meta is not none and 'html_theme.sidebar_secondary.remove' in meta) or not theme_secondary_sidebar_items %}
{%- block css %}
{# The data-cfasync attribute disables CloudFlare's Rocket loader so that #}
{# mode/theme are correctly set before the browser renders the page. #}
Expand Down Expand Up @@ -96,6 +81,10 @@
<div class="bd-container">
<div class="bd-container__inner bd-page-width">
{# Primary sidebar #}
{# If we have no sidebar TOC, pop the TOC component from the sidebar list #}
{% if get_sidebar_toctree_length(show_nav_level=theme_show_nav_level|int) == 0 %}
{% set sidebars = sidebars | reject("in", "sidebar-nav-bs.html") | list %}
{% endif %}
Comment on lines +84 to +87
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

here's where that conditional ended up. It now calls get_sidebar_toctree_length which looks at the unrendered toctree

<div class="bd-sidebar-primary bd-sidebar{% if not sidebars %} hide-on-wide{% endif %}">
{% include "sections/sidebar-primary.html" %}
</div>
Expand Down
87 changes: 51 additions & 36 deletions src/pydata_sphinx_theme/toctree.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
"""Methods to build the toctree used in the html pages."""

from functools import lru_cache
from functools import cache
from itertools import count
from typing import Iterator, List, Union
from urllib.parse import urlparse
Expand Down Expand Up @@ -32,12 +32,48 @@
)


def get_unrendered_local_toctree(
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this is all copy-pasted from index_toctree at the end of this file (which now calls this helper). Needed to move to the beginning of the file so that get_sidebar_toctree_length can also call it.

app: Sphinx, pagename: str, startdepth: int, collapse: bool = True, **kwargs
):
"""."""
if "includehidden" not in kwargs:
kwargs["includehidden"] = False
if kwargs.get("maxdepth") == "":
kwargs.pop("maxdepth")

Check warning on line 42 in src/pydata_sphinx_theme/toctree.py

View check run for this annotation

Codecov / codecov/patch

src/pydata_sphinx_theme/toctree.py#L42

Added line #L42 was not covered by tests

toctree = TocTree(app.env)
if sphinx.version_info[:2] >= (7, 2):
from sphinx.environment.adapters.toctree import _get_toctree_ancestors

ancestors = [*_get_toctree_ancestors(app.env.toctree_includes, pagename)]
else:
ancestors = toctree.get_toctree_ancestors(pagename)

Check warning on line 50 in src/pydata_sphinx_theme/toctree.py

View check run for this annotation

Codecov / codecov/patch

src/pydata_sphinx_theme/toctree.py#L50

Added line #L50 was not covered by tests
try:
indexname = ancestors[-startdepth]
except IndexError:
# eg for index.rst, but also special pages such as genindex, py-modindex, search
# those pages don't have a "current" element in the toctree, so we can
# directly return an empty string instead of using the default sphinx
# toctree.get_toctree_for(pagename, app.builder, collapse, **kwargs)
return ""

return get_local_toctree_for(
toctree, indexname, pagename, app.builder, collapse, **kwargs
)


def add_toctree_functions(
app: Sphinx, pagename: str, templatename: str, context, doctree
) -> None:
"""Add functions so Jinja templates can add toctree objects."""

@lru_cache(maxsize=None)
def get_sidebar_toctree_length(
startdepth: int = 1, show_nav_level: int = 1, **kwargs
):
toctree = get_unrendered_local_toctree(app, pagename, startdepth)
return 0 if toctree is None else len(toctree)

@cache
def get_or_create_id_generator(base_id: str) -> Iterator[str]:
for n in count(start=1):
if n == 1:
Expand All @@ -53,7 +89,7 @@
"""
return next(get_or_create_id_generator(base_id))

@lru_cache(maxsize=None)
@cache
def generate_header_nav_before_dropdown(n_links_before_dropdown):
"""The cacheable part."""
try:
Expand Down Expand Up @@ -191,7 +227,7 @@

# Cache this function because it is expensive to run, and because Sphinx
# somehow runs this twice in some circumstances in unpredictable ways.
@lru_cache(maxsize=None)
@cache
def generate_toctree_html(
kind: str, startdepth: int = 1, show_nav_level: int = 1, **kwargs
) -> Union[BeautifulSoup, str]:
Expand Down Expand Up @@ -241,7 +277,7 @@
if kind == "sidebar":
# Add bootstrap classes for first `ul` items
for ul in soup("ul", recursive=False):
ul.attrs["class"] = ul.attrs.get("class", []) + ["nav", "bd-sidenav"]
ul.attrs["class"] = [*ul.attrs.get("class", []), "nav", "bd-sidenav"]

# Add collapse boxes for parts/captions.
# Wraps the TOC part in an extra <ul> to behave like chapters with toggles
Expand Down Expand Up @@ -276,7 +312,7 @@

return soup

@lru_cache(maxsize=None)
@cache
def generate_toc_html(kind: str = "html") -> BeautifulSoup:
"""Return the within-page TOC links in HTML."""
if "toc" not in context:
Expand All @@ -289,22 +325,22 @@
if ul is None:
return
if level <= (context["theme_show_toc_level"] + 1):
ul["class"] = ul.get("class", []) + ["visible"]
ul["class"] = [*ul.get("class", []), "visible"]
for li in ul("li", recursive=False):
li["class"] = li.get("class", []) + [f"toc-h{level}"]
li["class"] = [*li.get("class", []), f"toc-h{level}"]
Comment on lines +328 to +330
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

these (and similar changes below) were suggested by Ruff in my IDE, and TBH I did it mostly to make the squiggly underlines go away, they were distracting me. I guess there's probably a very small performance benefit?

add_header_level_recursive(li.find("ul", recursive=False), level + 1)

add_header_level_recursive(soup.find("ul"), 1)

# Add in CSS classes for bootstrap
for ul in soup("ul"):
ul["class"] = ul.get("class", []) + ["nav", "section-nav", "flex-column"]
ul["class"] = [*ul.get("class", []), "nav", "section-nav", "flex-column"]

for li in soup("li"):
li["class"] = li.get("class", []) + ["nav-item", "toc-entry"]
li["class"] = [*li.get("class", []), "nav-item", "toc-entry"]
if li.find("a"):
a = li.find("a")
a["class"] = a.get("class", []) + ["nav-link"]
a["class"] = [*a.get("class", []), "nav-link"]

# If we only have one h1 header, assume it's a title
h1_headers = soup.select(".toc-h1")
Expand Down Expand Up @@ -342,6 +378,7 @@

context["unique_html_id"] = unique_html_id
context["generate_header_nav_html"] = generate_header_nav_html
context["get_sidebar_toctree_length"] = get_sidebar_toctree_length
context["generate_toctree_html"] = generate_toctree_html
context["generate_toc_html"] = generate_toc_html
context["navbar_align_class"] = navbar_align_class
Expand All @@ -368,7 +405,7 @@
continue

# Add a class to indicate that this has children.
element["class"] = classes + ["has-children"]
element["class"] = [*classes, "has-children"]

# We're gonna add a checkbox.
toctree_checkbox_count += 1
Expand Down Expand Up @@ -455,29 +492,7 @@
# returning:
# return self.render_partial(TocTree(self.env).get_toctree_for(
# pagename, self, collapse, **kwargs))['fragment']

if "includehidden" not in kwargs:
kwargs["includehidden"] = False
if kwargs.get("maxdepth") == "":
kwargs.pop("maxdepth")

toctree = TocTree(app.env)
if sphinx.version_info[:2] >= (7, 2):
from sphinx.environment.adapters.toctree import _get_toctree_ancestors

ancestors = [*_get_toctree_ancestors(app.env.toctree_includes, pagename)]
else:
ancestors = toctree.get_toctree_ancestors(pagename)
try:
indexname = ancestors[-startdepth]
except IndexError:
# eg for index.rst, but also special pages such as genindex, py-modindex, search
# those pages don't have a "current" element in the toctree, so we can
# directly return an empty string instead of using the default sphinx
# toctree.get_toctree_for(pagename, app.builder, collapse, **kwargs)
return ""

toctree_element = get_local_toctree_for(
toctree, indexname, pagename, app.builder, collapse, **kwargs
toctree_element = get_unrendered_local_toctree(
app, pagename, startdepth, collapse, **kwargs
)
return app.builder.render_partial(toctree_element)["fragment"]
Loading