-
Notifications
You must be signed in to change notification settings - Fork 22
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
Support rgl and htmlwidgets in pkgdown #78
Conversation
- Allow packages like rgl to have low level and high level objects (see yihui/knitr#1892) - Export replay_html so other packages can add methods.
R/evaluate.R
Outdated
#' @param ... Additional parameters to pass to methods. | ||
#' | ||
#' @return character vector containing html | ||
#' @export |
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.
Does this have to be exported? i.e. are you providing a method for it in rgl? If so, I think we'll probably need to change the name, since otherwise I'll get confused because I have internal replay_html()
functions in multiple packages.
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.
I think it does need to be exported, or I'll get a warning when I try to put a method on it in rgl
. Currently I've got code there to avoid the warning, but I think if anyone else wants to use this they'd want something simpler. Or maybe we should just document that the method needs to be registered using code like:
registerS3method("replay_html", "rglRecordedplot",
replay_html.rglRecordedplot,
envir = asNamespace("downlit"))
@@ -150,12 +167,19 @@ unique_id <- function() { | |||
|
|||
# get MD5 digests of recorded plots so that merge_low_plot works | |||
digest_plot = function(x, level = 1) { | |||
if (!is.list(x) || level >= 3) return(digest::digest(x)) | |||
if (inherits(x, "otherRecordedplot")) |
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.
Does this class come from rgl?
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 class currently only exists in rgl
, where I used class c("rglRecordedPlot", "otherRecordedPlot")
for the object it's supposed to handle. The idea is that it's an object that has both high level and low level versions (like base graphics, where plot()
produces high level changes, but points()
produces low level changes). You don't want to display every low level change, you just want to display the plot after the last one.
As far as I know the only implementations of this style of graphics are base graphics and rgl
(that was written to emulate base), so it's not really necessary now: but if anyone else ever writes one, they wouldn't want their plots to be handled like rgl
plots in other respects.
I submitted similar changes to knitr
back in September (yihui/knitr#1892); Yihui hasn't merged them yet. In that PR I used the name "knitr_other_plot"
instead of "otherRecordedPlot"
, but I didn't like that name much, and it doesn't make sense to use it for pkgdown
. I will probably suggest modifying the knitr
PR to use the same name as used in pkgdown
, whatever that turns out to be.
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.
I forgot, "otherRecordedPlot"
isn't just an object with high and low levels, it could also be used just to break up a sequence of those. For example, knitr
treats "knit_image_paths"
objects like plots when deciding whether to break up a sequence of base graphics calls. The change would add "otherRecordedPlot"
objects to the list of things that count as plots.
# merge low-level plotting changes | ||
merge_low_plot = function(x, idx = sapply(x, evaluate::is.recordedplot)) { | ||
merge_low_plot = function(x, idx = vapply(x, is_plot_output, logical(1L))) { |
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.
I'm slightly nervous about modifying this code since it came from knitr. Are you planning to submit changes there as well?
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.
As mentioned a similar change was submitted in yihui/knitr#1892 .
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.
I just took a closer look at yihui/knitr#1892 . It didn't change the default for idx
in merge_low_plot
, because merge_low_plot
is called with an explicit value for idx
, calculated here: https://github.com/yihui/knitr/blob/ca09938f8bb08cdd949c3cc6b94d640f68529126/R/block.R#L400 . The findRecordedPlot
function is here: https://github.com/yihui/knitr/blob/ca09938f8bb08cdd949c3cc6b94d640f68529126/R/block.R#L370. It uses the formula I put in as the default in this PR.
(Since no longer exported)
Thanks @dmurdoch! |
This patch allows
rgl
orhtmlwidget
displays to appear inpkgdown
web sites. It requires a corresponding patch topkgdown
, available asFor
rgl
output, useFor
htmlwidgets
output, useremotes::install_github("dmurdoch/htmlwidgets@rglpatch")