-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Make hidden toctrees append their content after everything else in their section #3622
base: master
Are you sure you want to change the base?
Conversation
d7f6526
to
38d84ca
Compare
Test failure is garbage |
This would be really neat. |
I would also like to see this workaround merged |
Does that mean this has been implemented? |
I didn't find anything in the commit or change logs. |
Sorry, It was my mistake. I just removed |
Yes, it looks like that. |
I'm obviously not involved enough to make a judgement here, but I'm unsure if that's a good idea - it means that every time you do a release, you need to find all the old PRs and update their target branch to make them useful, or at least remind their owners to. Is there a way to search by target branch? In terms of cleaning up the accidentally-closed PRs like this one, I reckon you can just search by "most recent" in the closed queue, since they'll all be simultaneous. |
@tk0miya: Was this closed accidentally again, or are you rejecting this change? |
1.7 branch was deleted. |
@tk0miya: Ping, can you reopen? |
Oh, sorry... |
I've learnt my lesson - I should target master, not the versioned releases in future |
ac2571c
to
a917fa0
Compare
Just rebased this, as I was running into the issue again. Would appreciate some feedback on whether the idea sounds good.
|
a917fa0
to
d0e2ef2
Compare
+1. Two days into Sphinx and I was immediately bitten by this. If this isn't merged, is there a work-around? To me, an alternative would have been to hide section headings from the toc, but I couldn't find a way to do that automatically. |
@tk0miya Any chance this could be reviewed? The PR has been in the pipeline for few years now. This feature is pretty vital -- the fact that you cannot sanely combine headings and a hidden ::toctree directive on the same page is quite annoying. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not a documentation expert, but I say +1 for this change.
The patch works as promised for the HTML backend, but there seems to be an ordering issue with the latexpdf backend. Please verify this before merging the patch. With this patch I can achieve my desired document layout in HTML, but it feels a bit weird to insert the hidden ToC at the top of the page, while the actual ToC entries will be inserted at the end. In my opinion, it would be cleaner to implement a fix where a ToC can remain at the end (so the order of the rendering eg. in LaTeX will remain linear), but then have the option to raise the ToC level of the toc-tree relative to its current tree depth, My guess is that the patch proposed by @topiolli tried to do something like that but it was rejected. Now that I have at least an idea where to look thanks to this PR, I will tinker a bit with the sphinx.environment.adapters.toctree.TocTree to see if I can reach a solution that does this, and is well-behaved both in HTML and PDF. |
I toyed around with the approach of PR #8847 (which was rejected), hoping that this would behave nicer in PDF. However, there too, the Latex-rendered version doesn't pick up on the re-ordering that happens in the patched adapter TocTree. It is as if LaTeX decides on a document order for its ToC before the adapter.TocTree has run. Really strange. Anyway, this shows that any patch addressing this will need to check how non-HTML backends fare with them. Also, I prefer the basic approach of #8847 as it doesn't require me to put the toctree directive in an 'unnatural' place (top of the page). But at this point, I'd be happy with any sanctioned approach that can be made to work, warts and all. |
Seems like something deeper is going wrong. The LaTeX ToC isn't properly updated, but the same is true for a HTML ToC, it seems; they both go wrong in much the same way. The only thing that /does/ get properly updated is the HTML ToC pane. |
Would you mind taking a fresh look at #8847? It tackles the same problem but doesn't change existing semantics of |
@lsaffre @boypeet you both approved this PR; but there is a known issue with at least the LateX backend (see #3622 (comment)). Much as I like a good solution to the issue at hand (being able to combine headings and toctrees on the same page with some level of usefullness) it would not be a good idea to merge something into master that doesn't properly work with a major backend. |
Yes, wow, seems that the issue is more complex than I thought when I approved. I don't use the LaTeX builder, so I cannot comment further. |
…in their section This fixes the problem reported at: http://stackoverflow.com/q/25276415/102441 http://stackoverflow.com/q/39110429/102441
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3622 +/- ##
==========================================
- Coverage 86.99% 0 -87.00%
==========================================
Files 276 0 -276
Lines 52445 0 -52445
Branches 9206 0 -9206
==========================================
- Hits 45627 0 -45627
+ Misses 5163 0 -5163
+ Partials 1655 0 -1655 ☔ View full report in Codecov by Sentry. |
@eric-wieser -- I added a test, though I'm not entirely sure it captures what you wrote (I have had to use A |
Fundamentally, this is a workaround to the fact that the following is not possible in
rst
In particular, it seems to be a common thing to want that last piece of content to be a hidden toctree linking to other pages:
http://stackoverflow.com/q/25276415/102441
http://stackoverflow.com/q/39110429/102441
Right now, the above produces
It doesn't seem acceptable to change the behaviour of visible toctrees, but for users of hidden toctrees, this was almost certainly intended behaviour anyway. This makes the following produce the intended TOC tree, with the headings in order
which before this patch gives
and after this patch gives
Not clear to me whether this is a feature or a bug fix.
If changing the behaviour of
:hidden:
is not ok, would you prefer adding a:show-last:
field?