-
Notifications
You must be signed in to change notification settings - Fork 227
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
Implement image/png repr #343
Changes from 3 commits
eb94d22
8fb40c7
b5f42aa
a373f5c
fefe076
a589859
af699c2
bce06fa
113a15d
783419b
bc39d8f
bbfe34d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -53,6 +53,8 @@ export class MPLCanvasModel extends widgets.DOMWidgetModel { | |
this.ratio = (window.devicePixelRatio || 1) / backingStore; | ||
this._init_image(); | ||
|
||
this.acknowledged_rendered = false; | ||
|
||
this.on('msg:custom', this.on_comm_message.bind(this)); | ||
this.on('change:resizable', () => { | ||
this._for_each_view(function (view) { | ||
|
@@ -186,7 +188,11 @@ export class MPLCanvasModel extends widgets.DOMWidgetModel { | |
this.image.src = image_url; | ||
|
||
// Tell Jupyter that the notebook contents must change. | ||
this.send_message('ack'); | ||
if (!this.acknowledged_rendered) { | ||
this.send_message('ack'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need a round-trip to the front-end?? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like we were trying to update the mime bundle with an "update display" message, but honnestly I don't remmember. We should probably try to keep the PR as simple as possible for now... There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tried implementing it without the round-trip but I wasn't able. I need to look deeper into that. |
||
|
||
this.acknowledged_rendered = true; | ||
} | ||
|
||
this.waiting = false; | ||
} | ||
|
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.
@martinRenou why is it ok to remove this dpi scaling? It seems like we will now have imperfect behavior on hi dpi displays.
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.
This might have been a mistake, we should open an issue to track this
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'll do it, actually. If I understand correctly, the PNG doesn't get saved at high res.
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.
Note: we should not determine the dpi based on the browser because it could differ between clients.
It should be a fixed value.