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

Make widgets render using output areas in classic notebook. #1274

Merged
merged 20 commits into from Apr 18, 2017

Conversation

@jasongrout
Copy link
Member

commented Apr 10, 2017

This is work towards fixing #1272, and makes the rendering consistent between jupyterlab and the classic notebook.

Essentially, this gets rid of the special widget area above cells, and instead renders widgets using the widget display_data output messages.

This also finishes fixing #1237.

@gnestor

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

@jasongrout Let me know if you have any questions about this stuff 👌

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2017

Thanks! I'm working through it right now. You may notice that I'm using a slightly different approach (I think it's a bit cleaner) to get the output area, etc.

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2017

(for example, I'm directly requiring the output area, and using the output area prototype to register the mime type, rather than searching for an instance among the cells.)

@@ -46,7 +50,7 @@ var handle_cell = function(cell) {
}
};

function register_events(Jupyter, events) {
function register_events(Jupyter, events, outputarea) {

This comment has been minimized.

Copy link
@gnestor

gnestor Apr 10, 2017

Contributor

@jasongrout Yep, this is cleaner. Before we unwebpackified the notebook, doing this didn't work. I will update the mimerender extensions to reflect this 👍

This comment has been minimized.

Copy link
@jasongrout

jasongrout Apr 10, 2017

Author Member

Yeah, there are other kludgy workarounds I can now delete because we don't have webpack too, like https://github.com/jupyter-widgets/ipywidgets/blob/master/widgetsnbextension/src/widget_output.js#L93

@jasongrout jasongrout modified the milestone: 7.0 Apr 11, 2017

@jasongrout jasongrout changed the title WIP make widgets render using output areas in classic notebook. Make widgets render using output areas in classic notebook. Apr 11, 2017

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2017

CC @SylvainCorlay, @maartenbreddels - it would be great if someone looked at/tested this out too, to see what I might have missed.

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Apr 12, 2017

Any idea of the issue raised here (about Output not syncing is related to, or fixed by this) #1232

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2017

Any idea of the issue raised here (about Output not syncing is related to, or fixed by this) #1232

Oh, interesting. This might break #1232 - we'll probably need to do something like @gnestor does and hook into the clearing signals to remove views.

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2017

Do we have a test for the _view_count that would have failed here?

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Apr 12, 2017

Do you mean a unittest? Or sth automated? Or you mean just an example to demonstrate it?

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Apr 12, 2017

Btw, for ipyvolume, I've started doing testing in the notebook, as an integration test. Do you have experience or know someone who knows how to set up a full blown test with headless browser etc? Would be nice to automate integration tests.

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2017

CC @blink1073 - I think JLab probably currently has the nicest infrastructure for (Javascript) testing currently.

At various times, we've had headless browser testing for widgets using CasperJS. We don't have a good story for widgets right now.

@jasongrout jasongrout force-pushed the jasongrout:outputs branch from 23616cd to bc8c4c8 Apr 12, 2017

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2017

This almost calls the .remove() when appropriate (though it's totally kludgy). Ideally, we should have the output area itself sending a signal just before it empties itself that we could latch on to.

The only cases I've seen where something messes up now is something like this:

from ipywidgets import *
out = Output()
x = IntSlider()
out
with out:
    display(x)
    display(x)
x._view_count

returns 1 when it should return 2, and then evaluating the last two cells again we get _view_count as the correct 4. However, clearing (out.clear_output) does not correctly decrement the view count because it's only listening for changes at the cell level, not the output area level.

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 12, 2017

Okay, the last commit takes care of clearing an output area. However, it doesn't address the problem of the first _view_count returning 1. Not sure what's happening there yet.

@jasongrout jasongrout force-pushed the jasongrout:outputs branch from e927cf6 to 17b7ae7 Apr 13, 2017

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2017

@maartenbreddels - okay, _view_count looks like it is updating correctly now. It required a few fixes to the recent syncing, so I made those PRs first, and then rebased this on the current master.

I think this is ready to be reviewed.

@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2017

Feedback from @maartenbreddels:

  1. output widgets don't seem to be incrementing _view_count.

  2. Executing a blank cell to get rid of a widget doesn't seem to decrement _view_count

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

I don't have time to look into it now, but for the record, it seems like if a list of buffers is send, they are all the same.

@maartenbreddels

This comment has been minimized.

Copy link
Member

commented Apr 13, 2017

Ok, sorry for the noise, that was a bug in ipyvolume.

jasongrout added some commits Apr 13, 2017

Fix sync messages for the output widget.
The sync messages weren’t getting sent because the status handler was never getting called, which mean that once one message was sent and declared pending, we never realized when that message had been processed.
Fix calling .remove() on views.
This depends on jupyter/notebook#2411 to get all the corner cases. We left in various other event triggers to cover most cases even without that PR.
@jasongrout

This comment has been minimized.

Copy link
Member Author

commented Apr 13, 2017

@maartenbreddels - the output widget view counts should be fixed now, and if you run jupyter/notebook#2411, the view count should now be updated even if an empty cell is run.

@jasongrout jasongrout merged commit 897976f into jupyter-widgets:master Apr 18, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.