-
-
Notifications
You must be signed in to change notification settings - Fork 872
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
Fix conflict between collapse=TRUE and htmlwidgets #2212
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! This looks too complicated for me to maintain, so I wonder if there is a simpler solution. For example, look for the pattern ([lang]
is optional attributes)
```[lang]\n...```\n```[lang]\n```
and substitute with
```[lang]\n...\n...```
Since the pattern ```[lang]
won't match ```{=lang}
, HTML widgets shouldn't be touched.
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. |
…rom messing things up.
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. |
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My first idea would be like you to modify the collapse processing we have
Lines 214 to 217 in 77970b0
if (isTRUE(options$collapse)) { | |
r = sprintf('\n([%s]{3,})\n+\\1((\\{[.])?%s[^\n]*)?\n', fence_char, tolower(options$engine)) | |
x = gsub(r, '\n', x) | |
} |
I don't know if we could try something simpler but currently we are indeed trying to match this pattern with our current regex ("\n([
]{3,})\n+\1((\{[.])?r[^\n]*)?\n"`
```
```r
This is indeed not correct when we have some output like htmlwidgets
which will use raw code block that are not meant to be collapse into a code chunk
```r
# R code here producing HTML to insert asis
```
```{=html}
<!-- raw html in there -->
```
```r
# another R code with code result
```
```
# the result in code markup
```
We would need to parse and match blocks differently. We don't have a good parser for that.
So the wrapping with special content is a good trick that rely only on the fact our current regex expect only newline after the backticks ```
- so just adding something after that for htmlwidget output which as raw block would work.
This is a bit fragile though.
Fun Fact @yihui (or not 😅 ) : 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.
::: {.cell-output-display}
```{=html}
<raw html here>
```
:::
This is also for me another reason to do that at sew
method or hooks level because those function can be easily replace by tools like quarto and we would not create conflict (unlike modifying at the block.R
level I think)
Not that the global issue is not with htmlwidget only, but any R code that would produce a asis output using special Pandoc syntax raw block. Probably more content does not work with our simple collapse processing I guess - surprised that this was not reported before.
I am surprised that this was not reported before. Probably because a few people are using chunks mixing HTMLwidgets output and standard R output, and using collapse = TRUE
on them. I guess if we test every possible output with collapse = TRUE
, we could have other suprises 😅
R/block.R
Outdated
# regexp. | ||
|
||
if (isTRUE(options$collapse)) | ||
specials = vapply(sewn, function(s) !identical(class(s), 'character'), FALSE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will target also other class that HTMLwidget object, isn't it ? If we want to wrap the sewed output of HTMLwidget we could target a more scope class couldn't we ?
attr(,"class")
[1] "knit_asis" "knit_asis_htmlwidget"
R/block.R
Outdated
specialSeparator = paste('KNITR SPECIAL OBJECT', format(Sys.time(), '%OS6')) | ||
for (i in which(specials)) | ||
sewn[[i]] = c(specialSeparator, sewn[[i]], | ||
specialSeparator) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following previous comment, if we go with this trick, I think it would be better to do this directly in sew.knit_asis
when it inherit knit_asis_htmlwidget
? we can target thinks there.
Lines 481 to 485 in 77970b0
if (inherits(x, 'knit_asis_htmlwidget')) { | |
options$fig.cur = plot_counter() | |
options = reduce_plot_opts(options) | |
return(add_html_caption(options, x)) | |
} |
This would be the best place to add some content to the sewed output for html widget I think when collapse is true
We could then remove the special where we do the collapse processing, or use a special pattern that we register and clean all pattern in block.R
as you did
… a class, just with the opening markup.
I just discovered that it's not just htmlwidgets, also output from 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 Perhaps changing whatever function |
All those output should have the
We are handling asis object in |
You asked if all examples are "knit_asis". I am not sure. Some examples (e.g. 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. |
The result of this call should have the ---
output: html_document
---
```{r}
htmltools::browsable(htmltools::HTML("html code"))
```
Ok sure I can implement my idea. Waiting for @yihui opinion on my thoughs and comments first. Thanks @dmurdoch ! |
…output in `:::` markers.
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.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The current revision looks good to me. I need to check reverse dependencies to see if it would break any of them. We could be more conservative and apply the :::
div only when options$collapse
is TRUE
. Not sure if it's worth it. I'll know after the reverse dependency checks are done. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could be more conservative and apply the ::: div only when options$collapse is TRUE
Yes I think this needs to be scoped to collapse TRUE
.
Let me precise my previous remarks though...
I just gave the example of Quarto using divs with :::
to wrap to show an example. But Quarto adds the divs syntax for a purpose - such syntax will add AST node in Pandoc to deal with.
If we do the same, we add another layer in AST for Pandoc that we don't really use and can break other tools like Quarto.
My previous comment was mainly about doing your wrapping idea in another place closer to htmlwidget sewing and related to collapse part. So I think you idea of wrapping with a special token / line / knitr string is a good one. I believe we need this for collapse only so we could add and then remove when collapse is applied.
I don't think we need to keep such wrapping in the knitr output. @yihui this would mean no impact on reverse dependencies I believe.
Unless we think having this wrapping is beneficial for other purposes.
Hope my thoughts are shared clearer this time. Sorry for any misleading.
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. |
Yes I agree with that. I'll think about this and discuss with @yihui. Thanks for the PR @dmurdoch ! |
Thanks for sharing the ideas! I think I fully understand them now. The key is a unique token that is applied when |
Sounds like you have this under control. I won't make any more changes. |
Yes, I can carry on from here. Thank you! |
As reported in my comment to #2172,
collapse = TRUE
is incompatible with includinghtmlwidgets
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.