From 0d80363240d6cd04da50824df35d404d83179dc6 Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Fri, 7 Feb 2014 11:12:23 -0800 Subject: [PATCH 01/11] Fix incorrect usage of attrs --- IPython/html/static/notebook/js/widgets/widget.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/IPython/html/static/notebook/js/widgets/widget.js b/IPython/html/static/notebook/js/widgets/widget.js index 17eb1f10a..716b1c4b4 100644 --- a/IPython/html/static/notebook/js/widgets/widget.js +++ b/IPython/html/static/notebook/js/widgets/widget.js @@ -148,7 +148,7 @@ function(WidgetManager, _, Backbone){ } // Delete any key value pairs that the back-end already knows about. - var attrs = (method === 'patch') ? options.attrs : model.toJSON(options); + var attrs = (method === 'patch') ? model.changed : model.toJSON(options); if (this.key_value_lock !== null) { var key = this.key_value_lock[0]; var value = this.key_value_lock[1]; From cd72883fc7de6da7e43082fc428463aa0aa83a5d Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Fri, 7 Feb 2014 11:49:05 -0800 Subject: [PATCH 02/11] Revert "Fix incorrect usage of attrs" This reverts commit 04aa0885bb143fd0409fe133ca572004ffa6dc0e. --- IPython/html/static/notebook/js/widgets/widget.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/IPython/html/static/notebook/js/widgets/widget.js b/IPython/html/static/notebook/js/widgets/widget.js index 716b1c4b4..17eb1f10a 100644 --- a/IPython/html/static/notebook/js/widgets/widget.js +++ b/IPython/html/static/notebook/js/widgets/widget.js @@ -148,7 +148,7 @@ function(WidgetManager, _, Backbone){ } // Delete any key value pairs that the back-end already knows about. - var attrs = (method === 'patch') ? model.changed : model.toJSON(options); + var attrs = (method === 'patch') ? options.attrs : model.toJSON(options); if (this.key_value_lock !== null) { var key = this.key_value_lock[0]; var value = this.key_value_lock[1]; From 36b576b0bd90a2a9e8f9acee859556e362a69975 Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Fri, 7 Feb 2014 11:52:26 -0800 Subject: [PATCH 03/11] Let backbone get changed attrs --- IPython/html/static/notebook/js/widgets/widget.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/IPython/html/static/notebook/js/widgets/widget.js b/IPython/html/static/notebook/js/widgets/widget.js index 17eb1f10a..8ed27055e 100644 --- a/IPython/html/static/notebook/js/widgets/widget.js +++ b/IPython/html/static/notebook/js/widgets/widget.js @@ -203,7 +203,7 @@ function(WidgetManager, _, Backbone){ // Push this model's state to the back-end // // This invokes a Backbone.Sync. - this.save(this.changedAttributes(), {patch: true, callbacks: callbacks}); + this.save({patch: true, callbacks: callbacks}); }, _pack_models: function(value) { From 5667947f9d39a55d46abed2dce5182320e99d4bc Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Fri, 7 Feb 2014 11:56:37 -0800 Subject: [PATCH 04/11] Revert "Let backbone get changed attrs" This reverts commit 4dd4990ff146508894353e390601c659d2638ed3. --- IPython/html/static/notebook/js/widgets/widget.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/IPython/html/static/notebook/js/widgets/widget.js b/IPython/html/static/notebook/js/widgets/widget.js index 8ed27055e..17eb1f10a 100644 --- a/IPython/html/static/notebook/js/widgets/widget.js +++ b/IPython/html/static/notebook/js/widgets/widget.js @@ -203,7 +203,7 @@ function(WidgetManager, _, Backbone){ // Push this model's state to the back-end // // This invokes a Backbone.Sync. - this.save({patch: true, callbacks: callbacks}); + this.save(this.changedAttributes(), {patch: true, callbacks: callbacks}); }, _pack_models: function(value) { From a173e684c63d579461bc505b724005d525d16e87 Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Fri, 7 Feb 2014 12:32:53 -0800 Subject: [PATCH 05/11] Added test that shows the problem. --- IPython/html/tests/widgets/widget.js | 65 ++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 14 deletions(-) diff --git a/IPython/html/tests/widgets/widget.js b/IPython/html/tests/widgets/widget.js index db26f9903..06623d838 100644 --- a/IPython/html/tests/widgets/widget.js +++ b/IPython/html/tests/widgets/widget.js @@ -8,13 +8,14 @@ var recursive_compare = function(a, b) { if (same) { if (a instanceof Object) { - for (var key in a) { + var key; + for (key in a) { if (a.hasOwnProperty(key) && !recursive_compare(a[key], b[key])) { same = false; break; } } - for (var key in b) { + for (key in b) { if (b.hasOwnProperty(key) && !recursive_compare(a[key], b[key])) { same = false; break; @@ -26,7 +27,7 @@ var recursive_compare = function(a, b) { } return same; -} +}; // Test the widget framework. casper.notebook_test(function () { @@ -58,7 +59,6 @@ casper.notebook_test(function () { var output = that.evaluate(function(input) { var model = new IPython.WidgetModel(IPython.notebook.kernel.widget_manager, undefined); var results = model._pack_models(input); - delete model; return results; }, {input: input}); that.test.assert(recursive_compare(input, output), @@ -68,7 +68,6 @@ casper.notebook_test(function () { var output = that.evaluate(function(input) { var model = new IPython.WidgetModel(IPython.notebook.kernel.widget_manager, undefined); var results = model._unpack_models(input); - delete model; return results; }, {input: input}); that.test.assert(recursive_compare(input, output), @@ -79,15 +78,53 @@ casper.notebook_test(function () { test_unpack(input); }; - test_packing({0: 'hi', 1: 'bye'}) - test_packing(['hi', 'bye']) - test_packing(['hi', 5]) - test_packing(['hi', '5']) - test_packing([1.0, 0]) - test_packing([1.0, false]) - test_packing([1, false]) - test_packing([1, false, {a: 'hi'}]) - test_packing([1, false, ['hi']]) + test_packing({0: 'hi', 1: 'bye'}); + test_packing(['hi', 'bye']); + test_packing(['hi', 5]); + test_packing(['hi', '5']); + test_packing([1.0, 0]); + test_packing([1.0, false]); + test_packing([1, false]); + test_packing([1, false, {a: 'hi'}]); + test_packing([1, false, ['hi']]); + + // Test multi-set, single touch code. First create a custom widget. + this.evaluate(function() { + var MultiSetView = IPython.DOMWidgetView.extend({ + render: function(){ + this.model.set('a', 1); + this.model.set('b', 2); + this.model.set('c', 3); + this.touch(); + }, + }); + WidgetManager.register_widget_view('MultiSetView', MultiSetView); + }, {}); + }); + + // Try creating the multiset widget, verify that sets the values correctly. + var multiset = {}; + multiset.index = this.append_cell( + 'from IPython.utils.traitlets import Unicode, CInt\n' + + 'class MultiSetWidget(widgets.Widget):\n' + + ' _view_name = Unicode("MultiSetView", sync=True)\n' + + ' a = CInt(0, sync=True)\n' + + ' b = CInt(0, sync=True)\n' + + ' c = CInt(0, sync=True)\n' + + 'multiset = MultiSetWidget()\n' + + 'display(multiset)\n' + + 'print(multiset.model_id)'); + this.execute_cell_then(multiset.index, function(index) { + multiset.model_id = this.get_output_cell(index).text.trim(); + }); + + this.wait_for_widget(multiset); + + index = this.append_cell( + 'print("%d%d%d" % (multiset.a, multiset.b, multiset.c))'); + this.execute_cell_then(index, function(index) { + this.test.assertEquals(this.get_output_cell(index).text.trim(), '123', + 'Multiple model.set calls and one view.touch update state in back-end.'); }); var textbox = {}; From fbf700e5d2d8f243eca8140465ae25af6fd9a833 Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Fri, 7 Feb 2014 13:36:32 -0800 Subject: [PATCH 06/11] Fixed typo in new test --- IPython/html/tests/widgets/widget.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/IPython/html/tests/widgets/widget.js b/IPython/html/tests/widgets/widget.js index 06623d838..e0e89cc0f 100644 --- a/IPython/html/tests/widgets/widget.js +++ b/IPython/html/tests/widgets/widget.js @@ -98,7 +98,7 @@ casper.notebook_test(function () { this.touch(); }, }); - WidgetManager.register_widget_view('MultiSetView', MultiSetView); + IPython.WidgetManager.register_widget_view('MultiSetView', MultiSetView); }, {}); }); From 770d2bd3a0b3ef9c996287b83a22b650e89795a4 Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Fri, 7 Feb 2014 13:37:37 -0800 Subject: [PATCH 07/11] Only send diff message if diff isn't corrupt. Diff will corrupt if more then one model.set(...) call is made before model.save (or view.touch() in our case). --- .../html/static/notebook/js/widgets/widget.js | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/IPython/html/static/notebook/js/widgets/widget.js b/IPython/html/static/notebook/js/widgets/widget.js index 17eb1f10a..640f539e5 100644 --- a/IPython/html/static/notebook/js/widgets/widget.js +++ b/IPython/html/static/notebook/js/widgets/widget.js @@ -32,6 +32,7 @@ function(WidgetManager, _, Backbone){ // An ID unique to this model. // comm : Comm instance (optional) this.widget_manager = widget_manager; + this._set_calls = 0; this.pending_msgs = 0; this.msg_throttle = 3; this.msg_buffer = null; @@ -135,6 +136,12 @@ function(WidgetManager, _, Backbone){ return callbacks; }, + set: function(key, val, options) { + // Set a value. + this._set_calls++; + return WidgetModel.__super__.set.apply(this, arguments); + }, + sync: function (method, model, options) { // Handle sync to the back-end. Called when a model.save() is called. @@ -158,6 +165,7 @@ function(WidgetManager, _, Backbone){ } // Only sync if there are attributes to send to the back-end. + attrs = this._pack_models(attrs); if (_.size(attrs) > 0) { // If this message was sent via backbone itself, it will not @@ -197,13 +205,26 @@ function(WidgetManager, _, Backbone){ // Since the comm is a one-way communication, assume the message // arrived. Don't call success since we don't have a model back from the server // this means we miss out on the 'sync' event. + this._set_calls = 0; }, save_changes: function(callbacks) { // Push this model's state to the back-end // // This invokes a Backbone.Sync. - this.save(this.changedAttributes(), {patch: true, callbacks: callbacks}); + + // Backbone only remembers the diff of the most recent set() + // opertation. Calling set multiple times in a row results in a + // loss of diff information which means we need to send a full + // state. If diffing is important to the user, model.set(...) should + // only be called once prior to a view.touch(). If multiple + // parameters need to be set, use the model.set({key1: val1, key2: val2, ...}) + // signature. + if (self._set_calls <= 1) { + this.save(this.changedAttributes(), {patch: true, callbacks: callbacks}); + } else { + this.save(null, {patch: false, callbacks: callbacks}); + } }, _pack_models: function(value) { From f4f2c92285a0fc826e29b7bd4e6a74631db286ef Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Fri, 7 Feb 2014 14:22:39 -0800 Subject: [PATCH 08/11] Keep a running diff instead of forcing a full state update --- .../html/static/notebook/js/widgets/widget.js | 27 +++++++------------ 1 file changed, 10 insertions(+), 17 deletions(-) diff --git a/IPython/html/static/notebook/js/widgets/widget.js b/IPython/html/static/notebook/js/widgets/widget.js index 640f539e5..9967be7ab 100644 --- a/IPython/html/static/notebook/js/widgets/widget.js +++ b/IPython/html/static/notebook/js/widgets/widget.js @@ -32,7 +32,7 @@ function(WidgetManager, _, Backbone){ // An ID unique to this model. // comm : Comm instance (optional) this.widget_manager = widget_manager; - this._set_calls = 0; + this._buffered_state_diff = {}; this.pending_msgs = 0; this.msg_throttle = 3; this.msg_buffer = null; @@ -138,8 +138,13 @@ function(WidgetManager, _, Backbone){ set: function(key, val, options) { // Set a value. - this._set_calls++; - return WidgetModel.__super__.set.apply(this, arguments); + var return_value = WidgetModel.__super__.set.apply(this, arguments); + + // Backbone only remembers the diff of the most recent set() + // opertation. Calling set multiple times in a row results in a + // loss of diff information. Here we keep our own running diff. + this._buffered_state_diff = $.extend(this._buffered_state_diff, this.changedAttributes() || {}); + return return_value; }, sync: function (method, model, options) { @@ -205,26 +210,14 @@ function(WidgetManager, _, Backbone){ // Since the comm is a one-way communication, assume the message // arrived. Don't call success since we don't have a model back from the server // this means we miss out on the 'sync' event. - this._set_calls = 0; + this._buffered_state_diff = {}; }, save_changes: function(callbacks) { // Push this model's state to the back-end // // This invokes a Backbone.Sync. - - // Backbone only remembers the diff of the most recent set() - // opertation. Calling set multiple times in a row results in a - // loss of diff information which means we need to send a full - // state. If diffing is important to the user, model.set(...) should - // only be called once prior to a view.touch(). If multiple - // parameters need to be set, use the model.set({key1: val1, key2: val2, ...}) - // signature. - if (self._set_calls <= 1) { - this.save(this.changedAttributes(), {patch: true, callbacks: callbacks}); - } else { - this.save(null, {patch: false, callbacks: callbacks}); - } + this.save(this._buffered_state_diff, {patch: true, callbacks: callbacks}); }, _pack_models: function(value) { From 7b2787e5085e648c63329875a2b1df6ee43f4bac Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Fri, 7 Feb 2014 20:56:22 -0800 Subject: [PATCH 09/11] Fixed typo --- IPython/html/static/notebook/js/widgets/widget.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/IPython/html/static/notebook/js/widgets/widget.js b/IPython/html/static/notebook/js/widgets/widget.js index 9967be7ab..f535f4266 100644 --- a/IPython/html/static/notebook/js/widgets/widget.js +++ b/IPython/html/static/notebook/js/widgets/widget.js @@ -141,7 +141,7 @@ function(WidgetManager, _, Backbone){ var return_value = WidgetModel.__super__.set.apply(this, arguments); // Backbone only remembers the diff of the most recent set() - // opertation. Calling set multiple times in a row results in a + // operation. Calling set multiple times in a row results in a // loss of diff information. Here we keep our own running diff. this._buffered_state_diff = $.extend(this._buffered_state_diff, this.changedAttributes() || {}); return return_value; From 3d408f52413945a00334288b2a0a9652c4d701ee Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Mon, 10 Feb 2014 15:46:05 -0800 Subject: [PATCH 10/11] Added a test to make sure full state was not getting sent. --- IPython/html/tests/widgets/widget.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/IPython/html/tests/widgets/widget.js b/IPython/html/tests/widgets/widget.js index e0e89cc0f..3067ab606 100644 --- a/IPython/html/tests/widgets/widget.js +++ b/IPython/html/tests/widgets/widget.js @@ -111,6 +111,10 @@ casper.notebook_test(function () { ' a = CInt(0, sync=True)\n' + ' b = CInt(0, sync=True)\n' + ' c = CInt(0, sync=True)\n' + + ' d = CInt(-1, sync=True)\n' + // See if it sends a full state. + ' def _handle_receive_state(self, sync_data):\n' + + ' widgets.Widget._handle_receive_state(self, sync_data)\n'+ + ' self.d = len(sync_data)\n' + 'multiset = MultiSetWidget()\n' + 'display(multiset)\n' + 'print(multiset.model_id)'); @@ -127,6 +131,13 @@ casper.notebook_test(function () { 'Multiple model.set calls and one view.touch update state in back-end.'); }); + index = this.append_cell( + 'print("%d" % (multiset.d))'); + this.execute_cell_then(index, function(index) { + this.test.assertEquals(this.get_output_cell(index).text.trim(), '3', + 'Multiple model.set calls sent a partial state.'); + }); + var textbox = {}; throttle_index = this.append_cell( 'import time\n' + From 1c564131c5d7509ed939155aefbdb84eaf8e625b Mon Sep 17 00:00:00 2001 From: Jonathan Frederic Date: Mon, 10 Feb 2014 15:46:28 -0800 Subject: [PATCH 11/11] Fixed, set on recieve update triggering echos. --- IPython/html/static/notebook/js/widgets/widget.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/IPython/html/static/notebook/js/widgets/widget.js b/IPython/html/static/notebook/js/widgets/widget.js index f535f4266..8f69a7d52 100644 --- a/IPython/html/static/notebook/js/widgets/widget.js +++ b/IPython/html/static/notebook/js/widgets/widget.js @@ -94,7 +94,7 @@ function(WidgetManager, _, Backbone){ _.each(state, function(value, key) { that.key_value_lock = [key, value]; try { - that.set(key, that._unpack_models(value)); + WidgetModel.__super__.set.apply(that, [key, that._unpack_models(value)]); } finally { that.key_value_lock = null; }