-
Notifications
You must be signed in to change notification settings - Fork 946
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 binary synchronization and make Image use binary values #1643
Conversation
We do this primarily so that we have a basic widget that exercises the binary messages.
var blob = new Blob([this.model.get('value')], {type: `image/${this.model.get('format')}`}); | ||
var url = URL.createObjectURL(blob); | ||
var oldurl = this.el.src; | ||
this.el.src = url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the createObjectURL is deprecated (firefox gives you a warning on that).
https://developer.mozilla.org/en-US/docs/Web/API/URL/createObjectURL
The proper way would be to use this.el.srcObject = blob
I think. We may want to polyfill this with https://github.com/webrtc/adapter , which is what I do in https://github.com/maartenbreddels/ipywebrtc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the noise, that was only for HTMLMediaElement, not HTMLImageElement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I took some of this code from https://developer.mozilla.org/en-US/docs/Web/API/Blob
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's completely correct.
@maartenbreddels - our problem is more serious than my simple workaround. Since underscore is doing the deep compare, any arraybuffer or dataview object anywhere nested in the JSON of a deserialized trait is going to have this issue. I'm tempted to just override the backbone function and insert a special-case call to our own isequal. |
I guess both Backbone's set and changedAttributes? |
I think we override set and call our own isEqual. We're slowly completely reimplementing backbone... |
Or we fork and publish underscore with a fixed _.isEquals, maybe less work. |
Note that lodash has specific references to typed arrays, indicating they handle that case: https://github.com/lodash/lodash/blob/8e631dfcd496bc355ee7ceeb959421b0788b9bbc/.internal/baseIsEqualDeep.js#L56 |
And then we have to fork backbone and update its dependency. I really don't want to maintain an underscore fork, especially when lodash seems to work in this case. |
This indicates that we can replace underscore with lodash for backbone, but requires a special webpack config. We can't do that in jlab, though. Too bad. |
b33ce39
to
5c87a42
Compare
@maartenbreddels - I think this is ready for review and testing. I could also remove underscore from controls, but that's not pressing. |
Underscore 1.8.3’s _.eq assumes all DataView and ArrayBuffers are the same, so backbone will not trigger a change event when attributes are set (see jashkenas/underscore#2692). Lodash 4 does distinguish between different dataviews or arraybuffers, so we'll use the Lodash isEqual.
5c87a42
to
626e73f
Compare
626e73f
to
b566bc1
Compare
Fixes #1642 by working around an underscore bug.
This also makes the image use a binary value so that we have at least one core widget using the binary values and messages.
This means that custom deserializers must not return a DataView or ArrayBuffer (other typed arrays like Uint8Array are fine, though).
Test code:
Then
and also make sure the image is displayed when the page is refreshed.
CC @maartenbreddels