-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Stop creating duplicate REPL and allow new REPL instance #23496
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
Conversation
notebookDocument = interactiveWindowObject.notebookEditor.notebook; | ||
} | ||
// Handle case where user has closed REPL window, and re-opens. | ||
if (notebookEditor && notebookDocument) { |
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.
shouldn't these be set to undefined if the REPL window closes?
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.
Exactly, that is the intended behavior and not properly handling that was the root cause of the problems. Hence why we would want to watch for user when they close the REPL window and explicitly set them to undefined:
if (!interactiveWindowIsOpen) {
notebookEditor = undefined;
notebookDocument = undefined;
}
so then we can create a valid and fresh REPL instance again when user shift+enter.
With the code block that starts at line 109 (if statement block), we need in order to show the "new" instance of REPL after user has exited out of previous REPL instance and want to trigger REPL again. Excluding the showNotebook call stopped REPL from being shown if user has exited out of one REPL instance.
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 also runs every time the command is run, right? It's probably fine to show bring the editor up front every time for that command, but selecting the kernel every time is a bit strange - maybe that should go into the conditional above where the editor is being created.
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 believe so, I think we should be fine but just wanted to be safe..
I removed it in the latest commit
Resolves: #23495
Resolves: #23500