Fix a number of issues related to opening new notebooks

news/2 Fixes/7980.md

@@ -0,0 +1 @@
+When creating a new blank notebook, it has existing text in it already.

news/2 Fixes/8132.md

@@ -0,0 +1 @@
+Cannot create more than one blank notebook.

news/2 Fixes/8263.md

@@ -0,0 +1 @@
+When updating the Python extension, unsaved changes to notebooks are lost.

src/client/datascience/interactive-ipynb/nativeEditor.ts

@@ -21,7 +21,7 @@ import {
 import { ContextKey } from '../../common/contextKey';
 import { traceError } from '../../common/logger';
 import { IFileSystem, TemporaryFile } from '../../common/platform/types';
-import { GLOBAL_MEMENTO, IConfigurationService, IDisposableRegistry, IMemento } from '../../common/types';
+import { GLOBAL_MEMENTO, IConfigurationService, IDisposableRegistry, IMemento, WORKSPACE_MEMENTO } from '../../common/types';
 import { createDeferred, Deferred } from '../../common/utils/async';
 import * as localize from '../../common/utils/localize';
 import { StopWatch } from '../../common/utils/stopWatch';
@@ -112,7 +112,8 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
         @inject(IJupyterDebugger) jupyterDebugger: IJupyterDebugger,
         @inject(INotebookImporter) private importer: INotebookImporter,
         @inject(IDataScienceErrorHandler) errorHandler: IDataScienceErrorHandler,
-        @inject(IMemento) @named(GLOBAL_MEMENTO) private globalStorage: Memento
+        @inject(IMemento) @named(GLOBAL_MEMENTO) private globalStorage: Memento,
+        @inject(IMemento) @named(WORKSPACE_MEMENTO) private localStorage: Memento
     ) {
         super(
             listeners,
@@ -580,15 +581,30 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
      * @memberof NativeEditor
      */
     private async getStoredContents(): Promise<string | undefined> {
-        const data = this.globalStorage.get<{ contents?: string; lastModifiedTimeMs?: number }>(this.getStorageKey());
+        const key = this.getStorageKey();
+        const data = this.globalStorage.get<{ contents?: string; lastModifiedTimeMs?: number }>(key);
         // Check whether the file has been modified since the last time the contents were saved.
         if (data && data.lastModifiedTimeMs && !this.isUntitled && this.file.scheme === 'file') {
             const stat = await this.fileSystem.stat(this.file.fsPath);
             if (stat.mtime > data.lastModifiedTimeMs) {
                 return;
             }
         }
-        return data ? data.contents : undefined;
+        if (data && !this.isUntitled && data.contents) {
+            return data.contents;
+        }
+
+        const workspaceData = this.localStorage.get<string>(key);
+        if (workspaceData && !this.isUntitled) {
+            // Make sure to clear so we don't use this again.
+            this.localStorage.update(key, undefined);
+
+            // Transfer this to global storage so we use that next time instead
+            const stat = await this.fileSystem.stat(this.file.fsPath);
+            this.globalStorage.update(key, { contents: workspaceData, lastModifiedTimeMs: stat ? stat.mtime : undefined });
+
+            return workspaceData;
+        }
     }
 
     /**
@@ -605,7 +621,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor {
         const key = this.getStorageKey();
         // Keep track of the time when this data was saved.
         // This way when we retrieve the data we can compare it against last modified date of the file.
-        await this.globalStorage.update(key, { contents, lastModifiedTimeMs: Date.now() });
+        await this.globalStorage.update(key, contents ? { contents, lastModifiedTimeMs: Date.now() } : undefined);
     }
 
     private async close(): Promise<void> {

src/client/datascience/interactive-ipynb/nativeEditorProvider.ts

@@ -21,6 +21,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
     private executedEditors: Set<string> = new Set<string>();
     private notebookCount: number = 0;
     private openedNotebookCount: number = 0;
+    private nextNumber: number = 1;
     constructor(
         @inject(IServiceContainer) private serviceContainer: IServiceContainer,
         @inject(IAsyncDisposableRegistry) asyncRegistry: IAsyncDisposableRegistry,
@@ -135,9 +136,9 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
      * @private
      * @memberof NativeEditorProvider
      */
-    private onDidChangeActiveTextEditorHandler(editor?: TextEditor){
+    private onDidChangeActiveTextEditorHandler(editor?: TextEditor) {
         // I we're a source control diff view, then ignore this editor.
-        if (!editor || this.isEditorPartOfDiffView(editor)){
+        if (!editor || this.isEditorPartOfDiffView(editor)) {
             return;
         }
         this.openNotebookAndCloseEditor(editor.document, true).ignoreErrors();
@@ -162,15 +163,24 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
 
     private onOpenedEditor(e: INotebookEditor) {
         this.activeEditors.set(e.file.fsPath, e);
+        this.disposables.push(e.saved(this.onSavedEditor.bind(this, e.file.fsPath)));
         this.openedNotebookCount += 1;
     }
 
+    private onSavedEditor(oldPath: string, e: INotebookEditor) {
+        // Switch our key for this editor
+        if (this.activeEditors.has(oldPath)) {
+            this.activeEditors.delete(oldPath);
+        }
+        this.activeEditors.set(e.file.fsPath, e);
+    }
+
     private async getNextNewNotebookUri(): Promise<Uri> {
         // Start in the root and look for files starting with untitled
         let number = 1;
         const dir = this.workspace.rootPath;
         if (dir) {
-            const existing = await this.fileSystem.search(`${dir}/${localize.DataScience.untitledNotebookFileName()}-*.ipynb`);
+            const existing = await this.fileSystem.search(path.join(dir, `${localize.DataScience.untitledNotebookFileName()}-*.ipynb`));
 
             // Sort by number
             existing.sort();
@@ -181,24 +191,26 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
                 if (match && match.length > 1) {
                     number = parseInt(match[2], 10);
                 }
+                return Uri.file(path.join(dir, `${localize.DataScience.untitledNotebookFileName()}-${number + 1}`));
             }
-            return Uri.file(path.join(dir, `${localize.DataScience.untitledNotebookFileName()}-${number}`));
         }
 
-        return Uri.file(`${localize.DataScience.untitledNotebookFileName()}-${number}`);
+        const result = Uri.file(`${localize.DataScience.untitledNotebookFileName()}-${this.nextNumber}`);
+        this.nextNumber += 1;
+        return result;
     }
 
     private openNotebookAndCloseEditor = async (document: TextDocument, closeDocumentBeforeOpeningNotebook: boolean) => {
         // See if this is an ipynb file
         if (this.isNotebook(document) && this.configuration.getSettings().datascience.useNotebookEditor &&
             !this.activeEditors.has(document.uri.fsPath)) {
             try {
-                const contents = document.  getText();
+                const contents = document.getText();
                 const uri = document.uri;
                 const closeActiveEditorCommand = 'workbench.action.closeActiveEditor';
 
                 if (closeDocumentBeforeOpeningNotebook) {
-                    if (!this.documentManager.activeTextEditor || this.documentManager.activeTextEditor.document !== document){
+                    if (!this.documentManager.activeTextEditor || this.documentManager.activeTextEditor.document !== document) {
                         await this.documentManager.showTextDocument(document);
                     }
                     await this.cmdManager.executeCommand(closeActiveEditorCommand);
@@ -207,7 +219,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
                 // Open our own editor.
                 await this.open(uri, contents);
 
-                if (!closeDocumentBeforeOpeningNotebook){
+                if (!closeDocumentBeforeOpeningNotebook) {
                     // Then switch back to the ipynb and close it.
                     // If we don't do it in this order, the close will switch to the wrong item
                     await this.documentManager.showTextDocument(document);
@@ -226,14 +238,14 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
      * @param {TextEditor} editor
      * @memberof NativeEditorProvider
      */
-    private isEditorPartOfDiffView(editor?: TextEditor){
-        if (!editor){
+    private isEditorPartOfDiffView(editor?: TextEditor) {
+        if (!editor) {
             return false;
         }
         // There's no easy way to determine if the user is openeing a diff view.
         // One simple way is to check if there are 2 editor opened, and if both editors point to the same file
         // One file with the `file` scheme and the other with the `git` scheme.
-        if (this.documentManager.visibleTextEditors.length <= 1){
+        if (this.documentManager.visibleTextEditors.length <= 1) {
             return false;
         }
 
@@ -242,18 +254,18 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp
         // Possible we have a git diff view (with two editors git and file scheme), and we open the file view
         // on the side (different view column).
         const gitSchemeEditor = this.documentManager.visibleTextEditors.find(editorUri =>
-                                editorUri.document.uri.scheme === 'git' &&
-                                this.fileSystem.arePathsSame(editorUri.document.uri.fsPath, editor.document.uri.fsPath));
+            editorUri.document.uri.scheme === 'git' &&
+            this.fileSystem.arePathsSame(editorUri.document.uri.fsPath, editor.document.uri.fsPath));
 
-        if (!gitSchemeEditor){
+        if (!gitSchemeEditor) {
             return false;
         }
 
         const fileSchemeEditor = this.documentManager.visibleTextEditors.find(editorUri =>
-                                    editorUri.document.uri.scheme === 'file' &&
-                                    this.fileSystem.arePathsSame(editorUri.document.uri.fsPath, editor.document.uri.fsPath) &&
-                                    editorUri.viewColumn === gitSchemeEditor.viewColumn);
-        if (!fileSchemeEditor){
+            editorUri.document.uri.scheme === 'file' &&
+            this.fileSystem.arePathsSame(editorUri.document.uri.fsPath, editor.document.uri.fsPath) &&
+            editorUri.viewColumn === gitSchemeEditor.viewColumn);
+        if (!fileSchemeEditor) {
             return false;
         }
 

src/test/datascience/interactive-ipynb/nativeEditor.unit.test.ts

@@ -85,6 +85,7 @@ suite('Data Science - Native Editor', () => {
     let jupyterDebugger: IJupyterDebugger;
     let importer: INotebookImporter;
     let storage: MockMemento;
+    let localStorage: MockMemento;
     let storageUpdateSpy: sinon.SinonSpy<[string, any], Thenable<void>>;
     const baseFile = `{
  "cells": [
@@ -185,6 +186,7 @@ suite('Data Science - Native Editor', () => {
 
     setup(() => {
         storage = new MockMemento();
+        localStorage = new MockMemento();
         storageUpdateSpy = sinon.spy(storage, 'update');
         configService = mock(ConfigurationService);
         fileSystem = mock(FileSystem);
@@ -253,7 +255,8 @@ suite('Data Science - Native Editor', () => {
             instance(jupyterDebugger),
             instance(importer),
             instance(dsErrorHandler),
-            storage
+            storage,
+            localStorage
         );
     }
 
@@ -429,11 +432,11 @@ suite('Data Science - Native Editor', () => {
         expect(contents.metadata!.language_info!.version).to.not.equal('10.11.12');
 
         // When a cell is executed, then ensure we store the python version info in the notebook data.
-        const version: Version = {build: [], major: 10, minor: 11, patch: 12, prerelease: [], raw: '10.11.12'};
-        when(executionProvider.getUsableJupyterPython()).thenResolve(({version} as any));
+        const version: Version = { build: [], major: 10, minor: 11, patch: 12, prerelease: [], raw: '10.11.12' };
+        when(executionProvider.getUsableJupyterPython()).thenResolve(({ version } as any));
 
         try {
-            editor.onMessage(InteractiveWindowMessages.SubmitNewCell, {code: 'hello', id: '1'});
+            editor.onMessage(InteractiveWindowMessages.SubmitNewCell, { code: 'hello', id: '1' });
         } catch {
             // Ignore errors related to running cells, assume that works.
             noop();
@@ -453,4 +456,23 @@ suite('Data Science - Native Editor', () => {
         contents = JSON.parse(editor.contents) as nbformat.INotebookContent;
         expect(contents.metadata!.language_info!.version).to.equal('10.11.12');
     });
+
+    test('Opening file with local storage but no global will still open with old contents', async () => {
+        // This test is really for making sure when a user upgrades to a new extension, we still have their old storage
+        const file = Uri.parse('file://foo.ipynb');
+
+        // Initially nothing in memento
+        expect(storage.get(`notebook-storage-${file.toString()}`)).to.be.undefined;
+        expect(localStorage.get(`notebook-storage-${file.toString()}`)).to.be.undefined;
+
+        // Put the regular file into the local storage
+        localStorage.update(`notebook-storage-${file.toString()}`, baseFile);
+        const editor = createEditor();
+        await editor.load('', file);
+
+        // It should load with that value
+        expect(editor.contents).to.be.equal(baseFile);
+        expect(editor.cells).to.be.lengthOf(3);
+
+    });
 });

src/test/datascience/interactive-ipynb/nativeEditorProvider.unit.test.ts

@@ -34,6 +34,9 @@ suite('Data Science - Native Editor Provider', () => {
     let dsErrorHandler: IDataScienceErrorHandler;
     let cmdManager: ICommandManager;
     let svcContainer: IServiceContainer;
+    let eventEmitter: EventEmitter<TextEditor>;
+    let editor: typemoq.IMock<INotebookEditor>;
+    let file: Uri;
 
     setup(() => {
         svcContainer = mock(ServiceContainer);
@@ -43,9 +46,36 @@ suite('Data Science - Native Editor Provider', () => {
         dsErrorHandler = mock(DataScienceErrorHandler);
         cmdManager = mock(CommandManager);
         workspace = mock(WorkspaceService);
+        eventEmitter = new EventEmitter<TextEditor>();
     });
 
-    function createNotebookProvider() {
+    function createNotebookProvider(shouldOpenNotebookEditor: boolean) {
+        editor = typemoq.Mock.ofType<INotebookEditor>();
+        when(configService.getSettings()).thenReturn({ datascience: { useNotebookEditor: true } } as any);
+        when(doctManager.onDidChangeActiveTextEditor).thenReturn(eventEmitter.event);
+        when(doctManager.visibleTextEditors).thenReturn([]);
+        editor.setup(e => e.closed).returns(() => new EventEmitter<INotebookEditor>().event);
+        editor.setup(e => e.executed).returns(() => new EventEmitter<INotebookEditor>().event);
+        editor.setup(e => (e as any).then).returns(() => undefined);
+        when(svcContainer.get<INotebookEditor>(INotebookEditor)).thenReturn(editor.object);
+
+        // Ensure the editor is created and the load and show methods are invoked.
+        const invocationCount = shouldOpenNotebookEditor ? 1 : 0;
+        editor
+            .setup(e => e.load(typemoq.It.isAny(), typemoq.It.isAny()))
+            .returns((_a1: string, f: Uri) => {
+                file = f;
+                return Promise.resolve();
+            })
+            .verifiable(typemoq.Times.exactly(invocationCount));
+        editor
+            .setup(e => e.show())
+            .returns(() => Promise.resolve())
+            .verifiable(typemoq.Times.exactly(invocationCount));
+        editor
+            .setup(e => e.file)
+            .returns(() => file);
+
         return new NativeEditorProvider(
             instance(svcContainer),
             instance(mock(AsyncDisposableRegistry)),
@@ -71,28 +101,7 @@ suite('Data Science - Native Editor Provider', () => {
         return textEditor.object;
     }
     async function testAutomaticallyOpeningNotebookEditorWhenOpeningFiles(uri: Uri, shouldOpenNotebookEditor: boolean) {
-        const eventEmitter = new EventEmitter<TextEditor>();
-        const editor = typemoq.Mock.ofType<INotebookEditor>();
-        when(configService.getSettings()).thenReturn({ datascience: { useNotebookEditor: true } } as any);
-        when(doctManager.onDidChangeActiveTextEditor).thenReturn(eventEmitter.event);
-        when(doctManager.visibleTextEditors).thenReturn([]);
-        editor.setup(e => e.closed).returns(() => new EventEmitter<INotebookEditor>().event);
-        editor.setup(e => e.executed).returns(() => new EventEmitter<INotebookEditor>().event);
-        editor.setup(e => (e as any).then).returns(() => undefined);
-        when(svcContainer.get<INotebookEditor>(INotebookEditor)).thenReturn(editor.object);
-
-        // Ensure the editor is created and the load and show methods are invoked.
-        const invocationCount = shouldOpenNotebookEditor ? 1 : 0;
-        editor
-            .setup(e => e.load(typemoq.It.isAny(), typemoq.It.isAny()))
-            .returns(() => Promise.resolve())
-            .verifiable(typemoq.Times.exactly(invocationCount));
-        editor
-            .setup(e => e.show())
-            .returns(() => Promise.resolve())
-            .verifiable(typemoq.Times.exactly(invocationCount));
-
-        const notebookEditor = createNotebookProvider();
+        const notebookEditor = createNotebookProvider(shouldOpenNotebookEditor);
 
         // Open a text document.
         const textDoc = createTextDocument(uri, 'hello');
@@ -122,4 +131,11 @@ suite('Data Science - Native Editor Provider', () => {
     test('Do not open the notebook editor when an ipynb file is opened with a git scheme (comparing staged/modified files)', async () => {
         await testAutomaticallyOpeningNotebookEditorWhenOpeningFiles(Uri.parse('git://some//text file.txt'), false);
     });
+    test('Multiple new notebooks have new names', async () => {
+        const provider = createNotebookProvider(false);
+        const n1 = await provider.createNew();
+        expect(n1.file.fsPath).to.be.include('Untitled-1');
+        const n2 = await provider.createNew();
+        expect(n2.file.fsPath).to.be.include('Untitled-2');
+    });
 });