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

Add compatibility for sphinx4 #1123

Merged
merged 24 commits into from Jul 19, 2021
Merged

Add compatibility for sphinx4 #1123

merged 24 commits into from Jul 19, 2021

Conversation

Blendify
Copy link
Member

@Blendify Blendify commented Apr 10, 2021

Sphinx 4.x moves javascript and css declarations to script_files and css_files. This patch fixes duplicated link and script tags when using sphinx4

Sphinx 4.x changes the default setting of html_codeblock_linenos_style to 'inline' and deprecates html_codeblock_linenos_style. The css for .hll to make the highlight span the full line causes the line number and code to be split on separate lines. I could not get the highlight to span the full line so it was removed completely. I also made line numbers not selectable by the user.

These changes also add the needed changes to the tox testing enviorment alhough, sphinx4 is not officially released to pypi yet. You will need to test these changes locally using: pip install git+https://github.com/sphinx-doc/sphinx@master

Sphinx 4.x moves javascript and css declarations to `script_files` and `css_files`. This patch fixes duplicatd link and script tags when using sphinx4

Sphinx 4.x changes the default setting of html_codeblock_linenos_style to 'inline' and deprecates `html_codeblock_linenos_style`. The css for `.hll` to make the hightling span the full line cause the line number and code to be split on seperate lines. I could not get the highlight to span the full line so it was removed completely. I also made line numbers not selectible by the user.

This changes also adds the needed changes to the tox testing enviorment alhough, sphinx4 is not officially released to pypi yet. You will need to test these changes locally using: `pip install git+https://github.com/sphinx-doc/sphinx@master`
@Blendify Blendify added this to the 1.0 milestone Apr 10, 2021
@Blendify Blendify requested review from agjohnson, stsewd and a team April 10, 2021 20:53
@agjohnson
Copy link
Collaborator

agjohnson commented Jun 24, 2021

On a quick QA pass across latest in each 1.x, 2.x, 3.x, and 4.x, I found the following issues strictly on Sphinx 4 output (1.8, 2.4, 3.5 were all similar in comparison):

  1. Mathjax looks broken
  2. Line numbers in numbered code blocks are selectable and have a different background color
  3. Color changed on API level 1 element, identifier text is not dark gray anymore and is instead blue

The following examples are Sphinx 3.5 on the left, 4.0 on the right.

1

image

2

image
image

3

image

Copy link
Collaborator

@agjohnson agjohnson left a comment

Choose a reason for hiding this comment

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

I noted some display QA issues separately, but this looks really close. I was expecting more changes. Seems there might be some issue with the JS injection at least, given Mathjax was not happy.

sphinx_rtd_theme/layout.html Outdated Show resolved Hide resolved
src/sass/_theme_rst.sass Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@Blendify
Copy link
Member Author

I am unable to figure out why the tests aren't passing for older sphinx versions, I tested locally and cannot reproduce. If anyone has any ideas here let me know.

@agjohnson
Copy link
Collaborator

Actually, I'm not sure how the new versions were working. I believe the issue was that the test cases were not using sphinx_rtd_theme in extensions = [], so the application event wasn't even being attached. I guess new Sphinx must do this automatically somewhere -- app.add_html_theme perhaps?

Pushed up a fix, but only tested against a few versions. CI can do the rest

@agjohnson
Copy link
Collaborator

Following up on the issues I reported:

  • Mathjax looks broken

I see the issue now. Sphinx 4 seems to request mathjax JS from jsdelivr. uBlock was blocking this for me, allowing solved the issue

  • Display inconsistency on code blocks with line numbers

I didn't aim for parity here, as the first and last line number block are not easily selectable, but it's very close.

image

I noticed the selectable line numbers still, but rebuilding assets did resolve the selectable text issue locally.

  • API level 1 element colors wrong

Looks good! I do see a minor spacing issue though.

@Blendify
Copy link
Member Author

Actually, I'm not sure how the new versions were working. I believe the issue was that the test cases were not using sphinx_rtd_theme in extensions = [], so the application event wasn't even being attached. I guess new Sphinx must do this automatically somewhere -- app.add_html_theme perhaps?

Pushed up a fix, but only tested against a few versions. CI can do the rest

Is it still possible that users in production will come across the same issue?


I also made a PR for sphinx to expose the sphinx version tuple see: sphinx-doc/sphinx#9447

Looks good! I do see a minor spacing issue though.

This seems to be from a mixture of different font types. I have fixed the issue in my recent commit.


In the future, I would like to completely redo the API styles in a much more and cleaner way; both visually and the code.


Is there anything else still standing?

I have noticed there is sometimes an issue with the logo not working but I think we can fix that with #1171

Blendify and others added 3 commits July 14, 2021 16:48
This fixes the different spacing issues that were noted.
Many of our PRs have out of date assets, making it hard to test the
latest changes. This will at least throw an error on the PR when build
assets aren't updated.
@agjohnson
Copy link
Collaborator

agjohnson commented Jul 15, 2021

Is it still possible that users in production will come across the same issue?

Like users not using shinx_rtd_theme as an extension? Yes seems that's a problem with the current implementation, it does require the theme is used as an extension. Might need to rethink this.

Edit: rethought. See e6bd3f0

This seems to be from a mixture of different font types. I have fixed the issue in my recent commit.

Font types do look different going back to look at 0.5.2 release docs, but don't seem to be the issue I'm noticing:

Sphinx 1.8
image

Sphinx 4.0
image

Theme version 0.5.2 looks like Sphinx 1.8 above though. Sphinx 4 seems to have different class names on the domain elements.

Remaining

  • Resolve differences in class names for domain objects

1.8:
image

4.0:
image

The spacing on the element looks broken back to Sphinx 1.8 + 0.5.2, so not a pressing thing for this PR. It doesn't look great though, so worth fixing separately, maybe for 1.0, or 1.1

@agjohnson
Copy link
Collaborator

I included ee32a53 here from #1175 to help with out of date assets.

@agjohnson
Copy link
Collaborator

I implemented the version string -> version_info tuple conversion in Jinja instead. It's not great, but Jinja doesn't have native filters for tuples 🤷

@Blendify
Copy link
Member Author

@agjohnson so the only thing currently remaining is to increase the padding on the right side of the class names? I am not sure exactly what you meant by your comment.

@agjohnson
Copy link
Collaborator

The padding is wrong in Sphinx 1.8 and theme 0.5.2. Sphinx 4 and 0.5.2 has the correct padding. We'll want to come back to that, it's been broken for a while.

The last issue I noted are the difference in font styles, note the italicized font style on keywords that are highlighted.

@Blendify
Copy link
Member Author

The difference in font style comes down to in sphinx 1.8 these keywords were given the property class name, in sphinx 4.0 these are just given the class name k I could just add the k class to the property css selector however I am not sure how predictable the class k is used. I am not sure if this is a bug in sphinx or the desired change but in sphinx 4.0 it is much harder to select that keyword in a predictable way.

Perhaps we should report the issue to sphinx to hear back if this is a bug or not.

The other thing I am going to do is make a PR for the theme to replace the API demo documentation with examples from https://www.sphinx-doc.org/en/master/usage/restructuredtext/domains.html to ensure we are writing the API signatures in the expected way. Looking at the page it looks like it expecting const to be at the end of the line.

@agjohnson
Copy link
Collaborator

The k looks familiar to me, this is what I'm used to dealing with from Pygments. I wasn't familiar with the previous class names actually, but assume they come from Pygments too. You can probably find a list of these short class names in Pygment docs. This doesn't seem like a bug.

Here's an example CSS output from Pygments style, you can see the class names aligning with a descriptive name of the type:
https://github.com/richleland/pygments-css/blob/master/default.css

I'd probably say we'd want to keep the API examples the same for now, so we can trace back errors through releases of the theme. Right now you can go through our previous release docs and see the old examples of API docs.

We did discuss building out a wider test matrix for QA testing the theme, and I could see us having stronger QA test docs there.

@Blendify
Copy link
Member Author

@agjohnson okay hopefully everything is good to go now.

@agjohnson
Copy link
Collaborator

Agreed, that looks good now. We should probably trace back the spacing issue on these, but I see that issue in 0.5.2 anyways, that's not a Sphinx4 change -- actually Sphinx4 looks correct, so 🤷

Thanks for pushing this forward @Blendify ! If we can close the remaining issues, we should be able to get an RC out this week I think.

@Blendify
Copy link
Member Author

@agjohnson I am okay pushing everything else back to the next RC or 1.1 with the exception of #1171 and maybe #813

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.

None yet

3 participants