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

Merge in upstream changes from mkdocs-material #338

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

Conversation

jbms
Copy link
Owner

@jbms jbms commented Apr 11, 2024

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.

@jbms jbms requested a review from 2bndy5 April 11, 2024 04:24
@jbms jbms force-pushed the merge-upstream branch 3 times, most recently from 7b53c8b to 593856d Compare April 11, 2024 05:49
@2bndy5

This comment was marked as resolved.

@jbms

This comment was marked as outdated.

@jbms jbms force-pushed the merge-upstream branch 2 times, most recently from 0550769 to 076acb1 Compare April 11, 2024 06:13
@jbms

This comment was marked as resolved.

@2bndy5

This comment was marked as off-topic.

@2bndy5

This comment was marked as resolved.

@2bndy5

This comment was marked as resolved.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 11, 2024

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
Copy link
Owner Author

jbms commented Apr 11, 2024

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
Copy link
Collaborator

2bndy5 commented Apr 11, 2024

I won't force push anything

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

@2bndy5

This comment was marked as resolved.

@jbms
Copy link
Owner Author

jbms commented Apr 11, 2024

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.

@jbms

This comment was marked as resolved.

@2bndy5

This comment was marked as outdated.

@2bndy5
Copy link
Collaborator

2bndy5 commented Apr 11, 2024

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

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

<!-- 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
Copy link
Owner Author

jbms commented Apr 11, 2024

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
Copy link
Collaborator

2bndy5 commented Apr 11, 2024

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
Copy link
Owner Author

jbms commented Apr 11, 2024

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

@2bndy5

This comment was marked as resolved.

@2bndy5

This comment was marked as resolved.

@2bndy5 2bndy5 force-pushed the merge-upstream branch 2 times, most recently from ab3e22a to 3748ea9 Compare October 4, 2024 10:09
Copy link

codecov bot commented Oct 4, 2024

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.

jbms and others added 22 commits October 14, 2024 01:28
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants