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.

Sign up for GitHub

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

Push state in comm open msg #84

Merged
merged 4 commits into from
Jul 12, 2015
Merged

Push state in comm open msg #84

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e7ccb92
Push state in comm open msg :texas:
jdfreder Jul 11, 2015
9229450
Don't send first state push :flag:
jdfreder Jul 11, 2015
6ce8d6f
Remove JS this from python
jdfreder Jul 11, 2015
98aff2c
Push model info, whoops!
jdfreder Jul 12, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 36 additions & 21 deletions ipywidgets/static/widgets/js/manager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ define([
*/
WidgetManager._managers.push(this);

// Attach a comm manager to the
// Attach a comm manager to the
this.keyboard_manager = notebook.keyboard_manager;
this.notebook = notebook;
this.comm_manager = comm_manager;
Expand Down Expand Up @@ -61,14 +61,14 @@ define([
WidgetManager._save_callback = null;

WidgetManager.register_widget_model = function (model_name, model_type) {
/**
/**
* Registers a widget model by name.
*/
WidgetManager._model_types[model_name] = model_type;
};

WidgetManager.register_widget_view = function (view_name, view_type) {
/**
/**
* Registers a widget view by name.
*/
WidgetManager._view_types[view_name] = view_type;
Expand All @@ -77,7 +77,7 @@ define([
WidgetManager.set_state_callbacks = function (load_callback, save_callback, options) {
/**
* Registers callbacks for widget state persistence.
*
*
* Parameters
* ----------
* load_callback: function()
Expand Down Expand Up @@ -107,7 +107,7 @@ define([

// Use local storage to persist widgets across page refresh by default.
// LocalStorage is per domain, so we need to explicitly set the URL
// that the widgets are associated with so they don't show on other
// that the widgets are associated with so they don't show on other
// pages hosted by the noteboook server.
var url = [window.location.protocol, '//', window.location.host, window.location.pathname].join('');
var key = 'widgets:' + url;
Expand All @@ -129,11 +129,11 @@ define([
*/
var cell = this.get_msg_cell(msg.parent_header.msg_id);
if (cell === null) {
return Promise.reject(new Error("Could not determine where the display" +
return Promise.reject(new Error("Could not determine where the display" +
" message was from. Widget will not be displayed"));
} else {
return this.display_view_in_cell(cell, model)
.catch(utils.reject('Could not display view', true));
.catch(utils.reject('Could not display view', true));
}
};

Expand All @@ -149,7 +149,7 @@ define([
that._handle_display_view(view);
view.trigger('displayed');
return view;
}).catch(utils.reject('Could not create or display view', true));
}).catch(utils.reject('Could not create or display view', true));
} else {
return Promise.reject(new Error('Cell does not have a `widgetarea` defined'));
}
Expand All @@ -163,24 +163,24 @@ define([
*/
if (this.keyboard_manager) {
this.keyboard_manager.register_events(view.el);

if (view.additional_elements) {
for (var i = 0; i < view.additional_elements.length; i++) {
this.keyboard_manager.register_events(view.additional_elements[i]);
}
}
}
}
};

WidgetManager.prototype.create_view = function(model, options) {
/**
* Creates a promise for a view of a given model
*
* Make sure the view creation is not out of order with
* Make sure the view creation is not out of order with
* any state updates.
*/
model.state_change = model.state_change.then(function() {

return utils.load_class(model.get('_view_name'), model.get('_view_module'),
WidgetManager._view_types).then(function(ViewType) {

Expand All @@ -199,7 +199,7 @@ define([
});
var id = utils.uuid();
model.views[id] = model.state_change;
model.state_change.then(function(view) {
model.state_change.then(function(view) {
view.once('remove', function() {
delete view.model.views[id];
}, this);
Expand Down Expand Up @@ -228,8 +228,8 @@ define([
return callbacks.iopub.get_cell();
}
}
// Not triggered by a cell or widget (no get_cell callback

// Not triggered by a cell or widget (no get_cell callback
// exists).
return null;
};
Expand Down Expand Up @@ -280,11 +280,26 @@ define([
/**
* Handle when a comm is opened.
*/
return this.create_model({
model_name: msg.content.data.model_name,
model_module: msg.content.data.model_module,
comm: comm,
}).catch(utils.reject("Couldn't create a model.", true));
// For < 4.x
var model;
if (msg.content.data.model_name) {
Copy link
Member

Choose a reason for hiding this comment

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

In which case could this happen? Is this to handle an older version of the Python side?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

Copy link
Member

Choose a reason for hiding this comment

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

How do you see this occurring ?

Copy link

Choose a reason for hiding this comment

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

the removal of this did break Interact.jl fwiw. probably good to have a deprecation for such things before breaking changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we broke it in a later commit, sorry @shashi ! @SylvainCorlay and I will try to be better about things like this. It was removed in 474f08c

Copy link

Choose a reason for hiding this comment

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

I don't quite understand the whole process but that sounds like an awful lot of things to support. But yeah, if you want to do it, I won't say no :) I'd be happy if I had the good stack traces back, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that for each registered kernel, we should have the Javascript in a different directory, (one per installed kernelspec). The requirejs path would be automatically prefixed with a string corresponding to the current kernel.

I think this is a good idea and I'm +1 for it - it should be addressed with the other packaging problems https://jupyter.hackpad.com/Packaging-crate-PbIgxnC71or . @shashi with the feature @SylvainCorlay is describing, packaging would be more like npm. You, in the Julia widgets, would depend on a specific widget JS version X. That would allow us to continue chugging along without affecting you, and when you were ready to upgrade, you could.

@SylvainCorlay despite that, I still think for things like this, which are "widget msg spec" changing, we need deprecation warnings. Julia (@shashi) and Haskell (@sumitsahrawat) implementations both exist now, and we should do our best to help them transition to our latest Javascript.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I know deprecation warnings can be a pain to keep track of, but I think they are much less of a pain for us than it is for our friends to migrate without them.

Copy link

Choose a reason for hiding this comment

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

Nice! Great to hear!

Copy link
Member

Choose a reason for hiding this comment

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

@jdfreder thanks, I will look at the hackpad.

model = this.create_model({
model_name: msg.content.data.model_name,
model_module: msg.content.data.model_module,
comm: comm,
});
} else {
model = this.create_model({
model_name: msg.content.data._model_name,
model_module: msg.content.data._model_module,
comm: comm,
}).then(function(model) {
model.set_state(msg.content.data);
return model;
});
}

return model.catch(utils.reject("Couldn't create a model.", true));
};

WidgetManager.prototype.create_model = function (options) {
Expand Down
92 changes: 44 additions & 48 deletions ipywidgets/widgets/widget.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ def _json_to_widget(x, obj):
class CallbackDispatcher(LoggingConfigurable):
"""A structure for registering and running callbacks"""
callbacks = List()

def __call__(self, *args, **kwargs):
"""Call all of the registered callbacks."""
value = None
Expand All @@ -74,7 +74,7 @@ def register_callback(self, callback, remove=False):
Method to be registered or unregistered.
remove=False: bool
Whether to unregister the callback."""

# (Un)Register the callback.
if remove and callback in self.callbacks:
self.callbacks.remove(callback)
Expand All @@ -96,7 +96,7 @@ def m(self, *args, **kwargs):


def register(key=None):
"""Returns a decorator registering a widget class in the widget registry.
"""Returns a decorator registering a widget class in the widget registry.
If no key is provided, the class name is used as a key. A key is
provided for each core IPython widget so that the frontend can use
this key regardless of the language of the kernel"""
Expand Down Expand Up @@ -141,29 +141,29 @@ def handle_comm_opened(comm, msg):
# Traits
#-------------------------------------------------------------------------
_model_module = Unicode(None, allow_none=True, help="""A requirejs module name
in which to find _model_name. If empty, look in the global registry.""")
_model_name = Unicode('WidgetModel', help="""Name of the backbone model
registered in the front-end to create and sync this widget with.""")
in which to find _model_name. If empty, look in the global registry.""", sync=True)
_model_name = Unicode('WidgetModel', help="""Name of the backbone model
registered in the front-end to create and sync this widget with.""", sync=True)
_view_module = Unicode(help="""A requirejs module in which to find _view_name.
If empty, look in the global registry.""", sync=True)
_view_name = Unicode(None, allow_none=True, help="""Default view registered in the front-end
to use to represent the widget.""", sync=True)
comm = Instance('ipykernel.comm.Comm', allow_none=True)
msg_throttle = Int(3, sync=True, help="""Maximum number of msgs the

msg_throttle = Int(3, sync=True, help="""Maximum number of msgs the
front-end can send before receiving an idle msg from the back-end.""")

version = Int(0, sync=True, help="""Widget's version""")
keys = List()
def _keys_default(self):
return [name for name in self.traits(sync=True)]

_property_lock = Dict()
_holding_sync = False
_states_to_send = Set()
_display_callbacks = Instance(CallbackDispatcher, ())
_msg_callbacks = Instance(CallbackDispatcher, ())

#-------------------------------------------------------------------------
# (Con/de)structor
#-------------------------------------------------------------------------
Expand All @@ -187,8 +187,7 @@ def open(self):
"""Open a comm to the frontend if one isn't already open."""
if self.comm is None:
args = dict(target_name='ipython.widget',
data={'model_name': self._model_name,
'model_module': self._model_module})
data=self.get_state())
if self._model_id is not None:
args['comm_id'] = self._model_id
self.comm = Comm(**args)
Expand All @@ -198,12 +197,9 @@ def _comm_changed(self, name, new):
if new is None:
return
self._model_id = self.model_id

self.comm.on_msg(self._handle_msg)
Widget.widgets[self.model_id] = self

# first update
self.send_state()

@property
def model_id(self):
Expand All @@ -220,7 +216,7 @@ def __setattr__(self, name, value):
"""Overload of HasTraits.__setattr__to handle read-only-ness of widget
attributes """
if (self._read_only_enabled and self.has_trait(name) and
self.trait_metadata(name, 'read_only')):
self.trait_metadata(name, 'read_only')):
raise TraitError('Widget attribute "%s" is read-only.' % name)
else:
super(Widget, self).__setattr__(name, value)
Expand All @@ -236,7 +232,7 @@ def close(self):
Widget.widgets.pop(self.model_id, None)
self.comm.close()
self.comm = None

def send_state(self, key=None):
"""Sends the widget state, or a piece of it, to the front-end.

Expand Down Expand Up @@ -316,9 +312,9 @@ def on_msg(self, callback, remove=False):
----------
callback: callable
callback will be passed three arguments when a message arrives::

callback(widget, content, buffers)

remove: bool
True if the callback should be unregistered."""
self._msg_callbacks.register_callback(callback, remove=remove)
Expand All @@ -330,9 +326,9 @@ def on_displayed(self, callback, remove=False):
----------
callback: method handler
Must have a signature of::

callback(widget, **kwargs)

kwargs from display are passed through without modification.
remove: bool
True if the callback should be unregistered."""
Expand All @@ -356,7 +352,7 @@ def _lock_property(self, **properties):
The value should be the JSON state of the property.

NOTE: This, in addition to the single lock for all state changes, is
flawed. In the future we may want to look into buffering state changes
flawed. In the future we may want to look into buffering state changes
back to the front-end."""
self._property_lock = properties
try:
Expand All @@ -373,7 +369,7 @@ def _allow_write(self):
self._read_only_enabled = False
yield
finally:
self._read_only_enabled = True
self._read_only_enabled = True

@contextmanager
def hold_sync(self):
Expand All @@ -385,7 +381,7 @@ def hold_sync(self):
self._holding_sync = True
yield
finally:
self._holding_sync = False
self._holding_sync = False
self.send_state(self._states_to_send)
self._states_to_send.clear()

Expand All @@ -400,7 +396,7 @@ def _should_send_property(self, key, value):
return False
else:
return True

# Event handlers
@_show_traceback
def _handle_msg(self, msg):
Expand Down Expand Up @@ -479,7 +475,7 @@ class DOMWidget(Widget):
visible = Bool(True, allow_none=True, help="Whether the widget is visible. False collapses the empty space, while None preserves the empty space.", sync=True)
_css = Tuple(sync=True, help="CSS property list: (selector, key, value)")
_dom_classes = Tuple(sync=True, help="DOM classes applied to widget.$el.")

width = CUnicode(sync=True)
height = CUnicode(sync=True)
# A default padding of 2.5 px makes the widgets look nice when displayed inline.
Expand All @@ -493,33 +489,33 @@ class DOMWidget(Widget):
border_width = CUnicode(sync=True)
border_radius = CUnicode(sync=True)
border_style = CaselessStrEnum(values=[ # http://www.w3schools.com/cssref/pr_border-style.asp
'none',
'hidden',
'dotted',
'dashed',
'solid',
'double',
'groove',
'ridge',
'inset',
'outset',
'initial',
'none',
'hidden',
'dotted',
'dashed',
'solid',
'double',
'groove',
'ridge',
'inset',
'outset',
'initial',
'inherit', ''],
default_value='', sync=True)

font_style = CaselessStrEnum(values=[ # http://www.w3schools.com/cssref/pr_font_font-style.asp
'normal',
'italic',
'oblique',
'initial',
'inherit', ''],
'normal',
'italic',
'oblique',
'initial',
'inherit', ''],
default_value='', sync=True)
font_weight = CaselessStrEnum(values=[ # http://www.w3schools.com/cssref/pr_font_weight.asp
'normal',
'bold',
'bolder',
'normal',
'bold',
'bolder',
'lighter',
'initial',
'initial',
'inherit', ''] + list(map(str, range(100,1000,100))),
default_value='', sync=True)
font_size = CUnicode(sync=True)
Expand Down