Skip to content

make sure that we run our show code when a custom editor is shown #14022

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.

Sign up for GitHub

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

Merged
Ian Huff (IanMatthewHuff) merged 2 commits into from
Sep 22, 2020
Merged

make sure that we run our show code when a custom editor is shown #14022

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a62809
make sure that we run our show code when a custom editor is shown
Sep 21, 2020
6e5d426
Trigger notebook load in the constructor, not show
Sep 22, 2020
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/14016.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Have Custom Editors load on editor show unless autostart is disabled.
6 changes: 4 additions & 2 deletions src/client/datascience/notebookStorage/nativeEditorProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -276,8 +276,10 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit
// Get the model
const model = await this.loadModel({ file: resource });

// Load it (should already be visible)
return this.createNotebookEditor(model, panel);
// Load it
const resultEditor = this.createNotebookEditor(model, panel);
await resultEditor.show(); // Make sure we are shown and run our show code

@IanMatthewHuff Ian Huff (IanMatthewHuff) Sep 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

In nativeEditorProviderOld show() is run on loadNotebookEditor. In the custom editor show is not run. Presumably due to the comment that I removed that it should already be visible. However the show() function of interactiveBase.ts is where we run createNotebookIfProviderConnectionExists. Since we didn't call show for the custom editor that code that creates the notebook never gets run and the notebook never auto starts regardless of the setting.

The design on show doing the notebook load seems a bit off in general, but adding in the show here seems the easiest and cleanest way to keep this symmetrical with the old editor, especially if we are considering a release port of this.

@rchiodo Rich Chiodo (rchiodo) Sep 21, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I'm surprised this works? This looks very similar to the race condition we used to have here where we wait for show before resolving. That was the cause #13901

Could we instead just call show and not wait for it? That would make me more comfortable

@rchiodo Rich Chiodo (rchiodo) Sep 21, 2020

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Or maybe have the createNotebookIfProviderConnectionExists in the constructor/load of the window instead?

@IanMatthewHuff Ian Huff (IanMatthewHuff) Sep 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yeah, this works in all scenarios when I tested the basic matrix of (Auto Load On / Off vs Old Editor / Custom Editor). I'm not seeing the race condition looking at the code, but I believe ya.

I might take a peek at putting it in constructor. Off hand it feels like it belongs more there anyways. But I feel like I recall moving it out into the show for a reason before. I'll try it and see how it looks.

@IanMatthewHuff Ian Huff (IanMatthewHuff) Sep 22, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Rich Chiodo (@rchiodo) Moved it into the constructor and tested the same four scenarios. Looks good there. And we don't need it in the show at all anymore or to call the show from the native provider.

return resultEditor;
} catch (exc) {
// Send telemetry indicating a failure
sendTelemetryEvent(Telemetry.OpenNotebookFailure);
Expand Down