Add option to make creating a Window synchronous #137

NHDaly · 2018-07-27T20:33:55Z

This is the remaining part of #136: making window creation synchronous.

I've added an :async param to the dictionary options to create the window, and it will cause the function to block until the window is created, the blink.js javascript is loaded, and the WebSocket to communicate with julia is open.

This PR definitely needs a design review. I haven't actually done much javascript development ever, so a lot of this was murky water for me. I think it works, but i'm not sure I did everything the best way.

Also, I set the default to be synchronous, with the optional :async flag. I think that's the right default value, but just like in #136, we should think about whether to flip it right away or roll this out somehow nicer. That said, I think probably it would be fine to Just Do It! :)

For #108

NHDaly · 2018-07-27T20:34:36Z

(This PR is currently based on the synchronous branch, but actually i don't think that's necessary.. I think they touch entirely separate stuff. I can rebase it back onto master if you'd prefer!)

NHDaly · 2018-07-27T20:40:53Z

Also, this shaved around 20seconds off of test time on my machine, so that's cool! :)

NHDaly · 2018-08-14T22:48:34Z

Same here. This passes all tests, and I think, greatly simplifies using Blink.

Would anyone object to me just merging these in? :))))

src/AtomShell/window.jl

NHDaly

Okay, I've rebased, which should make this PR much easier to Review. :) Thanks again!!

Overall, I'm less sure about this PR than the last one. It's got a couple weird javascripty and Muxy things in here that i'm pretty unfamiliar with.

A thorough review is definitely appreciated if you have time. :)

NHDaly · 2018-08-15T13:00:36Z

res/blink.js

+  // Window creation callback
+  if (typeof callback_id !== 'undefined') {
+    Blink.sock.onopen = ()=>{ cb(callback_id, true); }
+  }


This weirded me out a tad. Do you think this is the best place to mark the window as having finished loading? I think so, since by this point it has finished loading all of its blink-related JS, which seems to be the last thing Electron does after creating the window.

But are there any async-y sorts of things I might be missing? Is there a better place to put this that you can think of?

This seems fine for now -- even if anything is running async at this point communication between Julia and Electron is possible so it makes sense to return from the Window() call.

Okay excellent. Thanks!

NHDaly · 2018-08-15T13:02:44Z

src/AtomShell/window.jl

+  callback = !is_async(opts)
+  if callback
+      id, cond = Blink.callback!()
+      url *= "?callback=$id"


Also, is there a better way to pass the callback ID than through a query param like this? I'm just not very familiar with the framework being used here. I tried to stick it into req[:params] like how the page id is passed (here), but i couldn't figure it out...

I feel like there should be a better way, but I haven't found it. Good enough for now, imho.

👍 Okay cool! I added a TODO comment there in case someone else comes across it and knows a better way! :)

NHDaly · 2018-08-15T13:09:11Z

Also, if you think this looks okay, I'll comment it better before merging.

pfitzseb · 2018-08-21T16:48:21Z

This should be good to go after a rebase, imo.

NHDaly · 2018-09-07T13:05:55Z

src/content/main.html

@@ -2,6 +2,7 @@
 <html>
  <head>
    <script>var id = {{id}}</script>
+    <script>{{optional_create_window_callback}}</script>


Would it be better to put the <script></script> tags in the mustache template as well so that there isn't an empty script node sitting around if the callback isn't being used?
I had it this way originally, because i think it makes the .html file more readable: you know that's going to be a script node, no matter what its contents end up being.

NHDaly · 2018-09-07T13:08:09Z

Okay! I've rebased and commented it better! :) PTAL when you get a chance!

Thanks! :)

NHDaly · 2018-09-22T13:50:00Z

Hi, I think the async behavior of Window() just bit someone, so it would be nice to move forward on this. :)

If someone would be willing to give it a stamp, I'll merge it in! :)

NHDaly · 2018-09-22T13:51:25Z

Also, one more thought: I currently implemented the async option for Window by parsing it out of the opts passed to Window(), but in retrospect I actually don't think that makes any sense. Rereading the code, I've just realized that the opts dict only exists to pass arbitrary options through to Electron. So I'm going to change it now to be a regular, julia keyword parameter. If you have any objections, let me know and I can either stop working on that, or revert the commit if I've already committed it! :)

NHDaly · 2018-09-22T14:34:33Z

Okay, PTAL if you can! :)

@time

Setting `:async=>false` causes `Window()` to block until the window is loaded and the socket is open: ```julia julia> @time Window(Blink.@d(:async=>false)); 0.229105 seconds (4.10 k allocations: 365.495 KiB) julia> @time Window(Blink.@d(:async=>true)); 0.018366 seconds (1.14 k allocations: 39.156 KiB) ``` Defaults to true (async), maintaining previous behavior. Also removed all the now-unneccessary `sleep()`s from all tests.

Change `:async => false` opt to `async=false` keyword argument.

NHDaly · 2018-09-24T19:34:40Z

Alright, everything passes. I'm going to Merge this now!

It adds a flag, async, which defaults to true, but with async=false will make Window creation synchronous. This matches the default async=true for body!, etc.

Assuming everyone agrees, we can follow this up with a PR to change those defaults to true! :)

NHDaly force-pushed the synchronous_Window branch 3 times, most recently from c80875a to 744e0dd Compare July 27, 2018 20:40

rdeits reviewed Aug 15, 2018

View reviewed changes

src/AtomShell/window.jl Outdated Show resolved Hide resolved

NHDaly mentioned this pull request Aug 15, 2018

Make body!() and content!() synchronous. #136

Merged

NHDaly force-pushed the synchronous_Window branch from 23767d9 to e28e525 Compare August 15, 2018 12:56

NHDaly changed the title ~~Make creating a Window synchronous~~ Add option to make creating a Window synchronous Aug 15, 2018

NHDaly force-pushed the synchronous_Window branch 3 times, most recently from 3b29fa2 to e87fcf0 Compare August 15, 2018 13:05

NHDaly commented Aug 15, 2018

View reviewed changes

NHDaly force-pushed the synchronous_Window branch 2 times, most recently from 7b4eb62 to 8d0e960 Compare September 7, 2018 12:57

NHDaly commented Sep 7, 2018

View reviewed changes

NHDaly force-pushed the synchronous_Window branch from 8d0e960 to 17ddf37 Compare September 7, 2018 13:07

NHDaly force-pushed the synchronous_Window branch 2 times, most recently from 9a6a884 to 303fb3e Compare September 10, 2018 20:52

NHDaly mentioned this pull request Sep 22, 2018

body! stopped working (Julia 1.0, linux) #154

Closed

NHDaly force-pushed the synchronous_Window branch from 303fb3e to c8abde2 Compare September 22, 2018 13:48

NHDaly force-pushed the synchronous_Window branch from 3bbcf19 to b412b66 Compare September 22, 2018 14:33

NHDaly added 2 commits September 24, 2018 10:53

Change Window() async option to pure julia kwarg.

1d81745

Change `:async => false` opt to `async=false` keyword argument.

added test that Window(async=true) is faster than async=false.

10cc2ba

NHDaly force-pushed the synchronous_Window branch from b412b66 to 10cc2ba Compare September 24, 2018 14:53

NHDaly merged commit 93aeb16 into JuliaGizmos:master Sep 24, 2018

NHDaly deleted the synchronous_Window branch September 24, 2018 19:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to make creating a Window synchronous #137

Add option to make creating a Window synchronous #137

NHDaly commented Jul 27, 2018 •

edited

Loading

NHDaly commented Jul 27, 2018 •

edited

Loading

NHDaly commented Jul 27, 2018

NHDaly commented Aug 14, 2018

NHDaly left a comment

NHDaly Aug 15, 2018

pfitzseb Aug 21, 2018

NHDaly Sep 7, 2018

NHDaly Aug 15, 2018

pfitzseb Aug 21, 2018

NHDaly Sep 7, 2018

NHDaly commented Aug 15, 2018

pfitzseb commented Aug 21, 2018

NHDaly Sep 7, 2018

NHDaly commented Sep 7, 2018

NHDaly commented Sep 22, 2018

NHDaly commented Sep 22, 2018

NHDaly commented Sep 22, 2018

NHDaly commented Sep 24, 2018

Add option to make creating a Window synchronous #137

Add option to make creating a Window synchronous #137

Conversation

NHDaly commented Jul 27, 2018 • edited Loading

NHDaly commented Jul 27, 2018 • edited Loading

NHDaly commented Jul 27, 2018

NHDaly commented Aug 14, 2018

NHDaly left a comment

Choose a reason for hiding this comment

NHDaly Aug 15, 2018

Choose a reason for hiding this comment

pfitzseb Aug 21, 2018

Choose a reason for hiding this comment

NHDaly Sep 7, 2018

Choose a reason for hiding this comment

NHDaly Aug 15, 2018

Choose a reason for hiding this comment

pfitzseb Aug 21, 2018

Choose a reason for hiding this comment

NHDaly Sep 7, 2018

Choose a reason for hiding this comment

NHDaly commented Aug 15, 2018

pfitzseb commented Aug 21, 2018

NHDaly Sep 7, 2018

Choose a reason for hiding this comment

NHDaly commented Sep 7, 2018

NHDaly commented Sep 22, 2018

NHDaly commented Sep 22, 2018

NHDaly commented Sep 22, 2018

NHDaly commented Sep 24, 2018

NHDaly commented Jul 27, 2018 •

edited

Loading

NHDaly commented Jul 27, 2018 •

edited

Loading