Skip to content

LaTeX captions of literal blocks now glued to framed verbatim#2297

Merged
tk0miya merged 6 commits intosphinx-doc:masterfrom
jfbu:literalblockcaption
Mar 6, 2016
Merged

LaTeX captions of literal blocks now glued to framed verbatim#2297
tk0miya merged 6 commits intosphinx-doc:masterfrom
jfbu:literalblockcaption

Conversation

@jfbu
Copy link
Copy Markdown
Contributor

@jfbu jfbu commented Feb 5, 2016

This could fix #2262

Tested on top of 1.3.5 release.

The 1.3.5 Verbatim environment in sphinx.sty has many places allowing a
page break after the caption: from the \smallskip, and from the list,
and from the \MakeFramed because framed.sty explains it encourages page
breaks above it.

This commit sets up the caption from inside the framed environment.

Test project on
https://github.com/sphinx-doc/sphinx/files/101910/2262.zip
compiles correctly. I have also tested with xcolor as its use has been
introduced in later commit 476f809.

More extensive tests needed.

modified:   sphinx/texinputs/sphinx.sty
modified:   sphinx/writers/latex.py

@jfbu
Copy link
Copy Markdown
Contributor Author

jfbu commented Feb 5, 2016

Sorry, I issued the PR too early. There is an issue with framed environment expanding multiple times its contents, hence the caption number is increased at least twice. I will look for a work-around.

@jfbu
Copy link
Copy Markdown
Contributor Author

jfbu commented Feb 5, 2016

I know nothing to travis-ci, but it appears the test fails simply because the latex builder has been modified, hence some assert is not verified.

@jfbu jfbu mentioned this pull request Feb 5, 2016
Closed
@jfbu
Address sphinx-doc#2262 (latex): avoid pagebreaks after captions of l…
89e8cb3 
…iteral blocks

This could fix sphinx-doc#2262

The 1.3.5 Verbatim environment from sphinx.sty has many places allowing
a page break after the caption: from the \smallskip, from the list
environment, and from the \MakeFramed: indeed framed package
documentation explains that it encourages page breaks above it.

The only way to avoid the pagebreaks is to put the caption inside the
environment whic is started by \MakeFramed. However, as this environment
typesets multiple times its contents, we must inhibit within it the
increase of counters (only the literal-block counter is concerned), this
is done thanks to a switch provided by the package amsmath which is
already loaded by sphinx.sty.

	modified:   sphinx/texinputs/sphinx.sty
	modified:   sphinx/writers/latex.py
	modified:   tests/test_directive_code.py
@jfbu jfbu force-pushed the literalblockcaption branch from 0347395 to 89e8cb3 Compare February 10, 2016 09:55
@jfbu
Copy link
Copy Markdown
Contributor Author

jfbu commented Feb 10, 2016

I have updated tests/test_directive_code.py, for make test to succeed; then I fused the commits into a single one.

@jfbu
Fix sphinx-doc#2262 (latex): avoid pagebreaks after captions of liter…
59de4bf 
…al blocks

This could fix sphinx-doc#2262
and also perhaps sphinx-doc#2319

As now the caption is set in an environment it appears to possibly solve
also issue sphinx-doc#2319 Table counter is overrided by code-block's in LaTeX

The parent commit used only \FirstFrameCommand of latex package
framed, but \FrameCommand also must be customized in case the
framed contents fit on a single page. This is fixed here.

	modified:   sphinx/texinputs/sphinx.sty
@jfbu
Copy link
Copy Markdown
Contributor Author

jfbu commented Feb 14, 2016

Could also solve #2319

@tk0miya
Copy link
Copy Markdown
Member

tk0miya commented Feb 15, 2016

@jfbu Sorry, I cannnot determine this change is stable or not (I'm not good at LaTeX macros...)
Do you know this change has any side effects? Does this change has any limitations?

tk0miya
tk0miya reviewed Feb 15, 2016
View reviewed changes
Comment thread sphinx/texinputs/sphinx.sty Outdated
% first portion of split frames:
\let\FirstFrameCommand\FrameCommand

% Unneeded %'s after control words removed.
Copy link
Copy Markdown
Member

@tk0miya tk0miya Feb 15, 2016

Choose a reason for hiding this comment

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

Could you remove this line? This is not a description of macros.
Please note the description of codes (cf. purpose, behaviors and how it works).

Copy link
Copy Markdown
Contributor Author

@jfbu jfbu Feb 15, 2016

Choose a reason for hiding this comment

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

done, sorry.

@jfbu
Removes unneeded comment from sphinx.sty
31925e8 
	modified:   sphinx/texinputs/sphinx.sty
@jfbu
Copy link
Copy Markdown
Contributor Author

jfbu commented Feb 15, 2016

@tk0miya It is hard for me to answer with confidence, as I am not familiar enough yet with Sphinx latex writer, and I don't know all circumstances which trigger use of Verbatim environment (this seems to come from highlighting.py). As far as I can tell, this should not introduce more incompatibilities than 1.3.5. code (which looks already like some kind of hack). If it was used directly in a latex source, it would not impact other direct uses of framed environment, as long naturally as the user is not herself modifying \FrameCommand to do something else than standard framing (such modification would simply overwrites sphinx.sty code if not done within a group). The framed environment is enhanced in the following way: if macro \MyVerbatimTitle is non empty it gives a title to be typeset above the frame (must be "vertical" material); if it is empty nothing special happens. It is reset to empty after each use. Precautions are taken by the code in case the title increases some counters: because framed typesets multiple times its contents. Thus the code uses the already existing amsmath hack into \stepcounter which inhibits it in such circumstances.

@jfbu
Fix comments.
60742ab 
	modified:   sphinx/texinputs/sphinx.sty
@tk0miya
Copy link
Copy Markdown
Member

tk0miya commented Mar 4, 2016

Sorry for late response. I read your changes now, and it looks good to me.
I will merge this after reviewing again in this weekend.

Before merging, I want to hear you that is this conflicts with minted (ref #2304).
If any investigation is needed, I'll postpone merging to next major release.

@tk0miya tk0miya self-assigned this Mar 4, 2016
@jfbu
Copy link
Copy Markdown
Contributor Author

jfbu commented Mar 4, 2016

@tk0miya I can confirm that this patch is completely independent from the ones discussed in #2304.

  1. I have tested that 'preamble': '\\usepackage[draft]{minted}\\fvset{breaklines=true}' is compatible,
  2. I have also tested that a merge of https://github.com/jfbu/sphinx/tree/codeautolinebreakV3 with the present one results in ok behaviours. (the merge needs some manual intervention, if the present PR is incorporated I can move proposal codeautolinebreakV3 on top of it).

Regarding #2304, I am not too much favorable to minted in the sense that Pygmentization is already done by the Sphinx processing. I find it simpler to exploit that on the LaTeX side. If Sphinx switches to minted.sty I think it should drop entirely use of fancyvrb.sty.

The present PR is not concerned with fancyvrb.sty nor potential use of minted but only with use of package framed.

@jfbu
Copy link
Copy Markdown
Contributor Author

jfbu commented Mar 4, 2016

I will update the PR for better choice of macro names in the patched sphinx.sty.

jfbu added 2 commits March 4, 2016 12:46
@jfbu
Changes of LaTeX macro names in sphinx.sty to use Sphinx prefix.
038dd00 
	modified:   sphinx/texinputs/sphinx.sty
	modified:   sphinx/writers/latex.py
	modified:   tests/test_directive_code.py
@jfbu
Fix broken test_literalinclude_caption_latex
12c6c9d 
	modified:   tests/test_directive_code.py
@tk0miya
Copy link
Copy Markdown
Member

tk0miya commented Mar 4, 2016

Thank you for comment and update!
I will review again and merge it :-)

@tk0miya tk0miya added this to the 1.4 milestone Mar 4, 2016
tk0miya added a commit that referenced this pull request Mar 6, 2016
@tk0miya
Merge pull request #2297 from jfbu/literalblockcaption
9d82cad 
LaTeX captions of literal blocks now glued to framed verbatim
@tk0miya tk0miya merged commit 9d82cad into sphinx-doc:master Mar 6, 2016
@tk0miya
Copy link
Copy Markdown
Member

tk0miya commented Mar 6, 2016

Just merged. Thank you for contribution!
It will be released as 1.4b1:-)

@tk0miya tk0miya mentioned this pull request Mar 6, 2016
Closed
@jfbu jfbu deleted the literalblockcaption branch March 6, 2016 07:23
@jfbu jfbu mentioned this pull request Jun 4, 2016
Merged
jfbu added a commit to jfbu/sphinx that referenced this pull request Jun 4, 2016
@jfbu
LaTeX: load amstext in sphinx.sty even if not using amsmath
401302d 
Needed in particular for:

- use of \iffirstchoice@ at commit 9d82cad (PR sphinx-doc#2297)

- use of \text at commit 488ee52 addressing issue sphinx-doc#2501
@jfbu jfbu mentioned this pull request Jun 4, 2016
Merged
@jfbu jfbu mentioned this pull request Jan 31, 2021
Closed
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Sep 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@tk0miya tk0miya

Labels

builder:latex

Projects

None yet

Milestone

1.4

Development

Successfully merging this pull request may close these issues.

literal_block and its caption has been separated by pagebreak in LaTeX output

2 participants

@jfbu @tk0miya