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

LaTeX: make contents, topic, and sidebar separately customizable #12704

Merged
merged 35 commits into from
Aug 19, 2024

Conversation

jfbu
Copy link
Contributor

@jfbu jfbu commented Jul 28, 2024

And give them some nice defaults to start with.

Subject:

Feature or Bugfix

  • Feature

Purpose

So far, and since dawn of time the contents, topic, and sidebar 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)

Capture d’écran 2024-07-28 à 19 18 48

Here is how it looked at 7.4.0:

Capture d’écran 2024-07-28 à 19 17 25

Here is how it wil look with this PR merged:

Capture d’écran 2024-07-28 à 19 16 03

@jfbu jfbu added type:enhancement enhance or introduce a new feature builder:latex labels Jul 28, 2024
@jfbu jfbu added this to the 8.x milestone Jul 28, 2024
And give them some nice defaults to start with.
@jfbu jfbu force-pushed the latex_contents_vs_topic_vs_sidebar branch from 768c32c to e903f38 Compare July 28, 2024 19:01
@jfbu jfbu force-pushed the latex_contents_vs_topic_vs_sidebar branch from b6a5506 to 0f1d446 Compare July 29, 2024 08:24
@AA-Turner
Copy link
Member

I'm not qualified to review this -- @jfbu please merge when you're happy!

A

@jfbu
Copy link
Contributor Author

jfbu commented Jul 31, 2024

@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?

@AA-Turner
Copy link
Member

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

@jfbu
Copy link
Contributor Author

jfbu commented Jul 31, 2024

Yes, understood. Thanks.

@jfbu
Copy link
Contributor Author

jfbu commented Jul 31, 2024

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).

@picnixz
Copy link
Member

picnixz commented Aug 5, 2024

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?

@jfbu
Copy link
Contributor Author

jfbu commented Aug 5, 2024

@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.

@jfbu jfbu requested a review from picnixz August 5, 2024 07:02
@jfbu
Copy link
Contributor Author

jfbu commented Aug 8, 2024

I updated the sidebar top width which on further thoughs looked too thin in comparison of bottom width in particular which has a shadow. New looks:
Capture d’écran 2024-08-08 à 21 49 12

Copy link
Member

@picnixz picnixz left a 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.

CHANGES.rst Show resolved Hide resolved
doc/latex.rst Outdated

Default: ``5pt``
At 8.1.0 it became possible to style separately the :dudir:`topic`,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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`,

Copy link
Contributor Author

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?

Copy link
Member

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`.
Copy link
Member

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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
Copy link
Member

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).

Copy link
Contributor Author

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 Show resolved Hide resolved
doc/latex.rst Outdated
Comment on lines 1581 to 1582
``\sphinxstylenotetitle``; ``\sphinxdotitlerow{note}{#1}``
``\sphinxstylehinttitle``; ``\sphinxdotitlerow{hint}{#1}``
Copy link
Member

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in next push.

doc/latex.rst Outdated Show resolved Hide resolved
sphinx/texinputs/sphinx.sty Outdated Show resolved Hide resolved
@@ -113,4 +148,31 @@
\spewnotes
}

% 8.1.0
\newenvironment{sphinxtopic}
{\begin{sphinxShadowBox}\relax}{\end{sphinxShadowBox}}
Copy link
Member

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.

Copy link
Contributor Author

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 😃

Copy link
Contributor Author

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.

Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

@jfbu jfbu Aug 13, 2024

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.

Copy link
Member

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!

Copy link
Contributor Author

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...

sphinx/texinputs/sphinxlatexshadowbox.sty Show resolved Hide resolved
@picnixz
Copy link
Member

picnixz commented Aug 12, 2024

I forgot commenting about the design itself.

  • Contents: shadows are a bit too large IMO. I would halve them.
  • Sidebar: I personally don't like the new look and would I preferred the original look by default. The issue is that I cannot tell whether it's a shadow or a border... and it seems they are being mixed.

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.

@jfbu jfbu force-pushed the latex_contents_vs_topic_vs_sidebar branch from 6e6ec19 to cb795d6 Compare August 13, 2024 09:24
@jfbu
Copy link
Contributor Author

jfbu commented Aug 18, 2024

@picnixz
Here is contents with 2pt, 3pt or 4pt shadow. 4pt is the legacy setting, and the rounded corner is since 7.4.0.

Capture d’écran 2024-08-18 à 21 56 20

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 contents directive hence usually will be encountered if at all at start of chapters.

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.

@picnixz
Copy link
Member

picnixz commented Aug 19, 2024

The design is not one one sees on every page

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.

@picnixz picnixz self-requested a review August 19, 2024 07:43
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Final formulation nitpicks.

sphinx/texinputs/sphinx.sty Outdated Show resolved Hide resolved
sphinx/texinputs/sphinx.sty Outdated Show resolved Hide resolved
doc/latex.rst Outdated Show resolved Hide resolved
doc/latex.rst Outdated Show resolved Hide resolved
doc/latex.rst Outdated Show resolved Hide resolved
doc/latex.rst Outdated Show resolved Hide resolved
doc/latex.rst Outdated Show resolved Hide resolved
doc/latex.rst Outdated Show resolved Hide resolved
doc/latex.rst Outdated Show resolved Hide resolved
doc/latex.rst Show resolved Hide resolved
@jfbu
Copy link
Contributor Author

jfbu commented Aug 19, 2024

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.

@picnixz
Copy link
Member

picnixz commented Aug 19, 2024

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.

@jfbu jfbu merged commit 1c131df into sphinx-doc:master Aug 19, 2024
22 checks passed
@jfbu
Copy link
Contributor Author

jfbu commented Aug 19, 2024

Merged! Thanks again for you helping hand!

@jfbu jfbu deleted the latex_contents_vs_topic_vs_sidebar branch August 19, 2024 18:45
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2024
@AA-Turner AA-Turner modified the milestones: 8.x, 8.1.0 Oct 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
builder:latex type:enhancement enhance or introduce a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants