From c4e6a5f4e5daef430e7a47257ce1d6d42a41e6af Mon Sep 17 00:00:00 2001 From: Julius Krumbiegel Date: Tue, 18 Jun 2024 13:05:30 +0200 Subject: [PATCH 1/9] change `include_plotlysj` to `require-loaded` --- .../ext/QuartoNotebookWorkerPlotlyBaseExt.jl | 2 +- src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl index 57ecf1c..24cac98 100644 --- a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl +++ b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl @@ -13,7 +13,7 @@ function Base.show(io::IO, ::MIME"text/html", wrapper::PlotlyBasePlot) # We want to embed only the minimum markup needed to render the # plotlyjs plots, otherwise a full HTML page is generated for every # plot which does not render correctly in our context. - PlotlyBase.to_html(io, wrapper.value; include_plotlyjs = "require", full_html = false) + PlotlyBase.to_html(io, wrapper.value; include_plotlyjs = "require-loaded", full_html = false) end end diff --git a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl index 89db953..1db3756 100644 --- a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl +++ b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl @@ -16,7 +16,7 @@ function Base.show(io::IO, ::MIME"text/html", wrapper::PlotlyJSSyncPlot) PlotlyJS.PlotlyBase.to_html( io, wrapper.value.plot; - include_plotlyjs = "require", + include_plotlyjs = "require-loaded", full_html = false, ) end From 14430841d3a3d7065a3761c63016e42c92bbfe2d Mon Sep 17 00:00:00 2001 From: Julius Krumbiegel Date: Tue, 18 Jun 2024 15:32:38 +0200 Subject: [PATCH 2/9] move mimetype wrapping before expansion --- src/QuartoNotebookWorker/src/render.jl | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/QuartoNotebookWorker/src/render.jl b/src/QuartoNotebookWorker/src/render.jl index 8b129c2..ebd14d2 100644 --- a/src/QuartoNotebookWorker/src/render.jl +++ b/src/QuartoNotebookWorker/src/render.jl @@ -47,10 +47,15 @@ function _render_thunk( # so we need to catch that and return an error cell if that's the case. expansion = nothing is_expansion = false + # Intercept objects prior to rendering so that we can wrap specific + # types in our own `WrapperType` to customised rendering instead of + # what the package defines itself. + # Calling the wrapping mechanism here allows it to affect expansion + value = Base.@invokelatest _mimetype_wrapper(captured.value) if !captured.error try - expansion = Base.@invokelatest expand(captured.value) - is_expansion = _is_expanded(captured.value, expansion) + expansion = Base.@invokelatest expand(value) + is_expansion = _is_expanded(value, expansion) catch error backtrace = catch_backtrace() return ((; @@ -92,7 +97,7 @@ function _render_thunk( end else results = Base.@invokelatest render_mimetypes( - REPL.ends_with_semicolon(code) ? nothing : captured.value, + REPL.ends_with_semicolon(code) ? nothing : value, cell_options; inline, ) @@ -105,11 +110,11 @@ function _render_thunk( results, display_results, output = captured.output, - error = captured.error ? string(typeof(captured.value)) : nothing, + error = captured.error ? string(typeof(value)) : nothing, backtrace = collect( eachline( IOBuffer( - clean_bt_str(captured.error, captured.backtrace, captured.value), + clean_bt_str(captured.error, captured.backtrace, value), ), ), ), @@ -301,11 +306,6 @@ Base.showable(mime::MIME, w::WrapperType) = Base.showable(mime, w.value) # for inline code chunks, `inline` should be set to `true` which causes "text/plain" output like # what you'd get from `print` (Strings without quotes) and not from `show("text/plain", ...)` function render_mimetypes(value, cell_options; inline::Bool = false) - # Intercept objects prior to rendering so that we can wrap specific - # types in our own `WrapperType` to customised rendering instead of - # what the package defines itself. - value = _mimetype_wrapper(value) - to_format = NotebookState.OPTIONS[]["format"]["pandoc"]["to"] result = Dict{String,@NamedTuple{error::Bool, data::Vector{UInt8}}}() From f9faa0f6e64df7b7ba8d4b4609d10ab5d42fc2af Mon Sep 17 00:00:00 2001 From: Julius Krumbiegel Date: Tue, 18 Jun 2024 15:32:52 +0200 Subject: [PATCH 3/9] forward PlotlyJS to PlotlyBase wrapper mechanism --- .../ext/QuartoNotebookWorkerPlotlyJSExt.jl | 18 +----------------- 1 file changed, 1 insertion(+), 17 deletions(-) diff --git a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl index 1db3756..b3c22d7 100644 --- a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl +++ b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl @@ -3,22 +3,6 @@ module QuartoNotebookWorkerPlotlyJSExt import QuartoNotebookWorker import PlotlyJS -QuartoNotebookWorker._mimetype_wrapper(p::PlotlyJS.SyncPlot) = PlotlyJSSyncPlot(p) - -struct PlotlyJSSyncPlot <: QuartoNotebookWorker.WrapperType - value::PlotlyJS.SyncPlot -end - -function Base.show(io::IO, ::MIME"text/html", wrapper::PlotlyJSSyncPlot) - # We want to embed only the minimum markup needed to render the - # plotlyjs plots, otherwise a full HTML page is generated for every - # plot which does not render correctly in our context. - PlotlyJS.PlotlyBase.to_html( - io, - wrapper.value.plot; - include_plotlyjs = "require-loaded", - full_html = false, - ) -end +QuartoNotebookWorker._mimetype_wrapper(p::PlotlyJS.SyncPlot) = QuartoNotebookWorker._mimetype_wrapper(p.plot) end From bb622b0dc281af7185c48c4a3ace537fe6b7229c Mon Sep 17 00:00:00 2001 From: Julius Krumbiegel Date: Tue, 18 Jun 2024 15:34:20 +0200 Subject: [PATCH 4/9] add mechanism to insert require.js preamble into its own cell before first plotly plot --- .../ext/QuartoNotebookWorkerPlotlyBaseExt.jl | 49 +++++++++++++++++-- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl index 24cac98..0830da3 100644 --- a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl +++ b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl @@ -4,16 +4,59 @@ import QuartoNotebookWorker import PlotlyBase QuartoNotebookWorker._mimetype_wrapper(p::PlotlyBase.Plot) = PlotlyBasePlot(p) - struct PlotlyBasePlot <: QuartoNotebookWorker.WrapperType value::PlotlyBase.Plot end -function Base.show(io::IO, ::MIME"text/html", wrapper::PlotlyBasePlot) +struct PlotlyBasePlotWithoutRequireJS + plot::PlotlyBase.Plot +end + +struct PlotlyRequireJSConfig end + +const FIRST_PLOT_DISPLAYED = Ref(false) + +function QuartoNotebookWorker.expand(p::PlotlyBasePlot) + + plotcell = QuartoNotebookWorker.Cell(PlotlyBasePlotWithoutRequireJS(p.value)) + + # Quarto expects that the require.js preamble which Plotly needs to function + # comes in its own cell, which will then be hoisted into the HTML page header. + # So we cannot have that preamble concatenated with every plot's HTML content. + # Instead, e keep track whether a Plotly plot is the first per notebook, and in that + # case have it expand into the preamble cell and the plot cell. If it's not the + # first time, we expand only into the plot cell. + cells = if !FIRST_PLOT_DISPLAYED[] + [QuartoNotebookWorker.Cell(PlotlyRequireJSConfig()), plotcell] + else + [plotcell] + end + + FIRST_PLOT_DISPLAYED[] = true + + return cells +end + +function Base.show(io::IO, ::MIME"text/html", p::PlotlyBasePlotWithoutRequireJS) # We want to embed only the minimum markup needed to render the # plotlyjs plots, otherwise a full HTML page is generated for every # plot which does not render correctly in our context. - PlotlyBase.to_html(io, wrapper.value; include_plotlyjs = "require-loaded", full_html = false) + # "require-loaded" means that we pass the require.js preamble ourselves. + PlotlyBase.to_html(io, p.plot; include_plotlyjs = "require-loaded", full_html = false) +end + +function Base.show(io::IO, ::MIME"text/html", ::PlotlyRequireJSConfig) + print(io, PlotlyBase._requirejs_config()) +end + +function reset_first_plot_displayed_flag!() + FIRST_PLOT_DISPLAYED[] = false +end + +function __init__() + if ccall(:jl_generating_output, Cint, ()) == 0 + QuartoNotebookWorker.add_package_refresh_hook!(reset_first_plot_displayed_flag!) + end end end From e1d44ef4d8019ec6465ccbd09dd71b617f0965ea Mon Sep 17 00:00:00 2001 From: Julius Krumbiegel Date: Tue, 18 Jun 2024 15:39:09 +0200 Subject: [PATCH 5/9] Revert "move mimetype wrapping before expansion" This reverts commit 14430841d3a3d7065a3761c63016e42c92bbfe2d. --- src/QuartoNotebookWorker/src/render.jl | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/QuartoNotebookWorker/src/render.jl b/src/QuartoNotebookWorker/src/render.jl index ebd14d2..8b129c2 100644 --- a/src/QuartoNotebookWorker/src/render.jl +++ b/src/QuartoNotebookWorker/src/render.jl @@ -47,15 +47,10 @@ function _render_thunk( # so we need to catch that and return an error cell if that's the case. expansion = nothing is_expansion = false - # Intercept objects prior to rendering so that we can wrap specific - # types in our own `WrapperType` to customised rendering instead of - # what the package defines itself. - # Calling the wrapping mechanism here allows it to affect expansion - value = Base.@invokelatest _mimetype_wrapper(captured.value) if !captured.error try - expansion = Base.@invokelatest expand(value) - is_expansion = _is_expanded(value, expansion) + expansion = Base.@invokelatest expand(captured.value) + is_expansion = _is_expanded(captured.value, expansion) catch error backtrace = catch_backtrace() return ((; @@ -97,7 +92,7 @@ function _render_thunk( end else results = Base.@invokelatest render_mimetypes( - REPL.ends_with_semicolon(code) ? nothing : value, + REPL.ends_with_semicolon(code) ? nothing : captured.value, cell_options; inline, ) @@ -110,11 +105,11 @@ function _render_thunk( results, display_results, output = captured.output, - error = captured.error ? string(typeof(value)) : nothing, + error = captured.error ? string(typeof(captured.value)) : nothing, backtrace = collect( eachline( IOBuffer( - clean_bt_str(captured.error, captured.backtrace, value), + clean_bt_str(captured.error, captured.backtrace, captured.value), ), ), ), @@ -306,6 +301,11 @@ Base.showable(mime::MIME, w::WrapperType) = Base.showable(mime, w.value) # for inline code chunks, `inline` should be set to `true` which causes "text/plain" output like # what you'd get from `print` (Strings without quotes) and not from `show("text/plain", ...)` function render_mimetypes(value, cell_options; inline::Bool = false) + # Intercept objects prior to rendering so that we can wrap specific + # types in our own `WrapperType` to customised rendering instead of + # what the package defines itself. + value = _mimetype_wrapper(value) + to_format = NotebookState.OPTIONS[]["format"]["pandoc"]["to"] result = Dict{String,@NamedTuple{error::Bool, data::Vector{UInt8}}}() From 0d5c13ffc7990abf75b1ff3bd8784253ae0ae7e2 Mon Sep 17 00:00:00 2001 From: Julius Krumbiegel Date: Tue, 18 Jun 2024 15:39:44 +0200 Subject: [PATCH 6/9] change forwarding mechanism to `expand` --- src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl index b3c22d7..b25cb6e 100644 --- a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl +++ b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyJSExt.jl @@ -3,6 +3,6 @@ module QuartoNotebookWorkerPlotlyJSExt import QuartoNotebookWorker import PlotlyJS -QuartoNotebookWorker._mimetype_wrapper(p::PlotlyJS.SyncPlot) = QuartoNotebookWorker._mimetype_wrapper(p.plot) +QuartoNotebookWorker.expand(p::PlotlyJS.SyncPlot) = QuartoNotebookWorker.expand(p.plot) end From 03e3c0d1b37485b0d03d306ba90691cbe52ce402 Mon Sep 17 00:00:00 2001 From: Julius Krumbiegel Date: Tue, 18 Jun 2024 15:46:39 +0200 Subject: [PATCH 7/9] don't user mimetype_wrapper at all --- .../ext/QuartoNotebookWorkerPlotlyBaseExt.jl | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl index 0830da3..76ec1aa 100644 --- a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl +++ b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl @@ -3,11 +3,6 @@ module QuartoNotebookWorkerPlotlyBaseExt import QuartoNotebookWorker import PlotlyBase -QuartoNotebookWorker._mimetype_wrapper(p::PlotlyBase.Plot) = PlotlyBasePlot(p) -struct PlotlyBasePlot <: QuartoNotebookWorker.WrapperType - value::PlotlyBase.Plot -end - struct PlotlyBasePlotWithoutRequireJS plot::PlotlyBase.Plot end @@ -16,9 +11,9 @@ struct PlotlyRequireJSConfig end const FIRST_PLOT_DISPLAYED = Ref(false) -function QuartoNotebookWorker.expand(p::PlotlyBasePlot) +function QuartoNotebookWorker.expand(p::PlotlyBase.Plot) - plotcell = QuartoNotebookWorker.Cell(PlotlyBasePlotWithoutRequireJS(p.value)) + plotcell = QuartoNotebookWorker.Cell(PlotlyBasePlotWithoutRequireJS(p)) # Quarto expects that the require.js preamble which Plotly needs to function # comes in its own cell, which will then be hoisted into the HTML page header. From 5932d5e4a538a947deeaeb14da4d148d10a8bbd1 Mon Sep 17 00:00:00 2001 From: Julius Krumbiegel Date: Tue, 18 Jun 2024 17:01:43 +0200 Subject: [PATCH 8/9] forward mime types and fix tests --- .../ext/QuartoNotebookWorkerPlotlyBaseExt.jl | 5 +++++ test/testsets/integrations/PlotlyJS.jl | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl index 76ec1aa..a880295 100644 --- a/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl +++ b/src/QuartoNotebookWorker/ext/QuartoNotebookWorkerPlotlyBaseExt.jl @@ -40,6 +40,11 @@ function Base.show(io::IO, ::MIME"text/html", p::PlotlyBasePlotWithoutRequireJS) PlotlyBase.to_html(io, p.plot; include_plotlyjs = "require-loaded", full_html = false) end +Base.show(io::IO, M::MIME, p::PlotlyBasePlotWithoutRequireJS) = show(io, M, p.plot) +Base.show(io::IO, m::MIME"text/plain", p::PlotlyBasePlotWithoutRequireJS) = + show(io, m, p.plot) +Base.showable(M::MIME, p::PlotlyBasePlotWithoutRequireJS) = showable(M, p.plot) + function Base.show(io::IO, ::MIME"text/html", ::PlotlyRequireJSConfig) print(io, PlotlyBase._requirejs_config()) end diff --git a/test/testsets/integrations/PlotlyJS.jl b/test/testsets/integrations/PlotlyJS.jl index e98c132..077ad73 100644 --- a/test/testsets/integrations/PlotlyJS.jl +++ b/test/testsets/integrations/PlotlyJS.jl @@ -6,7 +6,7 @@ if Sys.iswindows() else test_example(joinpath(@__DIR__, "../../examples/integrations/PlotlyJS.qmd")) do json cells = json["cells"] - for nth in (4, 6) + for nth in (6, 9) cell = cells[nth] outputs = cell["outputs"] @test length(outputs) == 1 From 24db3d8b82e015d600f90046af424091bd1affd4 Mon Sep 17 00:00:00 2001 From: Julius Krumbiegel Date: Tue, 18 Jun 2024 17:40:44 +0200 Subject: [PATCH 9/9] test presence of preamble cell --- test/testsets/integrations/PlotlyJS.jl | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/test/testsets/integrations/PlotlyJS.jl b/test/testsets/integrations/PlotlyJS.jl index 077ad73..6c9c272 100644 --- a/test/testsets/integrations/PlotlyJS.jl +++ b/test/testsets/integrations/PlotlyJS.jl @@ -6,6 +6,13 @@ if Sys.iswindows() else test_example(joinpath(@__DIR__, "../../examples/integrations/PlotlyJS.qmd")) do json cells = json["cells"] + preamble_cell = cells[5] + outputs = preamble_cell["outputs"] + @test length(outputs) == 1 + data = outputs[1]["data"] + @test keys(data) == Set(["text/html", "text/plain"]) + @test startswith(data["text/html"], "