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

Fix #9944 (LaTeX writer visit_desc_content()) #9946

Merged
merged 5 commits into from Dec 9, 2021
Merged

Conversation

jfbu
Copy link
Contributor

@jfbu jfbu commented Dec 6, 2021

Relates #9926

With this, the following

.. option:: -mmmx
            -msse
            -msse2

   .. note::

      A note in the option description

.. c:function:: PyObject *PyType_GenericAlloc(PyTypeObject *type, Py_ssize_t nitems)

   .. note::

      Extra vertical space also in this case if note is first item

.. cpp:class:: Data

   .. cpp:union:: @data

      .. cpp:var:: int a

      .. cpp:var:: double b

Explicit ref: :cpp:var:Data::@data::a. Short-hand ref: :cpp:var:Data::a.

produces

Capture d’écran 2021-12-05 à 21 44 56

Comparing to the output shown in issue #9944, it only fixes the vertical space for the option with note and nested declaration case, not for the note at start of function description.

This PR removes from LaTeX markup an ~ which was added (14 years ago) at 3761223 to fix some "bad formatting issue in case of empty description".

(edit after further investigation) the ~ was needed at least in nested cases like the cpp example above. Without it and at Sphinx <1.5 the cpp example above gives mangled pdf output.

Capture d’écran 2021-12-06 à 09 52 49

But since 1.5, the LaTeX mark-up adds \pysigstartmultiline/\pysigstopmultiline pairs. The \pysigstopmultiline appears to fix the same issue but without the extra spacing. So it seems ~ should not be inserted, at least in such examples.

note: I did not need to update tests as apparently no unit test existed to check for the ~ insertion in latex object descriptions in some cases.

this PR replaces #9945 which was accidentally closed from renaming the branch at my fork underlying it

@jfbu jfbu added this to the 4.3.2 milestone Dec 6, 2021
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. But it's better to merge this into the 4.x branch instead.

Whether or not to allow linebreaks is matter of debate because user of
pdf can not reflow the displayed text, and overfull lines may lead to
loss of data beyond page border.

Since Sphinx 1.5, inline literals allow linebreaks after the characters
".", ",", ";", "?", "!", "/" and "\" (cf the inlineliteralwraps key of
latex_elements['sphinxsetup']).

With pdflatex the dash "-" does not allow a linebreak when used with a
monospace font (btw the dash is escaped to \sphinxhyphen{} which expands
back to a dash).  But this failed with xelatex, so previous commits
ensured nobreak behaviour by redefining \sphinxhyphen to be
\sphinxhyphennobreak when encountered in inline and parsed literals.

This commit adds \sphinxhyphenin{inline,parsed}literal macros which
default to \sphinxhyphennobreak and thus allow to customize how the
dashes from sources will behave in such contexts in pdf output.
The "~" was added 14 years ago at 3761223

Seems not to be needed anymore and causes extra vertical space in some
circumstances.
t
@jfbu jfbu changed the base branch from 4.3.x to 4.x December 9, 2021 17:20
@jfbu
Copy link
Contributor Author

jfbu commented Dec 9, 2021

Thanks for review @tk0miya I have rebased on 4.x and will merge after testing completes

@jfbu jfbu modified the milestones: 4.3.2, 4.4.0 Dec 9, 2021
@jfbu jfbu merged commit 7d6d562 into sphinx-doc:4.x Dec 9, 2021
@jfbu jfbu deleted the 9949_latex branch December 9, 2021 17:27
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 9, 2022
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.

None yet

2 participants