-
-
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
LaTeX: make contents, topic, and sidebar separately customizable #12704
LaTeX: make contents, topic, and sidebar separately customizable #12704
Conversation
And give them some nice defaults to start with.
768c32c
to
e903f38
Compare
b6a5506
to
0f1d446
Compare
I'm not qualified to review this -- @jfbu please merge when you're happy! A |
@AA-Turner Do you think this can be part of a 8.0.d release or should wait 8.1.0? i.e. If merging now, should I create a 8.0.3 entry with a Feature added sub-section? |
This will go into 8.1 -- 0.0.X releases are bug-fix only for problems in feature releases. There's no reason we can't do Sphinx 8.1 fairly soon though, perhaps end of August or early September. A |
Yes, understood. Thanks. |
I have prepared the CHANGES entry for 8.1.0. (in a prior comment I had not yet seen that there was a 8.1.0 section ready after recent refactoring of CHANGES.rst). |
I'm interested in reviewing it but only next week (those changes are too hard to review on a phone). Could you wait until then please? |
@picnixz Thanks! I will thus officially request your review 😃 but don't feel time-pressured I was targeting perhaps September or October or whenever we do a release with new features. One aspect is that contrarily to two years ago at 5.1.0 when we added much customizability to admonitions, topics/contents (still then handled via idenical rendering, due to legacy situation which this PR lifts) and code-blocks (#10648), this time for this last piece (which is relatively minor addition which uses existing code-base to separate topics/contents/sidebar into three independent entities), the new defaults do exploit the new capacities. I think PDF output from Sphinx should come "batteries-included" else big projects which concentrates most of their efforts on how HTML/ePub look like and lack LaTeX-self-confident workforce will never use what we offer for PDF output. |
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.
Could we have some tests? just to check that the arguments are correctly forwarded? it's sometimes hard to spot a typo in LaTeX, especially when the IDE support is not good.
doc/latex.rst
Outdated
|
||
Default: ``5pt`` | ||
At 8.1.0 it became possible to style separately the :dudir:`topic`, |
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.
At 8.1.0 it became possible to style separately the :dudir:`topic`, | |
Since Sphinx 8.1.0, it is now possible to style separately the :dudir:`topic`, |
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.
Don't want to nit-pick, not native speaker but once I adopt Since
shouldn't I drop the now
?
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.
Err.. yes, it sounds better (I'm also not an English native).
doc/latex.rst
Outdated
|
||
See :ref:`additionalcss` for ``div.topic_box-shadow`` which allows to | ||
configure separately the widths of the vertical and horizontal shadows. | ||
.. versionchanged:: 8.1.0 New separate defaults. See :ref:`additionalcss`. |
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 don't know whether it should be a versionchanged or a deprecated since they are kept for backward compatibility.
Also, I would separate 8.1.0 from "New":
.. versionchanged:: 8.1.0
New...
Same obversation for the other .. version*
directives that you added.
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 don't know whether it should be a versionchanged or a deprecated since they are kept for backward compatibility.
I will check that.
Also, I would separate 8.1.0 from "New":
.. versionchanged:: 8.1.0 New...
Formerly I think I used
.. versionchanged:: X.y.z
description
and one can see many examples in doc/latex.rst. Now I am very confused about how this directive is supposed to be used.
Same obversation for the other
.. version*
directives that you added.
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.
All those versionchanged
removed in next push, and a clearer explanation in "important" box before.
doc/latex.rst
Outdated
margin. | ||
*except* for: | ||
|
||
- the contents_ directive: ``4pt 4pt``, i.e. the shadow has a width of |
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.
This might seem an overkill, but would it be possible to create a picture that displays the mdiffernt metrics, a bit like https://mirror.init7.net/ctan/macros/latex/contrib/geometry/geometry.pdf (page 3)? I think a picture would be easier to understand than a descriptive text. This can be done in a separate PR since you did not change the actual description (just put it in a bullet list).
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.
arrgghh... I am starting to think we should have used longfbox
whose doc already has nice pictures (sadly the package is broken for years because it was developed at a time pict2e
defined a \@tempdimd
but later dropped it) 😃. Well, doesn't CSS documentation sites for HTML fit the bill already? The behavior of box-shadow
is directly emulating it, with a more restricted syntax, but the way "inset" keyword or negative dimensions impact result should be exactly as in CSS. At least this is what I remember from when this was done.
doc/latex.rst
Outdated
``\sphinxstylenotetitle``; ``\sphinxdotitlerow{note}{#1}`` | ||
``\sphinxstylehinttitle``; ``\sphinxdotitlerow{hint}{#1}`` |
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.
Could you align the column? (namely align ``\sphinxdotitlerow
with ``{\footnotesize
from above)
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.
Done in next push.
@@ -113,4 +148,31 @@ | |||
\spewnotes | |||
} | |||
|
|||
% 8.1.0 | |||
\newenvironment{sphinxtopic} | |||
{\begin{sphinxShadowBox}\relax}{\end{sphinxShadowBox}} |
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.
Do you really need the \relax
? if not, maybe it's better to explicitly use [topic]
even though it's the default.
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.
The environment now admis an optional argument. I put the \relax
to be extra sure some [
will not fool it. However you are right it can not happen for topic directive as it requires a title so Sphinx will always output stuff like this
\begin{sphinxtopic}
\sphinxstyletopictitle{A}
and there can't be a problem.
Yes using [topic]
explictly is clearer for people reading this .sty
file but it is probably slightly less efficient, now that doesn't matter. If people want to use directly the environment they have to be cautious though with the [
trap, so adding the [topic]
here would help them avoid mistakes.
+0 for using [topic]
will wait my 12 hours of usual sleep for a decision 😃
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.
After some closer look, and for reasons already mentioned, the \relax
is not needed indeed. Besides Sphinx escapes [
to {[}
to avoid potential problems (mainly in lists). Then I hesitated with using the explicit [topic]
or nothing at all, and opted for nothing at all, in the end. No strong opinion.
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 personally prefer the explicit topic
because the default was only kept for backwards compatibility. In the future, we might at some point remove it or change it for whatever reasons. How do you feel about this?
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.
ok, agreed. I have pushed with [topic]
added.
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.
@picnixz For the record there is a subtle point with LaTeX environments. With \newenvironment{test}{}{}
and
a%
\begin{test}
b%
\end{test}c
you will find a bc
in output, because the EOL of \begin{test}\n
gives a space token which is not ignored as we are mid-paragraph. If now the test
environment is defined via \newenvironment{test}[1][topic]{}{}
the output will be abc
. This is because the environment searches for [
and swallows spaces doing so.
Our sphinxtopic
, sphinxcontents
, sphinxsidebar
all inject the [
and the EOL space is not swallowed. This does not matter because the paragraph is not started, so TeX ignores this space, but it sees it.
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.
Oh thank you for this! One thing I always found extremely tricky in LaTeX (and TeX) is about those space tokens and how they are dealt with... So it helps a lot when you explain it!
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.
About my example, LaTeX provides \ignorespacesafterend
which you can use when defining the end
part of an environment but no \ignorespacesatbegin
which user could put in the begin
part.
About what you say about spaces I fully agree. There is a blatant lack of a serious document "TeX/LaTeX spaces explained". With the promotion of LaTeX3 with whom programmers do not have to worry at all anymore about spaces in their source code (they get ignored by default), I am sure situation will degrade because the folks doing LaTeX "programming" have less incentives to care about spaces...
I forgot commenting about the design itself.
I have other PRs to do so I won't be able to add some visual examples now but I'll try to do it this week. |
6e6ec19
to
cb795d6
Compare
@picnixz I am not enthused by 2pt, neutral about 3pt, and more in favor of 4pt. The design is not one one sees on every page, it is only for EDIT: Forgot to say that I used .. contents::
:local: which has the side effect to suppress the title (if none is explicitly given) from the Docutils node. For the rendering with title bar see previous screenshots in this thread. |
Ok you convinced me with this one. I think 4pt is ok. Sorry for forgetting about this by the way... and thank you as always for your precise reports... I'll review the latest changes now. |
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.
Final formulation nitpicks.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Thanks a lot for your help @picnixz. I have some tasks now so will make a final check in a few hours, expecting to merge this around 9PM European summer time. |
I won't be online at that time but AFAICT, nothing else stands out. And if there are typos we need to fix, then we'll fix them in a follow-up PR. |
Merged! Thanks again for you helping hand! |
And give them some nice defaults to start with.
Subject:
Feature or Bugfix
Purpose
So far, and since dawn of time the
contents
,topic
, andsidebar
directives have been styled the same way (except perhaps for their titles which had separate customizing LaTeX macros associated with them) simply because they were mapped to the same LaTeX environment (which very long time ago was incompatible with pagebreaks).The aim of this PR is to separate them, and also to give them nice defaults immediately.
Detail
Here is how it looked at 6.1.0 (contents, then sidebar, and finally topic)
Here is how it looked at 7.4.0:
Here is how it wil look with this PR merged: