search

JuliaGizmos / Interact.jl

Interactive widgets to play with your Julia code
Other
520 stars 75 forks source link

Blink: output doesn't update with more than one observable #344

Closed JobJob closed 4 years ago

JobJob commented 4 years ago

Managed to whittle this down to a very simple repro. At the REPL:

using Blink, Interact
s = slider(1:10)
# ui = vbox(s, observe(s)); # updates fine when slider is moved
ui = vbox(s, observe(s), observe(s)); # neither outputs update when slider is moved
w = Window()
body!(w, ui)

When the slider is moved the observe(s) outputs don't update. The value of s[] is in sync with the slider though in Julia.

The same example in Jupyter Notebook (without the last two lines) works fine, it also works with Mux.

This isn't just happening in this contrived example. The actual code that I ran into this with has a PlotlyJS plot between the slider and some output.

https://github.com/JuliaGizmos/WebIO.jl/issues/357 might also be related to this.

How do you even see what messages get sent to the Blink Window in order to debug this? Does this update happen on the js side, or is it slider->julia->window? What code should I look at to get started trying to fix this? It's been a while...

This is with Julia 1.1.0

(v1.1) pkg> st --manifest WebIO Blink Interact
    Status `~/.julia/environments/v1.1/Manifest.toml`
  [bf4720bc] AssetRegistry v0.1.0
  [9e28174c] BinDeps v0.8.10
  [ad839575] Blink v0.12.0
  [70588ee8] CSSUtil v0.1.0
  [34da2185] Compat v2.2.0
  [de31a74c] FunctionalCollections v0.5.0
  [c601a237] Interact v0.10.3
  [d3863d7c] InteractBase v0.10.3
  [97c1335a] JSExpr v0.5.1
  [682c06a0] JSON v0.21.0
  [bcebb21b] Knockout v0.2.3
  [50d2b5c4] Lazy v0.13.2
  [1914dd2f] MacroTools v0.5.2
  [ffc61752] Mustache v0.5.13
  [a975b10e] Mux v0.7.0
  [510215fc] Observables v0.2.3
  [bac558e1] OrderedCollections v1.1.0
  [189a3867] Reexport v0.2.0
  [ae029012] Requires v0.5.2
  [0f1e0344] WebIO v0.8.11
  [104b5d7c] WebSockets v1.5.2
  [cc8bc4a8] Widgets v0.6.2
  [2a0f44e3] Base64
  [8ba89e20] Distributed
  [56ddb016] Logging
  [44cfe95a] Pkg
  [9a3f8284] Random
  [6462fe0b] Sockets
  [cf7118a7] UUIDs
JobJob commented 4 years ago

I added some debugging output on the Julia side of WebIO and tracked this down to something going wrong on the isopen call here (probably a deadlock):

I noticed that Blink's isopen changed slightly when the webio integration got moved to Blink.jl previously it was called on the Page (BlinkConnection defined here)

And now it's called on the Window (WebIOBlinkComm defined here)

So I made the obvious change, and it works:

Base.isopen(comm::WebIOBlinkComm) = active(comm.window.content)

I have no idea what the difference is between those two, or the exact cause of the problem, but it fixes my cases, and seems reasonable.

Will PR to Blink if everyone/anyone's happy? (besides me, I'm already happy 😄)

JobJob commented 4 years ago

Just checked and it also fixes https://github.com/JuliaGizmos/WebIO.jl/issues/357 for me.

It'd be good to have a test for this in WebIO, ~I saw that most of test/communication.jl is commented out.~ oh I see there's test/multiple-connections.jl

rafaqz commented 4 years ago

I'm still experiencing this. Observables holding an image update in atom or served with mux, but don't update with a Blink.jl electron window. Any ideas?

JobJob commented 4 years ago

Ahh bummer. @rafaqz does add Blink#jb/fix-isopen from the package REPL fix this for you?

I figured this had probably been fixed in @travigd's recent-ish comms overhaul so didn't follow this up. (Aside: commanding work in this org btw @travigd ❤️)

Anyway, @travigd was there a reason to change the Blink isopen call from

Base.isopen(comm::WebIOBlinkComm) = active(comm.window.content)

to

Base.isopen(comm::WebIOBlinkComm) = active(comm.window)

when the webio integration got moved to Blink.jl?

If not I can PR to Blink from add my branch there.

twavv commented 4 years ago

was there a reason to change the Blink isopen call

I don't think so, though I'm curious why active(window) and active(window.content) are different.

Feel free to make the PR. :^)

commanding work in this org btw @travigd ❤️

Thanks! I just wish I had more time to dedicate - clearly there are still quite a few issues around the Gizmos ecosystem, but $DAYJOB is eating up all of my time.

rafaqz commented 4 years ago

@JobJob add Blink#jb/fix-isopen fixes the problem for me.

Would be great to get your changes into master for Blink.jl.

JobJob commented 4 years ago

Thanks for checking @rafaqz

Thanks! I just wish I had more time to dedicate - clearly there are still quite a few issues around the Gizmos ecosystem, but $DAYJOB is eating up all of my time.

😄 Know the feeling very well