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 #3491 via equalizing vertical spaces for latex tables of all types #3504

Merged
merged 4 commits into from
Mar 4, 2017

Conversation

jfbu
Copy link
Contributor

@jfbu jfbu commented Mar 3, 2017

Subject: make vertical spaces before latex tables, between caption and table, and after table, independent of whether rendered via tabular, tabulary, with or without threeparttable, and longtable.

Feature or Bugfix

Both. Novel user interface via macros \sphinxtablepre, \sphinxtablepost
and \sphinxbelowtablecaptionskip \sphinxbelowcaptionskip \sphinxbelowcaptionspace allows to customize the spaces.

Detail

I have removed longtable form the latex template and put it into the sphinx.sty file because anyhow the productionlist environment requireslongtable. So it is currently a dependency of sphinx.sty and not user removable.

Test:

This is FIRST paragraph.
A centered regular table will now follow.\ |strutdw|

.. table:: This is a table with a very long title very long indeed
    :align: center

    =========== ========
    Header 1    Header 2
    =========== ========
    Cell 1      Cell 2
    Cell 3      Cell 4
    =========== ========

This is SECOND paragraph.
A centered regular table will now follow. |strutup| |strutdw|

.. table::
    :align: center

    =========== ========
    Header 1    Header 2
    =========== ========
    Cell 1      Cell 2
    Cell 3      Cell 4
    =========== ========

This is THIRD paragraph.
A longtable will now follow.  |strutup| |strutdw|

.. table::
    :class: longtable

    =========== ========
    Header 1    Header 2
    =========== ========
    Cell 1      Cell 2
    Cell 3      Cell 4
    =========== ========

This is a paragraph. No table no table no table will follow. |strutup|


This is FOURTH paragraph.
A longtable will now follow. |strutdw|

.. table:: This is a longtable
    :class: longtable

    =========== ========
    Header 1    Header 2
    =========== ========
    Cell 1      Cell 2
    Cell 3      Cell 4
    =========== ========

This is a paragraph. No table no table no table will follow. |strutup|


.. |strutup| raw:: latex

   \smash{\color{blue}\rule{1pt}{\dimexpr\sphinxtablepost+\baselineskip+\parskip\relax}}

.. |strutdw| raw:: latex

   \smash{\color{blue}\rule[-\dimexpr\sphinxtablepre+\parskip+\baselineskip\relax]{1pt}{\dimexpr\sphinxtablepre+\parskip+\baselineskip\relax}}

Result:

capture d ecran 2017-03-03 a 23 53 19

The most painful was to control the below caption space. Independently of patch done in sphinx.sty now, threepartable still obeys \belowcaptionskip but longtable does not respond to it (the two have completely distinct ways of making captions). There are no blue rules due to the added LaTeX hacking which would have been needed to get them to display from the threeparttable captions.

The default is a bit opened up, but Sphinx by default has \baselineskip + \parskip between baselines of two consecutive paragraphs, it is natural that table is similarly at least so separated.

One notes a Table 3, which is explained by the fact that longtable always increments the table counter whether or not there is a caption. (this is a pure LaTeX problem, nothing to do with Sphinx).

I said in #3491 that fix would wait, but this was because I knew one had to dig into the source codes of the relevant packages, and in the end I did this expedition, so we can as well fix the issue now. This is for master, because it is too complicated to fix on stable due to big refactoring already done on master regarding LaTeX tables.

…f all types

User interface via macros ``\sphinxtablepre``, ``\sphinxtablepost``
and ``\sphinxbelowtablecaptionskip``.
@jfbu jfbu force-pushed the fixtablevspaces branch from d33f075 to 5f71a2c Compare March 3, 2017 23:08
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 with nits.

@@ -8,7 +8,7 @@
<%- endif -%>
<%= table.get_colspec() %>
<%- if table.caption -%>
\caption{<%= ''.join(table.caption) %>}<%= labels %>\\
\caption{<%= ''.join(table.caption) %>\strut}<%= labels %>\\*[\sphinxafterLTcaptionskip]
Copy link
Member

Choose a reason for hiding this comment

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

Other macros for longtable is named as "...longtable..." with full spelling. But this is abbreviated.
Is there any rule? IMO, it would be better to use same naming rule.

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 used abbreviations at first LT for longtable and TPT for threeparttable but when there is no caption for tabular, tabulary, then threeparttable is not used. Thus I opted for long names, and this one was very long... I will rename it too ;-)

By the way, at this location one can not refer to \baselineskip, this is why \sphinxatlongtablestart macro will store current value. Remark: longtable code, contrarily to threeparttable obtains a given (non-customizable a priori) distance from bottom of deepest descender to top of table, not from baseline of caption to top of table.

Also it may be (I checked but forgot) that for longtable top of table frame is at top edge of top frame line and the one of threeparttable is at bottom edge of top line (0.4 pt difference). I will need to check again.

@tk0miya tk0miya added this to the 1.6 milestone Mar 4, 2017
@@ -8,7 +8,7 @@
<%- endif -%>
<%= table.get_colspec() %>
<%- if table.caption -%>
\caption{<%= ''.join(table.caption) %>}<%= labels %>\\
\caption{<%= ''.join(table.caption) %>\strut}<%= labels %>\\*[\sphinxafterLTcaptionskip]
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 used abbreviations at first LT for longtable and TPT for threeparttable but when there is no caption for tabular, tabulary, then threeparttable is not used. Thus I opted for long names, and this one was very long... I will rename it too ;-)

By the way, at this location one can not refer to \baselineskip, this is why \sphinxatlongtablestart macro will store current value. Remark: longtable code, contrarily to threeparttable obtains a given (non-customizable a priori) distance from bottom of deepest descender to top of table, not from baseline of caption to top of table.

Also it may be (I checked but forgot) that for longtable top of table frame is at top edge of top frame line and the one of threeparttable is at bottom edge of top line (0.4 pt difference). I will need to check again.

\vskip-\dimexpr\baselineskip+\parskip\relax
\leavevmode\null\par
\vskip\dimexpr\baselineskip+\parskip\relax
\vskip\dimexpr\sphinxtablepre\relax
Copy link
Contributor Author

Choose a reason for hiding this comment

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

these strange incantations are because code of longtable puts the top of table at some distance of bottom of previous text as decided by the deepest descenders, whereas threeparttable will put at some distance of the baseline of previous text. Lines 70 and 71 reset the depth to zero so longtable will refer to previous baseline (if there is one). The line 72 is simply because I decided for easier sharing with threeparttable that distance from previous baseline to top of caption would be baselineskip + parskip + extraskip, so the customization by user is for extraskip, which may be negative.

\LTpre\z@skip\LTpost\z@skip
\edef\sphinxbaselineskip{\dimexpr\the\dimexpr\baselineskip\relax\relax}}%
\def\sphinxattablestart{\par\vskip\dimexpr\sphinxtablepre\relax}%
\def\sphinxatlongtableend{\vskip\sphinxtablepost\relax}%
Copy link
Contributor Author

Choose a reason for hiding this comment

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

alternatively I could have \LTpost\sphinxtablepost\relax above and then \sphinxatlongtableend can be left empty. For symmetry and more powerful \sphinxatlongtableend I do it this way.

Remark: if one assigns \LTpost globally as a length like longtable wants us to do, it means we can not change font size. But in fact LaTeX in its all is very hostile to font size changes. For example the captions systematically reset the font size to normal font size, making it a priori impossible to make small floats. There are many such hidden constraints in LaTeX's core design. It is not because people massively use software that it is well designed.

+\sphinxbelowtablecaptionskip\relax}%
\def\sphinxafterTPTcaption % will be injected in a threeparttable macro
{\vskip\dimexpr-\dp\strutbox-1.2\baselineskip
+\sphinxbelowtablecaptionskip\relax
Copy link
Contributor Author

Choose a reason for hiding this comment

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

threeparttable has a hard-coded 0.2\baselineskip at some deep level... no other way than patching internal macro to compensate it. The code here achieves a given distance from last baseline of caption to top rule of table (hence added [t] in table templates). I will make some additional checks it works as intended.

@@ -72,7 +72,6 @@
'polyglossia': '',
'fontpkg': '\\usepackage{times}',
'fncychap': '\\usepackage[Bjarne]{fncychap}',
'longtable': '\\usepackage{longtable}',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

currently longtable is used by legacy productionlist environment, hence its loading can not be left to user choice. Thus I killed that line and in template, OK ?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's ok.

@jfbu
Copy link
Contributor Author

jfbu commented Mar 4, 2017

Thanks for review @tk0miya, current choice of extra pre and post skips:

\newcommand*\sphinxtablepre {0pt}%
\newcommand*\sphinxtablepost{\bigskipamount}%

gives result as in image in top post. Should I add some non-zero distance in the \sphinxtablepre?

Both are adjustments added or subtracted to default \baselineskip+\parskip. But at bottom the distance is to baseline of next text, hence it has less effect visually than on top, where it measures distance down from baseline of previous text.

@tk0miya
Copy link
Member

tk0miya commented Mar 4, 2017

I feel it's okay to your choice.

in particular:
``\sphinxbelowtablecaptionskip`` renamed to ``\sphinxbelowcaptionskip``
as it may be extended in future to usage for captions of literal blocks.
@jfbu
Copy link
Contributor Author

jfbu commented Mar 4, 2017

no need to patch macro of threeparttable after all, one only needs locally re-assigning \belowcaptionskip. Used name with @TPT because strictly for internal use.

\sphinxbelowtablecaptionskip renamed to \sphinxbelowcaptionskip as it may be extended in future to usage for captions of literal blocks.

@jfbu
Copy link
Contributor Author

jfbu commented Mar 4, 2017

oh, see only now your comment, thanks. I feel above skip is a bit too tight. I will experiment.

@jfbu
Copy link
Contributor Author

jfbu commented Mar 4, 2017

Finally I have reduced a bit the after table space. I feel it is ok with text before and after and not too bad if two tables are in succession.

capture d ecran 2017-03-04 a 16 24 54

@tk0miya
Copy link
Member

tk0miya commented Mar 4, 2017

to be honest, I don't have good eye to check spacing. So I feel two examples are almost same for me...
So go ahead as you like.

@jfbu
Copy link
Contributor Author

jfbu commented Mar 4, 2017

Thanks for review @tk0miya ! Is the incrementing of table counter by longtable even if no caption a known issue in Sphinx ? I feel it is strange. But it is pure LaTeX issue, un-related to Sphinx code.

@jfbu
Copy link
Contributor Author

jfbu commented Mar 4, 2017

Before merge I will rename \sphinxbelowcaptionskip to \sphinxbelowcaptionspace because it does not act like \belowcaptionskip of LaTeX but designates exactly distance from last baseline of caption to top rule of table.

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.

2 participants