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

#1618 make search results reader friendly #4022

Merged

Conversation

TimKam
Copy link
Member

@TimKam TimKam commented Aug 23, 2017

Subject: Make search results reader friendly
Fixes #1618

Feature or Bugfix

  • Bugfix?

Purpose

See my comment on the related issue.
There's one fairly simply way to improve the readability of the search results without adding additional build steps or output files. Currently, the search displays results snippets by requesting the corresponding source files from the server/local file system and extracting the text from them. It's possible to adjust this functionality so that it requests the HTML files instead of the source files.

Detail

  • Then, it's fairly simple to extract the text from the HTML during client/browser runtime. Of course, this increases load sizes and computing time a bit, but I don't think the change in performance is significant.
  • This would make the pretty search results extension obsolete.
  • Can this break anything? I don't think anyone expects the search results to display the source snippets. We could of course add a configuration option and only activate the changes if the option is set to True.

Relates

request results as HTML instead of source files
retrieve preview snippet text from HTML
@TimKam
Copy link
Member Author

TimKam commented Oct 19, 2017

@tk0miya does this PR make sense to you?

@Blendify
Copy link
Contributor

Poke

@TimKam
Copy link
Member Author

TimKam commented Feb 24, 2018

@tk0miya could you provide feedback? Is this something that might get merged eventually?

@codecov
Copy link

codecov bot commented Jul 16, 2018

Codecov Report

Merging #4022 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #4022      +/-   ##
=========================================
- Coverage    82.1%   82.1%   -0.01%     
=========================================
  Files         297     303       +6     
  Lines       39827   40165     +338     
  Branches     6152    6206      +54     
=========================================
+ Hits        32701   32976     +275     
- Misses       5764    5820      +56     
- Partials     1362    1369       +7
Impacted Files Coverage Δ
sphinx/make_mode.py 0% <0%> (ø)
sphinx/search/__init__.py 96.9% <0%> (ø)
sphinx/quickstart.py 0% <0%> (ø)
sphinx/errors.py 72.72% <0%> (ø)
sphinx/__init__.py 60% <0%> (ø)
sphinx/apidoc.py 0% <0%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07e387b...1a80152. Read the comment docs.

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

Sorry for very very very late reviewing.
LGTM with nits!

suffix = '.txt';
}
$.ajax({url: DOCUMENTATION_OPTIONS.URL_ROOT + '_sources/' + item[5] + (item[5].slice(-suffix.length) === suffix ? '' : suffix),
$.ajax({url: DOCUMENTATION_OPTIONS.URL_ROOT + item[0] + '.html',
Copy link
Member

Choose a reason for hiding this comment

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

We have to give a name of extension as a template variable. Sometimes it might be changed by html_file_suffix.
http://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_file_suffix

@tk0miya
Copy link
Member

tk0miya commented Jul 17, 2018

In addition, the warning of html_copy_source should be removed.
http://www.sphinx-doc.org/en/master/usage/configuration.html#confval-html_copy_source

Copy link
Member

@tk0miya tk0miya left a comment

Choose a reason for hiding this comment

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

LGTM!

@tk0miya tk0miya modified the milestones: 1.8.0, 2.0 Aug 19, 2018
@TimKam
Copy link
Member Author

TimKam commented Aug 27, 2018

@tk0miya Can this be merged? (Just to be sure)

@tk0miya
Copy link
Member

tk0miya commented Aug 28, 2018

Yes, of course :-)
After merged, please add an entry for CHANGES file.

@TimKam
Copy link
Member Author

TimKam commented Aug 28, 2018

Great. I added the change info on this branch and suppose I need to control-merge again eventually to make the build pass.

@tk0miya
Copy link
Member

tk0miya commented Aug 29, 2018

Coud you merge HEAD of master again please?
Then tests will be passed.

@TimKam TimKam merged commit bc02abc into sphinx-doc:master Aug 29, 2018
Blendify added a commit to Blendify/sphinx that referenced this pull request Apr 22, 2021
This seems to have been a mistake with sphinx-doc#4022 the ajax call functions correctly without the source files being included in the build (they are never used).

I have tested this out on several themes and now everything works correctly with `html_copy_source = False`
Blendify added a commit to Blendify/sphinx that referenced this pull request Apr 23, 2021
This seems to have been a mistake with sphinx-doc#4022 the ajax call functions correctly without the source files being included in the build (they are never used).

I have tested this out on several themes and now everything works correctly with `html_copy_source = False`
tk0miya pushed a commit that referenced this pull request Apr 24, 2021
This seems to have been a mistake with #4022 the ajax call functions correctly without the source files being included in the build (they are never used).

I have tested this out on several themes and now everything works correctly with `html_copy_source = False`
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HTML Search results aren't reader friendly
3 participants