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

don't respect beamer theme's buildin theorem/proof block #1143

Closed
XiangyunHuang opened this issue Apr 19, 2021 · 20 comments · Fixed by #1145
Closed

don't respect beamer theme's buildin theorem/proof block #1143

XiangyunHuang opened this issue Apr 19, 2021 · 20 comments · Fixed by #1145
Labels
bug an unexpected problem or unintended behavior next to consider for next release
Milestone

Comments

@XiangyunHuang
Copy link

XiangyunHuang commented Apr 19, 2021

here is a minimal, self-contained, and reproducible example

---
title: "beamer"
documentclass: beamer
output: 
  bookdown::pdf_book: 
    base_format: rmarkdown::beamer_presentation
    toc: no
    latex_engine: xelatex
    citation_package: natbib
    theme: Xiaoshan # https://www.ctan.org/pkg/pgfornament-han 
    template: null
link-citations: yes
colorlinks: yes
---

```{r setup, include=FALSE}
knitr::opts_chunk$set(echo = FALSE)
```

## some blocks from beamer theme [*Xiaoshan*](https://github.com/liantze/pgfornament-han/)

::: {.block data-latex="{Metropolis}"}
block is ok.
:::


::: {.exampleblock data-latex="{Metropolis}"}
exampleblock is also ok.
:::

::: {.theorem data-latex="{Metropolis}"}
theorem is not ok.
:::

my session info

xfun::session_info('bookdown')
R version 4.0.4 (2021-02-15)
Platform: x86_64-apple-darwin20.3.0 (64-bit)
Running under: macOS Big Sur 10.16, RStudio 1.4.1106

Locale: en_US.UTF-8 / en_US.UTF-8 / en_US.UTF-8 / C / en_US.UTF-8 / en_US.UTF-8

Package version:
  base64enc_0.1.3   bookdown_0.21     digest_0.6.27     evaluate_0.14     glue_1.4.2       
  graphics_4.0.4    grDevices_4.0.4   highr_0.9         htmltools_0.5.1.1 jsonlite_1.7.2   
  knitr_1.32        magrittr_2.0.1    markdown_1.1      methods_4.0.4     mime_0.10        
  rlang_0.4.10      rmarkdown_2.7     stats_4.0.4       stringi_1.5.4     stringr_1.4.0    
  tinytex_0.31      tools_4.0.4       utils_4.0.4       xfun_0.22         yaml_2.2.1      

This bug also reported here

@cderv
Copy link
Collaborator

cderv commented Apr 19, 2021

Oh yes I see what is the issue.

I think this message is printed in the console during R Markdown rendering:

[WARNING] data-latex attribute can't be used with one of bookdown custom environment. It has been removed.

As bookdown already use a theorem environment built-in, one can't use the custom block syntax from https://bookdown.org/yihui/rmarkdown-cookbook/custom-blocks.html with one of the supported environemnt.
We do that to prevent a double processing by rmarkdown specific Lua filter for LaTeX divs (which is used in this example).

Obviously we are not supporting beamer correctly in the Lua filter or at least in our logic.
Also, maybe there should be a way to opt-out using bookdown's environment feature and let users customize directly according to its needs.

Is theorem an environment built in beamer ? I don't know much.

@cderv cderv added the bug an unexpected problem or unintended behavior label Apr 19, 2021
@cderv
Copy link
Collaborator

cderv commented Apr 19, 2021

Is theorem an environment built in beamer ? I don't know much.

It seems to be at least for Metropolis theme in beamer
https://www.tug.org/texlive//Contents/live/texmf-dist/doc/latex/beamertheme-metropolis/metropolistheme.pdf

This will conflict with our mechanism in restore_block2() where we add the definition in preamble if not already.

bookdown/R/latex.R

Lines 233 to 236 in df6786e

# add the necessary definition in the preamble when block2 engine (\BeginKnitrBlock) or pandoc
# fenced div (\begin) is used
if (length(grep(sprintf('^\\\\(BeginKnitrBlock|begin)\\{(%s)\\}', paste(all_math_env, collapse = '|')), x)) &&
length(grep('^\\s*\\\\newtheorem\\{theorem\\}', head(x, i))) == 0) {

If we support beamer the \begin{theorem} will be written in tex file which will trigger the above and add a new definition - which should not be there as beamer already defines one.

We could

  • make an exception for \begin{theorem}{Metropolis} (but I don't know if there is other)
  • provide a way to opt out this mechanism in bookdown::pdf_book() that one would need to use with a beamer template defining theorem
  • Opt out automatically for \documentclass[]{beamer}

The more generic in the second one. 🤔

@yihui
Copy link
Member

yihui commented Apr 19, 2021

@XiangyunHuang By using the data-latex attribute, did you just want to provide a name to the theorem? If so, the syntax is:

::: {.theorem name="Metropolis"}
theorem is ok.
:::

But we don't support beamer yet. @cderv I don't remember why we didn't support beamer, but I feel we should:

-- TODO: should we support beamer also ?

@XiangyunHuang
Copy link
Author

@yihui When I use the way you provided, it's not ok, either. I have upgraded bookdown to latest dev version, here is new session info

xfun::session_info('bookdown')
#> R version 4.0.4 (2021-02-15)
#> Platform: x86_64-apple-darwin20.3.0 (64-bit)
#> Running under: macOS Big Sur 11.2.3
#> 
#> Locale: en_US.UTF-8 / en_US.UTF-8 / en_US.UTF-8 / C / en_US.UTF-8 / en_US.UTF-8
#> 
#> Package version:
#>   base64enc_0.1.3   bookdown_0.21.11  digest_0.6.27     evaluate_0.14    
#>   glue_1.4.2        graphics_4.0.4    grDevices_4.0.4   highr_0.9        
#>   htmltools_0.5.1.1 jsonlite_1.7.2    knitr_1.32        magrittr_2.0.1   
#>   markdown_1.1      methods_4.0.4     mime_0.10         rlang_0.4.10     
#>   rmarkdown_2.7     stats_4.0.4       stringi_1.5.4     stringr_1.4.0    
#>   tinytex_0.31      tools_4.0.4       utils_4.0.4       xfun_0.22        
#>   yaml_2.2.1

Created on 2021-04-20 by the reprex package (v2.0.0)

截屏2021-04-20 上午8 48 51

@yihui
Copy link
Member

yihui commented Apr 20, 2021

That's expected. As I said above, the theorem Divs don't work for beamer yet. We'll probably fix it.

@cderv cderv added the next to consider for next release label Apr 20, 2021
@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

@yihui we don't support it because the previous syntax does not support either

> dir.create(tmp_dir <- tempfile())
> owd <- setwd(tmp_dir)
> xfun::write_utf8(c(
+ '---
+ title: "Minimal Working Example"
+ output: bookdown::beamer_presentation2
+ ---
+ 
+ ```{theorem}
+ test
+ ```'
+ ), "test.Rmd")
> rmarkdown::render("test.Rmd", "bookdown::beamer_presentation2", quiet = TRUE)
! LaTeX Error: Command \theorem already defined.
               Or name \end... illegal, see p.192 of the manual.

Error: LaTeX failed to compile test.tex. See https://yihui.org/tinytex/r/#debugging for debugging tips. See test.log for more info.
Run `rlang::last_error()` to see where the error occurred.
> setwd(owd)
> unlink(tmp_dir, recursive = TRUE)

To run the above code: Copy then use reprex::reprex_rescue() before pasting in a script

Problem is the same here: beamer already define a theorem environment and currently our internal restore_block2 does not take that into account. I will then add in the preamble a second definition. Hence the issue

! LaTeX Error: Command \theorem already defined.

Fixing this should allow to support the new syntax too.

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

Looking more into beamer by doing small quick test, it seems that the following among bookdown Theorem and proof env are already defined when using a documentclass beamer

  • theorem
  • lemma
  • corollary
  • definition
  • example
  • solution

The other are not defined and their definition by bookdown in preamble does not cause error

  • proposition
  • conjecture
  • exercise
  • hypothesis
  • remark

So we are in a hybrid state with beamer.

Currently what we do when we detect than one of the environment is used:

  • if \newtheorem{theorem} is already define in the tex document, we don't add any definition
  • if not, bookdown will add definition for all this environment in the preamble before compiling to PDF.

So we could either do if we detect it is beamer used:

  1. we don't add any new definition. This would mean that missing environment should be defined by the user in a preamble.tex as bookdown won't add any. Also internationalization won't work for those environment.
  2. we only add add the missing environment defintion. internationalization won't work for env define in beamer though.

1 is the simpler and we let the user handle the rest. 2 is the better to support all env documented as supported in bookdown. I wonder if we could modify the already define one 🤔

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

Ok I found the source: https://github.com/josephwright/beamer/blob/56b5d77a5b1ad886f452f23148b0e2e36094c689/base/beamerbasetheorems.sty#L114

This where the \newtheorem happens and translation is done in a better way than in bookdown using \translate command from the translator CTAN package.

So I tend to think that with beamer we do nothing special to let user customize as they need.

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

Oh and also @XiangyunHuang, I hope you notice that you don't need bookdown for this example to work.

---
title: "beamer"
output: 
  rmarkdown::beamer_presentation:
    toc: no
    latex_engine: xelatex
    citation_package: natbib
    theme: Xiaoshan # https://www.ctan.org/pkg/pgfornament-han 
    template: null
link-citations: yes
colorlinks: yes
---

```{r setup, include=FALSE}
knitr::opts_chunk$set(echo = FALSE)
```

## some blocks from beamer theme [*Xiaoshan*](https://github.com/liantze/pgfornament-han/)

::: {.block data-latex="{Metropolis}"}
block is ok.
:::


::: {.exampleblock data-latex="{Metropolis}"}
exampleblock is also ok.
:::

::: {.theorem data-latex="{Metropolis}"}
theorem is not ok.
:::

But you may need to use bookdown for its other features. (Cross referencies I believe ?)

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

we don't support it because the previous syntax does not support either

#1145 will fix this for now.

@XiangyunHuang
Copy link
Author

But you may need to use bookdown for its other features. (Cross referencies I believe ?)

Yes.

@cderv cderv added this to the v0.22 milestone Apr 20, 2021
@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

@XiangyunHuang just to confirm, what output is expected for this ?

::: {.theorem data-latex="{Metropolis}"}
theorem is not ok.
:::

@XiangyunHuang
Copy link
Author

@cderv The expected output is

截屏2021-04-21 上午12 33 00

here is a minimal Xiaoshan beamer demo.

% !TEX program = xelatex
\documentclass{beamer}
\usetheme{Xiaoshan}

\title{Xiaoshan}

\begin{document}

\begin{frame}
  \maketitle
\end{frame}

\begin{frame}
  \frametitle{some blocks from beamer theme Xiaoshan}

  \begin{block}{Metropolis}
   block is ok.
  \end{block}

  \begin{exampleblock}{Metropolis}
   exampleblock is ok.
  \end{exampleblock}

  \begin{alertblock}{Metropolis}
    alertblock is ok.
  \end{alertblock}
\end{frame}

\begin{frame}
  \frametitle{some blocks from beamer theme Xiaoshan}

  \begin{theorem}[Metropolis]
    theorem is ok.
  \end{theorem}

  \begin{proof}[Metropolis]
    proof is also ok.
  \end{proof}
\end{frame}

\end{document}

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

Thanks. That makes more sense then.You want \begin{theorem}[Metropolis] and not \begin{theorem}{Metropolis}

This means that it should be without bookdown support

::: {.theorem data-latex="[Metropolis]"}
theorem is not ok.
:::

and as @yihui has shown above this would be the bookdown ways if it was supported

::: {.theorem name="Metropolis"}
theorem is ok.
:::

We'll fix this for next version

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

Can you try the PR now ?

remotes::install_github("rstudio/bookdown#1145")

beamer should be supported now as other format using the same syntax as other format

---
title: "beamer"
output: 
  bookdown::beamer_presentation2:
    toc: no
    latex_engine: xelatex
    citation_package: natbib
    theme: Xiaoshan # https://www.ctan.org/pkg/pgfornament-han 
    template: null
link-citations: yes
colorlinks: yes
---

```{r setup, include=FALSE}
knitr::opts_chunk$set(echo = FALSE)
```

## some blocks from beamer theme [*Xiaoshan*](https://github.com/liantze/pgfornament-han/)

::: {.block latex="{Metropolis}"}
block is ok.
:::


::: {.exampleblock latex="{Metropolis}"}
exampleblock is also ok.
:::

::: {.theorem name="Metropolis"}
theorem is now ok.
:::

image

This would allow the referencing to work by adding an id but I am seing that beamer Theorem environment are not numbered. I don't know them so maybe it is expected.

@yihui @XiangyunHuang what do you think ?

Other solution is to completely ignore bookdown feature for beamer output and just use R Markdown custom block feature for LateX

This would mean that this would work in that case

---
title: "beamer"
output: 
  bookdown::beamer_presentation2:
    toc: no
    latex_engine: xelatex
    citation_package: natbib
    theme: Xiaoshan # https://www.ctan.org/pkg/pgfornament-han 
    template: null
link-citations: yes
colorlinks: yes
---

```{r setup, include=FALSE}
knitr::opts_chunk$set(echo = FALSE)
```

## some blocks from beamer theme [*Xiaoshan*](https://github.com/liantze/pgfornament-han/)

::: {.block latex="{Metropolis}"}
block is ok.
:::

::: {.exampleblock latex="{Metropolis}"}
exampleblock is also ok.
:::

::: {.theorem latex="[Metropolis]"}
theorem is now ok.
:::

This what you would write when not using bookdown but using rmarkdown::beamer_presentation

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

Oh now I see the screenshot here, there is maybe a styling issue with the current solution.

Afterall, I should maybe ignore the Lua filter for beamer presentation then - this could be simpler 🤔

Or this may be the sign of an issue in how I build the tex content in the Lua filter 😓

@cderv
Copy link
Collaborator

cderv commented Apr 20, 2021

Ok I know what is going one with the styling. This is because we add a special syntax for linking to theorem

\protect\hypertarget{thm:unlabeled-div-1}{}\label{thm:unlabeled-div-1}

This seems to cause the empty line with beamer. It did no seem to cause anything with other pdf format.

So as I said after all I may just ignore beamer in bookdown and make sure the latex= attributes can be used. Instead of trying to adapt the above only for beamer usage. I'll do that tomorrow and leave the PR as is for you to try if you want.

@XiangyunHuang
Copy link
Author

XiangyunHuang commented Apr 21, 2021

@cderv

remotes::install_github("rstudio/bookdown#1145")

is fine. but I hope we don't change

::: {.block data-latex="{Metropolis}"}
block is ok.
:::

to

::: {.block latex="{Metropolis}"}
block is ok.
:::

the block / exampleblock / theorem be more consistent the better, both beamer and book documentclass.

@cderv
Copy link
Collaborator

cderv commented Apr 21, 2021

data-latex and latex are both working - one is an alias to the other.

See PR #1145 for following discussion on the syntax.

@github-actions
Copy link

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug an unexpected problem or unintended behavior next to consider for next release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants