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 interactive window restore #11575

Merged
merged 8 commits into from
Oct 24, 2022
Merged

fix interactive window restore #11575

merged 8 commits into from
Oct 24, 2022

Conversation

amunger
Copy link
Contributor

@amunger amunger commented Oct 7, 2022

Fixes #11574
part of #10808

  • Most of the restoration is done just by recovering the editor, the controller is automatically selected.
  • We need to make sure the events are all hooked up correctly even if no cells are run in the IW. (closing the IW should remove it from the list of windows)
  • Get the editor information from the windows API instead of calling an API that might end up creating a new one.
    • If the editor is closed before the extension does it's restore logic, we don't want to restore that editor

@codecov-commenter
Copy link

codecov-commenter commented Oct 7, 2022

Codecov Report

Merging #11575 (5a20e5c) into main (a9dd309) will increase coverage by 0%.
The diff coverage is 41%.

@@           Coverage Diff           @@
##            main   #11575    +/-   ##
=======================================
  Coverage     62%      62%            
=======================================
  Files        482      486     +4     
  Lines      34464    34594   +130     
  Branches    5602     5614    +12     
=======================================
+ Hits       21634    21732    +98     
- Misses     10763    10790    +27     
- Partials    2067     2072     +5     
Impacted Files Coverage Δ
src/interactive-window/types.ts 100% <ø> (ø)
...rc/interactive-window/interactiveWindowProvider.ts 75% <12%> (-3%) ⬇️
src/interactive-window/interactiveWindow.ts 74% <48%> (-2%) ⬇️
src/platform/errors/sessionDisposedError.ts 60% <0%> (-40%) ⬇️
...s/controllers/kernelRanking/kernelRankingHelper.ts 70% <0%> (-9%) ⬇️
src/kernels/kernelProvider.base.ts 89% <0%> (-7%) ⬇️
src/notebooks/debugger/debuggingManagerBase.ts 66% <0%> (-6%) ⬇️
.../kernels/jupyter/jupyterUriProviderRegistration.ts 81% <0%> (-4%) ⬇️
src/kernels/raw/session/rawJupyterSession.node.ts 80% <0%> (-3%) ⬇️
src/kernels/kernelAutoReConnectMonitor.ts 79% <0%> (-2%) ⬇️
... and 34 more

@amunger amunger requested a review from rebornix October 7, 2022 22:26
@amunger amunger marked this pull request as ready for review October 7, 2022 22:31
};
// don't start the kernel if the IW has only been restored from a previous session
if (this.initialized) {
this.startKernel().ignoreErrors();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this event is what sets the controller on restore. It is called even when the controller is initially set, and will also take care of the case that the user changes the controller before running any cells

Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

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

I'm afraid that checking visibleNotebookEditors might not work with hidden IW tabs.

src/interactive-window/interactiveWindowProvider.ts Outdated Show resolved Hide resolved
@amunger amunger force-pushed the aamunger/IWRestore branch 2 times, most recently from 66c93d4 to c8048c3 Compare October 20, 2022 17:47
Copy link
Member

@rebornix rebornix left a comment

Choose a reason for hiding this comment

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

It looks good to me. Only nit about typings check.

@@ -95,7 +95,7 @@ export class InteractiveWindow implements IInteractiveWindowLoadable {
return this._submitters;
}
public get notebookDocument(): NotebookDocument {
return this.notebookEditor.notebook;
return this.notebookEditor?.notebook;
Copy link
Member

Choose a reason for hiding this comment

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

Would this change the typing to notebookDocument: NotebookDocument | undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It actually was already that, but I suppose I should go through with the rest of the corrections for that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'd rather tackle this in debt week - there is a lot of code in InteractiveWindow that relies on notebookDocument not being null, which would add a lot of extra undefined checking. Now that I've poked around quite a bit this iteration, I have a better idea of what could be done, including not relying on the editor to be the same throughout the lifetime of the IW object (separate issue).

const document = await workspace.openNotebookDocument(this.notebookEditorOrTab.input.uri);
const editor = await window.showNotebookDocument(document, {
viewColumn: this.notebookEditorOrTab.group.viewColumn
public async ensureInitialized() {
Copy link
Member

Choose a reason for hiding this comment

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

We can probably use some typescript type checking trick here since notebookDocument can be undefined:

interface IResolvedInteractiveWindow {
    notebookDocument: NotebookDocument;
}

class InteractiveWindow {
    ensureInitialized(): this is IResolvedInteractiveWindow;
}

@amunger amunger merged commit a9287ec into main Oct 24, 2022
@amunger amunger deleted the aamunger/IWRestore branch October 24, 2022 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Interactive window creation fails after a restored one for the same document was closed
3 participants