diff --git a/extensions/notebook/src/common/constants.ts b/extensions/notebook/src/common/constants.ts index 2b49d0563b..9c5b6a2736 100644 --- a/extensions/notebook/src/common/constants.ts +++ b/extensions/notebook/src/common/constants.ts @@ -84,8 +84,6 @@ export const pythonWindowsInstallUrl = 'https://go.microsoft.com/fwlink/?linkid= export const pythonMacInstallUrl = 'https://go.microsoft.com/fwlink/?linkid=2163337'; export const pythonLinuxInstallUrl = 'https://go.microsoft.com/fwlink/?linkid=2163336'; -export const notebookLanguages = ['notebook', 'ipynb']; - export const KNOX_ENDPOINT_SERVER = 'host'; export const KNOX_ENDPOINT_PORT = 'knoxport'; export const KNOX_ENDPOINT_GATEWAY = 'gateway'; diff --git a/extensions/notebook/src/common/notebookUtils.ts b/extensions/notebook/src/common/notebookUtils.ts index 6dcfc67b59..05952cd906 100644 --- a/extensions/notebook/src/common/notebookUtils.ts +++ b/extensions/notebook/src/common/notebookUtils.ts @@ -7,7 +7,7 @@ import * as azdata from 'azdata'; import * as os from 'os'; import * as vscode from 'vscode'; import * as nls from 'vscode-nls'; -import { getErrorMessage, isEditorTitleFree } from '../common/utils'; +import { getErrorMessage } from '../common/utils'; const localize = nls.loadMessageBundle(); @@ -20,21 +20,7 @@ export class NotebookUtils { constructor() { } public async newNotebook(options?: azdata.nb.NotebookShowOptions): Promise { - const title = this.findNextUntitledEditorName(); - const untitledUri = vscode.Uri.parse(`untitled:${title}`); - return azdata.nb.showNotebookDocument(untitledUri, options); - } - - private findNextUntitledEditorName(): string { - let nextVal = 0; - // Note: this will go forever if it's coded wrong, or you have infinite Untitled notebooks! - while (true) { - let title = `Notebook-${nextVal}`; - if (isEditorTitleFree(title)) { - return title; - } - nextVal++; - } + return azdata.nb.showNotebookDocument(vscode.Uri.from({ scheme: 'untitled' }), options); } public async openNotebook(): Promise { @@ -116,12 +102,7 @@ export class NotebookUtils { } public async analyzeNotebook(oeContext?: azdata.ObjectExplorerContext): Promise { - // Ensure we get a unique ID for the notebook. For now we're using a different prefix to the built-in untitled files - // to handle this. We should look into improving this in the future - let title = this.findNextUntitledEditorName(); - let untitledUri = vscode.Uri.parse(`untitled:${title}`); - - let editor = await azdata.nb.showNotebookDocument(untitledUri, { + let editor = await azdata.nb.showNotebookDocument(vscode.Uri.from({ scheme: 'untitled' }), { connectionProfile: oeContext ? oeContext.connectionProfile : undefined, providerId: JUPYTER_NOTEBOOK_PROVIDER, preview: false, diff --git a/extensions/notebook/src/common/utils.ts b/extensions/notebook/src/common/utils.ts index c75712f963..1f5015f597 100644 --- a/extensions/notebook/src/common/utils.ts +++ b/extensions/notebook/src/common/utils.ts @@ -10,7 +10,7 @@ import * as nls from 'vscode-nls'; import * as vscode from 'vscode'; import * as azdata from 'azdata'; import * as crypto from 'crypto'; -import { notebookLanguages, notebookConfigKey, pinnedBooksConfigKey, AUTHTYPE, INTEGRATED_AUTH, KNOX_ENDPOINT_PORT, KNOX_ENDPOINT_SERVER } from './constants'; +import { notebookConfigKey, pinnedBooksConfigKey, AUTHTYPE, INTEGRATED_AUTH, KNOX_ENDPOINT_PORT, KNOX_ENDPOINT_SERVER } from './constants'; import { IPrompter, IQuestion, QuestionTypes } from '../prompts/question'; import { BookTreeItemFormat } from '../book/bookTreeItem'; import * as loc from './localizedConstants'; @@ -268,13 +268,6 @@ export function isPackageSupported(pythonVersion: string, packageVersionConstrai return supportedVersionFound; } -export function isEditorTitleFree(title: string): boolean { - - let hasTextDoc = vscode.workspace.textDocuments.findIndex(doc => doc.isUntitled && doc.fileName === title && !notebookLanguages.find(lang => lang === doc.languageId)) > -1; - let hasNotebookDoc = azdata.nb.notebookDocuments.findIndex(doc => doc.isUntitled && doc.fileName === title) > -1; - return !hasTextDoc && !hasNotebookDoc; -} - export function getClusterEndpoints(serverInfo: azdata.ServerInfo): bdc.IEndpointModel[] { let endpoints: RawEndpoint[] = serverInfo.options['clusterEndpoints']; if (!endpoints || endpoints.length === 0) { return []; } diff --git a/extensions/notebook/src/jupyter/jupyterController.ts b/extensions/notebook/src/jupyter/jupyterController.ts index a809733de8..5995421c82 100644 --- a/extensions/notebook/src/jupyter/jupyterController.ts +++ b/extensions/notebook/src/jupyter/jupyterController.ts @@ -29,8 +29,6 @@ import { ManagePackagesDialogModel, ManagePackageDialogOptions } from '../dialog import { PyPiClient } from './pypiClient'; import { JupyterExecuteProvider } from './jupyterExecuteProvider'; -let untitledCounter = 0; - export class JupyterController { private _jupyterInstallation: JupyterServerInstallation; private _serverInstanceFactory: ServerInstanceFactory = new ServerInstanceFactory(); @@ -133,10 +131,7 @@ export class JupyterController { } private async handleNewNotebookTask(oeContext?: azdata.ObjectExplorerContext, profile?: azdata.IConnectionProfile): Promise { - // Ensure we get a unique ID for the notebook. For now we're using a different prefix to the built-in untitled files - // to handle this. We should look into improving this in the future - let untitledUri = vscode.Uri.parse(`untitled:Notebook-${untitledCounter++}`); - let editor = await azdata.nb.showNotebookDocument(untitledUri, { + let editor = await azdata.nb.showNotebookDocument(vscode.Uri.from({ scheme: 'untitled' }), { connectionProfile: profile, providerId: constants.jupyterNotebookProviderId, preview: false, diff --git a/extensions/notebook/src/protocol/notebookUriHandler.ts b/extensions/notebook/src/protocol/notebookUriHandler.ts index f75d0e896f..e0058ac6c7 100644 --- a/extensions/notebook/src/protocol/notebookUriHandler.ts +++ b/extensions/notebook/src/protocol/notebookUriHandler.ts @@ -12,7 +12,7 @@ import * as nls from 'vscode-nls'; const localize = nls.loadMessageBundle(); import { IQuestion, QuestionTypes } from '../prompts/question'; import CodeAdapter from '../prompts/adapter'; -import { getErrorMessage, isEditorTitleFree } from '../common/utils'; +import { getErrorMessage } from '../common/utils'; import * as constants from '../common/constants'; import { readJson } from 'fs-extra'; @@ -101,14 +101,25 @@ export class NotebookUriHandler implements vscode.UriHandler { } contents = await this.download(url); } - let untitledUriPath = this.getUntitledUriPath(path.basename(uri.fsPath)); - let untitledUri = uri.with({ authority: '', scheme: 'untitled', path: untitledUriPath }); + let untitledUri = uri.with({ authority: '', scheme: 'untitled', path: path.basename(uri.fsPath) }); if (path.extname(uri.fsPath) === '.ipynb') { await azdata.nb.showNotebookDocument(untitledUri, { initialContent: contents, preserveFocus: true }); } else { + // Append a numbered suffix to the path if an untitled text document already has the same title. + // The UntitledTextEditorService won't create automatically incremented titles if we provide a filePath like here. + // Duplicates should be formatted as 'Readme-1.txt', not 'Readme.txt-1' + let updatedPath: string; + let fileExt = path.extname(untitledUri.fsPath); + let baseFileName = untitledUri.fsPath.slice(0, untitledUri.fsPath.length - fileExt.length); + for (let titleCounter = 1; vscode.workspace.textDocuments.some(doc => doc.isUntitled && doc.fileName === untitledUri.fsPath); titleCounter++) { + updatedPath = `${baseFileName}-${titleCounter}${fileExt}`; + } + if (updatedPath) { + untitledUri = untitledUri.with({ path: updatedPath }); + } let doc = await vscode.workspace.openTextDocument(untitledUri); let editor = await vscode.window.showTextDocument(doc, vscode.ViewColumn.Active, true); await editor.edit(builder => { @@ -143,21 +154,4 @@ export class NotebookUriHandler implements vscode.UriHandler { }); }); } - - private getUntitledUriPath(originalTitle: string): string { - let title = originalTitle; - let nextVal = 0; - let ext = path.extname(title); - while (!isEditorTitleFree(title)) { - if (ext) { - // Need it to be `Readme-0.txt` not `Readme.txt-0` - let titleStart = originalTitle.slice(0, originalTitle.length - ext.length); - title = `${titleStart}-${nextVal}${ext}`; - } else { - title = `${originalTitle}-${nextVal}`; - } - nextVal++; - } - return title; - } } diff --git a/extensions/notebook/src/test/common/notebookUtils.test.ts b/extensions/notebook/src/test/common/notebookUtils.test.ts index 2434225e56..74fcb734db 100644 --- a/extensions/notebook/src/test/common/notebookUtils.test.ts +++ b/extensions/notebook/src/test/common/notebookUtils.test.ts @@ -50,12 +50,12 @@ describe('notebookUtils Tests', function (): void { should(azdata.nb.notebookDocuments.length).equal(0, 'There should be not any open Notebook documents'); await notebookUtils.newNotebook(undefined); should(azdata.nb.notebookDocuments.length).equal(1, 'There should be exactly 1 open Notebook document'); - should(azdata.nb.notebookDocuments[0].fileName).equal('Notebook-0', 'The first Untitled Notebook should have an index of 0'); + should(azdata.nb.notebookDocuments[0].fileName).equal('Notebook-1', 'The first Untitled Notebook should have an index of 1'); await vscode.commands.executeCommand('workbench.action.closeActiveEditor'); should(azdata.nb.notebookDocuments.length).equal(0, 'There should be not any open Notebook documents'); await notebookUtils.newNotebook(undefined); should(azdata.nb.notebookDocuments.length).equal(1, 'There should be exactly 1 open Notebook document after second opening'); - should(azdata.nb.notebookDocuments[0].fileName).equal('Notebook-0', 'The first Untitled Notebook should have an index of 0 after closing first Untitled Notebook'); + should(azdata.nb.notebookDocuments[0].fileName).equal('Notebook-1', 'The first Untitled Notebook should have an index of 1 after closing first Untitled Notebook'); await vscode.commands.executeCommand('workbench.action.closeActiveEditor'); }); @@ -65,7 +65,7 @@ describe('notebookUtils Tests', function (): void { should(azdata.nb.notebookDocuments.length).equal(1, 'There should be exactly 1 open Notebook document'); const secondNotebook = await notebookUtils.newNotebook(undefined); should(azdata.nb.notebookDocuments.length).equal(2, 'There should be exactly 2 open Notebook documents'); - should(secondNotebook.document.fileName).equal('Notebook-1', 'The second Untitled Notebook should have an index of 1'); + should(secondNotebook.document.fileName).equal('Notebook-2', 'The second Untitled Notebook should have an index of 2'); await vscode.commands.executeCommand('workbench.action.closeActiveEditor'); await vscode.commands.executeCommand('workbench.action.closeActiveEditor'); should(azdata.nb.notebookDocuments.length).equal(0, 'There should be not any open Notebook documents'); diff --git a/extensions/notebook/src/test/common/utils.test.ts b/extensions/notebook/src/test/common/utils.test.ts index 1c7e938773..9dea386a72 100644 --- a/extensions/notebook/src/test/common/utils.test.ts +++ b/extensions/notebook/src/test/common/utils.test.ts @@ -10,7 +10,6 @@ import * as os from 'os'; import * as path from 'path'; import * as utils from '../../common/utils'; import { MockOutputChannel } from './stubs'; -import * as vscode from 'vscode'; import * as azdata from 'azdata'; import { sleep } from './testUtils'; @@ -274,37 +273,6 @@ describe('Utils Tests', function () { }); }); - describe('isEditorTitleFree', () => { - afterEach(async () => { - await vscode.commands.executeCommand('workbench.action.closeActiveEditor'); - }); - - it('title is free', () => { - should(utils.isEditorTitleFree('MyTitle')).be.true(); - }); - - it('title is not free with text document sharing name', async () => { - const editorTitle = 'Untitled-1'; - should(utils.isEditorTitleFree(editorTitle)).be.true('Title should be free before opening text document'); - await vscode.workspace.openTextDocument(); - should(utils.isEditorTitleFree(editorTitle)).be.false('Title should not be free after opening text document'); - }); - - it('title is not free with notebook document sharing name', async () => { - const editorTitle = 'MyUntitledNotebook'; - should(utils.isEditorTitleFree(editorTitle)).be.true('Title should be free before opening notebook'); - await azdata.nb.showNotebookDocument(vscode.Uri.parse(`untitled:${editorTitle}`)); - should(utils.isEditorTitleFree('MyUntitledNotebook')).be.false('Title should not be free after opening notebook'); - }); - - it('title is not free with notebook document sharing name created through command', async () => { - const editorTitle = 'Notebook-0'; - should(utils.isEditorTitleFree(editorTitle)).be.true('Title should be free before opening notebook'); - await vscode.commands.executeCommand('_notebook.command.new'); - should(utils.isEditorTitleFree(editorTitle)).be.false('Title should not be free after opening notebook'); - }); - }); - describe('getClusterEndpoints', () => { const baseServerInfo: azdata.ServerInfo = { serverMajorVersion: -1, diff --git a/src/sql/workbench/api/browser/mainThreadNotebookDocumentsAndEditors.ts b/src/sql/workbench/api/browser/mainThreadNotebookDocumentsAndEditors.ts index 22efe6a888..8260e91d3f 100644 --- a/src/sql/workbench/api/browser/mainThreadNotebookDocumentsAndEditors.ts +++ b/src/sql/workbench/api/browser/mainThreadNotebookDocumentsAndEditors.ts @@ -28,6 +28,7 @@ import { ITextFileService } from 'vs/workbench/services/textfile/common/textfile import { NotebookEditor } from 'sql/workbench/contrib/notebook/browser/notebookEditor'; import { extHostNamedCustomer, IExtHostContext } from 'vs/workbench/services/extensions/common/extHostCustomers'; import { SqlExtHostContext, SqlMainContext } from 'vs/workbench/api/common/extHost.protocol'; +import { IUntitledTextEditorService } from 'vs/workbench/services/untitled/common/untitledTextEditorService'; class MainThreadNotebookEditor extends Disposable { private _contentChangedEmitter = new Emitter(); @@ -321,7 +322,8 @@ export class MainThreadNotebookDocumentsAndEditors extends Disposable implements @IInstantiationService private _instantiationService: IInstantiationService, @INotebookService private readonly _notebookService: INotebookService, @IFileService private readonly _fileService: IFileService, - @ITextFileService private readonly _textFileService: ITextFileService + @ITextFileService private readonly _textFileService: ITextFileService, + @IUntitledTextEditorService private readonly _untitledEditorService: IUntitledTextEditorService, ) { super(); if (extHostContext) { @@ -343,15 +345,47 @@ export class MainThreadNotebookDocumentsAndEditors extends Disposable implements } } + /** + * Attempts to create a new notebook with the specified provider and contents. Used for VS Code extension compatibility. + */ async $tryCreateNotebookDocument(providerId: string, contents?: azdata.nb.INotebookContents): Promise { let input = await this._notebookService.createNotebookInputFromContents(providerId, contents); return input.resource; } $tryShowNotebookDocument(resource: UriComponents, options: INotebookShowOptions): Promise { + // Append a numbered suffix if an untitled notebook is already open with the same path + if (resource.scheme === Schemas.untitled) { + if (!resource.path || this.untitledEditorTitleExists(resource.path)) { + resource.path = this.createPrefixedNotebookFilePath(resource.path); + } + } return Promise.resolve(this.doOpenEditor(resource, options)); } + private untitledEditorTitleExists(filePath: string): boolean { + return !!this._untitledEditorService.get(URI.from({ scheme: Schemas.untitled, path: filePath })); + } + + private createPrefixedNotebookFilePath(prefix?: string): string { + if (!prefix) { + prefix = 'Notebook'; + } + let prefixFileName = (counter: number): string => { + return `${prefix}-${counter}`; + }; + + let counter = 1; + // Get document name and check if it exists + let filePath = prefixFileName(counter); + while (this.untitledEditorTitleExists(filePath)) { + counter++; + filePath = prefixFileName(counter); + } + + return filePath; + } + $trySetTrusted(uriComponent: UriComponents, isTrusted: boolean): Promise { let uri = URI.revive(uriComponent); return this._notebookService.setTrusted(uri, isTrusted); diff --git a/src/sql/workbench/api/common/extHostNotebookDocumentsAndEditors.ts b/src/sql/workbench/api/common/extHostNotebookDocumentsAndEditors.ts index 785a6f50ac..874c8c3060 100644 --- a/src/sql/workbench/api/common/extHostNotebookDocumentsAndEditors.ts +++ b/src/sql/workbench/api/common/extHostNotebookDocumentsAndEditors.ts @@ -218,6 +218,9 @@ export class ExtHostNotebookDocumentsAndEditors implements ExtHostNotebookDocume //#endregion //#region Extension accessible methods + /** + * Attempts to create a new notebook with the specified provider and contents. Used for VS Code extension compatibility. + */ async createNotebookDocument(providerId: string, contents?: azdata.nb.INotebookContents): Promise { let uriComps = await this._proxy.$tryCreateNotebookDocument(providerId, contents); let uri = URI.revive(uriComps); @@ -241,6 +244,9 @@ export class ExtHostNotebookDocumentsAndEditors implements ExtHostNotebookDocume return uri; } + /** + * Attempts to open an existing notebook. Used for VS Code extension compatibility. + */ async openNotebookDocument(uri: vscode.Uri): Promise { let docData = this._documents.get(uri.toString()); if (!docData) {