-
Notifications
You must be signed in to change notification settings - Fork 85
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
Support binary data in comm messages via buffers
#4208
Conversation
// Don't use a renderer for non-widget MIME types | ||
if (mimeType === 'text/plain' || | ||
mimeType === 'text/html' || | ||
mimeType === 'image/png') { |
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.
Before this change, ipympl outputs would end up using a simple image/png
renderer.
@@ -1,32 +0,0 @@ | |||
/*--------------------------------------------------------------------------------------------- |
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 wasn't used anymore.
@@ -1286,6 +1287,7 @@ export class JupyterKernel extends EventEmitter implements vscode.Disposable { | |||
private async sendToSocket(id: string, type: string, dest: JupyterSocket, | |||
parent: JupyterMessageHeader, message: JupyterMessageSpec, metadata: object = {}) { | |||
const msg: JupyterMessage = { | |||
// TODO: Do comms ever try to send buffers back to the kernel? |
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.
It didn't seem to be the case in my testing.
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.
Looks good to me, but I am not familiar enough with the architecture decisions to pass judgement on the impact of changing the message structure nesting levels.
src/vs/workbench/contrib/positronOutputWebview/browser/notebookOutputWebviewServiceImpl.ts
Show resolved
Hide resolved
src/vs/workbench/services/languageRuntime/common/languageRuntimeIPyWidgetClient.ts
Show resolved
Hide resolved
The smoke tests are failing because I missed a few more places affected by this change... Working on it. |
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.
src/vs/workbench/services/languageRuntime/common/languageRuntimeIPyWidgetClient.ts
Outdated
Show resolved
Hide resolved
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 fixes the problem observed in #4172 where Positron will crash once before ipydatagrid
loads 🎉
Thank you for the reviews! I believe I've addressed all of the comments. |
Addresses #3974 and #4173.
This PR plumbs
buffers
from jupyter-adapter messages through to the main thread and specifically to our IPyWidgets notebook renderer. This unlocks a bunch of widget types (thanks to @isabelizimm for these examples):ipywidgets.Image
ipympl
(matplotlib's interactive backend)Note: Save doesn't work yet. We'll need to handle the
clicked-data-url
webview message in ourNotebookOutputWebview
.ipydatagrid
bqplot
There appears to be a console warning here, we can address that in a follow-up.