Skip to content

Pass the output area object in events specific to the output area. #2411

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

Merged
merged 2 commits into from
Apr 18, 2017

Conversation

jasongrout
Copy link
Member

Also, add a new event triggered right before the output area is cleared. This is useful if an output renderer has cleanup work to do before the DOM element is removed off of the page.

@gnestor and I both want to add mimebundle renderers that should have a method called when the rendered value needs to be cleaned up.

Passing the output area in also makes it easy to listen for this event at the notebook level once and perform some action on whatever output area triggers the event.

Also, add a new event triggered right before the output area is cleared. This is useful if an output renderer has cleanup work to do before the DOM element is removed off of the page.
@jasongrout
Copy link
Member Author

It would be even better if the cell could keep track of the rendered values, and take an optional callback for when that rendered value would be cleared. But we can hack around that with an event like this.

@gnestor
Copy link
Contributor

gnestor commented Apr 13, 2017

This looks like a good solution to me. Are there advantages to attaching event handlers to the output element vs. the output area?

@jasongrout
Copy link
Member Author

I'm not sure I understand your question. We trigger on the output element because that is what has the jquery event system because it is a jquery object. The bubbling then goes up through the DOM, so you can listen to all of these events by doing something like notebook.container.on('clearing', '.output', myhandler);.

jasongrout added a commit to jasongrout/ipywidgets that referenced this pull request Apr 13, 2017
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.
@gnestor
Copy link
Contributor

gnestor commented Apr 14, 2017

This looks like a good solution to me. Are there advantages to attaching event handlers to the output element vs. the output area?

What I mean is this.element.on('cleared', handler) vs. this.events.on('clear_output.CodeCell', handler) (which is actually the code cell, not output area).

Since the output element already has changed, cleared, and resizeOutput events, adding a clearing event makes sense.

@jasongrout
Copy link
Member Author

  1. We want to trigger for output areas, not code cells, since we might have output areas not associated with code cells (as in output widgets).

  2. I can't find it now, but I thought at one point @minrk preferred having output area events on the output area, rather than global events. It doesn't matter to me either way - I'm essentially doing the same handler either way. @minrk, do you have a preference between events on the outputarea element instance vs. a global (but scoped) event?

@jasongrout
Copy link
Member Author

I also figured keeping the events on the output area instance was more backwards compatible....

@gnestor gnestor requested a review from rgbkrk April 17, 2017 19:07
CodeCell.prototype.clear_output = function (wait) {
this.output_area.clear_output(wait);
this.set_input_prompt();
CodeCell.prototype.clear_output = function (wait, ignore_queue) {
Copy link
Member

Choose a reason for hiding this comment

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

Should ignore_queue have a default argument since prior calls to this would not have had an argument? Perhaps undefined is ok to pass through to this.output_area.clear_output?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call. I'll check.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's fine the way it is. undefined is treated as false, which means things should proceed as normal in the clear_output function.

@@ -942,7 +942,7 @@ define([
};


OutputArea.prototype.clear_output = function(wait, ignore_que) {
OutputArea.prototype.clear_output = function(wait, ignore_clear_queue) {
Copy link
Member

Choose a reason for hiding this comment

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

Certainly better naming. 😄

Copy link
Member Author

Choose a reason for hiding this comment

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

It took me a while of staring at it to figure out the double negative logic and what it was supposed to say :).

@rgbkrk
Copy link
Member

rgbkrk commented Apr 17, 2017

do you have a preference between events on the outputarea element instance vs. a global (but scoped) event?

I prefer global events that are scoped too.

@jasongrout
Copy link
Member Author

I prefer global events that are scoped too.

I prefer that too, all else being equal, but was avoiding making too many huge changes, especially backwards-incompatible ones.

@rgbkrk rgbkrk merged commit 6cebd3e into jupyter:master Apr 18, 2017
@gnestor gnestor added this to the 5.1 milestone Apr 21, 2017
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants