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

revealjs does not work with knitr embed engine #4712

Closed
4 tasks done
aronatkins opened this issue Mar 8, 2023 · 8 comments · Fixed by #4732
Closed
4 tasks done

revealjs does not work with knitr embed engine #4712

aronatkins opened this issue Mar 8, 2023 · 8 comments · Fixed by #4732
Assignees
Labels
bug Something isn't working
Milestone

Comments

@aronatkins
Copy link
Contributor

Bug description

Using Quarto 1.2.335 (which is included in recent RStudio dailies, including 2023.03.0+385). This problem can be recreated without using the RStudio IDE when rendering using Quarto from the command-line.

Given a code.R file:

cat("this is code.R\n")

The following revealjs document does not show the embedded code:

---
title: embedding with reveal
format: revealjs
engine: knitr
---

## Embed!

```{embed, file = "code.R"}
```

image

The generated HTML contains a block, but no code:

<section id="embed" class="slide level2">
<h2>Embed!</h2>
<div class="cell" data-file="code.R">

</div>
<div class="footer footer-default">

</div>
</section>

In contrast, when we do not output to HTML, the embedded code is shown.

---
title: embedding without reveal
engine: knitr
---

## Embed!

```{embed, file = "code.R"}
```

image

Using the most recent version of knitr from CRAN and R 3.6.3 on macOS 12.6.3.

R -s -e "packageVersion('knitr')"
#> [1] ‘1.42’

Discovered while trying to apply the workaround from #1237

Checklist

  • Please include a minimal, fully reproducible example in a single .qmd file? Please provide the whole file rather than the snippet you believe is causing the issue.
  • Please format your issue so it is easier for us to read the bug report.
  • Please document the RStudio IDE version you're running (if applicable), by providing the value displayed in the "About RStudio" main menu dialog?
  • Please document the operating system you're running. If on Linux, please provide the specific distribution.
@aronatkins aronatkins added the bug Something isn't working label Mar 8, 2023
@aronatkins
Copy link
Contributor Author

Have tried both embed and verbatim.

@cscheid cscheid self-assigned this Mar 8, 2023
@cscheid cscheid added this to the v1.4 milestone Mar 8, 2023
@cscheid
Copy link
Collaborator

cscheid commented Mar 8, 2023

The reason the report in #1237 isn't resolved for include specifically is that the include shortcode resolution happens in typescript, before Pandoc. Other shortcodes like meta, var, etc, happen in Lua.

We need to add include processing to Lua in one shape or another, but this is easier said than done. We need to be aware of a number of issues, and it's also going to be the case that a "Lua {{< include >}}" will behave differently than a typescript one out of necessity. This is going to be very confusing for users.

@cderv
Copy link
Collaborator

cderv commented Mar 8, 2023

@aronatkins Did you try pre release yet ?

I can't reproduce with 1.3. From

---
title: embedding with reveal
format: revealjs
engine: knitr
---

## Embed!

```{embed, file = "code.R"}
```

I get

image

But I can reproduce with verbatim.

Using also knitr 1.42

Either some adjusments in quarto or something with knitr - I can have a look to this @cscheid

@cscheid cscheid assigned cderv and unassigned cscheid Mar 8, 2023
@cderv
Copy link
Collaborator

cderv commented Mar 8, 2023

Oh I know - this is a side effect of a default we have for revealjs.
This is the default for revealjs format in Quarto.

execute:
  echo: false

We do that because minimal space on slides so by default we choose to hide code source. This is documented for this format.
https://quarto.org/docs/presentations/revealjs/#code-echo

For verbatim or embed to work, echo: true is needed. Add #| echo: true in your chunk (or echo = TRUE next to engine name)

We should probably handle specifically those engine with an option hook as they need to have echo = TRUE for knitr to output with source hook, and they have no output.

@cderv cderv modified the milestones: v1.4, v1.3 Mar 8, 2023
@cderv
Copy link
Collaborator

cderv commented Mar 8, 2023

In fact there is something new now since #3429 - v1.3 has added internally a embed handler for this feature thttps://quarto.org/docs/prerelease/1.3/embed.html

For now we pass every cell handlers from quarto to knitr

# pass through all languages handled by cell handlers in quarto
langs = lapply(
setNames(handledLanguages, handledLanguages),
function(lang) {
function(options) {
knitr:::one_string(c(
paste0("```{", lang, "}"),
options$yaml.code,
options$code,
"```"
))
}
}
)
knitr::knit_engines$set(langs)

This creates a conflict (and why I get something with 1.3 using embed on cell with knitr - but wrong output when looking at HTML produced).

@cscheid @dragonstyle do all the language handler we have are supposed to pass to knitr ? Does setting

```{embed}

inside a document using knitr computation engine is supposed to be handled by our internal handler, or should we NOT overwrite knitr own embed engine ?

@cscheid @dragonstyle I need your input before doing more I have a fix already for verbatim engine, but embed knitr engine was not used as I would expect. I understand why now. I have some ideas on what to do, but need the full context to be sure.

@cderv cderv added the needs-discussion Issues that require a team-wide discussion before proceeding further label Mar 8, 2023
@cderv
Copy link
Collaborator

cderv commented Mar 8, 2023

What I think we should do : embed should be filtered out handledLanguages to support as knitr's engine for cells

I'll go with that, unless you think otherwise.

cderv added a commit that referenced this issue Mar 9, 2023
…bed` handler

Currently the `embed` handler is only for Jupyter.

Discovered while looking at #4712 - revealjs does not work with knitr embed engine
cderv added a commit that referenced this issue Mar 9, 2023
…bed` handler

Currently the `embed` handler is only for Jupyter.

Discovered while looking at #4712 - revealjs does not work with knitr embed engine
@cderv cderv removed the needs-discussion Issues that require a team-wide discussion before proceeding further label Mar 9, 2023
@aronatkins
Copy link
Contributor Author

In case others find this issue, #1237 suggested using the https://github.com/quarto-ext/include-code-files extension.

This extension has worked and lets me include code from external files into a Quarto+revealjs presentation.

@cscheid
Copy link
Collaborator

cscheid commented Mar 10, 2023

This will be fixed by #4732

cscheid pushed a commit that referenced this issue Mar 10, 2023
* Do not override the knitr's `embed` engine with internal quarto's `embed` handler

Currently the `embed` handler is only for Jupyter.

Discovered while looking at #4712 - revealjs does not work with knitr embed engine

* Set echo = TRUE for embed and verbatim engine when revealjs

because those engine expect echo, but revealjs default to FALSE

* Add embed to ignored engine for chunk hook

* Add test for knitr revealjs fix regarding verbatim and embed

* Revert "Do not override the knitr's `embed` engine with internal quarto's `embed` handler"

This reverts commit 53d8703.

* Internal languages are only handlers of type cell

Other language are shortcodes

* For embed and verbatim engine, always set to TRUE

This will avoid format defaulting to echo: false to hide those chunks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants