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 support for IPyWidgets in Interactive Window #4208

Merged
merged 3 commits into from
Dec 10, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions news/2 Fixes/4203.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix support for IPyWidgets in Interactive Window.
42 changes: 22 additions & 20 deletions src/client/datascience/ipywidgets/commonMessageCoordinator.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,8 @@ export class CommonMessageCoordinator {
this.jupyterOutput = this.serviceContainer.get<IOutputChannel>(IOutputChannel, JUPYTER_OUTPUT_CHANNEL);
}

public static async create(identity: Uri, serviceContainer: IServiceContainer): Promise<CommonMessageCoordinator> {
const result = new CommonMessageCoordinator(identity, serviceContainer);
await result.initialize();
return result;
public static create(identity: Uri, serviceContainer: IServiceContainer): CommonMessageCoordinator {
Copy link
Contributor

Choose a reason for hiding this comment

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

The purpose of this factory method was to ensure that initialize is called once (and only once) after creation and that the object is not used until the initialization is done. By taking out the call to initialize and making initialize public, we now rely on the user of the class to be diligent and to not:

  • call initialize twice
  • use the object before calling initialize
  • use the object before the call to initialize finishes

return new CommonMessageCoordinator(identity, serviceContainer);
}

public dispose() {
Expand Down Expand Up @@ -94,6 +92,19 @@ export class CommonMessageCoordinator {
this.getIPyWidgetScriptSource()?.onMessage(message, payload);
}

public async initialize() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this function need to be async? It does not seem to use await anywhere.

const dispatcher = this.getIPyWidgetMessageDispatcher();
const promises = [];
if (dispatcher) {
promises.push(dispatcher.initialize());
}
const scriptSource = this.getIPyWidgetScriptSource();
if (scriptSource) {
promises.push(scriptSource.initialize());
}
return Promise.all(promises);
}

private hash(s: string): string {
return this.hashFn().update(s).digest('hex');
}
Expand Down Expand Up @@ -164,6 +175,9 @@ export class CommonMessageCoordinator {
this.ipyWidgetMessageDispatcher = this.serviceContainer
.get<IPyWidgetMessageDispatcherFactory>(IPyWidgetMessageDispatcherFactory)
.create(this.identity);
this.disposables.push(
this.ipyWidgetMessageDispatcher.postMessage(this.postEmitter.fire.bind(this.postEmitter))
);
}
return this.ipyWidgetMessageDispatcher;
}
Expand All @@ -183,23 +197,11 @@ export class CommonMessageCoordinator {
this.serviceContainer.get<IPersistentStateFactory>(IPersistentStateFactory),
this.serviceContainer.get<IExtensionContext>(IExtensionContext)
);
this.disposables.push(this.ipyWidgetScriptSource.postMessage(this.postEmitter.fire.bind(this.postEmitter)));
this.disposables.push(
this.ipyWidgetScriptSource.postInternalMessage(this.postEmitter.fire.bind(this.postEmitter))
);
}
return this.ipyWidgetScriptSource;
}

private async initialize() {
const dispatcher = this.getIPyWidgetMessageDispatcher();
const promises = [];
if (dispatcher) {
this.disposables.push(dispatcher.postMessage(this.postEmitter.fire.bind(this.postEmitter)));
promises.push(dispatcher.initialize());
}
const scriptSource = this.getIPyWidgetScriptSource();
if (scriptSource) {
this.disposables.push(scriptSource.postMessage(this.postEmitter.fire.bind(this.postEmitter)));
this.disposables.push(scriptSource.postInternalMessage(this.postEmitter.fire.bind(this.postEmitter)));
promises.push(scriptSource.initialize());
}
return Promise.all(promises);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@ export class NotebookIPyWidgetCoordinator implements INotebookKernelResolver {
// entire VS code session, we have a map of notebook document to message coordinator
let promise = this.messageCoordinators.get(document.uri.toString());
if (!promise) {
promise = CommonMessageCoordinator.create(document.uri, this.serviceContainer);
const coordinator = CommonMessageCoordinator.create(document.uri, this.serviceContainer);
const promise = coordinator.initialize().then(() => coordinator);
DonJayamanne marked this conversation as resolved.
Show resolved Hide resolved
this.messageCoordinators.set(document.uri.toString(), promise);
}
return Cancellation.race(() => promise!.then(this.attachCoordinator.bind(this, document, webview)), token);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,7 @@ export class WebviewIPyWidgetCoordinator implements IInteractiveWindowListener {
// There should be an instance of the WebviewMessageCoordinator per notebook webview or interactive window. Create
// the message coordinator as soon as we're sure what notebook we're in.
this.notebookIdentity = args.resource;
this.messageCoordinator = await CommonMessageCoordinator.create(this.notebookIdentity, this.serviceContainer);
this.messageCoordinator = CommonMessageCoordinator.create(this.notebookIdentity, this.serviceContainer);
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the reason for this change was to allow for a specialization of the postEmitter.event of the CommonMessageCoordinator before the initialization takes place.

To avoid making initialize public and relying on end-user cooperation, I would recommend modifying the factory method create to take an optional parameter that allows the caller to inject the postEmitter.

this.messageCoordinatorEvent = this.messageCoordinator.postMessage((e) => {
// Special case a specific message. It must be posted to the internal class, not the webview
if (e.message === InteractiveWindowMessages.ConvertUriForUseInWebViewRequest) {
Expand All @@ -71,5 +71,6 @@ export class WebviewIPyWidgetCoordinator implements IInteractiveWindowListener {
this.postEmitter.fire(e);
}
});
this.messageCoordinator.initialize();
DonJayamanne marked this conversation as resolved.
Show resolved Hide resolved
}
}