Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix incorrect Patch logic in widget code #5059

Merged
merged 11 commits into from Feb 11, 2014

Conversation

jdfreder
Copy link
Member

@jdfreder jdfreder commented Feb 7, 2014

This does not work in master

this.model.set('x', x);
this.model.set('y', y);
this.model.set('z', z);
this.touch();

instead one has to write

this.model.set('x', x);
this.touch();
this.model.set('y', y);
this.touch();
this.model.set('z', z);
this.touch();

or

this.model.set({'x': x, 'y': y, 'z': z});
this.touch();

because the attr diff is only maintained for the last set operation. This patch fixes this so the first method can be used.

@jdfreder
Copy link
Member Author

jdfreder commented Feb 7, 2014

@ellisonbg

  • This fix is working and ready for you to test with your custom widget.
  • I've also added a new test that confirms that the problem is fixed.

@jdfreder
Copy link
Member Author

jdfreder commented Feb 7, 2014

@ellisonbg I successfully made the changes we talked about in person (9140177), a running buffer of the diff is maintained by us now.

@ellisonbg
Copy link
Member

@jdfreder thanks I will test this later.

is there a test to verify this works and that we don't ever send a full
update in these situations?

On Fri, Feb 7, 2014 at 2:24 PM, Jonathan Frederic
notifications@github.comwrote:

@ellisonbg https://github.com/ellisonbg I successfully made the changes
we talked about in person (91401779140177),
a running buffer of the diff is maintained by us now.

Reply to this email directly or view it on GitHubhttps://github.com//pull/5059#issuecomment-34514172
.

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

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
Copy link
Member

Choose a reason for hiding this comment

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

'operation'

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@jdfreder
Copy link
Member Author

jdfreder commented Feb 8, 2014

is there a test to verify this works and that we don't ever send a full
update in these situations?

The test doesn't check to make sure that a full state isn't sent, should I add that test?

The test only makes sure that set,set,set,touch works.

@ellisonbg
Copy link
Member

If it is not too much trouble, it would be great to have a test that checks to make sure we don't send full states. That could really kill a widget with a large model.

@jdfreder
Copy link
Member Author

Test added! Found a bug in the process 😛

@jdfreder
Copy link
Member Author

  • and fixed

@ellisonbg
Copy link
Member

Nice, I will wait for the tests and then merge.

ellisonbg added a commit that referenced this pull request Feb 11, 2014
Fix incorrect `Patch` logic in widget code
@ellisonbg ellisonbg merged commit 5258b9e into ipython:master Feb 11, 2014
@jdfreder jdfreder deleted the widgets-patch-fix branch March 10, 2014 18:44
mattvonrocketstein pushed a commit to mattvonrocketstein/ipython that referenced this pull request Nov 3, 2014
Fix incorrect `Patch` logic in widget code
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants