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

Fix WebIO integration, make Window's sync by default. #208

Merged
merged 1 commit into from
Jul 24, 2019

Conversation

twavv
Copy link
Member

@twavv twavv commented Jul 24, 2019

I think the use of Condition over something like Future from Distributed (builtin package) was a mistake but trying to rectify that wouldn't be worth it right now (and I'm not sure that anyone else will agree with me on that haha).

Anyway, what this diff does:

  • Makes Window() sync by default (so that anything you do with the resulting Window is safe).
  • Adds Base.wait(::Window). After this returns, the Window should be safe to do things to (e.g. body!).
  • Window's that were initialized with a URL have no associated initialization and cannot be wait-ed (I'd rather raise an error that says what you're trying to do doesn't make sense than just silently allowing it).

If you try to do anything before waiting the Window, I make no guarantees and I don't think Blink, in general, should try to do so.

In the future™, we should probably just remove the async keyword in favor of telling the consumer to do windowtask = @async Window().

Ping @NHDaly.

@twavv
Copy link
Member Author

twavv commented Jul 24, 2019

Also: TESTS!!!!! :)))))

@codecov-io
Copy link

Codecov Report

Merging #208 into master will increase coverage by 2.43%.
The diff coverage is 76.92%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #208      +/-   ##
==========================================
+ Coverage   45.27%   47.71%   +2.43%     
==========================================
  Files          12       12              
  Lines         307      306       -1     
==========================================
+ Hits          139      146       +7     
+ Misses        168      160       -8
Impacted Files Coverage Δ
src/AtomShell/window.jl 38.66% <76.92%> (+3.45%) ⬆️
src/content/api.jl 15% <0%> (+3.88%) ⬆️
src/content/content.jl 100% <0%> (+11.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63fcfbb...90af75a. Read the comment docs.

Copy link
Member

@pfitzseb pfitzseb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@rdeits
Copy link
Collaborator

rdeits commented Jul 24, 2019

👍

@pfitzseb pfitzseb merged commit 6e07a9d into JuliaGizmos:master Jul 24, 2019
PallHaraldsson added a commit to PallHaraldsson/Blink.jl that referenced this pull request Nov 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants