quarto-dev / quarto-cli

Open-source scientific and technical publishing system built on Pandoc.
https://quarto.org
Other
3.93k stars 325 forks source link

Quarto 1.2.335 not respecting CSL "entry-spacing" value #4555

Closed Zuline closed 1 year ago

Zuline commented 1 year ago

Bug description

I'm using Quarto on an M1 Mac (13.1 (22C65)) inside VS Code (1.75.1).

Quarto version 1.2.335

The front matter of the document is:

format:
    pdf:
        toc: true
        toc-depth: 2
        number-sections: true
        number-depth: 3
        colorlinks: true
        lof: true
        lot: true
csl: mike-fs-harvard.csl
bibliography: bib.bib
papersize: a4

The .csl file is modified in only one aspect - using the Zotero visual editor - to add the "entry-spacing" value. This value sits inside the tag:

<bibliography et-al-min="5" et-al-use-first="4" entry-spacing="4" hanging-indent="true">

Whatever the value is set to, the spacing between the bibliography entries does not change...it is supposed to, but it doesn't.

I'm rendering to PDF using the "Render" button in VS Code.

Other values such as the "hanging-indent" value do work.

It seems like a bug.

The .zip contains a sample test.qmd file, a bib.bib file and the test.csl file.

The bibliography entries should show 4 lines spacing between them...they show 1.

test-quarto-csl.zip

Checklist

dragonstyle commented 1 year ago

We're a little at the edge of my knowledge here - using keep-tex to keep the LaTeX output, the bibliography appears to be properly setting up the environment using the spacing:

\begin{CSLReferences}{1}{4}
\leavevmode\vadjust pre{\hypertarget{ref-gahanDoingQualitativeResearch1998}{}}%
Gahan, C. \& Hannibal, M. (1998) \emph{Doing qualitative research using
{QSR NUD}*{IST}}. 1. publ. {London}, {SAGE}.

\leavevmode\vadjust pre{\hypertarget{ref-rowlingHarryPotterCursed2017}{}}%
Rowling, J.K., Tiffany, J. \& Thorne, J. (2017) \emph{Harry {Potter} and
the cursed child: Parts one and two: playscript}. Paperback edition.
{London}, {Sphere}.

\end{CSLReferences}

For the environment as defined:

\newenvironment{CSLReferences}[2] % #1 hanging-ident, #2 entry spacing
 {% don't indent paragraphs
  \setlength{\parindent}{0pt}
  % turn on hanging indent if param 1 is 1
  \ifodd #1
  \let\oldpar\par
  \def\par{\hangindent=\cslhangindent\oldpar}
  \fi
  % set entry spacing
  \setlength{\parskip}{#2\cslentryspacingunit}
 }%

I'm not sure why this is affecting the spacing that is being produced - can anyone with higher LaTeX chops chime in by any chance?

Zuline commented 1 year ago

Thanks Charles, we are well past my knowledge here. However following on from your work I wanted to know the value that was being used. It seems from the environment being set up that \parskip should be set at 4 - given the value being passed in.

I took the .tex file and added this line:

\message{The skip is \the\parskip}

Prior to this line:

\end{CSLReferences}

As I understand it that should write out the value held in \parskip

The value written out is:

The skip is 0.0pt

That seems to suggest that for whatever reason the variable, which we can see is there when the environment is set up, is not being passed in.

Or I could be wildly mistaken, given my shaky knowledge of this stuff.

We're a little at the edge of my knowledge here - using keep-tex to keep the LaTeX output, the bibliography appears to be properly setting up the environment using the spacing:

\begin{CSLReferences}{1}{4}
\leavevmode\vadjust pre{\hypertarget{ref-gahanDoingQualitativeResearch1998}{}}%
Gahan, C. \& Hannibal, M. (1998) \emph{Doing qualitative research using
{QSR NUD}*{IST}}. 1. publ. {London}, {SAGE}.

\leavevmode\vadjust pre{\hypertarget{ref-rowlingHarryPotterCursed2017}{}}%
Rowling, J.K., Tiffany, J. \& Thorne, J. (2017) \emph{Harry {Potter} and
the cursed child: Parts one and two: playscript}. Paperback edition.
{London}, {Sphere}.

\end{CSLReferences}

For the environment as defined:

\newenvironment{CSLReferences}[2] % #1 hanging-ident, #2 entry spacing
 {% don't indent paragraphs
  \setlength{\parindent}{0pt}
  % turn on hanging indent if param 1 is 1
  \ifodd #1
  \let\oldpar\par
  \def\par{\hangindent=\cslhangindent\oldpar}
  \fi
  % set entry spacing
  \setlength{\parskip}{#2\cslentryspacingunit}
 }%

I'm not sure why this is affecting the spacing that is being produced - can anyone with higher LaTeX chops chime in by any chance?

mcanouil commented 1 year ago

It seems the issue is originally from Pandoc template. The issue is probably reproducible using Pandoc alone.

Earlier in the template \parskip is set to 0 while the CSL spacing entry attribute is a multiplicator thus I believe it works as expected 2x0=0. https://github.com/quarto-dev/quarto-cli/blob/66aa11445d0e04e0fa771370cfc81a59239d06ed/src/resources/formats/pdf/pandoc/latex.template#L290. A simple fix would be to ensure the variable has a non zero value in the CSL if/else, i.e., set a unit value instead of using \parskip in the following https://github.com/quarto-dev/quarto-cli/blob/main/src/resources/formats/pdf/pandoc/latex.template#L322

Note that I did not run any code at this point.

Zuline commented 1 year ago

@mcanouil Thank you that makes perfect sense.

Just to prove the point I changed:

\setlength{\cslentryspacingunit}{\parskip}

to

\setlength{\cslentryspacingunit}{11pt}

Then with the value of entry-spacing in the .csl set to 2 I get the correct output and also this is written from my variable dump:

The skip is 22.0pt [1] [2] (./test.aux)

Clearly that's not a decent fix but it proves the point.

Surely the point of the entry-spacing variable in the .csl is to set the number of lines between each bibliography entry - hence the value is a multiplier rather than being in points. So why not set:

\setlength{\cslentryspacingunit}{value of line height derived from somewhere}

As I'm neither sure where the best place is to derive line height nor where this change needs to be...I'll have to leave that to others.

Zuline commented 1 year ago

Further to my last. I edited the file to:

\setlength{\cslentryspacingunit}{\baselineskip}

With the value of entry-spacing set to 1 it yields a very acceptable outcome and the variable dump informs me:

The skip is 13.6pt [1] [2] (./test.aux)

Which seems to be about right with 11pt font size and some baseline padding.

Is that an acceptable solution? If so where should it be corrected in the templates?

mcanouil commented 1 year ago

Note that I think it should be fixed upstream in Pandoc.

Zuline commented 1 year ago

@dragonstyle How do we go about getting this fixed in Pandoc as suggested by @mcanouil?

mcanouil commented 1 year ago

Small clarification:

This means, the fix has be be applied on Pandoc and Quarto, for example via a PR.

Why I am suggested to let Pandoc fix the issue and then get the fix is because they know the best the template and what would possibly be the best fix to avoid breakage elsewhere in the template.

Zuline commented 1 year ago

A PR is a black box to me...It would probably need to come from someone else.😄

Small clarification:

  • the issue is in Pandoc template
  • Quarto uses a slight modification of the template internally

This means, the fix has be be applied on Pandoc and Quarto, for example via a PR.

Why I am suggested to let Pandoc fix the issue and then get the fix is because they know the best the template and what would possibly be the best fix to avoid breakage elsewhere in the template.

Zuline commented 1 year ago

The interim fix might be to create a before-bib partial, though I can't get that to work.

The interim hack is to put this at the end of the document, after the References heading:

\setlength{\cslentryspacingunit}{\baselineskip}

It works perfectly, if a little hacky!!

Note: You have to have the following after the LaTeX snippet:

::: {#refs}
:::
kapsner commented 1 year ago

Thanks for these suggestions. I've added

format:
  pdf:
    include-in-header:
      - text: |
          \setlength{\cslentryspacingunit}{\baselineskip}

to the _quarto.yml, which works fine as well (when entry-spacing is >0 as described here)

Zuline commented 1 year ago

This is marked as "upstream"...how do we actually get it fixed in Pandoc and in Quarto as suggested by @mcanouil ?

cderv commented 1 year ago

This has been solved recently by Pandoc in 3.1.8 (https://github.com/jgm/pandoc/commit/b7e1ce422c9a76dffe763a4d31e0952e415775cc) as part of another problem (https://github.com/jgm/pandoc/issues/9053). Quarto uses already Pandoc 3.1.8 in its 1.4 pre-release version

They use a list to display reference, and entry-spacing is correctly taken into account.

So you have a fix by using quarto 1.4 now (https://quarto.org/docs/download/prerelease.html) (or wait for next stable release)