Fix conflict between collapse=TRUE and htmlwidgets

[dmurdoch]

As reported in my comment to #2172, collapse = TRUE is incompatible with including htmlwidgets output in a document. This patch fixes it by recording which sections of chunk output have special classes (as htmlwidgets sections do), and only attempting to collapse the plain sections.

It's a little messy, because the collapsing is implemented in a chunk hook after the code has been coalesced into one big string. I added attributes identifying which parts of that string shouldn't be touched.

[dmurdoch]

I tried a pattern matching approach first and it seemed too slow: sometimes the strings being examined can hold thousands of characters, e.g. the JSON for a complicated rgl scene. But I might have got the pattern wrong.

I agree that my current attempt is complicated. I'll take another look and see if I can improve it.

[dmurdoch]

This code uses a simpler approach. It just wraps the special results in markers that won't match what the markdown collapse code is looking for.

There's some chance that the special block will contain the marker and my cleanup will break it (but the markers have a time stamp; the next run should be okay), or that the special block will contain the pattern that the markdown chunk hook is looking for.

I think these are both unlikely, so I think this is a better patch than the previous one.

[dmurdoch]

One other comment: I removed the previous test code and didn't add a new test, because I don't know how to put together a test for this approach. It would need to do a visual evaluation of the final result of processing a full document.

[dmurdoch]

I just discovered that it's not just htmlwidgets, also output from browsable(HTML( .... )). In that case the class wasn't changed, so my original test missed it. I've just committed a test that finds those cases.

I think we don't want this to be specific, we want to recognize anything that isn't simple input or output and protect that from being modified.

The problem with changing sew is that sew is a generic, so you'd have to track down all packages that use it and change them too. rgl provides a method, but I could adapt if necessary. I don't know if any other packages do.

Perhaps changing whatever function knitr uses to wrap asis objects?

[cderv]

I think we don't want this to be specific, we want to recognize anything that isn't simple input or output and protect that from being modified.

All those output should have the knit_asis() class I believe. Do you have an example that miss that ?

Perhaps changing whatever function knitr uses to wrap asis objects?

We are handling asis object in sew.knit_asis() - that would be where to wrap such output. If you are referring to the the raw HTML part ```{=html} , this is handled by htmltools directly through htmltools:::html_preserve()

[dmurdoch]

You asked if all examples are "knit_asis". I am not sure. Some examples (e.g. htmltools::browsable(htmltools::HTML("html code")) don't start out with that class, and don't have that class after sew() is called, but they might have that class at some step in between.

I think the idea of wrapping the objects at that point is definitely worth a try, but I don't know where that code should go. Feel free to make changes on my "collapsebug" branch, or let me know if you want me to test a branch somewhere else.

[cderv]

(e.g. htmltools::browsable(htmltools::HTML("html code")) don't start out with that class, and don't have that class after sew()

The result of this call should have the knit_asis class when sew is called. This is because knit_print() will be called to print this and it will use htmltools:::knit_print.html which will apply knitr::asis_output(). I can also confirm that in debug mode when looking for the result of such chunkj

---
output: html_document
---

```{r}
htmltools::browsable(htmltools::HTML("html code"))

x [1] "\n{=html}\nhtml code\n\n" attr(,"class") [1] "knit_asis" attr(,"knit_cacheable") [1] NA

I think the idea of wrapping the objects at that point is definitely worth a try, but I don't know where that code should go. Feel free to make changes on my "collapsebug" branch, or let me know if you want me to test a branch somewhere else.

Ok sure I can implement my idea. Waiting for @yihui opinion on my thoughs and comments first.

Thanks @dmurdoch !

[dmurdoch]

I've just pushed a revision to the PR based on your suggestion. It appears to work for my test cases. You probably want to review the class names I chose.

Here's my test file.

---
title: "Vignette Title"
author: "Vignette Author"
date: "`r Sys.Date()`"
output: rmarkdown::html_vignette
vignette: >
  %\VignetteIndexEntry{Vignette Title}
  %\VignetteEngine{knitr::rmarkdown}
  %\VignetteEncoding{UTF-8}
---

```{r setup, include = FALSE}
knitr::opts_chunk$set(
  collapse = TRUE,
  comment = "#>"
)

# This is before the widget
x <- 2
x

library(leaflet)
leaflet() %>% addTiles()

# This is some more code after the widget
x <- 1
x

# And another widget:
leaflet() %>% addTiles()

library(htmltools)
# this is before the HTML
x <- 1
x

browsable(HTML("some html"))

# This is after the HTML

x <- 2
x

browsable(HTML("more html"))

# This is after the second HTML

x <- 3
x

library(rgl)
rgl::setupKnitr(autoprint = TRUE)

# this is before the rgl
x <- 1
x

plot3d(1:10, 1:10, 1:10)

# This is after the rgl

x <- 2
x

plot3d(1:10, 1:10, 1:10)

# This is after the second rgl

x <- 3
x

[dmurdoch]

I think it's difficult to remove anything after the collapsing has been done, because at that point you don't know where the boundaries are. So if you are inserting something that you plan to remove, it should contain some unique token. I used the current time in a previous attempt, but a random value using knitr's copy of the RNG would probably be better.

[cderv]

So if you are inserting something that you plan to remove, it should contain some unique token. I used the current time in a previous attempt, but a random value using knitr's copy of the RNG would probably be better.

Yes I agree with that. I'll think about this and discuss with @yihui. Thanks for the PR @dmurdoch !

[yihui]

Thanks for sharing the ideas! I think I fully understand them now. The key is a unique token that is applied when collapse = TRUE and removed later. I'm fine with a random string or a timestamp.

[dmurdoch]

Sounds like you have this under control. I won't make any more changes.

[yihui]

Yes, I can carry on from here. Thank you!