Skip to content
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

Click "more" for embeddable element doesn't work #2619

Closed
jbrea opened this issue Jul 27, 2023 · 8 comments · Fixed by #2632
Closed

Click "more" for embeddable element doesn't work #2619

jbrea opened this issue Jul 27, 2023 · 8 comments · Fixed by #2632
Labels
bug Something isn't working

Comments

@jbrea
Copy link
Contributor

jbrea commented Jul 27, 2023

The "more" button doesn't seem to work as expected (and reported in #1126) when embedded in an @htl block. It works normally for direct output.
more_bug.jl.txt
more_bug.webm

@disberd
Copy link
Contributor

disberd commented Aug 9, 2023

The problem also seems to appear within logs (e.g. @info) even without @htl:

0b26bb78-ab2a-4874-9efd-39dd003717c7.mp4

@Pangoraw Pangoraw added the bug Something isn't working label Aug 13, 2023
@jbrea
Copy link
Contributor Author

jbrea commented Aug 24, 2023

@disberd @Pangoraw Could somebody give me some pointers where to start to fix this bug? I guess the data is somehow not cached in the right js-variable, but I have little knowledge about Pluto internals and would therefore appreciate some input from a specialist.

@Pangoraw
Copy link
Collaborator

This is the code which gets called when More is clicked:

const actions_show_more = ({ pluto_actions, cell_id, node_ref, objectid, dim }) => {
const actions = pluto_actions ?? node_ref.current.closest("pluto-cell")._internal_pluto_actions
actions.reshow_cell(cell_id ?? node_ref.current.closest("pluto-cell").id, objectid, dim)
}

which in turns calls the following Julia code:

responses[:reshow_cell] = function response_reshow_cell(🙋::ClientRequest)
require_notebook(🙋)
cell = let
cell_id = UUID(🙋.body["cell_id"])
🙋.notebook.cells_dict[cell_id]
end
run = WorkspaceManager.format_fetch_in_workspace(
(🙋.session, 🙋.notebook),
cell.cell_id,
ends_with_semicolon(cell.code),
collect(keys(cell.published_objects)),
(parse(PlutoRunner.ObjectID, 🙋.body["objectid"], base=16), convert(Int64, 🙋.body["dim"])),
)
set_output!(cell, run, ExprAnalysisCache(🙋.notebook, cell); persist_js_state=true)
# send to all clients, why not
send_notebook_changes!(🙋 |> without_initiator)
end

Now it makes sense for More in logs to not be working since reshow_cell only rerenders the output of the cell so the object id will always be unfound in the output. For the @htl problem, we have to look at whether or not the object ids are consistent when reshowing.

Here is the showmore handling in the actual renderer:

extra_items = if showmore === nothing
tree_display_extra_items[cell_id] = Dict{ObjectDimPair,Int64}()
else
old = get!(() -> Dict{ObjectDimPair,Int64}(), tree_display_extra_items, cell_id)
old[showmore] = get(old, showmore, 0) + 1
old
end

@jbrea
Copy link
Contributor Author

jbrea commented Aug 24, 2023

Just noticed that 1d2c303 broke

const body = $(publish_to_js(body, e.script_id));
which relies on a two-argument publish_to_js. Does it make sense, if I try to track down the bug in a commit prior to 1d2c303 or should Base.show(io::IO, m::MIME"text/html", e::EmbeddableDisplay) first be fixed on main?

EDIT: see below.

@jbrea
Copy link
Contributor Author

jbrea commented Aug 24, 2023

Ah, in fact, the @htl example above works on master with const body = $(PublishedToJavascript(body)); instead of

const body = $(publish_to_js(body, e.script_id));

Is this the right way forward or will PublishedToJavascript be deprecated?

@disberd
Copy link
Contributor

disberd commented Aug 25, 2023

@jbrea I think the line you specifically referenced was something that was left unchanged (by mistake) on #2608

The publish_to_js function previously accepted a 2-argument version while it does not anymore now. (EDIT: Just realized now that indeed that is what you were also saying :D)

I think the best way forward here is to have const body = $(core_published_to_js(io, x)) as that is what internally is being called by the show method for PublishedToJavascript:

function Base.show(io::IO, ::MIME"text/javascript", published::PublishedToJavascript)
core_published_to_js(io, published.published_object)
end

I don't remember now if PublishedToJavascript is also used elsewhere or if it will be effectively deprecated as it was just used within publish_to_js

core_published_to_js is the new API which should be used inside Pluto (and is called directly when showing PublishedToJavascript objects as seen above) and is also accessed by AbstractPlutoDingetjes for providing this functionality in external packages when they need to provide efficient julia-js exchange in Pluto (see JuliaPluto/AbstractPlutoDingetjes.jl#11)

function core_published_to_js(io, x)
assertpackable(x)
id_start = objectid2str(x)
_notebook_id = get(io, :pluto_notebook_id, notebook_id[])::UUID
_cell_id = get(io, :pluto_cell_id, currently_running_cell_id[])::UUID
# The unique identifier of this object
id = "$_notebook_id/$id_start"
d = get!(Dict{String,Any}, cell_published_objects, _cell_id)
d[id] = x
write(io, "/* See the documentation for AbstractPlutoDingetjes.Display.published_to_js */ getPublishedObject(\"$(id)\")")
return nothing
end

@jbrea
Copy link
Contributor Author

jbrea commented Aug 25, 2023

Thanks, @disberd

const body = $(core_published_to_js(io, x))

I think this does not work, because of string interpolation. String interpolation of PublishedToJavascript(x) works, but the io there is the io of string interpolation, whereas the io in the above line refers to the io in Base.show(io::IO, m::MIME"text/html", e::EmbeddableDisplay). I guess, going through PublishedToJavascript is still the best, no?

@disberd
Copy link
Contributor

disberd commented Aug 26, 2023

@jbrea you are probably right, didn't think about the interpolation directly on the string

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants