Merge in upstream changes from mkdocs-material #338

jbms · 2024-04-11T04:24:07Z

This replaces the modified Sphinx search implementation with the lunr.js-based search implementation from the upstream mkdocs-material theme.

Maintaining the existing custom search was proving to be untenable. Using the upstream search implementation greatly reduces the size of the diff and should make it much easier to incorporate future changes.

The downsides are:

Search index is significantly bigger. However, individual pages no longer need to be fetched to display detailed search result info; consequently, the net effect may not be that large.
The Sphinx search implementation had special support for sphinx object descriptions which was especially helpful for API documentation.

Remaining work before merging this:

Update documentation to describe new features/changes
Fix search index for incremental builds
Better integration of Sphinx "objects" into the lunr.js-based search:
- typing the name of an API symbol often does not give that API symbol as the first result; we will need to fix that.
- search results for Sphinx objects displayed specially (e.g. tagged with "Python function") in the old search implementation. We should add something similar to the new search implementation.

2bndy5 · 2024-04-11T06:48:37Z

I hope you don't mind if I push some commits to your branch. I won't force push anything, so you keep squashing the history as you see fit.

jbms · 2024-04-11T07:04:13Z

I hope you don't mind if I push some commits to your branch. I won't force push anything, so you keep squashing the history as you see fit.

Yes please go ahead!

2bndy5 · 2024-04-11T07:53:13Z

I won't force push anything

I lied. I screwed up the typing on a commit I didn't run through mypy first...

jbms · 2024-04-11T16:36:45Z

I noticed you fixed some lint issues in the search plugin (which I copied unmodified from upstream except for replacing mkdocs with mkdocs_compat) --- unless they are necessary bug fixes it would be better to revert those to make it easier to merge changes in the future.

Similarly we can just check for the HTML builder in our own search extension sphinx_immaterial/search.py rather than modifying the upstream plugin.

2bndy5 · 2024-04-11T20:23:20Z

Locally, I've been playing with nav_adapt.py to experiment with page icons in the global ToC. It works if I hard-code the icon like so:

    def get_result(self) -> MkdocsNavEntry:
        return MkdocsNavEntry(
            title_text=cast(str, self._rendered_title_text),
            url=self._url,
            children=self._children,
            active=False,
            current=False,
            caption_only=False,
            meta={"icon": "material/emoticon-happy"},
            is_page=True,
        )

Where the is_page is only used in the nav-item.html

sphinx-immaterial/src/templates/partials/nav-item.html

Lines 44 to 45 in e439e1c

    
           <!-- Navigation link icon --> 
        
           {% if nav_item.is_page and nav_item.meta.icon %}

sphinx-immaterial/src/templates/partials/nav-item.html

Lines 59 to 60 in e439e1c

    
           <!-- Navigation link status --> 
        
           {% if nav_item.is_page and nav_item.meta.status %}

But I'm having trouble catching the metadata (field or directive) during the _TocVisitor process.

jbms · 2024-04-11T21:49:44Z

The _TocVisitor only has access to what is extracted from sphinx.environment.adapters.toctree.TocTree. I think you can access all of the doctrees from the builder, though, and from there extract the metadata for a given page.

The mkdocs-material theme embeds each svg inline. Potentially we could instead leverage our inline_icon mechanism to avoid duplicating them. Then we could insert them via the icon_html field that I added (needed to support the object description icons)

2bndy5 · 2024-04-11T23:14:50Z

It would be nice if we could support both, but I understand the bloated HTML concern with upstream's approach. Maybe we should just continue tracking this feature in #56 and table it for now.

IIRC, I had a branch that attempted to do the :si-icon: approach, but the ToC captions were an obstacle (aside from the astext() + _inject_wbr() usage).

jbms · 2024-04-11T23:22:42Z

To be clear we could support specifying the icon in the metadata, and still use our inline icon implementation for it.

codecov · 2024-10-04T10:33:05Z

Codecov Report

Attention: Patch coverage is 81.17284% with 61 lines in your changes missing coverage. Please review.

Project coverage is 81.56%. Comparing base (da75ebe) to head (1f2a86a).

Files with missing lines	Patch %	Lines
sphinx_immaterial/plugins/search/plugin.py	75.84%	57 Missing ⚠️
sphinx_immaterial/nav_adapt.py	80.00%	3 Missing ⚠️
sphinx_immaterial/mkdocs_compat/utils.py	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #338      +/-   ##
==========================================
+ Coverage   81.46%   81.56%   +0.10%     
==========================================
  Files          68       74       +6     
  Lines        8675     8892     +217     
==========================================
+ Hits         7067     7253     +186     
- Misses       1608     1639      +31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

This replaces the modified Sphinx search implementation with the lunr.js-based search implementation from the upstream mkdocs-material theme.

Maybe there's a more elegant way to approach this, but I'm just hacking this into something passable for CI

and add link to upstream docs

add info to doc and mention the `hide-edit-link` metadata for overriding per page

also resolves #337 by updating the link to upstream's source folder

revert lint fixes to plugins/search/plugin.py Fix spurious exception when another error has occurred.

probably a relic from the merge script rebase

also ran `npm audit fix` which bumped `node_modules/tar` and `tar` in package-lock

add `search-exclude` and `search-boost` metadata fields add test for search metadata

add test

jbms requested a review from 2bndy5 April 11, 2024 04:24

jbms force-pushed the merge-upstream branch 3 times, most recently from 7b53c8b to 593856d Compare April 11, 2024 05:49