Better htmlwidget support#1215
Conversation
…red from the console
…enc dependency clear, improve tests
DavisVaughan
left a comment
DavisVaughan
left a comment
There was a problem hiding this comment.
The code all seems good, but i have not actually been able to get this feature to work for me yet
Ping me on slack if the examples below (i.e. leaflet) work for you and we can try to figure out what I'm doing wrong before merging
| pub unsafe extern "C-unwind" fn ps_is_notebook() -> anyhow::Result<SEXP> { | ||
| let is_notebook = matches!( | ||
| Console::get().session_mode(), | ||
| SessionMode::Notebook | SessionMode::Background | ||
| ); | ||
| Ok(libr::Rf_ScalarLogical(is_notebook as i32)) | ||
| } |
There was a problem hiding this comment.
Can we call this ps_session_mode() and emit a length 1 character vector of "console", "notebook" or "background"? That seems a bit more general.
And then put this at the bottom of console_repl.rs, which is the file where SessionMode is defined.
And then probably utils.R would hold the R wrapper of session_mode()
| } | ||
|
|
||
| /// Emit a pre-rendered, self-contained HTML widget as a Jupyter `display_data` | ||
| /// message. Called from R via `.ps.Call("ps_html_widget_emit", html, label)`. |
There was a problem hiding this comment.
| /// message. Called from R via `.ps.Call("ps_html_widget_emit", html, label)`. | |
| /// message. |
| if (isTRUE(.ps.is_notebook())) { | ||
| view_html_widget_inline(x) | ||
| return(invisible(x)) | ||
| } | ||
|
|
||
| view_html_widget_viewer(x) |
There was a problem hiding this comment.
Nit: Make it a true if / else so you don't need a return? I think it makes it more pleasant to read that way
| /// - `contents` - The complete HTML document to emit as `text/html` | ||
| /// - `kind` - A short label used in the `text/plain` fallback | ||
| fn emit_html_output_jupyter( | ||
| iopub_tx: Sender<IOPubMessage>, |
There was a problem hiding this comment.
Small thing I just noticed.
You can make this &Sender<IOPubMessage> and then at the call sites you can just provide console.iopub_tx() without having to .clone() it out first.
| head_parts <- c( | ||
| '<meta charset="utf-8"/>', | ||
| dep_html, | ||
| if (nzchar(rendered$head %||% "")) rendered$head else NULL |
There was a problem hiding this comment.
I feel like the only thing this is guarding against is a "" head? But it's hard to figure that out.
I feel like you could lift this up as
if (identical(rendered$head, "")) {
rendered$head <- NULL
}and that would accomplish the same thing, since an existing NULL passes through fine.
| paste(parts, collapse = "\n") | ||
| } | ||
|
|
||
| # `htmlDependency()` allows `script`/`stylesheet` to be either a character |
There was a problem hiding this comment.
i see htmlDependency mentioned 3 times but have no idea what it is
| # `options(ark.html_widget.deduplicate = TRUE)` if you know your frontend | ||
| # shares scope across cells. | ||
| filter_seen_deps <- function(deps) { | ||
| if (!isTRUE(getOption("ark.html_widget.deduplicate", FALSE))) { |
There was a problem hiding this comment.
I would also be ok with not having this for now
Regardless, if you are confident you want this then please document this new option in doc/configuration.md
| #' @export | ||
| .ps.html_widget_reset_deps <- function() { |
There was a problem hiding this comment.
Rather than making it an export, you can also access it in tests via the secret .ps.internal() export, i.e. .ps.internal(html_widget_reset_deps())
| /// (see `crates/ark/src/modules/positron/html_widgets.R`). Console mode keeps | ||
| /// the temp-file flow via `ps_html_viewer`. | ||
| #[harp::register] | ||
| pub unsafe extern "C-unwind" fn ps_html_widget_emit( |
There was a problem hiding this comment.
My biggest comment is that maybe we should consider a more generic name for this
It isn't realllllly widget specific, all this does it emit HTML via display_data
So I've renamed it in #1242 along with some other minor tweaks, take a look and feel free to merge as is or take parts and close
There was a problem hiding this comment.
Is this supposed to work?
library(leaflet)
m <- leaflet() %>% addTiles()
m
I tried it in a qmd with inline output turned on and got a blank output pane
There was a problem hiding this comment.
another one that doesn't work for me
library(billboarder)
# data
data("prod_par_filiere")
billboarder(data = prod_par_filiere) %>%
bb_barchart(
mapping = aes(x = annee, y = prod_hydraulique),
color = "#102246"
) %>%
bb_y_grid(show = TRUE) %>%
bb_y_axis(
tick = list(format = suffix("TWh")),
label = list(
text = "production (in terawatt-hours)",
position = "outer-top"
)
) %>%
bb_legend(show = FALSE) %>%
bb_labs(
title = "French hydraulic production",
caption = "Data source: RTE (https://opendata.reseaux-energies.fr/)"
)2 participants
Fix #1034
This PR adds proper htmlwidget support for notebooks and inline quarto content. It follows suggestion 2 from @jmcphers and inlines the html directly, like the IRKernel. The reasons for this is mainly that supporting Jupyter Widgets would be a substantial addition with no user-visible upside since the one thing it provides (bidirectional communication) isn't possible with htmlwidgets anyway. If a compelling use case for bidirectional communication comes up we can revisit it.
We don't need pandoc or any other heavy dependency because the html is trivial to construct.
One wrinkle is that IRKernel defaults to deduplicating html dependencies from consecutive cells. This facility also exist in this PR but since Positrons notebook view renders each cell in isolation. Due to that the default for ark is to not deduplicate.
All changes only has an effect for notebooks. The standard path for viewer/plot pane remains unchanged