diff --git a/news/2 Fixes/7980.md b/news/2 Fixes/7980.md new file mode 100644 index 000000000000..d7c342489d5f --- /dev/null +++ b/news/2 Fixes/7980.md @@ -0,0 +1 @@ +When creating a new blank notebook, it has existing text in it already. diff --git a/news/2 Fixes/8132.md b/news/2 Fixes/8132.md new file mode 100644 index 000000000000..0cf1677adae0 --- /dev/null +++ b/news/2 Fixes/8132.md @@ -0,0 +1 @@ +Cannot create more than one blank notebook. diff --git a/news/2 Fixes/8263.md b/news/2 Fixes/8263.md new file mode 100644 index 000000000000..84e5069390b1 --- /dev/null +++ b/news/2 Fixes/8263.md @@ -0,0 +1 @@ +When updating the Python extension, unsaved changes to notebooks are lost. diff --git a/src/client/datascience/interactive-ipynb/nativeEditor.ts b/src/client/datascience/interactive-ipynb/nativeEditor.ts index a8cee683df7e..6fb62dd8fcb0 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditor.ts +++ b/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,7 +581,8 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { * @memberof NativeEditor */ private async getStoredContents(): Promise { - 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); @@ -588,7 +590,21 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { return; } } - return data ? data.contents : undefined; + if (data && !this.isUntitled && data.contents) { + return data.contents; + } + + const workspaceData = this.localStorage.get(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 { diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts index 7031c91578c0..547ab066a6e2 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts @@ -21,6 +21,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp private executedEditors: Set = new Set(); 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 { // 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,11 +191,13 @@ 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) => { @@ -193,12 +205,12 @@ export class NativeEditorProvider implements INotebookEditorProvider, IAsyncDisp 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; } diff --git a/src/test/datascience/interactive-ipynb/nativeEditor.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditor.unit.test.ts index 4753973f4f76..b875acbb65b3 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditor.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditor.unit.test.ts @@ -84,6 +84,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>; const baseFile = `{ "cells": [ @@ -184,6 +185,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,13 +255,14 @@ suite('Data Science - Native Editor', () => { instance(jupyterDebugger), instance(importer), instance(dsErrorHandler), - storage + storage, + localStorage ); } test('Create new editor and add some cells', async () => { const editor = createEditor(); - await editor.load(baseFile, Uri.parse('file://foo.ipynb')); + await editor.load(baseFile, Uri.parse('file:///foo.ipynb')); expect(editor.contents).to.be.equal(baseFile); editor.onMessage(InteractiveWindowMessages.InsertCell, { index: 0, cell: createEmptyCell('1', 1) }); expect(editor.cells).to.be.lengthOf(4); @@ -269,7 +272,7 @@ suite('Data Science - Native Editor', () => { test('Move cells around', async () => { const editor = createEditor(); - await editor.load(baseFile, Uri.parse('file://foo.ipynb')); + await editor.load(baseFile, Uri.parse('file:///foo.ipynb')); expect(editor.contents).to.be.equal(baseFile); editor.onMessage(InteractiveWindowMessages.SwapCells, { firstCellId: 'NotebookImport#0', secondCellId: 'NotebookImport#1' }); expect(editor.cells).to.be.lengthOf(3); @@ -279,7 +282,7 @@ suite('Data Science - Native Editor', () => { test('Edit/delete cells', async () => { const editor = createEditor(); - await editor.load(baseFile, Uri.parse('file://foo.ipynb')); + await editor.load(baseFile, Uri.parse('file:///foo.ipynb')); expect(editor.contents).to.be.equal(baseFile); expect(editor.isDirty).to.be.equal(false, 'Editor should not be dirty'); editor.onMessage(InteractiveWindowMessages.EditCell, { @@ -327,7 +330,7 @@ suite('Data Science - Native Editor', () => { return editor; } test('Editing a notebook will save uncommitted changes into memento', async () => { - const file = Uri.parse('file://foo.ipynb'); + const file = Uri.parse('file:///foo.ipynb'); // Initially nothing in memento expect(storage.get(`notebook-storage-${file.toString()}`)).to.be.undefined; @@ -336,7 +339,7 @@ suite('Data Science - Native Editor', () => { }); test('Opening a notebook will restore uncommitted changes', async () => { - const file = Uri.parse('file://foo.ipynb'); + const file = Uri.parse('file:///foo.ipynb'); when(fileSystem.stat(anything())).thenResolve({ mtime: 1 } as any); // Initially nothing in memento @@ -363,7 +366,7 @@ suite('Data Science - Native Editor', () => { }); test('Opening a notebook will restore uncommitted changes (ignoring contents of file)', async () => { - const file = Uri.parse('file://foo.ipynb'); + const file = Uri.parse('file:///foo.ipynb'); when(fileSystem.stat(anything())).thenResolve({ mtime: 1 } as any); // Initially nothing in memento @@ -391,7 +394,7 @@ suite('Data Science - Native Editor', () => { }); test('Opening a notebook will NOT restore uncommitted changes if file has been modified since', async () => { - const file = Uri.parse('file://foo.ipynb'); + const file = Uri.parse('file:///foo.ipynb'); when(fileSystem.stat(anything())).thenResolve({ mtime: 1 } as any); // Initially nothing in memento @@ -418,8 +421,8 @@ suite('Data Science - Native Editor', () => { expect(newEditor.cells).to.be.lengthOf(3); }); - test('Python version info will be updated in notebook when a cell has been executed', async () => { - const file = Uri.parse('file://foo.ipynb'); + test('Pyton version info will be updated in notebook when a cell has been executed', async () => { + const file = Uri.parse('file:///foo.ipynb'); const editor = createEditor(); await editor.load(baseFile, file); @@ -454,6 +457,24 @@ suite('Data Science - Native Editor', () => { 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); + }); + test('Export to Python script file from notebook.', async () => { // Temp file location needed for export const tempFile = { diff --git a/src/test/datascience/interactive-ipynb/nativeEditorProvider.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorProvider.unit.test.ts index 0bd37ec7514d..b8eef37d04e1 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorProvider.unit.test.ts +++ b/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; + let editor: typemoq.IMock; + 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(); }); - function createNotebookProvider() { + function createNotebookProvider(shouldOpenNotebookEditor: boolean) { + editor = typemoq.Mock.ofType(); + when(configService.getSettings()).thenReturn({ datascience: { useNotebookEditor: true } } as any); + when(docManager.onDidChangeActiveTextEditor).thenReturn(eventEmitter.event); + when(docManager.visibleTextEditors).thenReturn([]); + editor.setup(e => e.closed).returns(() => new EventEmitter().event); + editor.setup(e => e.executed).returns(() => new EventEmitter().event); + editor.setup(e => (e as any).then).returns(() => undefined); + when(svcContainer.get(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(); - const editor = typemoq.Mock.ofType(); - when(configService.getSettings()).thenReturn({ datascience: { useNotebookEditor: true } } as any); - when(docManager.onDidChangeActiveTextEditor).thenReturn(eventEmitter.event); - when(docManager.visibleTextEditors).thenReturn([]); - editor.setup(e => e.closed).returns(() => new EventEmitter().event); - editor.setup(e => e.executed).returns(() => new EventEmitter().event); - editor.setup(e => (e as any).then).returns(() => undefined); - when(svcContainer.get(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'); + }); });