IPEP 21: widget messages #4195

Merged
merged 47 commits into from Oct 24, 2013

Conversation

Projects
None yet
9 participants
Owner

minrk commented Sep 11, 2013

IPEP 21 has full description of the proposal.

This is just the basic communication tube, through which widgets might communicate.

It's pretty basic, but it seems to work reasonably well.

Some example use cases for the Comms implemented here: http://nbviewer.ipython.org/6547109

This also refactors callbacks in a clearer way, allowing callback cleanup since widget messages dramatically exacerbate #2524.

closes #2524.

cc: @dcjones, who has been implementing JS-based plotting widgets in IJulia.

Would be nice in the PEP to refer to the "kernel" rather than "Python" side, and to separate the things that any kernel must do from the specific implementation details in the IPython kernel.

Owner

minrk commented Sep 12, 2013

Would be nice in the PEP to refer to the "kernel" rather than "Python" side

I tried to do this, I will read through again and see where I slipped up. There are actually three pieces here:

  1. message spec (Kernel and Frontend)
  2. Python API implementing the spec
  3. Javascript API implementing the spec
Owner

minrk commented Sep 12, 2013

I read through the IPEP, and Python is never mentioned in the spec - I only use Kernel and Frontend there. When I switch gears to talk about the specific APIs of the Python and Javascript implementations, I discuss the languages in which those implementations exist. I have clarified the transition, though.

Owner

ellisonbg commented Sep 12, 2013

Great work, I love the symmetry and simplicity!

Widget Creation:

  • Should clarify how the widget id is selected - kernel picks.
  • Do the two sides share the widget id?
  • Clarify what the frontend should do when it receives the widget_create message
  • What happens with the frontend gets a widget_create message but doesn't know about that widget type? Seems like the frontend should send a widget_destroy message.
  • Do we allow the frontend to send the widget_create message?

Widget object:

  • I think we should try the simple design - just have data and not the full message. The data part is general enough to really cover everything. The only case where I could imagine we would want to include the metadata is if we started to define the details of the data JSON object to include things like our display messages.
Owner

ellisonbg commented Sep 12, 2013

I think the biggest question I have at this point is how widgets relate to our standard display messages. What happens if a widget needs to update the state of a matplotlib plot? Seems like there needs to be some coordination there. Do widgets handle the display messages? Do why manage their own OutputArea?

Another question is if we are going to have any notion of widget nesting. If there is a plot that updates with the values of 4 sliders and 2 checkboxes, is that 1 widget or 6?

@ellisonbg ellisonbg and 1 other commented on an outdated diff Sep 12, 2013

IPython/html/static/notebook/js/widget.js
+
+ var WidgetManager = function (kernel) {
+ this.widgets = {};
+ this.widget_types = {widget : Widget};
+ if (kernel !== undefined) {
+ this.init_kernel(kernel);
+ }
+ };
+
+ WidgetManager.prototype.init_kernel = function (kernel) {
+ // connect the kernel, and register message handlers
+ this.kernel = kernel;
+ var msg_types = ['widget_create', 'widget_destroy', 'widget_update'];
+ for (var i = 0; i < msg_types.length; i++) {
+ var msg_type = msg_types[i];
+ kernel.register_iopub_handler(msg_type, $.proxy(this[msg_type], this));
@ellisonbg

ellisonbg Sep 12, 2013

Owner

Do we make sure that we unregister these handlers when the widget is destroyed?

@minrk

minrk Sep 12, 2013

Owner

These are only registered once - there is only one WidgetManager.

@ellisonbg ellisonbg commented on an outdated diff Sep 12, 2013

IPython/html/static/notebook/js/widget.js
+ };
+
+ WidgetManager.prototype.unregister_widget = function (widget_id) {
+ // Remove a widget from the mapping
+ delete this.widgets[widget_id];
+ };
+
+ // widget message handlers
+
+ WidgetManager.prototype.widget_create = function (msg) {
+ var content = msg.content;
+ var constructor = this.widget_types[content.widget_type];
+ if (constructor === undefined) {
+ console.log("No such widget type registered: ", content.widget_type);
+ console.log("Available widget types are: ", this.widget_types);
+ return;
@ellisonbg

ellisonbg Sep 12, 2013

Owner

This would be where we would send back the destroy_widget message to let the kernel side know it didn't work.

Owner

ellisonbg commented Sep 12, 2013

In the IPEP, we should emphasize that the widget_create and widget_destroy messages mean that the receiver should do those actions. Part of what is a bit confusing with a fully symmetric protocol is preventing infinite loops of messages. To prevent this, we should be very clear that the handle methods should never send the same message type it is handling.

Owner

ellisonbg commented Sep 12, 2013

Could you point to your examples?

Owner

ellisonbg commented Sep 12, 2013

@ivanov and @jdfreder I think the best way for us to test this widget messages is to use it to write our higher level JS/Python APIs. You are the two leads on that (Paul on the Python side, Jon on the Javascript side). Can you both start to think about that stuff. I am more than will to have design discussion with both of you. I want us to start this stuff as soon as possible so we have time to iterate and bake the design before 2.0.

Owner

jdfreder commented Sep 12, 2013

@ellisonbg sounds like a good idea. When are you going to be back in town?

Owner

jasongrout commented Sep 12, 2013

In the PEP, you ask: "Question: the handle_foo messages get the data dict, not the full message. This means they don't have access to metadata, etc. Should the handlers get the full message instead?"

I think yes. It would be useful, for example, to access the metadata. It might also be useful to see the timestamp when a message was generated, for example. And at least passing in the metadata would be more consistent with other handlers, right?

Owner

ellisonbg commented Sep 12, 2013

@jdfreder I will be back Sat night, back in the office on Monday. I will be in touch.

Owner

ellisonbg commented Sep 12, 2013

@jasongrout makes a good point. I could imagine that for some widgets, the timestamps are important - they could be used to throttle the update rates.

Owner

ellisonbg commented Sep 12, 2013

I should mention - I play some last week with some slider widgets and quickly found that it was easy to flood the kernel with calls. I am wondering if we want to add code on the JS side that can automatically throttle the rate at which the update messages are send. I think this is going to be a really common problem that most highly interactive widgets have.

Owner

jasongrout commented Sep 12, 2013

Another question to consider: How do you lay out multiple widgets? Suppose I want two sliders next to each other, above an interactive plot, with two buttons off to the side? I can construct the necessary html, for example. How do I specify where to put the widgets in the html? This is related to @ellisonbg's comment about nested widgets.

For Sage interacts, we have 3 layout systems, two of which are currently active. One is a list of lists (i.e., rows of columns). Another that might be more applicable here is to just spit out the html, then give the "locations", which are jquery selectors for the places to put the widgets.

Owner

minrk commented Sep 12, 2013

An update from the dev meeting and today's discussion:

These are not widgets, they are the basic communication object used to implement widgets. As a result, they should not be called widgets, but something else. I think I'm going with Comm for now. When you create a Widget, it will use a Comm, but it will not be a Comm.

Widget Creation:

Should clarify how the widget id is selected - kernel picks.

The creator picks the ID, can be done from either side.

Do the two sides share the widget id?

Yes

Clarify what the frontend should do when it receives the widget_create message
What happens with the frontend gets a widget_create message but doesn't know about that widget type?
Seems like the frontend should send a widget_destroy message.

Right now, it just logs an error. widget_destroy isis probably the right thing to do.

Do we allow the frontend to send the widget_create message?

Yes, the spec is fully symmetrical. I initially didn't allow this, because of a note from the dev meeting,
but it is actually more complicated to add the asymmetry.

Widget object:

I think yes. It would be useful, for example, to access the metadata.

Okay, I will pass along the whole message - it's not appreciably more complicated.

In the IPEP, we should emphasize that the widget_create and widget_destroy messages mean that the receiver should do those actions.

Will do.

Part of what is a bit confusing with a fully symmetric protocol is preventing infinite loops of messages. To prevent this, we should be very clear that the handle methods should never send the same message type it is handling.

I don't think so. Other than creation / destruction, there is only one message type, so if message handlers should ever send a message back at all, this restriction cannot be enforced. It makes perfect sense for an update from one side to result in a different update coming back (e.g. changing a value causing a replot). It is up to the implementer to ensure they don't get stuck.

Owner

minrk commented Sep 12, 2013

@jasongrout - this is not GUI widgets, only the communication part. Layout, etc. is entirely independent of this.

Owner

minrk commented Sep 12, 2013

I play some last week with some slider widgets and quickly found that it was easy to flood the kernel with calls. I am wondering if we want to add code on the JS side that can automatically throttle the rate at which the update messages are send. I think this is going to be a really common problem that most highly interactive widgets have.

Yes, I ran into this myself, and quickly started throttling requests. I'm not sure how to do it in a generic way that we can reasonably enforce on all widget authors, though.

Owner

ellisonbg commented Sep 12, 2013

@minrk this brings up a good point about naming. The current naming
implies that there will be a 1-to-1 mapping between Widget objects and UI
controls, but that is definitely not the case. A single widget object
could manage 100 UI controls. Will have to think more about the naming of
things.

On Thu, Sep 12, 2013 at 2:53 PM, Min RK notifications@github.com wrote:

@jasongrout https://github.com/jasongrout - this is not GUI widgets,
only the communication part. Layout, etc. is entirely independent of this.


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/4195#issuecomment-24359014
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Owner

jasongrout commented Sep 12, 2013

@minrk: it makes sense that layout issues are above the communication layer. But the creation handler will have to address this somehow, so that's another vote for the whole message (including maybe layout metadata) being passed into the create handler.

Owner

minrk commented Sep 12, 2013

I think the biggest question I have at this point is how widgets relate to our standard display messages. What happens if a widget needs to update the state of a matplotlib plot? Seems like there needs to be some coordination there. Do widgets handle the display messages? Do why manage their own OutputArea?

That seems like it would be up to the Widget implementation - some might hook up events on an existing OutputArea, some might manipulate their own div elsewhere (sidebar namespace browser). It doesn't seem like a question that can have a generic answer.

Another question is if we are going to have any notion of widget nesting. If there is a plot that updates with the values of 4 sliders and 2 checkboxes, is that 1 widget or 6?

I don't know. I also am not sure this has any bearing on the code here - this is just the very basic communication code for allowing some object in the Kernel to talk to some object in the Frontend and vice versa.

Owner

jasongrout commented Sep 12, 2013

@minrk: if you're changing the names to reflect a message channel, maybe instead of update it should be send.

Owner

minrk commented Sep 12, 2013

@jasongrout reading my mind - I have switched create/update/destroy to more comm-appropriate open/send/close.

Owner

jasongrout commented Sep 12, 2013

I like the simplicity of the open/send/close/handlers idea. Basically you are just creating a private message channel on top of the existing messaging framework. I can see how just passing the data attribute makes sense---the IPython message wrapping the data attribute is nothing but routing information.

Owner

minrk commented Sep 12, 2013

I can see how just passing the data attribute makes sense---the IPython message wrapping the data attribute is nothing but routing information.

That's true - and if the entity constructing the object is interesting in timings, it can always add them to the data messages. I guess I will live it using just data for now, but we can think a bit more carefully later on about handing off the whole message. It is not at all a difficult change to make.

Owner

jasongrout commented Sep 12, 2013

So how do you handle a channel open message from a frontend to a kernel that has multiple frontends? Do you broadcast out an open message to all other frontends (like you would echo input)? What about channel close messages? Are send messages from the kernel sent to all frontends?

Owner

minrk commented Sep 12, 2013

So how do you handle a channel open message from a frontend to a kernel that has multiple frontends? Do you broadcast out an open message to all other frontends (like you would echo input)? What about channel close messages? Are send messages from the kernel sent to all frontends?

This is a good point, where there is an asymmetry - one kernel, possibly multiple frontends. It seems to me that it makes sense in general for a Comm on the Kernel side to re-broadcast both open and destroy messages that it has received. Then the only handling needed on the Frontend-side is to ignore open and close messages for Comms it has already opened or closed. I don't think that same generalization can be made for the 'normal' messages in between open and close, and that decision should be left to widget implementers.

@Carreau Carreau and 1 other commented on an outdated diff Sep 13, 2013

IPython/html/static/notebook/js/comm.js
+//
+// Distributed under the terms of the BSD License. The full license is in
+// the file COPYING, distributed as part of this software.
+//----------------------------------------------------------------------------
+
+//============================================================================
+// Comm and CommManager bases
+//============================================================================
+/**
+ * Base Comm classes
+ * @module IPython
+ * @namespace IPython
+ * @submodule comm
+ */
+
+var IPython = (function (IPython) {
@Carreau

Carreau Sep 13, 2013

Owner

Would you like to start using require ?

@ellisonbg

ellisonbg Sep 13, 2013

Owner

I would like to use require but that should be a separate PR.

Owner

ellisonbg commented Sep 13, 2013

A few more thoughts:

  • I thought about the name more and I am liking the name Channel instead of Comm. I like the open, close, send names though.
  • With our renames, these classes really become focused on communications. However, the handle_send method will presumably have all of the view logic for our "widgets" (here I am using that name to include the frontend UI side of things as well. IOW, I think it is important to cleanly separate the model from the view, but with the current design, the handle_send method would have the view logic.
  • I think it is important to have an object that is bound to a particular cell and has a handle on that cells DOM elements for view manipulation. Is that object the Comm/Channel object? Is that a new View object? Are Comm/Channel objects bound to cells?
Owner

ellisonbg commented Sep 13, 2013

I found @minrk example notebook here:

http://nbviewer.ipython.org/urls/gist.github.com/minrk/6547109/raw/fdd74c56bf50a488f13d8cf22809be7f563a35a7/Example+Widgets.ipynb

And this gives me a better idea of how he is envisioning the view logic happening:

  • Views are managed through our standard HTML/JavaScript representations. The approach Min is using right now is to 1) create an HTML repr that puts a raw div on the page with a class that matches the comm's id, 2) create a separate JS repr that pushes out and creates a JavaScript object to manage the view. This object is passed the comm and really acts as both view and controller (VC) in the MVC model.
  • The comm objects on the two sides are not ever subclassed to define the handlers logic, they are passed callables by the VC objects. This model is different from what is describe in the IPEP right now, where subclassing would need to be done to define the right handle_ methods. I like not having to subclass as it more cleanly separates the M part from the VC part.
  • Right now all of the widget view stuff is done as regular output. The problem with this is that when we do clear_output the widget views will be cleared as well. I propose that we create a separate "widget view" div between the input div and the output div. This would not be cleared when output is cleared.
  • We are going to need better logic for clear_output to prevent jitter and timing errors. I propose that we simply measure the current size of the output div and replace it with an empty div of the same size. New output would then replace the empty div and the as long as the output is the same size, there would be no jitter and we can do away with the timeout logic that currently prevents high refresh rates.
  • I think we should look at an a JS MVC framework like backbone or angular. I don't want us to reinvent solid MVC logic for our users as we will do it badly. I know that backbone has a pluggable M layer that we could tie into our Comm class. We could provide a small amount of glue code to pull everything together.

It appears that our Comm object is becoming a JSON+WebSocket style API. I know the MVC stuff is a separate layer on top of the communication stuff, but I think it is important to think about how the comm layer will be used...

Owner

minrk commented Sep 13, 2013

I found @minrk example notebook here

I put a much shorter link in the description of this PR yesterday.

  • I thought about the name more and I am liking the name Channel instead of Comm. I like the open, close, send names though.

That's confusing, because Channels send and receive messages on the IOPub channel and the Shell channel.
I think we shouldn't use Channel again for something that uses an existing Channel.

Views are managed through our standard HTML/JavaScript representations. The approach Min is using right now is to 1) create an HTML repr that puts a raw div on the page with a class that matches the comm's id, 2) create a separate JS repr that pushes out and creates a JavaScript object to manage the view. This object is passed the comm and really acts as both view and controller (VC) in the MVC model.

Note that I have no intention of suggesting that _repr_html_ should be the way this is done, nor do I propose that a javascript repr define the counterpart, that's just the quickest way to get things on the page when testing that the Comm works as intended. I would expect javascript counterparts to be defined in purely javascript plugins.

The comm objects on the two sides are not ever subclassed to define the handlers logic, they are passed callables by the VC objects. This model is different from what is describe in the IPEP right now, where subclassing would need to be done to define the right handle_ methods. I like not having to subclass as it more cleanly separates the M part from the VC part.

I updated the IPEP last night to reflect the initial design. My initial implementation had these as the empty Widget base classes, which I was subclassing (on both sides), but I realized that coupling whatever a single 'widget' is with a single comm was a bad choice, and unnecessarily restrictive.

Right now all of the widget view stuff is done as regular output. The problem with this is that when we do clear_output the widget views will be cleared as well. I propose that we create a separate "widget view" div between the input div and the output div. This would not be cleared when output is cleared.

Note that there are zero proposals for how to implement widgets in this code. It's strictly an exercise of how to get messages on the Comms to trigger events.

I propose that we create a separate "widget view" div between the input div and the output div. This would not be cleared when output is cleared.

I'm not sure this makes sense, but we can certainly give it a try.

We are going to need better logic for clear_output to prevent jitter and timing errors. I propose that we simply measure the current size of the output div and replace it with an empty div of the same size. New output would then replace the empty div and the as long as the output is the same size, there would be no jitter and we can do away with the timeout logic that currently prevents high refresh rates.

That's a good idea, and in my own demos I typically do something very similar with manual CSS.

I think we should look at an a JS MVC framework like backbone or angular. I don't want us to reinvent solid MVC logic for our users as we will do it badly. I know that backbone has a pluggable M layer that we could tie into our Comm class. We could provide a small amount of glue code to pull everything together.

I think that's a good idea for some widgets (and the notebook in general), though it doesn't apply to usage of the Comm in general, as use cases can be inverted (JS created rather than Python created), or have no visual element at all.

Owner

SylvainCorlay commented Sep 14, 2013

I tried the PR and the example notebook. This is really great!
It is probably the best mechanism to build d3.js-based interactive visualizations with callbacks to Python.

Owner

jasongrout commented Sep 15, 2013

I also just tried it out. I really like it, and I like how you are separating the communication layer from the widget layer. I guess the communication layer is basically implementing the user-defined messages, but in a better way (you have a specific private channel to a remote object).

Owner

SylvainCorlay commented Sep 19, 2013

I have been playing a bit with your code and it seems that the javascript function handle_open is executed before the Python function _repr_html_ and the div element does not exist yet.
In your test implementation of the slider widget, this makes the first call of get_div returns null. The widget still works as get_div is also called later.
I understand from your previous comments that you have not intention of suggesting that repr_html should be the way to implement this kind of widgets. But if this became the method of choice, handle_open should maybe be executed before _repr_html_.

Owner

jdfreder commented Sep 19, 2013

I think there is a small typo in the Sending messages with Comms section, because of renaming widget to comm:

of course, one side can send another widget_msg in reply

Owner

jdfreder commented Sep 19, 2013

minrk s/destroy/close

Also, at the top of the IPEP, in the Comm Creation section you probably want to also s/destroy/close this text:

If comm creation fails, a comm_destroy message should be sent

Unless there are comm_destroy and comm_close messages?

Owner

minrk commented Sep 19, 2013

@jdfreder yes, when I renamed everything, I missed a few things. There are only three message types: comm_open, comm_msg, comm_close.

@jdfreder jdfreder commented on an outdated diff Sep 19, 2013

IPython/html/static/notebook/js/comm.js
+ for (var i = 0; i < msg_types.length; i++) {
+ var msg_type = msg_types[i];
+ kernel.register_iopub_handler(msg_type, $.proxy(this[msg_type], this));
+ }
+ };
+
+ CommManager.prototype.register_target = function (target, constructor) {
+ // Register a constructor for a given target key
+ this.targets[target] = constructor;
+ };
+
+ CommManager.prototype.register_comm = function (comm) {
+ // Register a comm in the mapping
+ this.comms[comm.comm_id] = comm;
+ comm.kernel = this.kernel;
+ return comm.comm_id;
@jdfreder

jdfreder Sep 19, 2013

Owner

Why does this return comm_id if that's a property of the comm that is not assigned by this method (you can't call this method if your comm doesn't already have a comm_id assigned to it)?

Owner

SylvainCorlay commented Sep 23, 2013

If two browsers display the notebook containing the interactive widgets, one after the other, it happens that one of them will not display the example widgets at all. Functions handle_open, handle_msg and handle_close just don't get executed.

Owner

minrk commented Sep 23, 2013

@SylvainCorlay that's right - live sync of the notebook is entirely unsupported right now.

Owner

SylvainCorlay commented Sep 23, 2013

Regarding the "live sync"

  • the problem is that if there is another browser connected to the notebook server URL somewhere (even inactive), it seems that there is no guarantee that the widgets will be drawn when executing the cells, even if we restart the server.

Regarding the example notebook the Range widget:

  • We cannot use the comm_id as a css id or a css class because it can start with numbers. One should maybe prefix it with some letters or underscores...
  • In the Range widget example, in the handle_open javascript function, the call to get_div can be removed, as the div element does not exist yet at this point.
Owner

minrk commented Sep 23, 2013

the problem is that if there is another browser connected to the notebook server URL somewhere (even inactive), it seems that there is no guarantee that the widgets will be drawn when executing the cells, even if we restart the server.

This is true of any output (or input, for that matter) - simultaneous use of the notebook is simply not supported in any way at this point, because it can easily result in data loss.

Owner

jasongrout commented Sep 24, 2013

Just FYI for @SylvainCorlay, @williamstein implemented live syncing of IPython notebooks as part of https://cloud.sagemath.com. I don't know how it interfaces with the widget stuff (since William's live syncing syncs the documents that result from messages, not necessarily the messages). But it is out there and being worked on.

Owner

jasongrout commented Sep 28, 2013

What about using the name "pipe" instead of "comm"? I mean, after all, you can lay lots of pipes in a single channel in real life :). And it would make sense to people familiar with IPC, named pipes, etc.

Owner

jasongrout commented Oct 1, 2013

Any idea of how close this is to being merged? We're needing something like this in the Sage cell server in the next week or two, and we can either work with our current way of doing things (registering and handling new message types) or we can build on this foundation.

Owner

minrk commented Oct 1, 2013

I'm reorganizing the callback structure a little bit, which I should probably finish up today or tomorrow. This could be merged this week, I think.

Owner

minrk commented Oct 1, 2013

We chatted about your pipe proposal, and @ellisonbg and @takluyver found that pipe implied a more stream-like pattern, rather than a message-based one (MPI also uses COMM, for instance). I'm not thrilled about comm, but I think it's serviceable.

Owner

jasongrout commented Oct 1, 2013

Thanks. I really appreciate the reorganization of the callback structure.

Owner

minrk commented Oct 2, 2013

Callback structure should be just about done. I'll take another pass at APIs, testing, and docs tomorrow to make sure everything is in sync and properly explained. If you want to start trying to build things based on the APIs presented here, I don't expect it to change substantially between now and the first real use case feedback (could be yours!)

Obviously, this is a draft that may change if we find out it doesn't adequately address needs, but now is the time to start trying to build things to see if that will be the case.

Owner

jasongrout commented Oct 2, 2013

We'll probably look at it this Friday when we meet for the Sage programming group. I worked today to clean up our code to make it easier to adapt to this new comm system.

Owner

minrk commented Oct 2, 2013

I don't have anything more I want to do here before merging, if folks want to take another pass at review.

@jdfreder jdfreder and 1 other commented on an outdated diff Oct 2, 2013

IPython/html/static/services/kernels/js/kernel.js
+ $([IPython.events]).trigger('status_autorestarting.Kernel', {kernel: this});
+ $([IPython.events]).trigger('status_restarting.Kernel', {kernel: this});
+ } else if (execution_state === 'dead') {
+ this.stop_channels();
+ $([IPython.events]).trigger('status_dead.Kernel', {kernel: this});
+ }
+ };
+
+
+ // handle clear_output message
+ Kernel.prototype._handle_clear_output = function (msg) {
+ var callbacks = this.get_callbacks_for_msg(msg.parent_header.msg_id);
+ if (!callbacks || !callbacks.iopub) {
+ return;
+ }
+ var callback = callbacks.clear_output;
@jdfreder

jdfreder Oct 2, 2013

Owner

I think this should be callbacks.iopub.clear_output;, otherwise one can't register a clear_output callback.

@minrk

minrk Oct 2, 2013

Owner

yup, fixed and tested, thanks.

Owner

jasongrout commented Oct 3, 2013

How come IPython/core/interactiveshell.py still refers to widgets instead of comms? Is that intentional? (IPython/kernel/zmq/zmqshell.py also overrides the init_widgets method, so it should possibly be changed too)

Owner

jasongrout commented Oct 3, 2013

I think the comm.js file is not notebook-specific (we want to use it in the cell server, for example). Can it be moved to, say, IPython/html/static/base/js/?

@jasongrout jasongrout and 1 other commented on an outdated diff Oct 3, 2013

IPython/html/static/services/kernels/js/kernel.js
this.shell_channel = this.iopub_channel = this.stdin_channel = null;
};
// Main public methods.
+
+ // send a message on the Kernel's shell channel
+ Kernel.prototype.send_shell_message = function (msg_type, content, callbacks) {
+ var msg = this._get_msg(msg_type, content);
@jasongrout

jasongrout Oct 3, 2013

Owner

How about adding another argument to send_shell_message to set the metadata of a message? (We need this in the sage cell server; having this means that we don't have to override this function.)

@minrk

minrk Oct 3, 2013

Owner

makes sense, will do.

@jasongrout jasongrout commented on the diff Oct 3, 2013

IPython/html/static/services/kernels/js/kernel.js
// Payloads are handled by triggering events because we don't want the Kernel
// to depend on the Notebook or Pager classes.
for (var i=0; i<l; i++) {
- if (payload[i].source === 'page') {
- var data = {'text':payload[i].text}
- $([IPython.events]).trigger('open_with_text.Pager', data);
- } else if (payload[i].source === 'set_next_input') {
- if (callbacks.set_next_input !== undefined) {
- callbacks.set_next_input(payload[i].text)
+ var payload = payloads[i];
+ var callback = payload_callbacks[payload.source];
@jasongrout

jasongrout Oct 3, 2013

Owner

This assumes that a payload dictionary has a source key. I don't think that's in the message spec.

@minrk

minrk Oct 3, 2013

Owner

good point - we never got to actually specifying the payload part of the message spec, but it is part of the IPython implementation. I will add it to the spec for now, as it has been an assumption of every frontend implementation so far.

This is something we need to look into as a part of the spec-review we are trying to do for 2.0 (e.g. should payloads just be messages on iopub instead of inside execute_reply).

@jasongrout

jasongrout Oct 3, 2013

Owner

Our use of the payload didn't have a source :). (I know we aren't an "official" front end...) It's easy enough for us to correct, though.

@jasongrout

jasongrout Oct 3, 2013

Owner

Just FYI, we use payloads to automatically make all the files in the temporary execution directory available to the users. It's nice that a bit of code can be automatically run at the end of a code execution to send back information about what files were created. Of course, this could be accomplished other ways (like a permanent comm channel open on which we query about files, or a post-execution hook of some type that will send this metadata on iopub.)

@minrk

minrk Oct 3, 2013

Owner

I don't think the handling of payloads without a source key has changed any in this PR. It just checks the source key (undefined, no error), then checks if there is a callback (will also be undefined), so no callback will fire.

We just need to sit down and figure out the things that we have not fully specified and/or need to change because of too much IPython or Python-specificity. Obviously, you guys should be part of that conversation.

@jasongrout

jasongrout Oct 4, 2013

Owner

Ah, yes, good point about it not being backwards incompatible. As I was getting our callbacks to work with your new code, I saw that nice payload callback infrastructure, but then realized we'd have to make some changes to take advantage of it.

There are a few other changes I might suggest (not here) that take out some of the notebookisms in the code (places where we currently monkeypatch things...)

@jasongrout jasongrout commented on the diff Oct 3, 2013

IPython/html/static/services/kernels/js/kernel.js
- cb(content, metadata);
- }
- };
-
- if (content.payload !== undefined) {
- var payload = content.payload || [];
- this._handle_payload(callbacks, payload);
+ var parent_id = reply.parent_header.msg_id;
+ var callbacks = this.get_callbacks_for_msg(parent_id);
+ if (!callbacks || !callbacks.shell) {
+ return;
+ }
+ var shell_callbacks = callbacks.shell;
+
+ // clear callbacks on shell
+ delete callbacks.shell;
@jasongrout

jasongrout Oct 3, 2013

Owner

There is no checking of message type (even that the message type ends in _reply) before calling this function. Are we sure that whenever we send a message down the shell channel, the only message coming back with the appropriate parent message header is the final message from the computation?

Or perhaps should we at least check that the message type ends in _reply before invoking this handler?

@minrk

minrk Oct 3, 2013

Owner

yes - this is the semantics of the shell channel in general - each request has one and only one reply, so there's nothing to check.

@jasongrout

jasongrout Oct 3, 2013

Owner

It would be great to add a note to the message spec explicitly saying that the shell channel in general is a request/reply channel, initiated by the client, and the reply comes after the kernel is done with everything else.

@jasongrout

jasongrout Oct 4, 2013

Owner

Oh, I guess comm messages violate this idea that the shell channel is purely a reply/request channel.

@minrk

minrk Oct 4, 2013

Owner

that's true. Adding comm messages means there will be exactly zero or one replies, depending on the message type. It remains safe to clear the callbacks after one reply is seen.

Owner

minrk commented Oct 3, 2013

How come IPython/core/interactiveshell.py still refers to widgets instead of comms? Is that intentional? (IPython/kernel/zmq/zmqshell.py also overrides the init_widgets method, so it should possibly be changed too)

Mainly because I expect we will be initializing some widget type registration as a part of startup once some widgets exist, and the comm is really part of that. I can rename the method for now, in case that assumption doesn't turn out to be correct.

move to base/js

Sure, makes sense.

Owner

SylvainCorlay commented Oct 4, 2013

In the future is there any plan to provide a method to create notebooks with persistent interactive widgets?
I presume this could use

  • the metadata field of outputs to store some internal state of objects
  • the mechanism you provided to instantiate Python objects from javascript in your example notebook.
Owner

minrk commented Oct 4, 2013

Yes, absolutely - It will probably be at a higher level than just dumping things in metadata. There may be a 'widgets' field somewhere in the cell structure in the document.

@jasongrout jasongrout and 1 other commented on an outdated diff Oct 4, 2013

IPython/html/static/notebook/js/main.js
@@ -60,6 +60,7 @@ function (marked) {
IPython.tooltip = new IPython.Tooltip()
IPython.notification_area = new IPython.NotificationArea('#notification_area')
IPython.notification_area.init_notification_widgets();
+ IPython.comm_manager = new IPython.CommManager();
@jasongrout

jasongrout Oct 4, 2013

Owner

How about initializing the CommManager as part of the kernel startup, and considering it part of the kernel service? You could initialize it in the _kernel_started callback, and store it as an attribute of the kernel itself. I'm a little ambivalent about this idea. But really the comm manager is tied to the kernel, and if you have multiple kernels managed from a single page (like we do), it makes sense to store the comm manager with the kernel.

@jasongrout

jasongrout Oct 4, 2013

Owner

I've been thinking more about this. The Comm manager really is just a callback/sending mechanism of the kernel for routing messages. It seems natural to make the Comm manager an attribute of the kernel object (initialized at kernel startup), and then possibly expose the register function through the global IPython object via a wrapper function for convenience.

@minrk

minrk Oct 4, 2013

Owner

That's a good point - after looking at other files, base didn't quite feel right. I'll move it to the kernels service, and attach the comm_manager instance to the kernel object.

@minrk

minrk Oct 4, 2013

Owner

there is now kernel.comm_manager attribute, which is set on kernel creation.

@ellisonbg ellisonbg and 1 other commented on an outdated diff Oct 4, 2013

IPython/html/static/notebook/js/codecell.js
@@ -248,12 +248,22 @@ var IPython = (function (IPython) {
this.kernel.clear_callbacks_for_msg(this.last_msg_id);
@ellisonbg

ellisonbg Oct 4, 2013

Owner

Just to clarify, we do still need this in case someone presses shift enter before the previous reply comes back?

@minrk

minrk Oct 4, 2013

Owner

Yes, the two issues are unrelated.

Owner

jasongrout commented Oct 4, 2013

I just noticed my comm_open message looks like this:

{"parent_header":{},"msg_type":"comm_open","msg_id":"be48f448-5e09-4d14-8ec4-4d31da3ac01a","content":{"target_name":"test","data":{},"comm_id":"c6a64b8995004fa28cc8afaddc9261ec"},"header":{"date":"2013-10-04T17:28:52.909618","username":"kernel","session":"b69d0267-3991-4f9b-befb-34c8da3671a7","msg_id":"be48f448-5e09-4d14-8ec4-4d31da3ac01a","msg_type":"comm_open"},"metadata":{}}

Note that the parent_header is empty. Shouldn't the parent_header be set to the execution_request statement that called the python code that opened the comm session? It might be a problem with my code; I haven't checked if this happens with IPython.

Owner

minrk commented Oct 4, 2013

Note that the parent_header is empty.

Yup, I just forgot to add it. Should be fixed.

ellisonbg referenced this pull request Oct 5, 2013

Closed

Bret Victor style input widgets #4352

0 of 3 tasks complete
Owner

jasongrout commented Oct 5, 2013

I just pushed out a version of the Sage Cell server with this comm implementation: http://sagecell.sagemath.org/?q=tfihve

Carreau referenced this pull request Oct 6, 2013

Closed

Deprecated removal #4354

Owner

jasongrout commented Oct 8, 2013

I just pushed out a new version of the Sage Cell server that uses this comm implementation to have a 3d graphics widget based on three.js and William Stein's sage 3d graphics to three js shim: http://sagecell.sagemath.org/?q=snhxfg

The comm implementation seems to work well. The one improvement I can suggest now is that the python send message doesn't let you attach metadata like the javascript send methods do.

Owner

minrk commented Oct 8, 2013

The comm implementation seems to work well. The one improvement I can suggest now is that the python send message doesn't let you attach metadata like the javascript send methods do.

Of course! They should be symmetric now, thanks for catching that.

@ellisonbg ellisonbg commented on the diff Oct 9, 2013

IPython/kernel/comm/comm.py
+ _msg_callback = Any()
+ _close_callback = Any()
+
+ _closed = Bool(False)
+ comm_id = Unicode()
+ def _comm_id_default(self):
+ return uuid.uuid4().hex
+
+ primary = Bool(True, help="Am I the primary or secondary Comm?")
+
+ def __init__(self, data=None, **kwargs):
+ super(Comm, self).__init__(**kwargs)
+ get_ipython().comm_manager.register_comm(self)
+ if self.primary:
+ # I am primary, open my peer.
+ self.open(data)
@ellisonbg

ellisonbg Oct 9, 2013

Owner

I would have expected _open_data to be used here if data was None. Am I misunderstanding how this works?

@ellisonbg

ellisonbg Oct 9, 2013

Owner

Nevermind, I see the logic below.

@ellisonbg ellisonbg commented on the diff Oct 9, 2013

IPython/kernel/comm/comm.py
+ primary = Bool(True, help="Am I the primary or secondary Comm?")
+
+ def __init__(self, data=None, **kwargs):
+ super(Comm, self).__init__(**kwargs)
+ get_ipython().comm_manager.register_comm(self)
+ if self.primary:
+ # I am primary, open my peer.
+ self.open(data)
+
+ def _publish_msg(self, msg_type, data=None, metadata=None, **keys):
+ """Helper for sending a comm message on IOPub"""
+ data = {} if data is None else data
+ metadata = {} if metadata is None else metadata
+ self.session.send(self.iopub_socket, msg_type,
+ dict(data=data, comm_id=self.comm_id, **keys),
+ metadata=metadata,
@ellisonbg

ellisonbg Oct 9, 2013

Owner

We should update the msg spec to describe how metadata is handled.

@ellisonbg ellisonbg commented on the diff Oct 9, 2013

IPython/kernel/comm/manager.py
+
+#-----------------------------------------------------------------------------
+# Code
+#-----------------------------------------------------------------------------
+
+def lazy_keys(dikt):
+ """Return lazy-evaluated string representation of a dictionary's keys
+
+ Key list is only constructed if it will actually be used.
+ Used for debug-logging.
+ """
+ return LazyEvaluate(lambda d: list(d.keys()))
+
+
+def with_output(method):
+ """method decorator for ensuring output is handled properly in a message handler
@ellisonbg

ellisonbg Oct 9, 2013

Owner

Nice design of the with_output!

@ellisonbg ellisonbg commented on an outdated diff Oct 9, 2013

IPython/kernel/comm/manager.py
+ return self.shell.kernel.iopub_socket
+ session = Instance('IPython.kernel.zmq.session.Session')
+ def _session_default(self):
+ if self.shell is None:
+ return
+ return self.shell.kernel.session
+
+ comms = Dict()
+ targets = Dict()
+
+ # Public APIs
+
+ def register_target(self, target_name, f):
+ """Register a callable f for a given target name
+
+ f will be called with a Comm object as its only argument
@ellisonbg

ellisonbg Oct 9, 2013

Owner

Change this to a Comm instance to emphasize it isn't the class.

@ellisonbg

ellisonbg Oct 9, 2013

Owner

Also, update comment to reflect that we are now passing the (comm, msg).

@ellisonbg ellisonbg and 2 others commented on an outdated diff Oct 9, 2013

IPython/kernel/comm/manager.py
+ """Handler for comm_open messages"""
+ content = msg['content']
+ comm_id = content['comm_id']
+ target_name = content['target_name']
+ f = self.targets.get(target_name, None)
+ comm = Comm(comm_id=comm_id,
+ shell=self.shell,
+ iopub_socket=self.iopub_socket,
+ primary=False,
+ )
+ if f is None:
+ self.log.error("No such comm target registered: %s", target_name)
+ comm.close()
+ return
+ self.register_comm(comm)
+ f(comm, msg)
@ellisonbg

ellisonbg Oct 9, 2013

Owner

f here is user defined code and could easily raise an exception. What should we do in that case? Almost seems like we should catch all Exception and close the comm if it fails. But we probably don't want that to be entirely silent. Hmm...

@jasongrout

jasongrout Oct 9, 2013

Owner

I agree. Closing the comm isn't silent---it's sending the output to the right thing. Is there a way of distinguishing close events that were issued by the user, vs. those that were triggered by an error?

@minrk

minrk Oct 11, 2013

Owner

We could add a 'reason' key to the comm_close message.

@ellisonbg ellisonbg commented on the diff Oct 9, 2013

IPython/kernel/comm/comm.py
+ def on_msg(self, callback):
+ """Register a callback for comm_msg
+
+ Will be called with the `data` of any comm_msg messages.
+
+ Call `on_msg(None)` to disable an existing callback.
+ """
+ self._msg_callback = callback
+
+ # handling of incoming messages
+
+ def handle_close(self, msg):
+ """Handle a comm_close message"""
+ self.log.debug("handle_close[%s](%s)", self.comm_id, msg)
+ if self._close_callback:
+ self._close_callback(msg)
@ellisonbg

ellisonbg Oct 9, 2013

Owner

What if _close_callback raises?

@ellisonbg ellisonbg commented on the diff Oct 9, 2013

IPython/kernel/comm/comm.py
+ """
+ self._msg_callback = callback
+
+ # handling of incoming messages
+
+ def handle_close(self, msg):
+ """Handle a comm_close message"""
+ self.log.debug("handle_close[%s](%s)", self.comm_id, msg)
+ if self._close_callback:
+ self._close_callback(msg)
+
+ def handle_msg(self, msg):
+ """Handle a comm_msg message"""
+ self.log.debug("handle_msg[%s](%s)", self.comm_id, msg)
+ if self._msg_callback:
+ self._msg_callback(msg)
@ellisonbg

ellisonbg Oct 9, 2013

Owner

What if _msg_callback raises?

@ellisonbg ellisonbg commented on the diff Oct 9, 2013

IPython/html/static/base/js/utils.js
@@ -365,6 +365,18 @@ IPython.utils = (function (IPython) {
test.remove();
return Math.floor(points*pixel_per_point);
};
+
+ var always_new = function (constructor) {
@ellisonbg

ellisonbg Oct 9, 2013

Owner

Does this variant work if the caller does use new?

@ellisonbg ellisonbg and 1 other commented on an outdated diff Oct 9, 2013

IPython/html/static/notebook/js/codecell.js
@@ -248,12 +248,22 @@ var IPython = (function (IPython) {
this.kernel.clear_callbacks_for_msg(this.last_msg_id);
}
var callbacks = {
- 'execute_reply': $.proxy(this._handle_execute_reply, this),
- 'output': $.proxy(this.output_area.handle_output, this.output_area),
- 'clear_output': $.proxy(this.output_area.handle_clear_output, this.output_area),
- 'set_next_input': $.proxy(this._handle_set_next_input, this),
- 'input_request': $.proxy(this._handle_input_request, this)
+ shell : {
+ reply : $.proxy(this._handle_execute_reply, this),
+ payload : {
@ellisonbg

ellisonbg Oct 9, 2013

Owner

There is a bit of asymmetry in how set_next_input and page are handled here. Either both should call a $.proxy method or both should have an inline function for a handler. Probably both should use a method.

@ellisonbg

ellisonbg Oct 9, 2013

Owner

I should note - the other reason it should be a method is that other parts of the code base, might want to define custom execution logic, but still reuse some of the default handlers provided in the CodeCell.

@minrk

minrk Oct 11, 2013

Owner

Probably both should use a method.

Sure - there has never been an 'open-with-pager' method on the Notebook object (because it is a method on the Pager object), so I can add one.

@minrk

minrk Oct 11, 2013

Owner

Actually, this would be adding an 'open-with-pager' method to CodeCell, but I can certainly do that if you want.
I did add a get_callbacks method that constructs the default callback dict, so it can be reused as a starting point.

@ellisonbg ellisonbg commented on the diff Oct 9, 2013

IPython/html/static/services/kernels/js/comm.js
+
+ CommManager.prototype.init_kernel = function (kernel) {
+ // connect the kernel, and register message handlers
+ this.kernel = kernel;
+ var msg_types = ['comm_open', 'comm_msg', 'comm_close'];
+ for (var i = 0; i < msg_types.length; i++) {
+ var msg_type = msg_types[i];
+ kernel.register_iopub_handler(msg_type, $.proxy(this[msg_type], this));
+ }
+ };
+
+ CommManager.prototype.register_target = function (target_name, f) {
+ // Register a target function for a given target name
+ this.targets[target_name] = f;
+ };
+
@ellisonbg

ellisonbg Oct 9, 2013

Owner

We probably want an unregister_target method as well.

@ellisonbg ellisonbg commented on an outdated diff Oct 9, 2013

IPython/html/static/services/kernels/js/comm.js
+ delete this.comms[comm_id];
+ };
+
+ // comm message handlers
+
+ CommManager.prototype.comm_open = function (msg) {
+ var content = msg.content;
+ var f = this.targets[content.target_name];
+ if (f === undefined) {
+ console.log("No such target registered: ", content.target_name);
+ console.log("Available targets are: ", this.targets);
+ return;
+ }
+ var comm = new Comm(content.comm_id);
+ this.register_comm(comm);
+ f(comm, msg);
@ellisonbg

ellisonbg Oct 9, 2013

Owner

We probably need error handling wen calling this as well - minimally log a message to the console.

@ellisonbg ellisonbg commented on an outdated diff Oct 9, 2013

IPython/html/static/services/kernels/js/comm.js
+ this['_' + key + '_callback'] = callback;
+ };
+
+ Comm.prototype.on_msg = function (callback) {
+ this._register_callback('msg', callback);
+ };
+
+ Comm.prototype.on_close = function (callback) {
+ this._register_callback('close', callback);
+ };
+
+ // methods for handling incoming messages
+
+ Comm.prototype._maybe_callback = function (key, msg) {
+ var callback = this['_' + key + '_callback'];
+ if (callback) callback(msg);
@ellisonbg

ellisonbg Oct 9, 2013

Owner

Probably want error handling here as well.

Owner

ellisonbg commented Oct 9, 2013

Overall, this is looking really good. I just took another detailed review pass. There are a few comments inline, but I think we are getting close to being able to merge this.

Owner

jasongrout commented Oct 10, 2013

I'm always constructing Comm objects and passing in the target_name attribute. It seems more natural to just have the target_name be the first required argument of the Comm init function: Comm('myhandler', {'mydata': 'some data'})

Owner

jasongrout commented Oct 10, 2013

I posted initial work on an matplotlib backend using comm: http://thread.gmane.org/gmane.comp.python.matplotlib.devel/12509

Owner

minrk commented Oct 11, 2013

I think I have addressed your points, thanks guys.

Owner

jasongrout commented Oct 12, 2013

It seems that there is some asymmetry in how javascript and python comm objects are created and opened still. In javascript, I create a Comm object with an id and target_name, then call an open method with data, metdata, etc. In python, I create a Comm object with a target and data, and the open is done automatically. Can we make these a bit more uniform?

Owner

minrk commented Oct 12, 2013

I can make the signature uniform, I will do that. I cannot make instantiating a Comm on the js side open its counterpart, because there isn't a global handle to grab.

Owner

jasongrout commented Oct 12, 2013

The problem is that the javascript Comm this.kernel attribute isn't set? Perhaps that can be a parameter to the Comm initializer? Perhaps the CommManager can have a function to create and return a new Comm object that automatically sets the kernel correctly, and that would have the signature (target_name, data).

I find that I'm never using the comm id. That is an internal routing thing that my application has no interest in setting or seeing. So +1 to at least making the javascript Comm signature Comm(target_name) (or at least making it Comm(target_name, comm_id)).

Owner

minrk commented Oct 13, 2013

@jasongrout I made the first arg target_name, and added CommManager.new_comm that will do the same magic as Comm.__init__ in Python, so you can do:

var comm = comm_manger.new_comm(target, data);

just like the Python signature.

General question:

I made the js message signature:

Comm.send(data, callbacks, metadata);

I don't love this, because data, metadata, callbacks seems like a more logical order. However, I would expect specifying metadata to be fairly rare, and so I prioritized the more common usage of just data and callbacks.

Anyone have a preference on this?

Another general issue: when there are several args to a function, it makes sense to use a single params arg, which functions like kwargs (cf $.ajax). Should we use this in any/all of the Comm methods, and/or the CommManager.new_comm method, which takes several args?

Owner

jasongrout commented Oct 16, 2013

+1 to Comm.send(data, callbacks, metadata)

+0 to making the comm javascript objects take a params arg instead of separate params.

Owner

ellisonbg commented Oct 16, 2013

If we can avoid a params based API I think it is better.

Owner

minrk commented Oct 17, 2013

In that case, I think this is ready to go.

@jasongrout jasongrout and 1 other commented on an outdated diff Oct 17, 2013

IPython/html/static/services/kernels/js/comm.js
+ this.init_kernel(kernel);
+ }
+ };
+
+ CommManager.prototype.init_kernel = function (kernel) {
+ // connect the kernel, and register message handlers
+ this.kernel = kernel;
+ var msg_types = ['comm_open', 'comm_msg', 'comm_close'];
+ for (var i = 0; i < msg_types.length; i++) {
+ var msg_type = msg_types[i];
+ kernel.register_iopub_handler(msg_type, $.proxy(this[msg_type], this));
+ }
+ };
+
+ CommManager.prototype.new_comm = function (target_name, data, callbacks, metadata, comm_id) {
+ // Create a new Comm, register it, and open is Kernel-side counterpart
Owner

jasongrout commented Oct 17, 2013

Brian mentioned a few places that we should maybe handle errors (like callbacks, on_close, etc.). Did you decide we shouldn't catch errors?

Owner

minrk commented Oct 17, 2013

No, I added error handling.

Owner

minrk commented Oct 17, 2013

Maybe I missed some of them though, I'll go back through.

Owner

minrk commented Oct 17, 2013

Yup, I missed one. It should be caught now.

Owner

jasongrout commented Oct 17, 2013

Ah, I see you added error handling at the CommManager level on the python side. I missed that.

Owner

jasongrout commented Oct 17, 2013

I'll push this latest version to the sage cell server and test things out.

@jasongrout jasongrout and 1 other commented on an outdated diff Oct 17, 2013

IPython/html/static/services/kernels/js/comm.js
+ Comm.prototype.on_msg = function (callback) {
+ this._register_callback('msg', callback);
+ };
+
+ Comm.prototype.on_close = function (callback) {
+ this._register_callback('close', callback);
+ };
+
+ // methods for handling incoming messages
+
+ Comm.prototype._maybe_callback = function (key, msg) {
+ var callback = this['_' + key + '_callback'];
+ if (callback) {
+ try {
+ callback(msg);
+ catch (e) {
@jasongrout

jasongrout Oct 17, 2013

Owner

You're missing a } to close the try block.

@minrk

minrk Oct 17, 2013

Owner

that's what I get for pushing without testing. Should be fixed (and actually ran through my example widgets, to check).

jasongrout referenced this pull request in matplotlib/matplotlib Oct 18, 2013

Closed

Initial, very rough, comm-based backend #2524

Owner

jasongrout commented Oct 18, 2013

This (at least, my identical fix to yours) seems to work great. It's live on the Sage Cell server right now.

Owner

jasongrout commented Oct 19, 2013

Yet another example using the Comm framework: http://sagecell.sagemath.org/?q=fyjgmk

@damianavila damianavila commented on the diff Oct 20, 2013

IPython/html/static/notebook/js/codecell.js
@@ -27,16 +27,16 @@ var posEq = function(a, b) {return a.line == b.line && a.ch == b.ch;}
*/
CodeMirror.commands.delSpaceToPrevTabStop = function(cm){
var from = cm.getCursor(true), to = cm.getCursor(false), sel = !posEq(from, to);
- if (!posEq(from, to)) {cm.replaceRange("", from, to); return}
+ if (!posEq(from, to)) { cm.replaceRange("", from, to); return; }
@damianavila

damianavila Oct 20, 2013

Contributor

Sometimes, in this PR, I see spaces surrounding the expression, like in this line one, and sometimes I don't see it, like in the deleted line. Which is the convention to follow?

@minrk

minrk Oct 20, 2013

Owner

I don't think it's a big deal, but I like a bit more space. I only added it here because it was missing a semicolon, so I was already editing the line.

minrk added some commits Sep 10, 2013

@minrk minrk Improvements to kernel.js
- dispatch iopub handlers by msg_type
- add Kernel.send_shell_message public API
- use strict (and related fixes)
b96b72e
@minrk minrk add Kernel-side widgets 43fad3e
@minrk minrk add javascript-side widgets dd54103
@minrk minrk document widget messages cf3af2e
@minrk minrk fix js/Python WidgetManager symmetry
remove some log statements
b3b051f
@minrk minrk make js / Python widgets symmetrical
don't enforce creation on Kernel side

also removed weak refs - premature optimization,
we can think about this later.
53e6794
@minrk minrk update widget message spec doc to reflect symmetry cef6198
@minrk minrk fix Widget.primary 18265a2
@minrk minrk rename widget to comm ee7d313
@minrk minrk s/destroy/close 7bc356d
@minrk minrk pass whole message to Comm handlers a01663f
@minrk minrk COMM: mirror Python callback API in Javascript a84541d
@minrk minrk zmqshell has handle on Kernel f266afb
@minrk minrk add ZMQShell.set_parent
coalesces all `set_parent` calls into one method.
b61bd67
@minrk minrk fixup kernel Any trait 40a0288
@minrk minrk hook up output for comm messages d76b3be
@minrk minrk publish busy/idle when handling widget messages f64935c
@minrk minrk s/target/target_name f6f777f
@minrk minrk open is not an event
target callback receives comm and open message, not just comm.
fb3e875
@minrk minrk add utils.always_new
wrapper allows passing constructors as callbacks, where `new` is required.
5f2f04c
@minrk minrk refactor js callbacks
all callbacks get the whole message
814bf16
@minrk minrk only pass shell.reply callback to oinfo / complete
These should not have side effects, so no need to expose full callback structure.

Also, object_info method shouldn't have `_request` in its name.
807713d
@minrk minrk add output callbacks test notebook
This should be converted to js tests after the testing framework has been merged.
6d1ce7f
@minrk minrk jshint on codecell f19c977
@minrk minrk update message spec with comm messages 865885b
@minrk minrk update callback structure in js commands 0c566bc
@minrk minrk overzealous find/replace 637b126
@minrk minrk get clear_output callback properly d2ffe72
@minrk minrk test clear_output callback 8f75ae2
@minrk minrk move comm.js to base a681d2b
@minrk minrk add message metadata to comm and kernel.send_shell_message c064305
@minrk minrk s/init_widgets/init_comms de4b164
@minrk minrk mention payload source key in execute_result 9df4424
@minrk minrk move comm.js to kernel service f52bb4d
@minrk minrk attach comm_manager to kernel 2227b13
@minrk minrk make parent_header available from the Shell object 5bd8c19
@minrk minrk set parent header properly in comm messages 99da4b6
@minrk minrk allow metadata in Kernel-side Comm messages. 360345a
@minrk minrk add CodeCell.get_callbacks
for reusing default execute callbacks
a9d099c
@minrk minrk add unregister_target to CommManagers a9a6bd0
@minrk minrk log exceptions in Comm handlers 9ce5d2b
@minrk minrk add target_name as first arg to Comm constructor 6521d1b
@minrk minrk Add CommManager.new_comm
Javascript-side version for creating and connecting Comms in one call

Without a `get_ipython()`-like global handle,
Comm constructor can't do the same magic as the IPython one.
2afefa0
@minrk minrk catch errors in comm callbacks c4185b7
@minrk minrk allow callbacks on status messages 1da827d
@minrk minrk set parent of status messages for comm_msgs 261ea42
@minrk minrk don't expose comm_id arg via new_comm 01609ca
Owner

minrk commented Oct 23, 2013

I think this is ready to go, if anyone wants to have a last look.

Owner

jasongrout commented Oct 24, 2013

+1 to merging.

@minrk minrk added a commit that referenced this pull request Oct 24, 2013

@minrk minrk Merge pull request #4195 from minrk/widget-msg
IPEP 21:  widget messages
de09adf

@minrk minrk merged commit de09adf into ipython:master Oct 24, 2013

1 check passed

default The Travis CI build passed
Details
Owner

jasongrout commented Oct 24, 2013

Woohoo! Great work!

Owner

ellisonbg commented Oct 24, 2013

Awesome!

On Thu, Oct 24, 2013 at 12:55 PM, Jason Grout notifications@github.comwrote:

Woohoo! Great work!


Reply to this email directly or view it on GitHubhttps://github.com/ipython/ipython/pull/4195#issuecomment-27024534
.

Brian E. Granger
Cal Poly State University, San Luis Obispo
bgranger@calpoly.edu and ellisonbg@gmail.com

Owner

SylvainCorlay commented Oct 24, 2013

Great. Thank you.

Owner

fperez commented Oct 24, 2013

Fantastic job, guys. Very, very happy about this.

Owner

Carreau commented Oct 26, 2013

@minrk in this, kernel.js you changed object_info_request to object_info. it broke tooltip, and is an API change (imho) what should we do about ?

(I should have seen it, sorry)

@Carreau Carreau added a commit to Carreau/ipython that referenced this pull request Oct 26, 2013

@Carreau Carreau fix and add shim for change introduce by #4195
comm merging renamed object_info_request to object_info
bda4afa
Owner

minrk commented Oct 26, 2013

Must have been mixed up as part of the rebase, I know I tested tooltips. It was wrong to use _request in one method name, where we leave it off of the others (execute, complete).

minrk deleted the minrk:widget-msg branch Oct 26, 2013

Owner

SylvainCorlay commented Oct 31, 2013

In the current version of the example notebook, parameters 'data' and 'target_name' are inverted in the ctor of Comm. It should be "return Comm(target_name=self.target_name, data=data)"

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

@minrk minrk Merge pull request #4195 from minrk/widget-msg
IPEP 21:  widget messages
4dc6472

@mattvonrocketstein mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014

@Carreau Carreau fix and add shim for change introduce by #4195
comm merging renamed object_info_request to object_info
76adc47

@brandoncurtis brandoncurtis pushed a commit to brandoncurtis/sagecell that referenced this pull request Jan 30, 2015

Jason Grout Update to work with IPython comm infrastructure (ipython/ipython#4195) 52ed508
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment