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

No specific processing when inside quarto for special collapse ASIS TOKEN #2248

Merged
merged 1 commit into from
Mar 31, 2023

Conversation

cderv
Copy link
Collaborator

@cderv cderv commented Mar 31, 2023

to fix quarto-dev/quarto-cli#5052

I believe the issue is that we previously did not run as.character() when inside htmlwidget processing

knitr/R/output.R

Lines 481 to 487 in a007b37

if (inherits(x, 'knit_asis_htmlwidget')) {
options$fig.cur = plot_counter()
options = reduce_plot_opts(options)
return(add_html_caption(options, x))
}
}
x = as.character(x)

We were passing x

now we run

return(add_html_caption(options, wrap_asis(x, options)))

with

knitr/R/output.R

Lines 467 to 475 in 99cd65a

asis_token = '<!-- KNITR_ASIS_OUTPUT_TOKEN -->'
wrap_asis = function(x, options) {
x = as.character(x)
if ((n <- length(x)) == 0 || !out_format('markdown') || !isTRUE(options$collapse))
return(x)
x[1] = paste0(asis_token, x[1])
x[n] = paste0(x[n], asis_token)
x
}

And Quarto does a brute force replacement in namespace
https://github.com/quarto-dev/quarto-cli/blob/4977f4501632de13a92cca6cfebac26f4baa2326/src/resources/rmd/patch.R#L111-L118

So the x received in not the same.

Quick solution as 1.3 will go out really soon, is to ignore quarto when doing the processing. Especially because there is no issue.
See #2212 (review)

collapse = TRUE with htmlwidget wokrs with Quarto because Quarto is already wrapping (for other purpose) any knit_asis output, and especially HTMLwidgets by using hooks and replacement function. Example of what knitr receive before collapse.

@yihui Do you think this is enough, or we should rethink the fix ?

BTW I found this issue by chance following a report on community. We should really consider running Quarto checks with the dev version of knitr before doing a release. I'll add some run of the Quarto checks with dev knitr too.

@cderv cderv force-pushed the fix/knitr-quarto-asis-token branch from 27c0fdb to 6c9d332 Compare March 31, 2023 16:23
@cderv cderv requested a review from yihui March 31, 2023 16:25
Copy link
Owner

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Good catch! Thanks! I think this enough for now.

Someday I hope that we could avoid the assignInNamespace() hacks in Quarto... (I think I've mentioned this a few times in 2021)

@yihui
Copy link
Owner

yihui commented Mar 31, 2023

It seems we are having trouble with an rgl demo in the knitr-examples repo, but we can figure it out later. I'll merge this PR first.

@yihui yihui merged commit 039b6cf into master Mar 31, 2023
@yihui yihui deleted the fix/knitr-quarto-asis-token branch March 31, 2023 16:40
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

figure caption with plotly not showing
2 participants