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

検索機能の復旧 #123

Merged
merged 1 commit into from
Nov 30, 2024

Conversation

attakei
Copy link
Contributor

@attakei attakei commented Nov 30, 2024

概要

Sphinxjpサイト内の検索ページで、フロントエンド上での動作がしていない事象を改善するPRです。
(おそらくもう少し根本から対応したほうが良さそうなのですが、ひとまずパッチ的にPRを出しています)

現状について

概要の通り、SphinxjpのサイトでSphinx機能による検索が適切に動作していません。具体的には、検索を行おうとしても、検索ページへの遷移はするものの、「検索中…」の表示までで止まり検索結果が出ません。

想定している原因

おそらく、次のようなプロセスでこの事象が起きています。

  1. 使用しているSphinxのバージョンが新しくなった結果、documentation_options.jsを読み込む<script>タグに必要な属性が付与されなくなった。
  2. /documentation_options.js内で使用しているdocument.querySelector(~~)が機能しなくなり、DOCUMENTATION_OPTIONSの変数宣言自体に失敗した。
  3. DOCUMENTATION_OPTIONS を使用しているSphinxの各種JS処理が全滅した(?)。少なくとも、検索は機能しなくなった。

対処

  • source/theme/sphinxjp/static内のファイルのうち、「検索時に使用しており」「現行のSphinxのbasicテーマと実装レベルで差異がある」ファイルを除外。(basicテーマ内のものを使用する)
  • 上記の作業に合わせて、HTML側にsearchtools.jsの動作に必要な属性を追加。

※いずれも、「ローカルで検索機能が復旧した」レベルでの確認した範囲での修正内容。

- テーマ内のsearch系staticfilesの構造が適切に動作しないため除外して、
  Sphinx本体のassetsを利用する。
- 現行のsearchtoolsに合わせる形で、HTMLのいくつかの箇所に属性を追加する。
- basicテーマに付属している、他の事項については一旦何もしない。
Copy link
Member

@shimizukawa shimizukawa left a comment

Choose a reason for hiding this comment

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

修正ありがとうございます(気付いていませんでした・・)
コメントしました。わからないところがあるので教えてください

@@ -1,14 +0,0 @@
var DOCUMENTATION_OPTIONS = {
Copy link
Member

Choose a reason for hiding this comment

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

質問: このあたりの最近の変化を分かっていないので消した経緯を教えてください。

このファイルは最新Sphinxでは documentation_options.js.jinja というファイル名になっています。
消してしまって大丈夫でしょうか?
https://github.com/sphinx-doc/sphinx/blob/b458850b329f3112b6e8df7a19ff21fa478fb2d3/sphinx/themes/basic/static/documentation_options.js.jinja

Copy link
Contributor Author

Choose a reason for hiding this comment

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

消してしまって大丈夫でしょうか?

実際のビルド結果と簡単なSphinxの参照をした限り、消しても大丈夫な想定です。

  • HTMLビルダーがテーマのロードをする際に継承元を全部認識したうえで、staticフォルダー内をコピーする際には継承元のフォルダ→継承先のフォルダの順にコピーする。
    (今回だと、basicの中身→sphinxjpの中身)※ref1, ref2
  • コピー時に末尾が.jinjaとなっていると、Jinja2テンプレートとみなして処理する。
    (以前からある_t末尾のときと同じ)※ref

今のビルドだと下のような動きになっています。

  1. basic/static/documentation_options.js.jinjaから_static/documentation_options.jsを生成
  2. sphinxjp/static/documentation_options.js_tから_static/documentation_options.jsを生成
    (1で生成したファイルを上書き)

消した経緯

当時のテーマ周りの事情は把握していないですが、現状のバージョンにおける上記構造を踏まえて「削除が良さそう」と判断しています。

  • カスタムテーマになくても、basicを継承しているので必要なファイルはコピーできる。
  • layout.htmlへの編集を行うことで、継承元のsearchtools.jsは動作させられる。
  • であれば、メンテナンス的にもbasicにある方を使ったほうが良さそう。

Copy link
Member

Choose a reason for hiding this comment

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

理解しました。ありがとうございます。
手元でも動作を確認できたので、大丈夫そうです。

@@ -1,566 +0,0 @@
/*
* searchtools.js
Copy link
Member

Choose a reason for hiding this comment

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

searchtools.js も消しちゃって大丈夫なものでしょうか?
最新Sphinxにも存在しています。
https://github.com/sphinx-doc/sphinx/blob/b458850b329f3112b6e8df7a19ff21fa478fb2d3/sphinx/themes/basic/static/searchtools.js

@@ -78,7 +78,7 @@
</div>
</div>
{%- endmacro -%}
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="ja" lang="ja">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="ja" lang="ja" data-content_root="{{ content_root }}">
Copy link
Member

Choose a reason for hiding this comment

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

なるほど、7.2.0 からある属性なんですね。
sphinx-doc/sphinx@8e730ae

Copy link
Member

@shimizukawa shimizukawa left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -1,14 +0,0 @@
var DOCUMENTATION_OPTIONS = {
Copy link
Member

Choose a reason for hiding this comment

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

理解しました。ありがとうございます。
手元でも動作を確認できたので、大丈夫そうです。

@shimizukawa shimizukawa merged commit 71b6ef0 into sphinxjp:master Nov 30, 2024
1 check passed
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