From bb5e0ad9d943a14f5fc102b8694a86d215564799 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 27 May 2020 15:46:10 -0700 Subject: [PATCH 01/19] Turn on custom editor again --- package.json | 2 +- .../common/application/customEditorService.ts | 10 +- src/client/common/application/types.ts | 360 ++++++++++-------- .../interactive-ipynb/nativeEditorProvider.ts | 76 ++-- .../nativeEditorProviderOld.ts | 10 +- .../interactive-ipynb/nativeEditorStorage.ts | 9 + .../nativeEditorViewTracker.ts | 26 +- .../notebookModelEditEvent.ts | 19 + .../notebookStorageProvider.ts | 6 + src/client/datascience/serviceRegistry.ts | 10 +- src/client/datascience/types.ts | 2 + .../nativeEditorProvider.functional.test.ts | 2 +- .../datascience/mockCustomEditorService.ts | 38 +- .../nativeEditorViewTracker.unit.test.ts | 2 +- 14 files changed, 350 insertions(+), 222 deletions(-) create mode 100644 src/client/datascience/interactive-ipynb/notebookModelEditEvent.ts diff --git a/package.json b/package.json index 8285e8f5ae92..7b433bd83f02 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,7 @@ }, "languageServerVersion": "0.5.30", "publisher": "ms-python", - "enableProposedApi": false, + "enableProposedApi": true, "author": { "name": "Microsoft Corporation" }, diff --git a/src/client/common/application/customEditorService.ts b/src/client/common/application/customEditorService.ts index 57c968335961..626318f034e8 100644 --- a/src/client/common/application/customEditorService.ts +++ b/src/client/common/application/customEditorService.ts @@ -15,14 +15,18 @@ export class CustomEditorService implements ICustomEditorService { @inject(UseCustomEditorApi) private readonly useCustomEditorApi: boolean ) {} - public registerCustomEditorProvider( + // 2 should be temporary + public registerCustomEditorProvider2( viewType: string, provider: CustomEditorProvider, - options?: vscode.WebviewPanelOptions + options?: { + readonly webviewOptions?: vscode.WebviewPanelOptions; + readonly supportsMultipleEditorsPerDocument?: boolean; + } ): vscode.Disposable { if (this.useCustomEditorApi) { // tslint:disable-next-line: no-any - return (vscode.window as any).registerCustomEditorProvider(viewType, provider, options); + return (vscode.window as any).registerCustomEditorProvider2(viewType, provider, options); } else { return { dispose: noop }; } diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index b7ea624119f3..6a035291f10f 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -1181,242 +1181,286 @@ export interface IActiveResourceService { } // Temporary hack to get the nyc compiler to find these types. vscode.proposed.d.ts doesn't work for some reason. -//#region Custom editors: https://github.com/microsoft/vscode/issues/77131 // tslint:disable: interface-name +//#region Custom editor https://github.com/microsoft/vscode/issues/77131 + /** - * Defines the editing capability of a custom webview editor. This allows the webview editor to hook into standard - * editor events such as `undo` or `save`. + * Represents a custom document used by a [`CustomEditorProvider`](#CustomEditorProvider). * - * @param EditType Type of edits. + * Custom documents are only used within a given `CustomEditorProvider`. The lifecycle of a `CustomDocument` is + * managed by VS Code. When no more references remain to a `CustomDocument`, it is disposed of. */ -export interface CustomEditorEditingDelegate { - /** - * Event triggered by extensions to signal to VS Code that an edit has occurred. - */ - readonly onDidEdit: Event>; +export interface CustomDocument { /** - * Save the resource. - * - * @param document Document to save. - * @param cancellation Token that signals the save is no longer required (for example, if another save was triggered). - * - * @return Thenable signaling that the save has completed. + * The associated uri for this document. */ - save(document: CustomDocument, cancellation: CancellationToken): Thenable; + readonly uri: Uri; /** - * Save the existing resource at a new path. - * - * @param document Document to save. - * @param targetResource Location to save to. + * Dispose of the custom document. * - * @return Thenable signaling that the save has completed. + * This is invoked by VS Code when there are no more references to a given `CustomDocument` (for example when + * all editors associated with the document have been closed.) */ - saveAs(document: CustomDocument, targetResource: Uri): Thenable; + dispose(): void; +} +/** + * Event triggered by extensions to signal to VS Code that an edit has occurred on an [`CustomDocument`](#CustomDocument). + * + * @see [`CustomDocumentProvider.onDidChangeCustomDocument`](#CustomDocumentProvider.onDidChangeCustomDocument). + */ +export interface CustomDocumentEditEvent { /** - * Apply a set of edits. - * - * Note that is not invoked when `onDidEdit` is called because `onDidEdit` implies also updating the view to reflect the edit. - * - * @param document Document to apply edits to. - * @param edit Array of edits. Sorted from oldest to most recent. - * - * @return Thenable signaling that the change has completed. + * The document that the edit is for. */ - applyEdits(document: CustomDocument, edits: readonly EditType[]): Thenable; + readonly document: T; /** - * Undo a set of edits. - * - * This is triggered when a user undoes an edit. - * - * @param document Document to undo edits from. - * @param edit Array of edits. Sorted from most recent to oldest. + * Display name describing the edit. * - * @return Thenable signaling that the change has completed. + * This is shown in the UI to users. */ - undoEdits(document: CustomDocument, edits: readonly EditType[]): Thenable; + readonly label?: string; /** - * Revert the file to its last saved state. + * Undo the edit operation. * - * @param document Document to revert. - * @param edits Added or applied edits. - * - * @return Thenable signaling that the change has completed. + * This is invoked by VS Code when the user undoes this edit. To implement `undo`, your + * extension should restore the document and editor to the state they were in just before this + * edit was added to VS Code's internal edit stack by `onDidChangeCustomDocument`. */ - revert(document: CustomDocument, edits: CustomDocumentRevert): Thenable; + undo(): Thenable | void; /** - * Back up the resource in its current state. + * Redo the edit operation. * - * Backups are used for hot exit and to prevent data loss. Your `backup` method should persist the resource in - * its current state, i.e. with the edits applied. Most commonly this means saving the resource to disk in - * the `ExtensionContext.storagePath`. When VS Code reloads and your custom editor is opened for a resource, - * your extension should first check to see if any backups exist for the resource. If there is a backup, your - * extension should load the file contents from there instead of from the resource in the workspace. - * - * `backup` is triggered whenever an edit it made. Calls to `backup` are debounced so that if multiple edits are - * made in quick succession, `backup` is only triggered after the last one. `backup` is not invoked when - * `auto save` is enabled (since auto save already persists resource ). - * - * @param document Document to revert. - * @param cancellation Token that signals the current backup since a new backup is coming in. It is up to your - * extension to decided how to respond to cancellation. If for example your extension is backing up a large file - * in an operation that takes time to complete, your extension may decide to finish the ongoing backup rather - * than cancelling it to ensure that VS Code has some valid backup. + * This is invoked by VS Code when the user redoes this edit. To implement `redo`, your + * extension should restore the document and editor to the state they were in just after this + * edit was added to VS Code's internal edit stack by `onDidChangeCustomDocument`. */ - backup(document: CustomDocument, cancellation: CancellationToken): Thenable; + redo(): Thenable | void; } /** - * Event triggered by extensions to signal to VS Code that an edit has occurred on a CustomDocument``. + * Event triggered by extensions to signal to VS Code that the content of a [`CustomDocument`](#CustomDocument) + * has changed. + * + * @see [`CustomDocumentProvider.onDidChangeCustomDocument`](#CustomDocumentProvider.onDidChangeCustomDocument). */ -export interface CustomDocumentEditEvent { +export interface CustomDocumentContentChangeEvent { /** - * Document the edit is for. + * The document that the change is for. */ - readonly document: CustomDocument; + readonly document: T; +} +/** + * A backup for an [`CustomDocument`](#CustomDocument). + */ +export interface CustomDocumentBackup { /** - * Object that describes the edit. + * Unique identifier for the backup. * - * Edit objects are passed back to your extension in `undoEdits`, `applyEdits`, and `revert`. + * This id is passed back to your extension in `openCustomDocument` when opening a custom editor from a backup. */ - readonly edit: EditType; + readonly id: string; /** - * Display name describing the edit. + * Delete the current backup. + * + * This is called by VS Code when it is clear the current backup is no longer needed, such as when a new backup + * is made or when the file is saved. */ - readonly label?: string; + delete(): void; } /** - * Data about a revert for a `CustomDocument`. + * Additional information used to implement [`CustomEditableDocument.backup`](#CustomEditableDocument.backup). */ -export interface CustomDocumentRevert { +export interface CustomDocumentBackupContext { /** - * List of edits that were undone to get the document back to its on disk state. + * Suggested file location to write the new backup. + * + * Note that your extension is free to ignore this and use its own strategy for backup. + * + * For editors for workspace resource, this destination will be in the workspace storage. The path may not */ - readonly undoneEdits: readonly EditType[]; + readonly destination: Uri; +} +/** + * Additional information about the opening custom document. + */ +export interface CustomDocumentOpenContext { /** - * List of edits that were reapplied to get the document back to its on disk state. + * The id of the backup to restore the document from or `undefined` if there is no backup. + * + * If this is provided, your extension should restore the editor from the backup instead of reading the file + * the user's workspace. */ - readonly appliedEdits: readonly EditType[]; + readonly backupId?: string; } /** - * Represents a custom document used by a `CustomEditorProvider`. + * Provider for readonly custom editors that use a custom document model. + * + * Custom editors use [`CustomDocument`](#CustomDocument) as their document model instead of a [`TextDocument`](#TextDocument). * - * Custom documents are only used within a given `CustomEditorProvider`. The lifecycle of a - * `CustomDocument` is managed by VS Code. When no more references remain to a given `CustomDocument`, - * then it is disposed of. + * You should use this type of custom editor when dealing with binary files or more complex scenarios. For simple + * text based documents, use [`CustomTextEditorProvider`](#CustomTextEditorProvider) instead. * - * @param UserDataType Type of custom object that extensions can store on the document. + * @param T Type of the custom document returned by this provider. */ -export interface CustomDocument { +export interface CustomReadonlyEditorProvider { /** - * The associated viewType for this document. - */ - readonly viewType: string; - - /** - * The associated uri for this document. - */ - readonly uri: Uri; - - /** - * Event fired when there are no more references to the `CustomDocument`. + * Create a new document for a given resource. + * + * `openCustomDocument` is called when the first editor for a given resource is opened, and the resolve document + * is passed to `resolveCustomEditor`. The resolved `CustomDocument` is re-used for subsequent editor opens. + * If all editors for a given resource are closed, the `CustomDocument` is disposed of. Opening an editor at + * this point will trigger another call to `openCustomDocument`. + * + * @param uri Uri of the document to open. + * @param openContext Additional information about the opening custom document. + * @param token A cancellation token that indicates the result is no longer needed. + * + * @return The custom document. */ - readonly onDidDispose: Event; + openCustomDocument(uri: Uri, openContext: CustomDocumentOpenContext, token: CancellationToken): Thenable | T; /** - * Custom data that an extension can store on the document. + * Resolve a custom editor for a given resource. + * + * This is called whenever the user opens a new editor for this `CustomEditorProvider`. + * + * To resolve a custom editor, the provider must fill in its initial html content and hook up all + * the event listeners it is interested it. The provider can also hold onto the `WebviewPanel` to use later, + * for example in a command. See [`WebviewPanel`](#WebviewPanel) for additional details. + * + * @param document Document for the resource being resolved. + * @param webviewPanel Webview to resolve. + * @param token A cancellation token that indicates the result is no longer needed. + * + * @return Optional thenable indicating that the custom editor has been resolved. */ - userData?: UserDataType; + resolveCustomEditor(document: T, webviewPanel: WebviewPanel, token: CancellationToken): Thenable | void; } /** - * Provider for webview editors that use a custom data model. + * Provider for editiable custom editors that use a custom document model. * - * Custom webview editors use [`CustomDocument`](#CustomDocument) as their data model. + * Custom editors use [`CustomDocument`](#CustomDocument) as their document model instead of a [`TextDocument`](#TextDocument). * This gives extensions full control over actions such as edit, save, and backup. * - * You should use custom text based editors when dealing with binary files or more complex scenarios. For simple text - * based documents, use [`WebviewTextEditorProvider`](#WebviewTextEditorProvider) instead. + * You should use this type of custom editor when dealing with binary files or more complex scenarios. For simple + * text based documents, use [`CustomTextEditorProvider`](#CustomTextEditorProvider) instead. + * + * @param T Type of the custom document returned by this provider. */ -export interface CustomEditorProvider { +export interface CustomEditorProvider + extends CustomReadonlyEditorProvider { /** - * Defines the editing capability of a custom webview document. + * Signal that an edit has occurred inside a custom editor. + * + * This event must be fired by your extension whenever an edit happens in a custom editor. An edit can be + * anything from changing some text, to cropping an image, to reordering a list. Your extension is free to + * define what an edit is and what data is stored on each edit. + * + * Firing `onDidChange` causes VS Code to mark the editors as being dirty. This is cleared when the user either + * saves or reverts the file. * - * When not provided, the document is considered readonly. + * Editors that support undo/redo must fire a `CustomDocumentEditEvent` whenever an edit happens. This allows + * users to undo and redo the edit using VS Code's standard VS Code keyboard shortcuts. VS Code will also mark + * the editor as no longer being dirty if the user undoes all edits to the last saved state. + * + * Editors that support editing but cannot use VS Code's standard undo/redo mechanism must fire a `CustomDocumentContentChangeEvent`. + * The only way for a user to clear the dirty state of an editor that does not support undo/redo is to either + * `save` or `revert` the file. + * + * An editor should only ever fire `CustomDocumentEditEvent` events, or only ever fire `CustomDocumentContentChangeEvent` events. */ - readonly editingDelegate?: CustomEditorEditingDelegate; + readonly onDidChangeCustomDocument: Event> | Event>; + /** - * Resolve the model for a given resource. + * Save a custom document. * - * `resolveCustomDocument` is called when the first editor for a given resource is opened, and the resolve document - * is passed to `resolveCustomEditor`. The resolved `CustomDocument` is re-used for subsequent editor opens. - * If all editors for a given resource are closed, the `CustomDocument` is disposed of. Opening an editor at - * this point will trigger another call to `resolveCustomDocument`. + * This method is invoked by VS Code when the user saves a custom editor. This can happen when the user + * triggers save while the custom editor is active, by commands such as `save all`, or by auto save if enabled. + * + * To implement `save`, the implementer must persist the custom editor. This usually means writing the + * file data for the custom document to disk. After `save` completes, any associated editor instances will + * no longer be marked as dirty. * - * @param document Document to resolve. + * @param document Document to save. + * @param cancellation Token that signals the save is no longer required (for example, if another save was triggered). * - * @return The capabilities of the resolved document. + * @return Thenable signaling that saving has completed. */ - resolveCustomDocument(document: CustomDocument): Thenable; + saveCustomDocument(document: T, cancellation: CancellationToken): Thenable; /** - * Resolve a webview editor for a given resource. + * Save a custom document to a different location. * - * This is called when a user first opens a resource for a `CustomTextEditorProvider`, or if they reopen an - * existing editor using this `CustomTextEditorProvider`. + * This method is invoked by VS Code when the user triggers 'save as' on a custom editor. The implementer must + * persist the custom editor to `destination`. * - * To resolve a webview editor, the provider must fill in its initial html content and hook up all - * the event listeners it is interested it. The provider can also hold onto the `WebviewPanel` to use later, - * for example in a command. See [`WebviewPanel`](#WebviewPanel) for additional details + * When the user accepts save as, the current editor is be replaced by an non-dirty editor for the newly saved file. * - * @param document Document for the resource being resolved. - * @param webviewPanel Webview to resolve. + * @param document Document to save. + * @param destination Location to save to. + * @param cancellation Token that signals the save is no longer required. * - * @return Thenable indicating that the webview editor has been resolved. + * @return Thenable signaling that saving has completed. */ - resolveCustomEditor(document: CustomDocument, webviewPanel: WebviewPanel): Thenable; -} + saveCustomDocumentAs(document: T, destination: Uri, cancellation: CancellationToken): Thenable; -/** - * Provider for text based webview editors. - * - * Text based webview editors use a [`TextDocument`](#TextDocument) as their data model. This considerably simplifies - * implementing a webview editor as it allows VS Code to handle many common operations such as - * undo and backup. The provider is responsible for synchronizing text changes between the webview and the `TextDocument`. - * - * You should use text based webview editors when dealing with text based file formats, such as `xml` or `json`. - * For binary files or more specialized use cases, see [CustomEditorProvider](#CustomEditorProvider). - */ -export interface CustomTextEditorProvider { /** - * Resolve a webview editor for a given text resource. + * Revert a custom document to its last saved state. * - * This is called when a user first opens a resource for a `CustomTextEditorProvider`, or if they reopen an - * existing editor using this `CustomTextEditorProvider`. + * This method is invoked by VS Code when the user triggers `File: Revert File` in a custom editor. (Note that + * this is only used using VS Code's `File: Revert File` command and not on a `git revert` of the file). * - * To resolve a webview editor, the provider must fill in its initial html content and hook up all - * the event listeners it is interested it. The provider can also hold onto the `WebviewPanel` to use later, - * for example in a command. See [`WebviewPanel`](#WebviewPanel) for additional details. + * To implement `revert`, the implementer must make sure all editor instances (webviews) for `document` + * are displaying the document in the same state is saved in. This usually means reloading the file from the + * workspace. * - * @param document Document for the resource to resolve. - * @param webviewPanel Webview to resolve. + * @param document Document to revert. + * @param cancellation Token that signals the revert is no longer required. + * + * @return Thenable signaling that the change has completed. + */ + revertCustomDocument(document: T, cancellation: CancellationToken): Thenable; + + /** + * Back up a dirty custom document. * - * @return Thenable indicating that the webview editor has been resolved. + * Backups are used for hot exit and to prevent data loss. Your `backup` method should persist the resource in + * its current state, i.e. with the edits applied. Most commonly this means saving the resource to disk in + * the `ExtensionContext.storagePath`. When VS Code reloads and your custom editor is opened for a resource, + * your extension should first check to see if any backups exist for the resource. If there is a backup, your + * extension should load the file contents from there instead of from the resource in the workspace. + * + * `backup` is triggered whenever an edit it made. Calls to `backup` are debounced so that if multiple edits are + * made in quick succession, `backup` is only triggered after the last one. `backup` is not invoked when + * `auto save` is enabled (since auto save already persists resource ). + * + * @param document Document to backup. + * @param context Information that can be used to backup the document. + * @param cancellation Token that signals the current backup since a new backup is coming in. It is up to your + * extension to decided how to respond to cancellation. If for example your extension is backing up a large file + * in an operation that takes time to complete, your extension may decide to finish the ongoing backup rather + * than cancelling it to ensure that VS Code has some valid backup. */ - resolveCustomTextEditor(document: TextDocument, webviewPanel: WebviewPanel): Thenable; + backupCustomDocument( + document: T, + context: CustomDocumentBackupContext, + cancellation: CancellationToken + ): Thenable; } -//#endregion +// #endregion export const ICustomEditorService = Symbol('ICustomEditorService'); export interface ICustomEditorService { @@ -1429,17 +1473,33 @@ export interface ICustomEditorService { * * @return Disposable that unregisters the `WebviewCustomEditorProvider`. */ - registerCustomEditorProvider( + registerCustomEditorProvider2( viewType: string, - provider: CustomEditorProvider, - options?: WebviewPanelOptions + provider: CustomReadonlyEditorProvider | CustomEditorProvider, + options?: { + readonly webviewOptions?: WebviewPanelOptions; + + /** + * Only applies to `CustomReadonlyEditorProvider | CustomEditorProvider`. + * + * Indicates that the provider allows multiple editor instances to be open at the same time for + * the same resource. + * + * If not set, VS Code only allows one editor instance to be open at a time for each resource. If the + * user tries to open a second editor instance for the resource, the first one is instead moved to where + * the second one was to be opened. + * + * When set, users can split and create copies of the custom editor. The custom editor must make sure it + * can properly synchronize the states of all editor instances for a resource so that they are consistent. + */ + readonly supportsMultipleEditorsPerDocument?: boolean; + } ): Disposable; /** * Opens a file with a custom editor */ openEditor(file: Uri): Promise; } - export const IClipboard = Symbol('IClipboard'); export interface IClipboard { /** diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts index 7ca40e7eee0d..e8197f6bfff4 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts @@ -7,9 +7,10 @@ import { CancellationToken, Disposable, Event, EventEmitter, Uri, WebviewPanel } import { arePathsSame } from '../../../datascience-ui/react-common/arePathsSame'; import { CustomDocument, + CustomDocumentBackup, + CustomDocumentBackupContext, CustomDocumentEditEvent, - CustomDocumentRevert, - CustomEditorEditingDelegate, + CustomDocumentOpenContext, CustomEditorProvider, ICustomEditorService, IWorkspaceService @@ -23,24 +24,19 @@ import { } from '../../common/types'; import { createDeferred } from '../../common/utils/async'; import { DataScience } from '../../common/utils/localize'; -import { noop } from '../../common/utils/misc'; import { IServiceContainer } from '../../ioc/types'; import { captureTelemetry, sendTelemetryEvent } from '../../telemetry'; import { Telemetry } from '../constants'; import { NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; import { INotebookEditor, INotebookEditorProvider, INotebookModel } from '../types'; import { getNextUntitledCounter } from './nativeEditorStorage'; +import { NotebookModelEditEvent } from './notebookModelEditEvent'; import { INotebookStorageProvider } from './notebookStorageProvider'; // Class that is registered as the custom editor provider for notebooks. VS code will call into this class when // opening an ipynb file. This class then creates a backing storage, model, and opens a view for the file. @injectable() -export class NativeEditorProvider - implements - INotebookEditorProvider, - CustomEditorProvider, - IAsyncDisposable, - CustomEditorEditingDelegate { +export class NativeEditorProvider implements INotebookEditorProvider, CustomEditorProvider, IAsyncDisposable { public get onDidChangeActiveNotebookEditor(): Event { return this._onDidChangeActiveNotebookEditor.event; } @@ -53,10 +49,7 @@ export class NativeEditorProvider public get activeEditor(): INotebookEditor | undefined { return this.editors.find((e) => e.visible && e.active); } - public get editingDelegate(): CustomEditorEditingDelegate { - return this; - } - public get onDidEdit(): Event> { + public get onDidChangeCustomDocument(): Event { return this._onDidEdit.event; } @@ -67,7 +60,7 @@ export class NativeEditorProvider public static readonly customEditorViewType = 'NativeEditorProvider.ipynb'; protected readonly _onDidChangeActiveNotebookEditor = new EventEmitter(); protected readonly _onDidOpenNotebookEditor = new EventEmitter(); - protected readonly _onDidEdit = new EventEmitter>(); + protected readonly _onDidEdit = new EventEmitter(); protected customDocuments = new Map(); private readonly _onDidCloseNotebookEditor = new EventEmitter(); private openedEditors: Set = new Set(); @@ -97,40 +90,49 @@ export class NativeEditorProvider } // Register for the custom editor service. - customEditorService.registerCustomEditorProvider(NativeEditorProvider.customEditorViewType, this, { - enableFindWidget: true, - retainContextWhenHidden: true + customEditorService.registerCustomEditorProvider2(NativeEditorProvider.customEditorViewType, this, { + webviewOptions: { + enableFindWidget: true, + retainContextWhenHidden: true + }, + supportsMultipleEditorsPerDocument: true }); } - public async save(document: CustomDocument, cancellation: CancellationToken): Promise { + public async openCustomDocument( + uri: Uri, + _context: CustomDocumentOpenContext, // This has info about backups. right now we use our own data. + _cancellation: CancellationToken + ): Promise { + const model = await this.loadModel(uri); + return { + uri, + dispose: () => model.dispose() + }; + } + public async saveCustomDocument(document: CustomDocument, cancellation: CancellationToken): Promise { const model = await this.loadModel(document.uri); await this.storage.save(model, cancellation); } - public async saveAs(document: CustomDocument, targetResource: Uri): Promise { + public async saveCustomDocumentAs(document: CustomDocument, targetResource: Uri): Promise { const model = await this.loadModel(document.uri); await this.storage.saveAs(model, targetResource); } - public applyEdits(document: CustomDocument, edits: readonly NotebookModelChange[]): Promise { - return this.loadModel(document.uri).then((s) => { - if (s) { - edits.forEach((e) => s.update({ ...e, source: 'redo' })); - } - }); - } - public undoEdits(document: CustomDocument, edits: readonly NotebookModelChange[]): Promise { - return this.loadModel(document.uri).then((s) => { - if (s) { - edits.forEach((e) => s.update({ ...e, source: 'undo' })); - } - }); - } - public async revert(_document: CustomDocument, _edits: CustomDocumentRevert): Promise { - noop(); + public async revertCustomDocument(document: CustomDocument, cancellation: CancellationToken): Promise { + const model = await this.loadModel(document.uri); + await this.storage.revert(model, cancellation); } - public async backup(document: CustomDocument, cancellation: CancellationToken): Promise { + public async backupCustomDocument( + document: CustomDocument, + _context: CustomDocumentBackupContext, + cancellation: CancellationToken + ): Promise { const model = await this.loadModel(document.uri); await this.storage.backup(model, cancellation); + return { + id: document.uri.toString(), // This is used to restore on an open + delete: () => this.storage.deleteBackup(model) // This cleans up after save has happened. + }; } public async resolveCustomEditor(document: CustomDocument, panel: WebviewPanel) { @@ -265,7 +267,7 @@ export class NativeEditorProvider // Find the document associated with this edit. const document = this.customDocuments.get(model.file.fsPath); if (document) { - this._onDidEdit.fire({ document, edit: change }); + this._onDidEdit.fire(new NotebookModelEditEvent(document, model, change)); } } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts b/src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts index ade6c188e0aa..e679e6add347 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts @@ -3,7 +3,7 @@ 'use strict'; import { inject, injectable } from 'inversify'; import * as path from 'path'; -import { CancellationTokenSource, EventEmitter, TextDocument, TextEditor, Uri } from 'vscode'; +import { CancellationTokenSource, TextDocument, TextEditor, Uri } from 'vscode'; import { ICommandManager, @@ -14,6 +14,7 @@ import { import { JUPYTER_LANGUAGE } from '../../common/constants'; import { IFileSystem } from '../../common/platform/types'; import { IAsyncDisposableRegistry, IConfigurationService, IDisposableRegistry } from '../../common/types'; +import { noop } from '../../common/utils/misc'; import { IServiceContainer } from '../../ioc/types'; import { Commands } from '../constants'; import { IDataScienceErrorHandler, INotebookEditor } from '../types'; @@ -57,7 +58,7 @@ export class NativeEditorProviderOld extends NativeEditorProvider { this.cmdManager.registerCommand(Commands.SaveNotebookNonCustomEditor, async (resource: Uri) => { const customDocument = this.customDocuments.get(resource.fsPath); if (customDocument) { - await this.save(customDocument, new CancellationTokenSource().token); + await this.saveCustomDocument(customDocument, new CancellationTokenSource().token); } }) ); @@ -67,7 +68,7 @@ export class NativeEditorProviderOld extends NativeEditorProvider { async (resource: Uri, targetResource: Uri) => { const customDocument = this.customDocuments.get(resource.fsPath); if (customDocument) { - await this.saveAs(customDocument, targetResource); + await this.saveCustomDocumentAs(customDocument, targetResource); this.customDocuments.delete(resource.fsPath); this.customDocuments.set(targetResource.fsPath, { ...customDocument, uri: targetResource }); } @@ -97,8 +98,7 @@ export class NativeEditorProviderOld extends NativeEditorProvider { // Required for old editor this.customDocuments.set(file.fsPath, { uri: file, - viewType: NativeEditorProvider.customEditorViewType, - onDidDispose: new EventEmitter().event + dispose: noop }); } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index f4f5e46defcd..244e50243e37 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -519,6 +519,15 @@ export class NativeEditorStorage implements INotebookStorage { // Should send to extension context storage path return this.storeContentsInHotExitFile(model, cancellation); } + + public async revert(model: INotebookModel, _cancellation: CancellationToken): Promise { + // Revert to what is in the hot exit file + await this.loadFromFile(model.file); + } + + public async deleteBackup(model: INotebookModel): Promise { + return this.clearHotExit(model.file); + } /** * Stores the uncommitted notebook changes into a temporary location. * Also keep track of the current time. This way we can check whether changes were diff --git a/src/client/datascience/interactive-ipynb/nativeEditorViewTracker.ts b/src/client/datascience/interactive-ipynb/nativeEditorViewTracker.ts index 5444b4eeae3d..048845cff1b5 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorViewTracker.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorViewTracker.ts @@ -1,6 +1,7 @@ import { inject, injectable, named } from 'inversify'; import { Memento, Uri } from 'vscode'; import { IExtensionSingleActivationService } from '../../activation/types'; +import { UseCustomEditorApi } from '../../common/constants'; import { IDisposableRegistry, IMemento, WORKSPACE_MEMENTO } from '../../common/types'; import { INotebookEditor, INotebookEditorProvider } from '../types'; import { isUntitled } from './nativeEditorStorage'; @@ -16,10 +17,13 @@ export class NativeEditorViewTracker implements IExtensionSingleActivationServic constructor( @inject(INotebookEditorProvider) private readonly editorProvider: INotebookEditorProvider, @inject(IMemento) @named(WORKSPACE_MEMENTO) private readonly workspaceMemento: Memento, - @inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry + @inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry, + @inject(UseCustomEditorApi) private readonly useCustomEditorApi: boolean ) { - disposableRegistry.push(editorProvider.onDidOpenNotebookEditor(this.onOpenedEditor.bind(this))); - disposableRegistry.push(editorProvider.onDidCloseNotebookEditor(this.onClosedEditor.bind(this))); + if (!useCustomEditorApi) { + disposableRegistry.push(editorProvider.onDidOpenNotebookEditor(this.onOpenedEditor.bind(this))); + disposableRegistry.push(editorProvider.onDidCloseNotebookEditor(this.onClosedEditor.bind(this))); + } } public async activate(): Promise { @@ -27,13 +31,15 @@ export class NativeEditorViewTracker implements IExtensionSingleActivationServic const set = new Set(this.workspaceMemento.get(MEMENTO_KEY) || []); await this.workspaceMemento.update(MEMENTO_KEY, undefined); - // Then open each one. - set.forEach((l) => { - const uri = Uri.parse(l); - if (uri) { - this.editorProvider.open(uri).ignoreErrors(); - } - }); + // Then open each one if not using the custom editor api + if (!this.useCustomEditorApi) { + set.forEach((l) => { + const uri = Uri.parse(l); + if (uri) { + this.editorProvider.open(uri).ignoreErrors(); + } + }); + } } private onOpenedEditor(editor: INotebookEditor) { diff --git a/src/client/datascience/interactive-ipynb/notebookModelEditEvent.ts b/src/client/datascience/interactive-ipynb/notebookModelEditEvent.ts new file mode 100644 index 000000000000..08de8fd4be30 --- /dev/null +++ b/src/client/datascience/interactive-ipynb/notebookModelEditEvent.ts @@ -0,0 +1,19 @@ +import { CustomDocument, CustomDocumentEditEvent } from '../../common/application/types'; +import { NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; +import { INotebookModel } from '../types'; +export class NotebookModelEditEvent implements CustomDocumentEditEvent { + public label?: string | undefined; + constructor( + public readonly document: CustomDocument, + private readonly model: INotebookModel, + private readonly change: NotebookModelChange + ) { + this.label = change.kind; + } + public undo(): void | Thenable { + this.model.applyEdits([{ ...this.change, source: 'undo' }]); + } + public redo(): void | Thenable { + this.model.undoEdits([{ ...this.change, source: 'redo' }]); + } +} diff --git a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts index 6bb031cd91ef..64d6180d2044 100644 --- a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts +++ b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts @@ -52,6 +52,12 @@ export class NotebookStorageProvider implements INotebookStorageProvider { public backup(model: INotebookModel, cancellation: CancellationToken) { return this.storage.backup(model, cancellation); } + public revert(model: INotebookModel, cancellation: CancellationToken) { + return this.storage.revert(model, cancellation); + } + public deleteBackup(model: INotebookModel) { + return this.storage.deleteBackup(model); + } public load(file: Uri, contents?: string | undefined, skipDirtyContents?: boolean): Promise { const key = file.toString(); if (!this.storageAndModels.has(key)) { diff --git a/src/client/datascience/serviceRegistry.ts b/src/client/datascience/serviceRegistry.ts index 3b5ebd4ad77a..be42263b4864 100644 --- a/src/client/datascience/serviceRegistry.ts +++ b/src/client/datascience/serviceRegistry.ts @@ -5,7 +5,7 @@ import { IExtensionSingleActivationService } from '../activation/types'; import { IApplicationEnvironment } from '../common/application/types'; import { UseCustomEditorApi } from '../common/constants'; import { NativeNotebook } from '../common/experiments/groups'; -import { IExperimentsManager } from '../common/types'; +import { IExperimentsManager, IExtensionContext } from '../common/types'; import { ProtocolParser } from '../debugger/debugAdapter/Common/protocolParser'; import { IProtocolParser } from '../debugger/debugAdapter/types'; import { IServiceManager } from '../ioc/types'; @@ -101,7 +101,6 @@ import { PlotViewerProvider } from './plotting/plotViewerProvider'; import { PreWarmActivatedJupyterEnvironmentVariables } from './preWarmVariables'; import { ProgressReporter } from './progress/progressReporter'; import { RawNotebookProviderWrapper } from './raw-kernel/rawNotebookProviderWrapper'; -import { RawNotebookSupportedService } from './raw-kernel/rawNotebookSupportedService'; import { StatusProvider } from './statusProvider'; import { ThemeFinder } from './themeFinder'; import { @@ -146,7 +145,6 @@ import { IPlotViewer, IPlotViewerProvider, IRawNotebookProvider, - IRawNotebookSupportedService, IStatusProvider, IThemeFinder } from './types'; @@ -158,7 +156,10 @@ export function registerTypes(serviceManager: IServiceManager) { const enableProposedApi = serviceManager.get(IApplicationEnvironment).packageJson.enableProposedApi; const experiments = serviceManager.get(IExperimentsManager); const useVSCodeNotebookAPI = experiments.inExperiment(NativeNotebook.experiment); - serviceManager.addSingletonInstance(UseCustomEditorApi, enableProposedApi); + const context = serviceManager.get(IExtensionContext); + const insidersBuild = context.extensionPath.toLocaleLowerCase().includes('insiders'); + // tslint:disable-next-line: no-any + serviceManager.addSingletonInstance(UseCustomEditorApi, enableProposedApi && insidersBuild); // This condition is temporary. const notebookEditorProvider = useVSCodeNotebookAPI ? NotebookEditorProvider : enableProposedApi ? NativeEditorProvider : NativeEditorProviderOld; @@ -191,7 +192,6 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(INotebookStorage, NativeEditorStorage); serviceManager.addSingleton(INotebookStorageProvider, NotebookStorageProvider); serviceManager.addSingleton(IRawNotebookProvider, RawNotebookProviderWrapper); - serviceManager.addSingleton(IRawNotebookSupportedService, RawNotebookSupportedService); serviceManager.addSingleton(IJupyterNotebookProvider, JupyterNotebookProvider); serviceManager.add(IPlotViewer, PlotViewer); serviceManager.addSingleton(IKernelLauncher, KernelLauncher); diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index b80d78395dcf..a2944e8f5a15 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -1031,6 +1031,8 @@ export interface INotebookStorage { saveAs(model: INotebookModel, targetResource: Uri): Promise; backup(model: INotebookModel, cancellation: CancellationToken): Promise; load(file: Uri, contents?: string, skipDirtyContents?: boolean): Promise; + revert(model: INotebookModel, cancellation: CancellationToken): Promise; + deleteBackup(model: INotebookModel): Promise; } type WebViewViewState = { readonly visible: boolean; diff --git a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts index 5fe3dfe18c46..a499a03f7441 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts @@ -168,7 +168,7 @@ suite('DataScience - Native Editor Provider', () => { editor.setup((e) => (e as any).then).returns(() => undefined); customEditorService.setup((e) => (e as any).then).returns(() => undefined); customEditorService - .setup((c) => c.registerCustomEditorProvider(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .setup((c) => c.registerCustomEditorProvider2(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) .returns((_a1, _a2, _a3) => { return { dispose: noop }; }); diff --git a/src/test/datascience/mockCustomEditorService.ts b/src/test/datascience/mockCustomEditorService.ts index 21220b5e0c85..ab5b8833c116 100644 --- a/src/test/datascience/mockCustomEditorService.ts +++ b/src/test/datascience/mockCustomEditorService.ts @@ -1,7 +1,8 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. import { inject, injectable } from 'inversify'; -import { CancellationTokenSource, Disposable, EventEmitter, Uri, WebviewPanel, WebviewPanelOptions } from 'vscode'; +import { CancellationTokenSource, Disposable, Uri, WebviewPanel, WebviewPanelOptions } from 'vscode'; +import { CancellationToken } from 'vscode-languageclient'; import { CustomDocument, CustomEditorProvider, @@ -19,7 +20,7 @@ import { createTemporaryFile } from '../utils/fs'; @injectable() export class MockCustomEditorService implements ICustomEditorService { private provider: CustomEditorProvider | undefined; - private resolvedList = new Map>(); + private resolvedList = new Map | void>(); private undoStack = new Map(); private redoStack = new Map(); @@ -36,10 +37,27 @@ export class MockCustomEditorService implements ICustomEditorService { ); } - public registerCustomEditorProvider( + public registerCustomEditorProvider2( _viewType: string, provider: CustomEditorProvider, - _options?: WebviewPanelOptions | undefined + _options?: { + readonly webviewOptions?: WebviewPanelOptions; + + /** + * Only applies to `CustomReadonlyEditorProvider | CustomEditorProvider`. + * + * Indicates that the provider allows multiple editor instances to be open at the same time for + * the same resource. + * + * If not set, VS Code only allows one editor instance to be open at a time for each resource. If the + * user tries to open a second editor instance for the resource, the first one is instead moved to where + * the second one was to be opened. + * + * When set, users can split and create copies of the custom editor. The custom editor must make sure it + * can properly synchronize the states of all editor instances for a resource so that they are consistent. + */ + readonly supportsMultipleEditorsPerDocument?: boolean; + } ): Disposable { // Only support one view type, so just save the provider this.provider = provider; @@ -61,8 +79,12 @@ export class MockCustomEditorService implements ICustomEditorService { let resolved = this.resolvedList.get(file.toString()); if (!resolved) { // Pass undefined as the webview panel. This will make the editor create a new one - // tslint:disable-next-line: no-any - resolved = this.provider.resolveCustomEditor(this.createDocument(file), (undefined as any) as WebviewPanel); + resolved = this.provider.resolveCustomEditor( + this.createDocument(file), + // tslint:disable-next-line: no-any + (undefined as any) as WebviewPanel, + CancellationToken.None + ); this.resolvedList.set(file.toString(), resolved); } @@ -106,11 +128,9 @@ export class MockCustomEditorService implements ICustomEditorService { } private createDocument(file: Uri): CustomDocument { - const eventEmitter = new EventEmitter(); return { uri: file, - viewType: NativeEditorProvider.customEditorViewType, - onDidDispose: eventEmitter.event + dispose: noop }; } diff --git a/src/test/datascience/nativeEditorViewTracker.unit.test.ts b/src/test/datascience/nativeEditorViewTracker.unit.test.ts index cf7f9882b4cc..df237296e80e 100644 --- a/src/test/datascience/nativeEditorViewTracker.unit.test.ts +++ b/src/test/datascience/nativeEditorViewTracker.unit.test.ts @@ -75,7 +75,7 @@ suite('DataScience - View tracker', () => { function activate(): Promise { openedList = []; - const viewTracker = new NativeEditorViewTracker(instance(editorProvider), memento, []); + const viewTracker = new NativeEditorViewTracker(instance(editorProvider), memento, [], false); return viewTracker.activate(); } From 66ea1dde134c42a19ac06d4d60af4285c8b21447 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 27 May 2020 16:03:26 -0700 Subject: [PATCH 02/19] Fix copy problem --- src/client/datascience/serviceRegistry.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/client/datascience/serviceRegistry.ts b/src/client/datascience/serviceRegistry.ts index be42263b4864..1418558c8521 100644 --- a/src/client/datascience/serviceRegistry.ts +++ b/src/client/datascience/serviceRegistry.ts @@ -101,6 +101,7 @@ import { PlotViewerProvider } from './plotting/plotViewerProvider'; import { PreWarmActivatedJupyterEnvironmentVariables } from './preWarmVariables'; import { ProgressReporter } from './progress/progressReporter'; import { RawNotebookProviderWrapper } from './raw-kernel/rawNotebookProviderWrapper'; +import { RawNotebookSupportedService } from './raw-kernel/rawNotebookSupportedService'; import { StatusProvider } from './statusProvider'; import { ThemeFinder } from './themeFinder'; import { @@ -157,8 +158,7 @@ export function registerTypes(serviceManager: IServiceManager) { const experiments = serviceManager.get(IExperimentsManager); const useVSCodeNotebookAPI = experiments.inExperiment(NativeNotebook.experiment); const context = serviceManager.get(IExtensionContext); - const insidersBuild = context.extensionPath.toLocaleLowerCase().includes('insiders'); - // tslint:disable-next-line: no-any + const insidersBuild = context.globalStoragePath.toLocaleLowerCase().includes('insiders'); serviceManager.addSingletonInstance(UseCustomEditorApi, enableProposedApi && insidersBuild); // This condition is temporary. @@ -192,6 +192,7 @@ export function registerTypes(serviceManager: IServiceManager) { serviceManager.addSingleton(INotebookStorage, NativeEditorStorage); serviceManager.addSingleton(INotebookStorageProvider, NotebookStorageProvider); serviceManager.addSingleton(IRawNotebookProvider, RawNotebookProviderWrapper); + serviceManager.addSingleton(IRawNotebookSupportedService, RawNotebookSupportedService); serviceManager.addSingleton(IJupyterNotebookProvider, JupyterNotebookProvider); serviceManager.add(IPlotViewer, PlotViewer); serviceManager.addSingleton(IKernelLauncher, KernelLauncher); From d27e2f6076842f1b5b218c7eaba73eedad3cc73f Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 27 May 2020 16:03:49 -0700 Subject: [PATCH 03/19] Fix copy problem again --- src/client/datascience/serviceRegistry.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/client/datascience/serviceRegistry.ts b/src/client/datascience/serviceRegistry.ts index 1418558c8521..ccaaa14872fc 100644 --- a/src/client/datascience/serviceRegistry.ts +++ b/src/client/datascience/serviceRegistry.ts @@ -146,6 +146,7 @@ import { IPlotViewer, IPlotViewerProvider, IRawNotebookProvider, + IRawNotebookSupportedService, IStatusProvider, IThemeFinder } from './types'; From e49e9ad6127033353389f7c579d7b65415a3341e Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 27 May 2020 16:57:34 -0700 Subject: [PATCH 04/19] Get undo/redo to work --- package.json | 14 +++++++++++++- .../datascience/interactive-ipynb/nativeEditor.ts | 5 +++-- .../interactive-ipynb/nativeEditorProvider.ts | 2 +- .../interactive-ipynb/notebookModelEditEvent.ts | 4 ++-- 4 files changed, 19 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 7b433bd83f02..41a49cb212d7 100644 --- a/package.json +++ b/package.json @@ -90,12 +90,24 @@ "onCommand:python.datascience.selectJupyterInterpreter", "onCommand:python.datascience.selectjupytercommandline", "onCommand:python.enableSourceMapSupport", - "onCustomEditor:NativeEditorProvider.ipynb", + "onCustomEditor:ms-python.python.notebook.ipynb", "onNotebookEditor:jupyter-notebook", "workspaceContains:**/mspythonconfig.json" ], "main": "./out/client/extension", "contributes": { + "customEditors": [ + { + "viewType": "ms-python.python.notebook.ipynb", + "displayName": "Jupyter Notebook", + "selector": [ + { + "filenamePattern": "*.ipynb" + } + ], + "priority": "default" + } + ], "snippets": [ { "language": "python", diff --git a/src/client/datascience/interactive-ipynb/nativeEditor.ts b/src/client/datascience/interactive-ipynb/nativeEditor.ts index 27e656e6dc2f..abc7a4d821c4 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditor.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditor.ts @@ -552,8 +552,9 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { private async modelChanged(change: NotebookModelChange) { if (change.source !== 'user') { - // VS code is telling us to broadcast this to our UI. Tell the UI about the new change - await this.postMessage(InteractiveWindowMessages.UpdateModel, change); + // VS code is telling us to broadcast this to our UI. Tell the UI about the new change. Remove the + // the model so this doesn't have to be stringified + await this.postMessage(InteractiveWindowMessages.UpdateModel, { ...change, model: undefined }); } // Use the current state of the model to indicate dirty (not the message itself) diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts index e8197f6bfff4..90fc995b3c20 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts @@ -57,7 +57,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit return [...this.openedEditors]; } // Note, this constant has to match the value used in the package.json to register the webview custom editor. - public static readonly customEditorViewType = 'NativeEditorProvider.ipynb'; + public static readonly customEditorViewType = 'ms-python.python.notebook.ipynb'; protected readonly _onDidChangeActiveNotebookEditor = new EventEmitter(); protected readonly _onDidOpenNotebookEditor = new EventEmitter(); protected readonly _onDidEdit = new EventEmitter(); diff --git a/src/client/datascience/interactive-ipynb/notebookModelEditEvent.ts b/src/client/datascience/interactive-ipynb/notebookModelEditEvent.ts index 08de8fd4be30..05ca53a392c8 100644 --- a/src/client/datascience/interactive-ipynb/notebookModelEditEvent.ts +++ b/src/client/datascience/interactive-ipynb/notebookModelEditEvent.ts @@ -11,9 +11,9 @@ export class NotebookModelEditEvent implements CustomDocumentEditEvent { this.label = change.kind; } public undo(): void | Thenable { - this.model.applyEdits([{ ...this.change, source: 'undo' }]); + return this.model.undoEdits([{ ...this.change, source: 'undo' }]); } public redo(): void | Thenable { - this.model.undoEdits([{ ...this.change, source: 'redo' }]); + return this.model.applyEdits([{ ...this.change, source: 'redo' }]); } } From 0f48e1eccc7da668037775b7387bce3f92b53a42 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 29 May 2020 14:04:22 -0700 Subject: [PATCH 05/19] Some fixes for synching more than one editor --- .../interactive-common/synchronization.ts | 35 ++++++++++++++++--- .../interactive-ipynb/nativeEditorProvider.ts | 8 ++--- .../interactive-common/redux/helpers.ts | 2 +- 3 files changed, 34 insertions(+), 11 deletions(-) diff --git a/src/client/datascience/interactive-common/synchronization.ts b/src/client/datascience/interactive-common/synchronization.ts index 428bcd69e29d..bc9ddf0b5469 100644 --- a/src/client/datascience/interactive-common/synchronization.ts +++ b/src/client/datascience/interactive-common/synchronization.ts @@ -1,9 +1,16 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. import { CommonActionType, CommonActionTypeMapping } from '../../../datascience-ui/interactive-common/redux/reducers/types'; import { CssMessages, SharedMessages } from '../messages'; -import { IInteractiveWindowMapping, InteractiveWindowMessages, IPyWidgetMessages } from './interactiveWindowTypes'; +import { + IInteractiveWindowMapping, + InteractiveWindowMessages, + IPyWidgetMessages, + NotebookModelChange +} from './interactiveWindowTypes'; // Copyright (c) Microsoft Corporation. All rights reserved. // Licensed under the MIT License. @@ -24,8 +31,11 @@ export enum MessageType { noIdea = 1 << 2 } +// tslint:disable-next-line: no-any +type MessageAction = (payload: any) => boolean; + type MessageMapping = { - [P in keyof T]: MessageType; + [P in keyof T]: MessageType | MessageAction; }; export type IInteractiveActionMapping = MessageMapping; @@ -189,7 +199,7 @@ const messageWithMessageTypes: MessageMapping & Messa [InteractiveWindowMessages.UnfocusedCellEditor]: MessageType.syncWithLiveShare, [InteractiveWindowMessages.UpdateCellWithExecutionResults]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, - [InteractiveWindowMessages.UpdateModel]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, + [InteractiveWindowMessages.UpdateModel]: checkSyncUpdateModel, [InteractiveWindowMessages.UpdateKernel]: MessageType.syncAcrossSameNotebooks | MessageType.syncWithLiveShare, [InteractiveWindowMessages.UpdateDisplayData]: MessageType.syncWithLiveShare, [InteractiveWindowMessages.VariableExplorerToggle]: MessageType.other, @@ -223,6 +233,14 @@ const messageWithMessageTypes: MessageMapping & Messa [IPyWidgetMessages.IPyWidgets_mirror_execute]: MessageType.noIdea }; +/** + * Function to check if a NotebookModelChange should be sync'd across editors or not + */ +function checkSyncUpdateModel(payload: NotebookModelChange): boolean { + // Only sync user changes + return payload.source === 'user'; +} + /** * If the original message was a sync message, then do not send messages to extension. * We allow messages to be sent to extension ONLY when the original message was triggered by the user. @@ -245,9 +263,12 @@ export function checkToPostBasedOnOriginalMessageType(messageType?: MessageType) return true; } -export function shouldRebroadcast(message: keyof IInteractiveWindowMapping): [boolean, MessageType] { +// tslint:disable-next-line: no-any +export function shouldRebroadcast(message: keyof IInteractiveWindowMapping, payload: any): [boolean, MessageType] { // Get the configured type for this message (whether it should be re-broadcasted or not). - const messageType: MessageType | undefined = messageWithMessageTypes[message]; + const messageTypeOrFunc: MessageType | undefined | MessageAction = messageWithMessageTypes[message]; + const messageType = + typeof messageTypeOrFunc !== 'function' ? (messageTypeOrFunc as number) : MessageType.syncAcrossSameNotebooks; // Support for liveshare is turned off for now, we can enable that later. // I.e. we only support synchronizing across editors in the same session. if ( @@ -257,6 +278,10 @@ export function shouldRebroadcast(message: keyof IInteractiveWindowMapping): [bo return [false, MessageType.other]; } + if (typeof messageTypeOrFunc === 'function') { + return [messageTypeOrFunc(payload), messageType]; + } + return [ (messageType & MessageType.syncAcrossSameNotebooks) > 0 || (messageType & MessageType.syncWithLiveShare) > 0, messageType diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts index 90fc995b3c20..489bd6cdd6c9 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts @@ -189,7 +189,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit @captureTelemetry(Telemetry.CreateNewNotebook, undefined, false) public async createNew(contents?: string): Promise { // Create a new URI for the dummy file using our root workspace path - const uri = await this.getNextNewNotebookUri(); + const uri = this.getNextNewNotebookUri(); // Update number of notebooks in the workspace this.notebookCount += 1; @@ -271,11 +271,9 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit } } - private async getNextNewNotebookUri(): Promise { + private getNextNewNotebookUri(): Uri { // Just use the current counter. Counter will be incremented after actually opening a file. const fileName = `${DataScience.untitledNotebookFileName()}-${this.untitledCounter}.ipynb`; - const fileUri = Uri.file(fileName); - // Turn this back into an untitled - return fileUri.with({ scheme: 'untitled', path: fileName }); + return Uri.parse(`untitled:///${fileName}`); } } diff --git a/src/datascience-ui/interactive-common/redux/helpers.ts b/src/datascience-ui/interactive-common/redux/helpers.ts index e68b91d0c2a9..d0677e50a859 100644 --- a/src/datascience-ui/interactive-common/redux/helpers.ts +++ b/src/datascience-ui/interactive-common/redux/helpers.ts @@ -131,7 +131,7 @@ export function reBroadcastMessageIfRequired( } // Check if we need to re-broadcast this message to other editors/sessions. // tslint:disable-next-line: no-any - const result = shouldRebroadcast(message as any); + const result = shouldRebroadcast(message as any, payload); if (result[0]) { // Mark message as incoming, to indicate this will be sent into the other webviews. // tslint:disable-next-line: no-any From cc921af2565f523867ea6d90aa95787256730c90 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Wed, 3 Jun 2020 17:56:40 -0700 Subject: [PATCH 06/19] Fix untitled. Fix timeouts --- .../common/application/customEditorService.ts | 5 +- src/client/common/application/types.ts | 2 +- .../interactive-ipynb/nativeEditorProvider.ts | 70 ++++++++++++------- .../nativeEditorProviderOld.ts | 36 +++++++++- .../interactive-ipynb/nativeEditorStorage.ts | 15 ++-- .../notebookStorageProvider.ts | 39 ++--------- src/client/datascience/types.ts | 1 + .../nativeEditorProvider.functional.test.ts | 6 +- .../nativeEditorStorage.unit.test.ts | 5 +- .../datascience/mockCustomEditorService.ts | 2 +- 10 files changed, 108 insertions(+), 73 deletions(-) diff --git a/src/client/common/application/customEditorService.ts b/src/client/common/application/customEditorService.ts index 4cb377a17071..eaac661044bb 100644 --- a/src/client/common/application/customEditorService.ts +++ b/src/client/common/application/customEditorService.ts @@ -15,8 +15,7 @@ export class CustomEditorService implements ICustomEditorService { @inject(UseCustomEditorApi) private readonly useCustomEditorApi: boolean ) {} - // 2 should be temporary - public registerCustomEditorProvider2( + public registerCustomEditorProvider( viewType: string, provider: CustomEditorProvider, options?: { @@ -26,7 +25,7 @@ export class CustomEditorService implements ICustomEditorService { ): vscode.Disposable { if (this.useCustomEditorApi) { // tslint:disable-next-line: no-any - return (vscode.window as any).registerCustomEditorProvider2(viewType, provider, options); + return (vscode.window as any).registerCustomEditorProvider(viewType, provider, options); } else { return { dispose: noop }; } diff --git a/src/client/common/application/types.ts b/src/client/common/application/types.ts index f2ce6eb6ee00..f2ed06179966 100644 --- a/src/client/common/application/types.ts +++ b/src/client/common/application/types.ts @@ -1474,7 +1474,7 @@ export interface ICustomEditorService { * * @return Disposable that unregisters the `WebviewCustomEditorProvider`. */ - registerCustomEditorProvider2( + registerCustomEditorProvider( viewType: string, provider: CustomReadonlyEditorProvider | CustomEditorProvider, options?: { diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts index 6bf324c015f8..fe8625aa6764 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts @@ -2,8 +2,10 @@ // Licensed under the MIT License. 'use strict'; import { inject, injectable } from 'inversify'; +import * as os from 'os'; import * as uuid from 'uuid/v4'; -import { CancellationToken, Disposable, Event, EventEmitter, Uri, WebviewPanel } from 'vscode'; +import { Disposable, Event, EventEmitter, Uri, WebviewPanel } from 'vscode'; +import { CancellationToken } from 'vscode-languageclient'; import { arePathsSame } from '../../../datascience-ui/react-common/arePathsSame'; import { CustomDocument, @@ -77,7 +79,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit @inject(IWorkspaceService) protected readonly workspace: IWorkspaceService, @inject(IConfigurationService) protected readonly configuration: IConfigurationService, @inject(ICustomEditorService) private customEditorService: ICustomEditorService, - @inject(INotebookStorageProvider) private readonly storage: INotebookStorageProvider + @inject(INotebookStorageProvider) protected readonly storage: INotebookStorageProvider ) { traceInfo(`id is ${this._id}`); asyncRegistry.push(this); @@ -90,7 +92,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit } // Register for the custom editor service. - customEditorService.registerCustomEditorProvider2(NativeEditorProvider.customEditorViewType, this, { + customEditorService.registerCustomEditorProvider(NativeEditorProvider.customEditorViewType, this, { webviewOptions: { enableFindWidget: true, retainContextWhenHidden: true @@ -101,10 +103,10 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit public async openCustomDocument( uri: Uri, - _context: CustomDocumentOpenContext, // This has info about backups. right now we use our own data. + context: CustomDocumentOpenContext, // This has info about backups. right now we use our own data. _cancellation: CancellationToken ): Promise { - const model = await this.loadModel(uri); + const model = await this.loadModel(uri, undefined, context.backupId ? false : true); return { uri, dispose: () => model.dispose() @@ -112,15 +114,18 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit } public async saveCustomDocument(document: CustomDocument, cancellation: CancellationToken): Promise { const model = await this.loadModel(document.uri); - await this.storage.save(model, cancellation); + // 1 second timeout on save so don't wait. Just write and forget + this.storage.save(model, cancellation).ignoreErrors(); } public async saveCustomDocumentAs(document: CustomDocument, targetResource: Uri): Promise { const model = await this.loadModel(document.uri); - await this.storage.saveAs(model, targetResource); + // 1 second timeout on save so don't wait. Just write and forget + this.storage.saveAs(model, targetResource).ignoreErrors(); } public async revertCustomDocument(document: CustomDocument, cancellation: CancellationToken): Promise { const model = await this.loadModel(document.uri); - await this.storage.revert(model, cancellation); + // 1 second time limit on this so don't wait. + this.storage.revert(model, cancellation).ignoreErrors(); } public async backupCustomDocument( document: CustomDocument, @@ -128,10 +133,11 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit cancellation: CancellationToken ): Promise { const model = await this.loadModel(document.uri); - await this.storage.backup(model, cancellation); + const id = this.storage.getBackupId(model); + this.storage.backup(model, cancellation).ignoreErrors(); return { - id: document.uri.toString(), // This is used to restore on an open - delete: () => this.storage.deleteBackup(model) // This cleans up after save has happened. + id, + delete: () => this.storage.deleteBackup(model).ignoreErrors() // This cleans up after save has happened. }; } @@ -201,13 +207,16 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit return this.open(uri); } - public loadModel(file: Uri, contents?: string, skipDirtyContents?: boolean) { + public async loadModel(file: Uri, contents?: string, skipDirtyContents?: boolean) { // Every time we load a new untitled file, up the counter past the max value for this counter this.untitledCounter = getNextUntitledCounter(file, this.untitledCounter); - return this.storage.load(file, contents, skipDirtyContents).then((m) => { - this.trackModel(m); - return m; - }); + + // Load our model from our storage object. + const model = await this.storage.load(file, contents, skipDirtyContents); + + // Make sure to listen to events on the model + this.trackModel(model); + return model; } protected async createNotebookEditor(resource: Uri, panel?: WebviewPanel) { @@ -241,6 +250,16 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit this._onDidOpenNotebookEditor.fire(editor); } + protected async modelEdited(model: INotebookModel, change: NotebookModelChange) { + // Find the document associated with this edit. + const document = this.customDocuments.get(model.file.fsPath); + + // Tell VS code about model changes if not caused by vs code itself + if (document && change.kind !== 'save' && change.kind !== 'saveAs' && change.source === 'user') { + this._onDidEdit.fire(new NotebookModelEditEvent(document, model, change)); + } + } + private closedEditor(editor: INotebookEditor): void { this.openedEditors.delete(editor); this._onDidCloseNotebookEditor.fire(editor); @@ -263,17 +282,16 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit } } - private async modelEdited(model: INotebookModel, change: NotebookModelChange) { - // Find the document associated with this edit. - const document = this.customDocuments.get(model.file.fsPath); - if (document) { - this._onDidEdit.fire(new NotebookModelEditEvent(document, model, change)); - } - } - private getNextNewNotebookUri(): Uri { - // Just use the current counter. Counter will be incremented after actually opening a file. + // Because of this bug here: + // https://github.com/microsoft/vscode/issues/93441 + // We can't create 'untitled' files anymore. The untitled scheme will just be ignored. + // Instead we need to create untitled files in the temp folder and force a saveas whenever they're + // saved. + + // However if there are files already on disk, we should be able to overwrite them because + // they will only ever be used by 'open' editors. So just use the current counter for our untitled count. const fileName = `${DataScience.untitledNotebookFileName()}-${this.untitledCounter}.ipynb`; - return Uri.parse(`untitled:///${fileName}`); + return Uri.parse(`untitled:///${os.tmpdir}/${fileName}`); } } diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts b/src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts index e679e6add347..afaabe3625ec 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts @@ -5,6 +5,7 @@ import { inject, injectable } from 'inversify'; import * as path from 'path'; import { CancellationTokenSource, TextDocument, TextEditor, Uri } from 'vscode'; +import { CancellationToken } from 'vscode-jsonrpc'; import { ICommandManager, ICustomEditorService, @@ -17,10 +18,14 @@ import { IAsyncDisposableRegistry, IConfigurationService, IDisposableRegistry } import { noop } from '../../common/utils/misc'; import { IServiceContainer } from '../../ioc/types'; import { Commands } from '../constants'; -import { IDataScienceErrorHandler, INotebookEditor } from '../types'; +import { NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; +import { IDataScienceErrorHandler, INotebookEditor, INotebookModel } from '../types'; import { NativeEditorProvider } from './nativeEditorProvider'; import { INotebookStorageProvider } from './notebookStorageProvider'; +// tslint:disable-next-line:no-require-imports no-var-requires +const debounce = require('lodash/debounce') as typeof import('lodash/debounce'); + @injectable() export class NativeEditorProviderOld extends NativeEditorProvider { public get activeEditor(): INotebookEditor | undefined { @@ -34,6 +39,7 @@ export class NativeEditorProviderOld extends NativeEditorProvider { return [...this.activeEditors.values()]; } private activeEditors: Map = new Map(); + private readonly _autoSaveNotebookInHotExitFile = new WeakMap(); constructor( @inject(IServiceContainer) serviceContainer: IServiceContainer, @inject(IAsyncDisposableRegistry) asyncRegistry: IAsyncDisposableRegistry, @@ -129,6 +135,29 @@ export class NativeEditorProviderOld extends NativeEditorProvider { this._onDidChangeActiveNotebookEditor.fire(this.activeEditor); } + protected async modelEdited(model: INotebookModel, e: NotebookModelChange) { + const actualModel = e.model || model; // Test mocks can screw up bound values. + if (actualModel && e.kind !== 'save' && e.kind !== 'saveAs' && e.source === 'user') { + // This isn't necessary with the custom editor api because the custom editor will + // cause backup to be called appropriately. + let debounceFunc = this._autoSaveNotebookInHotExitFile.get(actualModel); + if (!debounceFunc) { + debounceFunc = debounce(this.autoSaveNotebookInHotExitFile.bind(this, actualModel), 250); + this._autoSaveNotebookInHotExitFile.set(actualModel, debounceFunc); + } + debounceFunc(); + } + } + private autoSaveNotebookInHotExitFile(model: INotebookModel) { + // Refetch settings each time as they can change before the debounce can happen + const fileSettings = this.workspace.getConfiguration('files', model.file); + // We need to backup, only if auto save if turned off and not an untitled file. + if (fileSettings.get('autoSave', 'off') !== 'off' && !model.isUntitled) { + return; + } + this.storage.backup(model, CancellationToken.None).ignoreErrors(); + } + /** * Open ipynb files when user opens an ipynb file. * @@ -165,6 +194,11 @@ export class NativeEditorProviderOld extends NativeEditorProvider { this.activeEditors.delete(oldPath); } this.activeEditors.set(e.file.fsPath, e); + + // Remove backup storage + this.loadModel(Uri.file(oldPath)) + .then((m) => this.storage.deleteBackup(m)) + .ignoreErrors(); } private openNotebookAndCloseEditor = async ( diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index f56be7c6b9ff..a5135fe87b2f 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -504,7 +504,12 @@ export class NativeEditorStorage implements INotebookStorage { return isUntitledFile(file); } - public async load(file: Uri, possibleContents?: string, skipDirtyContents?: boolean): Promise { + public getBackupId(model: INotebookModel): string { + const key = this.getStorageKey(model.file); + return this.getHashedFileName(key); + } + + public load(file: Uri, possibleContents?: string, skipDirtyContents?: boolean): Promise { return this.loadFromFile(file, possibleContents, skipDirtyContents); } public async save(model: INotebookModel, _cancellation: CancellationToken): Promise { @@ -516,7 +521,6 @@ export class NativeEditorStorage implements INotebookStorage { oldDirty: model.isDirty, newDirty: false }); - this.clearHotExit(model.file).ignoreErrors(); } public async saveAs(model: INotebookModel, file: Uri): Promise { @@ -532,7 +536,6 @@ export class NativeEditorStorage implements INotebookStorage { sourceUri: model.file }); this.savedAs.fire({ new: file, old }); - this.clearHotExit(old).ignoreErrors(); } public async backup(model: INotebookModel, cancellation: CancellationToken): Promise { // If we are already backing up, save this request replacing any other previous requests @@ -584,7 +587,7 @@ export class NativeEditorStorage implements INotebookStorage { private async clearHotExit(file: Uri): Promise { const key = this.getStorageKey(file); const filePath = this.getHashedFileName(key); - return this.writeToStorage(filePath, undefined); + await this.writeToStorage(filePath, undefined); } private async writeToStorage(filePath: string, contents?: string, cancelToken?: CancellationToken): Promise { @@ -593,10 +596,10 @@ export class NativeEditorStorage implements INotebookStorage { if (contents) { await this.fileSystem.createDirectory(path.dirname(filePath)); if (!cancelToken?.isCancellationRequested) { - return await this.fileSystem.writeFile(filePath, contents); + await this.fileSystem.writeFile(filePath, contents); } } else if (await this.fileSystem.fileExists(filePath)) { - return await this.fileSystem.deleteFile(filePath); + await this.fileSystem.deleteFile(filePath); } } } catch (exc) { diff --git a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts index 64d6180d2044..9b711ec1e75e 100644 --- a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts +++ b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts @@ -6,15 +6,13 @@ import { inject, injectable } from 'inversify'; import { EventEmitter, Uri } from 'vscode'; import { CancellationToken } from 'vscode-jsonrpc'; -import { IWorkspaceService } from '../../common/application/types'; +import { ICommandManager } from '../../common/application/types'; import { IDisposable, IDisposableRegistry } from '../../common/types'; import { DataScience } from '../../common/utils/localize'; -import { NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; import { INotebookModel, INotebookStorage } from '../types'; import { getNextUntitledCounter } from './nativeEditorStorage'; // tslint:disable-next-line:no-require-imports no-var-requires -const debounce = require('lodash/debounce') as typeof import('lodash/debounce'); export const INotebookStorageProvider = Symbol.for('INotebookStorageProvider'); export interface INotebookStorageProvider extends INotebookStorage { @@ -30,17 +28,16 @@ export class NotebookStorageProvider implements INotebookStorageProvider { private readonly storageAndModels = new Map>(); private models = new Set(); private readonly disposables: IDisposable[] = []; - private readonly _autoSaveNotebookInHotExitFile = new WeakMap(); constructor( @inject(INotebookStorage) private readonly storage: INotebookStorage, @inject(IDisposableRegistry) disposables: IDisposableRegistry, - @inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService + @inject(ICommandManager) private commandManager: ICommandManager ) { disposables.push(this); disposables.push(storage.onSavedAs((e) => this._savedAs.fire(e))); } - public save(model: INotebookModel, cancellation: CancellationToken) { - return this.storage.save(model, cancellation); + public async save(model: INotebookModel, cancellation: CancellationToken) { + await this.storage.save(model, cancellation); } public async saveAs(model: INotebookModel, targetResource: Uri) { const oldUri = model.file; @@ -49,6 +46,9 @@ export class NotebookStorageProvider implements INotebookStorageProvider { this.storageAndModels.delete(oldUri.toString()); this.storageAndModels.set(targetResource.toString(), Promise.resolve(model)); } + public getBackupId(model: INotebookModel): string { + return this.storage.getBackupId(model); + } public backup(model: INotebookModel, cancellation: CancellationToken) { return this.storage.backup(model, cancellation); } @@ -101,35 +101,10 @@ export class NotebookStorageProvider implements INotebookStorageProvider { () => { this.models.delete(model); this.storageAndModels.delete(model.file.toString()); - this._autoSaveNotebookInHotExitFile.delete(model); }, this, this.disposables ); - - // Ensure we save into back for hotexit - this.disposables.push(model.changed(this.modelChanged.bind(this, model))); return model; } - - private modelChanged(model: INotebookModel, e: NotebookModelChange) { - const actualModel = e.model || model; // Test mocks can screw up bound values. - if (actualModel) { - let debounceFunc = this._autoSaveNotebookInHotExitFile.get(actualModel); - if (!debounceFunc) { - debounceFunc = debounce(this.autoSaveNotebookInHotExitFile.bind(this, actualModel), 250); - this._autoSaveNotebookInHotExitFile.set(actualModel, debounceFunc); - } - debounceFunc(); - } - } - private autoSaveNotebookInHotExitFile(model: INotebookModel) { - // Refetch settings each time as they can change before the debounce can happen - const fileSettings = this.workspaceService.getConfiguration('files', model.file); - // We need to backup, only if auto save if turned off and not an untitled file. - if (fileSettings.get('autoSave', 'off') !== 'off' && !model.isUntitled) { - return; - } - this.storage.backup(model, CancellationToken.None).ignoreErrors(); - } } diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index 457291135142..29cd8939f186 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -1030,6 +1030,7 @@ export const INotebookStorage = Symbol('INotebookStorage'); export interface INotebookStorage { readonly onSavedAs: Event<{ new: Uri; old: Uri }>; + getBackupId(model: INotebookModel): string; save(model: INotebookModel, cancellation: CancellationToken): Promise; saveAs(model: INotebookModel, targetResource: Uri): Promise; backup(model: INotebookModel, cancellation: CancellationToken): Promise; diff --git a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts index f096b69ada17..2bf03ba81131 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts @@ -11,6 +11,7 @@ import { anything, instance, mock, when } from 'ts-mockito'; import { Matcher } from 'ts-mockito/lib/matcher/type/Matcher'; import * as typemoq from 'typemoq'; import { ConfigurationChangeEvent, EventEmitter, FileType, TextEditor, Uri, WebviewPanel } from 'vscode'; +import { CommandManager } from '../../../client/common/application/commandManager'; import { DocumentManager } from '../../../client/common/application/documentManager'; import { CustomDocument, @@ -168,7 +169,7 @@ suite('DataScience - Native Editor Provider', () => { editor.setup((e) => (e as any).then).returns(() => undefined); customEditorService.setup((e) => (e as any).then).returns(() => undefined); customEditorService - .setup((c) => c.registerCustomEditorProvider2(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) + .setup((c) => c.registerCustomEditorProvider(typemoq.It.isAny(), typemoq.It.isAny(), typemoq.It.isAny())) .returns((_a1, _a2, _a3) => { return { dispose: noop }; }); @@ -210,7 +211,8 @@ suite('DataScience - Native Editor Provider', () => { localMemento ); - const storage = new NotebookStorageProvider(notebookStorage, [], instance(workspace)); + const commandManager = mock(CommandManager); + const storage = new NotebookStorageProvider(notebookStorage, [], instance(commandManager)); registeredProvider = new NativeEditorProvider( instance(svcContainer), diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index 219e5eaf8a19..705268d52a56 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -9,6 +9,7 @@ import { Matcher } from 'ts-mockito/lib/matcher/type/Matcher'; import * as typemoq from 'typemoq'; import { ConfigurationChangeEvent, EventEmitter, FileType, TextEditor, Uri } from 'vscode'; +import { CommandManager } from '../../../client/common/application/commandManager'; import { DocumentManager } from '../../../client/common/application/documentManager'; import { IDocumentManager, @@ -329,7 +330,9 @@ suite('DataScience - Native Editor Storage', () => { localMemento ); - return new NotebookStorageProvider(notebookStorage, [], instance(workspace)); + const commandManager = mock(CommandManager); + + return new NotebookStorageProvider(notebookStorage, [], instance(commandManager)); } teardown(() => { diff --git a/src/test/datascience/mockCustomEditorService.ts b/src/test/datascience/mockCustomEditorService.ts index d4fb05101385..d0cd10574323 100644 --- a/src/test/datascience/mockCustomEditorService.ts +++ b/src/test/datascience/mockCustomEditorService.ts @@ -37,7 +37,7 @@ export class MockCustomEditorService implements ICustomEditorService { ); } - public registerCustomEditorProvider2( + public registerCustomEditorProvider( _viewType: string, provider: CustomEditorProvider, _options?: { From c3a470cd2f8eb3c6ed6bfe9b4e753a525bc7af93 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 5 Jun 2020 10:06:15 -0700 Subject: [PATCH 07/19] fix command manager to not be used. Not necessary with temp path. fix delete/insert commands to undo properly --- .../notebookStorageProvider.ts | 4 +- .../native-editor/redux/reducers/creation.ts | 42 +++++++++++++++---- .../native-editor/redux/reducers/index.ts | 2 +- .../nativeEditorProvider.functional.test.ts | 4 +- .../nativeEditorStorage.unit.test.ts | 5 +-- 5 files changed, 39 insertions(+), 18 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts index 9b711ec1e75e..e2381a48ef41 100644 --- a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts +++ b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts @@ -6,7 +6,6 @@ import { inject, injectable } from 'inversify'; import { EventEmitter, Uri } from 'vscode'; import { CancellationToken } from 'vscode-jsonrpc'; -import { ICommandManager } from '../../common/application/types'; import { IDisposable, IDisposableRegistry } from '../../common/types'; import { DataScience } from '../../common/utils/localize'; import { INotebookModel, INotebookStorage } from '../types'; @@ -30,8 +29,7 @@ export class NotebookStorageProvider implements INotebookStorageProvider { private readonly disposables: IDisposable[] = []; constructor( @inject(INotebookStorage) private readonly storage: INotebookStorage, - @inject(IDisposableRegistry) disposables: IDisposableRegistry, - @inject(ICommandManager) private commandManager: ICommandManager + @inject(IDisposableRegistry) disposables: IDisposableRegistry ) { disposables.push(this); disposables.push(storage.onSavedAs((e) => this._savedAs.fire(e))); diff --git a/src/datascience-ui/native-editor/redux/reducers/creation.ts b/src/datascience-ui/native-editor/redux/reducers/creation.ts index b77f3702f391..c105f5ab4d20 100644 --- a/src/datascience-ui/native-editor/redux/reducers/creation.ts +++ b/src/datascience-ui/native-editor/redux/reducers/creation.ts @@ -89,9 +89,9 @@ export namespace Creation { return arg.prevState; } - export function insertAbove(arg: NativeEditorReducerArg): IMainState { - const newVM = prepareCellVM(createEmptyCell(arg.payload.data.newCellId, null), false, arg.prevState.settings); + function insertAbove(arg: NativeEditorReducerArg): IMainState { const newList = [...arg.prevState.cellVMs]; + const newVM = arg.payload.data.vm; // Find the position where we want to insert let position = arg.prevState.cellVMs.findIndex((c) => c.cell.id === arg.payload.data.cellId); @@ -114,6 +114,34 @@ export namespace Creation { return result; } + export function insertExistingAbove(arg: NativeEditorReducerArg): IMainState { + const newVM = prepareCellVM(arg.payload.data.cell, false, arg.prevState.settings); + return insertAbove({ + ...arg, + payload: { + ...arg.payload, + data: { + cellId: arg.payload.data.cellId, + vm: newVM + } + } + }); + } + + export function insertNewAbove(arg: NativeEditorReducerArg): IMainState { + const newVM = prepareCellVM(createEmptyCell(arg.payload.data.newCellId, null), false, arg.prevState.settings); + return insertAbove({ + ...arg, + payload: { + ...arg.payload, + data: { + cellId: arg.payload.data.cellId, + vm: newVM + } + } + }); + } + export function insertBelow(arg: NativeEditorReducerArg): IMainState { const newVM = prepareCellVM(createEmptyCell(arg.payload.data.newCellId, null), false, arg.prevState.settings); const newList = [...arg.prevState.cellVMs]; @@ -145,7 +173,7 @@ export namespace Creation { const firstCellId = arg.prevState.cellVMs.length > 0 ? arg.prevState.cellVMs[0].cell.id : undefined; // Do what an insertAbove does - return insertAbove({ + return insertNewAbove({ ...arg, payload: { ...arg.payload, data: { cellId: firstCellId, newCellId: arg.payload.data.newCellId } } }); @@ -340,11 +368,11 @@ export namespace Creation { arg.prevState.cellVMs.length > arg.payload.data.index ? arg.prevState.cellVMs[arg.payload.data.index].cell : undefined; - return insertAbove({ + return insertExistingAbove({ ...disabledQueueArg, payload: { ...arg.payload, - data: { newCellId: arg.payload.data.cell.id, cellId: cellBelow ? cellBelow.id : undefined } + data: { cell: arg.payload.data.cell, cellId: cellBelow ? cellBelow.id : undefined } } }); case 'remove_all': @@ -397,11 +425,11 @@ export namespace Creation { payload: { ...arg.payload, data: { id: arg.payload.data.id, changes: arg.payload.data.forward } } }); case 'insert': - return insertAbove({ + return insertExistingAbove({ ...disabledQueueArg, payload: { ...arg.payload, - data: { newCellId: arg.payload.data.cell.id, cellId: arg.payload.data.codeCellAboveId } + data: { cell: arg.payload.data.cell, cellId: arg.payload.data.codeCellAboveId } } }); case 'remove': diff --git a/src/datascience-ui/native-editor/redux/reducers/index.ts b/src/datascience-ui/native-editor/redux/reducers/index.ts index 5b1d96747d9c..7dc413cdf472 100644 --- a/src/datascience-ui/native-editor/redux/reducers/index.ts +++ b/src/datascience-ui/native-editor/redux/reducers/index.ts @@ -19,7 +19,7 @@ export const reducerMap: Partial = { [CommonActionType.INSERT_ABOVE_AND_FOCUS_NEW_CELL]: Creation.insertAboveAndFocusCell, [CommonActionType.INSERT_ABOVE_FIRST_AND_FOCUS_NEW_CELL]: Creation.insertAboveFirstAndFocusCell, [CommonActionType.INSERT_BELOW_AND_FOCUS_NEW_CELL]: Creation.insertBelowAndFocusCell, - [CommonActionType.INSERT_ABOVE]: Creation.insertAbove, + [CommonActionType.INSERT_ABOVE]: Creation.insertNewAbove, [CommonActionType.INSERT_ABOVE_FIRST]: Creation.insertAboveFirst, [CommonActionType.INSERT_BELOW]: Creation.insertBelow, [CommonActionType.FOCUS_CELL]: Effects.focusCell, diff --git a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts index 2bf03ba81131..f678ffb6f1c8 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts @@ -11,7 +11,6 @@ import { anything, instance, mock, when } from 'ts-mockito'; import { Matcher } from 'ts-mockito/lib/matcher/type/Matcher'; import * as typemoq from 'typemoq'; import { ConfigurationChangeEvent, EventEmitter, FileType, TextEditor, Uri, WebviewPanel } from 'vscode'; -import { CommandManager } from '../../../client/common/application/commandManager'; import { DocumentManager } from '../../../client/common/application/documentManager'; import { CustomDocument, @@ -211,8 +210,7 @@ suite('DataScience - Native Editor Provider', () => { localMemento ); - const commandManager = mock(CommandManager); - const storage = new NotebookStorageProvider(notebookStorage, [], instance(commandManager)); + const storage = new NotebookStorageProvider(notebookStorage, []); registeredProvider = new NativeEditorProvider( instance(svcContainer), diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index 705268d52a56..4c6a621186f0 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -9,7 +9,6 @@ import { Matcher } from 'ts-mockito/lib/matcher/type/Matcher'; import * as typemoq from 'typemoq'; import { ConfigurationChangeEvent, EventEmitter, FileType, TextEditor, Uri } from 'vscode'; -import { CommandManager } from '../../../client/common/application/commandManager'; import { DocumentManager } from '../../../client/common/application/documentManager'; import { IDocumentManager, @@ -330,9 +329,7 @@ suite('DataScience - Native Editor Storage', () => { localMemento ); - const commandManager = mock(CommandManager); - - return new NotebookStorageProvider(notebookStorage, [], instance(commandManager)); + return new NotebookStorageProvider(notebookStorage, []); } teardown(() => { From ed58c18e8fc30f775cdcc6260db7c17c8cf725c8 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 5 Jun 2020 14:09:52 -0700 Subject: [PATCH 08/19] Fix functional test --- .../nativeEditorProvider.functional.test.ts | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts index f678ffb6f1c8..eccd92ecc0bc 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts @@ -11,6 +11,7 @@ import { anything, instance, mock, when } from 'ts-mockito'; import { Matcher } from 'ts-mockito/lib/matcher/type/Matcher'; import * as typemoq from 'typemoq'; import { ConfigurationChangeEvent, EventEmitter, FileType, TextEditor, Uri, WebviewPanel } from 'vscode'; +import { CancellationToken } from 'vscode-languageclient'; import { DocumentManager } from '../../../client/common/application/documentManager'; import { CustomDocument, @@ -35,13 +36,17 @@ import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; import { NativeEditorProvider } from '../../../client/datascience/interactive-ipynb/nativeEditorProvider'; import { NativeEditorStorage } from '../../../client/datascience/interactive-ipynb/nativeEditorStorage'; -import { NotebookStorageProvider } from '../../../client/datascience/interactive-ipynb/notebookStorageProvider'; +import { + INotebookStorageProvider, + NotebookStorageProvider +} from '../../../client/datascience/interactive-ipynb/notebookStorageProvider'; import { JupyterExecutionFactory } from '../../../client/datascience/jupyter/jupyterExecutionFactory'; import { IJupyterExecution, INotebookEditor, INotebookModel, - INotebookServerOptions + INotebookServerOptions, + INotebookStorage } from '../../../client/datascience/types'; import { IInterpreterService } from '../../../client/interpreter/contracts'; import { InterpreterService } from '../../../client/interpreter/interpreterService'; @@ -75,6 +80,7 @@ suite('DataScience - Native Editor Provider', () => { let panel: typemoq.IMock; let file: Uri; let model: INotebookModel; + let storageProvider: INotebookStorageProvider; setup(() => { svcContainer = mock(ServiceContainer); @@ -210,7 +216,7 @@ suite('DataScience - Native Editor Provider', () => { localMemento ); - const storage = new NotebookStorageProvider(notebookStorage, []); + storageProvider = new NotebookStorageProvider(notebookStorage, []); registeredProvider = new NativeEditorProvider( instance(svcContainer), @@ -219,7 +225,7 @@ suite('DataScience - Native Editor Provider', () => { instance(workspace), instance(configService), customEditorService.object, - storage + storageProvider ); return registeredProvider; @@ -270,7 +276,7 @@ suite('DataScience - Native Editor Provider', () => { let cells = model!.cells; expect(cells).to.be.lengthOf(1); insertCell(model!, 0, 'a=1'); - await sleep(500); // Wait long enough for the storage write. + await storageProvider.backup(model, CancellationToken.None); const uri = n1.file; // Act like a reboot From d10115641a50e362a328fabc0ca6ce403b0fd5b3 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 5 Jun 2020 16:09:27 -0700 Subject: [PATCH 09/19] Add experiment --- experiments.json | 12 ++++++++++++ src/client/common/experiments/groups.ts | 8 ++++++++ src/client/datascience/serviceRegistry.ts | 12 +++++++----- .../nativeEditorProvider.functional.test.ts | 4 +--- 4 files changed, 28 insertions(+), 8 deletions(-) diff --git a/experiments.json b/experiments.json index ec630cfc9b24..cd9d4c4dfdc5 100644 --- a/experiments.json +++ b/experiments.json @@ -196,6 +196,18 @@ "salt": "RunByLine", "max": 0, "min": 0 + }, + { + "name": "CustomEditorSupport - control", + "salt": "CustomEditorSupport", + "min": 0, + "max": 100 + }, + { + "name": "CustomEditorSupport - experiment", + "salt": "CustomEditorSupport", + "max": 0, + "min": 0 } ] diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts index a1bb54269880..862d163e0a62 100644 --- a/src/client/common/experiments/groups.ts +++ b/src/client/common/experiments/groups.ts @@ -102,3 +102,11 @@ export enum DeprecatePythonPath { control = 'DeprecatePythonPath - control', experiment = 'DeprecatePythonPath - experiment' } + +/* + * Experiment to check whether the extension should deprecate `python.pythonPath` setting + */ +export enum CustomEditorSupport { + control = 'CustomEditorSupport - control', + experiment = 'CustomEditorSupport - experiment' +} diff --git a/src/client/datascience/serviceRegistry.ts b/src/client/datascience/serviceRegistry.ts index 2ab93a3d7c1a..d390799766be 100644 --- a/src/client/datascience/serviceRegistry.ts +++ b/src/client/datascience/serviceRegistry.ts @@ -4,7 +4,7 @@ import { IExtensionSingleActivationService } from '../activation/types'; import { IApplicationEnvironment } from '../common/application/types'; import { UseCustomEditorApi } from '../common/constants'; -import { NativeNotebook } from '../common/experiments/groups'; +import { CustomEditorSupport, NativeNotebook } from '../common/experiments/groups'; import { StartPage } from '../common/startPage/startPage'; import { IStartPage } from '../common/startPage/types'; import { IExperimentsManager, IExtensionContext } from '../common/types'; @@ -163,15 +163,17 @@ export function registerTypes(serviceManager: IServiceManager) { const enableProposedApi = serviceManager.get(IApplicationEnvironment).packageJson.enableProposedApi; const experiments = serviceManager.get(IExperimentsManager); const useVSCodeNotebookAPI = experiments.inExperiment(NativeNotebook.experiment); + const inCustomEditorApiExperiment = experiments.inExperiment(CustomEditorSupport.experiment); const context = serviceManager.get(IExtensionContext); - const insidersBuild = context.globalStoragePath.toLocaleLowerCase().includes('insiders'); - serviceManager.addSingletonInstance(UseCustomEditorApi, enableProposedApi && insidersBuild); + const insidersVsCodeBuild = context.globalStoragePath.toLocaleLowerCase().includes('insiders'); + const usingCustomEditor = enableProposedApi && insidersVsCodeBuild && inCustomEditorApiExperiment; + serviceManager.addSingletonInstance(UseCustomEditorApi, usingCustomEditor); // This condition is temporary. - const notebookEditorProvider = useVSCodeNotebookAPI ? NotebookEditorProvider : enableProposedApi ? NativeEditorProvider : NativeEditorProviderOld; + const notebookEditorProvider = useVSCodeNotebookAPI ? NotebookEditorProvider : usingCustomEditor ? NativeEditorProvider : NativeEditorProviderOld; serviceManager.addSingleton(INotebookEditorProvider, notebookEditorProvider); if (!useVSCodeNotebookAPI) { - serviceManager.add(INotebookEditor, enableProposedApi ? NativeEditor : NativeEditorOldWebView); + serviceManager.add(INotebookEditor, usingCustomEditor ? NativeEditor : NativeEditorOldWebView); // These are never going to be required for new VSC NB. serviceManager.add(IInteractiveWindowListener, AutoSaveService); serviceManager.addSingleton(NativeEditorSynchronizer, NativeEditorSynchronizer); diff --git a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts index eccd92ecc0bc..dda03edf2c3c 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorProvider.functional.test.ts @@ -30,7 +30,6 @@ import { ConfigurationService } from '../../../client/common/configuration/servi import { CryptoUtils } from '../../../client/common/crypto'; import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, ICryptoUtils, IExtensionContext } from '../../../client/common/types'; -import { sleep } from '../../../client/common/utils/async'; import { noop } from '../../../client/common/utils/misc'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { InteractiveWindowMessages } from '../../../client/datascience/interactive-common/interactiveWindowTypes'; @@ -45,8 +44,7 @@ import { IJupyterExecution, INotebookEditor, INotebookModel, - INotebookServerOptions, - INotebookStorage + INotebookServerOptions } from '../../../client/datascience/types'; import { IInterpreterService } from '../../../client/interpreter/contracts'; import { InterpreterService } from '../../../client/interpreter/interpreterService'; From 2922e7ccfd853c4582de468e400d19d52288a764 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 5 Jun 2020 17:00:25 -0700 Subject: [PATCH 10/19] Since package json is enabling proposed api, turn of notebooks with experiment --- src/client/common/application/notebook.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/client/common/application/notebook.ts b/src/client/common/application/notebook.ts index 3f29475b92a3..431c4d106247 100644 --- a/src/client/common/application/notebook.ts +++ b/src/client/common/application/notebook.ts @@ -14,7 +14,8 @@ import type { NotebookOutputSelector } from 'vscode-proposed'; import { UseProposedApi } from '../constants'; -import { IDisposableRegistry } from '../types'; +import { NativeNotebook } from '../experiments/groups'; +import { IDisposableRegistry, IExperimentsManager } from '../types'; import { IVSCodeNotebook, NotebookCellLanguageChangeEvent, @@ -62,9 +63,10 @@ export class VSCodeNotebook implements IVSCodeNotebook { private readonly handledCellChanges = new WeakSet(); constructor( @inject(UseProposedApi) private readonly useProposedApi: boolean, - @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry + @inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry, + @inject(IExperimentsManager) readonly experimentManager: IExperimentsManager ) { - if (this.useProposedApi) { + if (this.useProposedApi && experimentManager.inExperiment(NativeNotebook.experiment)) { this.addEventHandlers(); } } From 384e38c12e4a2d125b96bda09e044a702573846e Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 5 Jun 2020 17:05:10 -0700 Subject: [PATCH 11/19] Turn off proposed api --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 22fd4fece9e3..f3c51d4c3bf8 100644 --- a/package.json +++ b/package.json @@ -8,7 +8,7 @@ }, "languageServerVersion": "0.5.30", "publisher": "ms-python", - "enableProposedApi": true, + "enableProposedApi": false, "author": { "name": "Microsoft Corporation" }, From a6a947989e12aab286f947c0238eb27b2870401b Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Fri, 5 Jun 2020 17:33:58 -0700 Subject: [PATCH 12/19] Upgrade VS code to 1.45 so can use new api outside of insiders --- .vscode/launch.json | 2 +- data/.vscode/settings.json | 2 +- news/1 Enhancements/10744.md | 1 + package-lock.json | 6 +++--- package.json | 20 +++++++++---------- .../interactive-ipynb/nativeEditor.ts | 2 +- .../datascience/jupyter/jupyterConnection.ts | 2 +- .../jupyter/jupyterExecutionFactory.ts | 6 +++--- src/client/datascience/jupyterDebugService.ts | 6 +++--- .../kernel-launcher/kernelProcess.ts | 2 +- .../datascience/liveshare/postOffice.ts | 2 +- .../datascience/multiplexingDebugService.ts | 2 +- src/client/datascience/serviceRegistry.ts | 8 ++------ src/client/datascience/types.ts | 2 +- .../testing/explorer/testTreeViewProvider.ts | 6 +++++- src/test/datascience/activation.unit.test.ts | 4 ++-- .../datascienceSurveyBanner.unit.test.ts | 2 +- src/test/datascience/mockQuickPick.ts | 2 +- .../nativeEditor.functional.test.tsx | 6 +++--- src/test/mocks/vsc/extHostedTypes.ts | 7 ++++++- src/test/mocks/vsc/index.ts | 4 +++- 21 files changed, 51 insertions(+), 43 deletions(-) create mode 100644 news/1 Enhancements/10744.md diff --git a/.vscode/launch.json b/.vscode/launch.json index b82d033204e4..c3c2fc2cb005 100644 --- a/.vscode/launch.json +++ b/.vscode/launch.json @@ -25,7 +25,7 @@ // Enable this to turn on redux logging during debugging "XVSC_PYTHON_FORCE_LOGGING": "1", // Enable this to try out new experiments locally - "XVSC_PYTHON_LOAD_EXPERIMENTS_FROM_FILE": "1", + "VSC_PYTHON_LOAD_EXPERIMENTS_FROM_FILE": "1", // Enable this to log telemetry to the output during debugging "XVSC_PYTHON_LOG_TELEMETRY": "1", // Enable this to log debugger output. Directory must exist ahead of time diff --git a/data/.vscode/settings.json b/data/.vscode/settings.json index 99acc159fcaa..97e5bc3cb0d6 100644 --- a/data/.vscode/settings.json +++ b/data/.vscode/settings.json @@ -1,3 +1,3 @@ { - "python.pythonPath": "/usr/bin/python3" + "python.pythonPath": "C:\\Users\\rchiodo.REDMOND\\AppData\\Local\\Continuum\\miniconda3\\envs\\jupyter\\python.exe" } diff --git a/news/1 Enhancements/10744.md b/news/1 Enhancements/10744.md new file mode 100644 index 000000000000..50f1fbc4ed17 --- /dev/null +++ b/news/1 Enhancements/10744.md @@ -0,0 +1 @@ +Enable the use of the custom editor for native notebooks. \ No newline at end of file diff --git a/package-lock.json b/package-lock.json index 0e4737f93f3e..952ee1ce5037 100644 --- a/package-lock.json +++ b/package-lock.json @@ -4119,9 +4119,9 @@ } }, "@types/vscode": { - "version": "1.43.0", - "resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.43.0.tgz", - "integrity": "sha512-kIaR9qzd80rJOxePKpCB/mdy00mz8Apt2QA5Y6rdrKFn13QNFNeP3Hzmsf37Bwh/3cS7QjtAeGSK7wSqAU0sYQ==", + "version": "1.45.1", + "resolved": "https://registry.npmjs.org/@types/vscode/-/vscode-1.45.1.tgz", + "integrity": "sha512-0NO9qrrEJBO8FsqHCrFMgR2suKnwCsKBWvRSb2OzH5gs4i3QO5AhEMQYrSzDbU/wLPt7N617/rN9lPY213gmwg==", "dev": true }, "@types/webpack": { diff --git a/package.json b/package.json index f3c51d4c3bf8..6eb7df15bd2a 100644 --- a/package.json +++ b/package.json @@ -99,16 +99,16 @@ "contributes": { "customEditors": [ { - "viewType": "ms-python.python.notebook.ipynb", - "displayName": "Jupyter Notebook", - "selector": [ - { - "filenamePattern": "*.ipynb" - } - ], - "priority": "default" + "viewType": "ms-python.python.notebook.ipynb", + "displayName": "Jupyter Notebook", + "selector": [ + { + "filenamePattern": "*.ipynb" + } + ], + "priority": "default" } - ], + ], "snippets": [ { "language": "python", @@ -3174,7 +3174,7 @@ "@types/tmp": "0.0.33", "@types/untildify": "^3.0.0", "@types/uuid": "^3.4.3", - "@types/vscode": "^1.43.0", + "@types/vscode": "^1.45.0", "@types/webpack-bundle-analyzer": "^2.13.0", "@types/winreg": "^1.2.30", "@types/ws": "^6.0.1", diff --git a/src/client/datascience/interactive-ipynb/nativeEditor.ts b/src/client/datascience/interactive-ipynb/nativeEditor.ts index c1aec716108a..84455e4186e3 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditor.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditor.ts @@ -577,7 +577,7 @@ export class NativeEditor extends InteractiveBase implements INotebookEditor { // Use the current state of the model to indicate dirty (not the message itself) if (this.model && change.newDirty !== change.oldDirty) { - this.modifiedEvent.fire(); + this.modifiedEvent.fire(this); if (this.model.isDirty) { await this.postMessage(InteractiveWindowMessages.NotebookDirty); } else { diff --git a/src/client/datascience/jupyter/jupyterConnection.ts b/src/client/datascience/jupyter/jupyterConnection.ts index b6859f49aaee..7c3f87ff6db1 100644 --- a/src/client/datascience/jupyter/jupyterConnection.ts +++ b/src/client/datascience/jupyter/jupyterConnection.ts @@ -237,7 +237,7 @@ class JupyterConnection implements IJupyterConnection { if (childProc) { childProc.on('exit', (c) => { // Our code expects the exit code to be of type `number` or `undefined`. - const code = typeof c === 'number' ? c : undefined; + const code = typeof c === 'number' ? c : 0; this.valid = false; this.localProcExitCode = code; this.eventEmitter.fire(code); diff --git a/src/client/datascience/jupyter/jupyterExecutionFactory.ts b/src/client/datascience/jupyter/jupyterExecutionFactory.ts index bd2d6db607c0..1d16e3253bf2 100644 --- a/src/client/datascience/jupyter/jupyterExecutionFactory.ts +++ b/src/client/datascience/jupyter/jupyterExecutionFactory.ts @@ -49,8 +49,8 @@ type JupyterExecutionClassType = { export class JupyterExecutionFactory implements IJupyterExecution, IAsyncDisposable { private executionFactory: RoleBasedFactory; private sessionChangedEventEmitter: EventEmitter = new EventEmitter(); - private serverStartedEventEmitter: EventEmitter = new EventEmitter< - INotebookServerOptions + private serverStartedEventEmitter: EventEmitter = new EventEmitter< + INotebookServerOptions | undefined >(); constructor( @@ -92,7 +92,7 @@ export class JupyterExecutionFactory implements IJupyterExecution, IAsyncDisposa return this.sessionChangedEventEmitter.event; } - public get serverStarted(): Event { + public get serverStarted(): Event { return this.serverStartedEventEmitter.event; } diff --git a/src/client/datascience/jupyterDebugService.ts b/src/client/datascience/jupyterDebugService.ts index 9377983c5391..f23e5771fb25 100644 --- a/src/client/datascience/jupyterDebugService.ts +++ b/src/client/datascience/jupyterDebugService.ts @@ -78,7 +78,7 @@ export class JupyterDebugService implements IJupyterDebugService, IDisposable { private breakpointEmitter: EventEmitter = new EventEmitter(); private debugAdapterTrackerFactories: DebugAdapterTrackerFactory[] = []; private debugAdapterTrackers: DebugAdapterTracker[] = []; - private sessionChangedEvent: EventEmitter = new EventEmitter(); + private sessionChangedEvent: EventEmitter = new EventEmitter(); private sessionStartedEvent: EventEmitter = new EventEmitter(); private sessionTerminatedEvent: EventEmitter = new EventEmitter(); private sessionCustomEvent: EventEmitter = new EventEmitter(); @@ -280,7 +280,7 @@ export class JupyterDebugService implements IJupyterDebugService, IDisposable { await this.sendConfigurationDone(); traceInfo('Session started.'); return attachPromise.then(() => { - this.sessionStartedEvent.fire(this.session); + this.sessionStartedEvent.fire(this.session!); }); } @@ -402,7 +402,7 @@ export class JupyterDebugService implements IJupyterDebugService, IDisposable { private onClose(): void { if (this.socket) { - this.sessionTerminatedEvent.fire(this.activeDebugSession); + this.sessionTerminatedEvent.fire(this.activeDebugSession!); this.session = undefined; this.sessionChangedEvent.fire(undefined); this.debugAdapterTrackers.forEach((d) => (d.onExit ? d.onExit(0, undefined) : noop())); diff --git a/src/client/datascience/kernel-launcher/kernelProcess.ts b/src/client/datascience/kernel-launcher/kernelProcess.ts index a41c0c9908f5..0f277ece37c2 100644 --- a/src/client/datascience/kernel-launcher/kernelProcess.ts +++ b/src/client/datascience/kernel-launcher/kernelProcess.ts @@ -126,7 +126,7 @@ export class KernelProcess implements IKernelProcess { } swallowExceptions(() => { this._process?.kill(); // NOSONAR - this.exitEvent.fire(); + this.exitEvent.fire({}); }); swallowExceptions(() => this.pythonKernelLauncher?.dispose()); swallowExceptions(async () => (this.connectionFile ? this.file.deleteFile(this.connectionFile) : noop())); diff --git a/src/client/datascience/liveshare/postOffice.ts b/src/client/datascience/liveshare/postOffice.ts index 803c8b422b62..cdcd548c4366 100644 --- a/src/client/datascience/liveshare/postOffice.ts +++ b/src/client/datascience/liveshare/postOffice.ts @@ -51,7 +51,7 @@ export class PostOffice implements IAsyncDisposable { } public async dispose() { - this.peerCountChangedEmitter.fire(); + this.peerCountChangedEmitter.fire(0); this.peerCountChangedEmitter.dispose(); if (this.hostServer) { traceInfo(`Shutting down live share api`); diff --git a/src/client/datascience/multiplexingDebugService.ts b/src/client/datascience/multiplexingDebugService.ts index 78f7d4440d0d..f4801a0d4c06 100644 --- a/src/client/datascience/multiplexingDebugService.ts +++ b/src/client/datascience/multiplexingDebugService.ts @@ -29,7 +29,7 @@ import { IJupyterDebugService } from './types'; @injectable() export class MultiplexingDebugService implements IJupyterDebugService { private lastStartedService: IDebugService | undefined; - private sessionChangedEvent: EventEmitter = new EventEmitter(); + private sessionChangedEvent: EventEmitter = new EventEmitter(); private sessionStartedEvent: EventEmitter = new EventEmitter(); private sessionTerminatedEvent: EventEmitter = new EventEmitter(); private sessionCustomEvent: EventEmitter = new EventEmitter(); diff --git a/src/client/datascience/serviceRegistry.ts b/src/client/datascience/serviceRegistry.ts index d390799766be..041151058865 100644 --- a/src/client/datascience/serviceRegistry.ts +++ b/src/client/datascience/serviceRegistry.ts @@ -2,12 +2,11 @@ // Licensed under the MIT License. 'use strict'; import { IExtensionSingleActivationService } from '../activation/types'; -import { IApplicationEnvironment } from '../common/application/types'; import { UseCustomEditorApi } from '../common/constants'; import { CustomEditorSupport, NativeNotebook } from '../common/experiments/groups'; import { StartPage } from '../common/startPage/startPage'; import { IStartPage } from '../common/startPage/types'; -import { IExperimentsManager, IExtensionContext } from '../common/types'; +import { IExperimentsManager } from '../common/types'; import { ProtocolParser } from '../debugger/debugAdapter/Common/protocolParser'; import { IProtocolParser } from '../debugger/debugAdapter/types'; import { IServiceManager } from '../ioc/types'; @@ -160,13 +159,10 @@ import { // tslint:disable-next-line: max-func-body-length export function registerTypes(serviceManager: IServiceManager) { - const enableProposedApi = serviceManager.get(IApplicationEnvironment).packageJson.enableProposedApi; const experiments = serviceManager.get(IExperimentsManager); const useVSCodeNotebookAPI = experiments.inExperiment(NativeNotebook.experiment); const inCustomEditorApiExperiment = experiments.inExperiment(CustomEditorSupport.experiment); - const context = serviceManager.get(IExtensionContext); - const insidersVsCodeBuild = context.globalStoragePath.toLocaleLowerCase().includes('insiders'); - const usingCustomEditor = enableProposedApi && insidersVsCodeBuild && inCustomEditorApiExperiment; + const usingCustomEditor = inCustomEditorApiExperiment; serviceManager.addSingletonInstance(UseCustomEditorApi, usingCustomEditor); // This condition is temporary. diff --git a/src/client/datascience/types.ts b/src/client/datascience/types.ts index 8621a263d9ab..5c50a0db09ed 100644 --- a/src/client/datascience/types.ts +++ b/src/client/datascience/types.ts @@ -280,7 +280,7 @@ export interface IGatherLogger extends INotebookExecutionLogger { export const IJupyterExecution = Symbol('IJupyterExecution'); export interface IJupyterExecution extends IAsyncDisposable { sessionChanged: Event; - serverStarted: Event; + serverStarted: Event; isNotebookSupported(cancelToken?: CancellationToken): Promise; isImportSupported(cancelToken?: CancellationToken): Promise; isSpawnSupported(cancelToken?: CancellationToken): Promise; diff --git a/src/client/testing/explorer/testTreeViewProvider.ts b/src/client/testing/explorer/testTreeViewProvider.ts index 54c8a5fa00de..aa5aa4405f5a 100644 --- a/src/client/testing/explorer/testTreeViewProvider.ts +++ b/src/client/testing/explorer/testTreeViewProvider.ts @@ -46,7 +46,11 @@ export class TestTreeViewProvider implements ITestTreeViewProvider, ITestDataIte this.testsAreBeingDiscovered = new Map(); this.disposables.push(this.testService.onDidStatusChange(this.onTestStatusChanged, this)); this.testStore.onDidChange((e) => this._onDidChangeTreeData.fire(e.data), this, this.disposables); - this.workspace.onDidChangeWorkspaceFolders(() => this._onDidChangeTreeData.fire(), this, this.disposables); + this.workspace.onDidChangeWorkspaceFolders( + () => this._onDidChangeTreeData.fire(undefined), + this, + this.disposables + ); if (Array.isArray(workspace.workspaceFolders) && workspace.workspaceFolders.length > 0) { this.refresh(workspace.workspaceFolders[0].uri); diff --git a/src/test/datascience/activation.unit.test.ts b/src/test/datascience/activation.unit.test.ts index f0343590d2b9..05bdd58f433b 100644 --- a/src/test/datascience/activation.unit.test.ts +++ b/src/test/datascience/activation.unit.test.ts @@ -91,7 +91,7 @@ suite('Data Science - Activation', () => { await testCreatingDaemonWhenOpeningANotebook(); // Trigger changes to interpreter. - interpreterEventEmitter.fire(); + interpreterEventEmitter.fire(interpreter); // Wait for debounce to complete. await fakeTimer.wait(); @@ -104,7 +104,7 @@ suite('Data Science - Activation', () => { }); test('Changing interpreter without opening a notebook does not result in a daemon being created', async () => { // Trigger changes to interpreter. - interpreterEventEmitter.fire(); + interpreterEventEmitter.fire(interpreter); // Assume a debounce is required and wait. await sleep(10); diff --git a/src/test/datascience/datascienceSurveyBanner.unit.test.ts b/src/test/datascience/datascienceSurveyBanner.unit.test.ts index 9c037b926277..84fc3a9ebe35 100644 --- a/src/test/datascience/datascienceSurveyBanner.unit.test.ts +++ b/src/test/datascience/datascienceSurveyBanner.unit.test.ts @@ -219,7 +219,7 @@ function preparePopup( // Fire the number of opens specifed so that it behaves like the real editor for (let i = 0; i < initialOpenCount; i += 1) { - openedEventEmitter.fire(); + openedEventEmitter.fire({} as any); } return result; diff --git a/src/test/datascience/mockQuickPick.ts b/src/test/datascience/mockQuickPick.ts index 3f8a7b0f3675..ad9d85059542 100644 --- a/src/test/datascience/mockQuickPick.ts +++ b/src/test/datascience/mockQuickPick.ts @@ -43,7 +43,7 @@ export class MockQuickPick implements QuickPick { } public set activeItems(items: QuickPickItem[]) { this._activeItems = items; - this.didChangeActiveEmitter.fire(); + this.didChangeActiveEmitter.fire(items); } public get onDidChangeActive(): Event { return this.didChangeActiveEmitter.event; diff --git a/src/test/datascience/nativeEditor.functional.test.tsx b/src/test/datascience/nativeEditor.functional.test.tsx index e978be8f85cd..d0e261f56de6 100644 --- a/src/test/datascience/nativeEditor.functional.test.tsx +++ b/src/test/datascience/nativeEditor.functional.test.tsx @@ -2247,7 +2247,7 @@ df.head()`; // Now that the notebook is dirty, change the active editor. const docManager = ioc.get(IDocumentManager) as MockDocumentManager; - docManager.didChangeActiveTextEditorEmitter.fire(); + docManager.didChangeActiveTextEditorEmitter.fire({} as any); // Also, send notification about changes to window state. windowStateChangeHandlers.forEach((item) => item({ focused: false })); windowStateChangeHandlers.forEach((item) => item({ focused: true })); @@ -2272,7 +2272,7 @@ df.head()`; // Now that the notebook is dirty, change the active editor. const docManager = ioc.get(IDocumentManager) as MockDocumentManager; - docManager.didChangeActiveTextEditorEmitter.fire(newEditor); + docManager.didChangeActiveTextEditorEmitter.fire(newEditor!); // At this point a message should be sent to extension asking it to save. // After the save, the extension should send a message to react letting it know that it was saved successfully. @@ -2305,7 +2305,7 @@ df.head()`; // Now that the notebook is dirty, change the active editor. // This should not trigger a save of notebook (as its configured to save only when window state changes). const docManager = ioc.get(IDocumentManager) as MockDocumentManager; - docManager.didChangeActiveTextEditorEmitter.fire(); + docManager.didChangeActiveTextEditorEmitter.fire({} as any); // Confirm the message is not clean, trying to wait for it to get saved will timeout (i.e. rejected). await expect(cleanPromise).to.eventually.be.rejected; diff --git a/src/test/mocks/vsc/extHostedTypes.ts b/src/test/mocks/vsc/extHostedTypes.ts index da5d6dacf5d1..e5ca75125f05 100644 --- a/src/test/mocks/vsc/extHostedTypes.ts +++ b/src/test/mocks/vsc/extHostedTypes.ts @@ -1143,7 +1143,9 @@ export namespace vscMockExtHostedTypes { Struct = 21, Event = 22, Operator = 23, - TypeParameter = 24 + TypeParameter = 24, + User = 25, + Issue = 26 } export enum CompletionItemTag { @@ -2067,6 +2069,9 @@ export namespace vscMockExtHostedTypes { Error.captureStackTrace(this, terminator); } } + public get code(): string { + return ''; + } } //#endregion diff --git a/src/test/mocks/vsc/index.ts b/src/test/mocks/vsc/index.ts index b309c0a55e58..9192ee777265 100644 --- a/src/test/mocks/vsc/index.ts +++ b/src/test/mocks/vsc/index.ts @@ -114,7 +114,9 @@ export namespace vscMock { Struct = 21, Event = 22, Operator = 23, - TypeParameter = 24 + TypeParameter = 24, + User = 25, + Issue = 26 } export enum SymbolKind { File = 0, From a115152e75295326091c386c5ce86db28387bb4b Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 8 Jun 2020 10:01:32 -0700 Subject: [PATCH 13/19] Update package.json dynamically --- package.json | 14 +------- package.nls.json | 3 +- package_proposed.json | 19 +++++++++++ .../common/application/customEditorService.ts | 32 ++++++++++++++++--- src/client/common/utils/localize.ts | 5 +++ 5 files changed, 55 insertions(+), 18 deletions(-) create mode 100644 package_proposed.json diff --git a/package.json b/package.json index 6eb7df15bd2a..ed98127c6833 100644 --- a/package.json +++ b/package.json @@ -91,24 +91,12 @@ "onCommand:python.datascience.selectJupyterInterpreter", "onCommand:python.datascience.selectjupytercommandline", "onCommand:python.enableSourceMapSupport", - "onCustomEditor:ms-python.python.notebook.ipynb", + "onNotebookEditor:jupyter-notebook", "workspaceContains:**/mspythonconfig.json" ], "main": "./out/client/extension", "contributes": { - "customEditors": [ - { - "viewType": "ms-python.python.notebook.ipynb", - "displayName": "Jupyter Notebook", - "selector": [ - { - "filenamePattern": "*.ipynb" - } - ], - "priority": "default" - } - ], "snippets": [ { "language": "python", diff --git a/package.nls.json b/package.nls.json index 9d2143865c5b..92d924071f11 100644 --- a/package.nls.json +++ b/package.nls.json @@ -510,5 +510,6 @@ "DataScience.continueRunByLine": "Stop", "DataScience.couldNotInstallLibrary": "Could not install {0}. If pip is not available, please use the package manager of your choice to manually install this library into your Python environment.", "DataScience.rawKernelSessionFailed": "Unable to start session for kernel {0}. Select another kernel to launch with.", - "DataScience.rawKernelConnectingSession": "Connecting to kernel." + "DataScience.rawKernelConnectingSession": "Connecting to kernel.", + "DataScience.reloadCustomEditor": "Please reload VS Code to use the custom editor API" } diff --git a/package_proposed.json b/package_proposed.json new file mode 100644 index 000000000000..c539fe3a3a7a --- /dev/null +++ b/package_proposed.json @@ -0,0 +1,19 @@ +{ + "activationEvents": [ + "onCustomEditor:ms-python.python.notebook.ipynb" + ], + "contributes": { + "customEditors": [ + { + "viewType": "ms-python.python.notebook.ipynb", + "displayName": "Jupyter Notebook", + "selector": [ + { + "filenamePattern": "*.ipynb" + } + ], + "priority": "default" + } + ] + } +} \ No newline at end of file diff --git a/src/client/common/application/customEditorService.ts b/src/client/common/application/customEditorService.ts index eaac661044bb..943636c604b8 100644 --- a/src/client/common/application/customEditorService.ts +++ b/src/client/common/application/customEditorService.ts @@ -2,18 +2,28 @@ // Licensed under the MIT License. 'use strict'; import { inject, injectable } from 'inversify'; +import * as path from 'path'; import * as vscode from 'vscode'; +import { DataScience } from '../../common/utils/localize'; -import { UseCustomEditorApi } from '../constants'; +import { EXTENSION_ROOT_DIR, UseCustomEditorApi } from '../constants'; +import { IFileSystem } from '../platform/types'; import { noop } from '../utils/misc'; -import { CustomEditorProvider, ICommandManager, ICustomEditorService } from './types'; +import { CustomEditorProvider, IApplicationEnvironment, ICommandManager, ICustomEditorService } from './types'; @injectable() export class CustomEditorService implements ICustomEditorService { constructor( @inject(ICommandManager) private commandManager: ICommandManager, - @inject(UseCustomEditorApi) private readonly useCustomEditorApi: boolean - ) {} + @inject(UseCustomEditorApi) private readonly useCustomEditorApi: boolean, + @inject(IApplicationEnvironment) private readonly appEnvironment: IApplicationEnvironment, + @inject(IFileSystem) private readonly fileSystem: IFileSystem + ) { + // Double check the package json has the necessary entries for contributing a custom editor + if (this.useCustomEditorApi && !appEnvironment.packageJson.contributes?.customEditors) { + this.rewritePackageJson().ignoreErrors(); + } + } public registerCustomEditorProvider( viewType: string, @@ -36,4 +46,18 @@ export class CustomEditorService implements ICustomEditorService { await this.commandManager.executeCommand('vscode.openWith', file, viewType); } } + + private async rewritePackageJson() { + const current = this.appEnvironment.packageJson; + const improvedContents = await this.fileSystem.readFile(path.join(EXTENSION_ROOT_DIR, 'package_proposed.json')); + const improved = { + ...current, + ...JSON.parse(improvedContents) + }; + await this.fileSystem.writeFile( + path.join(EXTENSION_ROOT_DIR, 'package.json'), + JSON.stringify(improved, null, 4) + ); + this.commandManager.executeCommand('python.reloadVSCode', DataScience.reloadCustomEditor()); + } } diff --git a/src/client/common/utils/localize.ts b/src/client/common/utils/localize.ts index e445902109fc..cd45f364b6dd 100644 --- a/src/client/common/utils/localize.ts +++ b/src/client/common/utils/localize.ts @@ -947,6 +947,11 @@ export namespace DataScience { 'DataScience.rawKernelConnectingSession', 'Connecting to kernel.' ); + + export const reloadCustomEditor = localize( + 'DataScience.reloadCustomEditor', + 'Please reload VS Code to use the custom editor API' + ); } export namespace StartPage { From 7861c7d2d12eca738661803c822813499a0c95cf Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 8 Jun 2020 10:24:04 -0700 Subject: [PATCH 14/19] Fix merge code to handle arrays --- package_proposed.json => customEditor.json | 0 .../common/application/customEditorService.ts | 13 ++++++++----- 2 files changed, 8 insertions(+), 5 deletions(-) rename package_proposed.json => customEditor.json (100%) diff --git a/package_proposed.json b/customEditor.json similarity index 100% rename from package_proposed.json rename to customEditor.json diff --git a/src/client/common/application/customEditorService.ts b/src/client/common/application/customEditorService.ts index 943636c604b8..85f657605e1b 100644 --- a/src/client/common/application/customEditorService.ts +++ b/src/client/common/application/customEditorService.ts @@ -48,12 +48,15 @@ export class CustomEditorService implements ICustomEditorService { } private async rewritePackageJson() { + // tslint:disable-next-line:no-require-imports no-var-requires + const _mergeWith = require('lodash/mergeWith') as typeof import('lodash/mergeWith'); const current = this.appEnvironment.packageJson; - const improvedContents = await this.fileSystem.readFile(path.join(EXTENSION_ROOT_DIR, 'package_proposed.json')); - const improved = { - ...current, - ...JSON.parse(improvedContents) - }; + const improvedContents = await this.fileSystem.readFile(path.join(EXTENSION_ROOT_DIR, 'customEditor.json')); + const improved = _mergeWith({ ...current }, JSON.parse(improvedContents), (l, r) => { + if (Array.isArray(l) && Array.isArray(r)) { + return [...l, ...r]; + } + }); await this.fileSystem.writeFile( path.join(EXTENSION_ROOT_DIR, 'package.json'), JSON.stringify(improved, null, 4) From 4715a36c1e4aadcfbeebea414402f4a3513747a2 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 8 Jun 2020 10:42:01 -0700 Subject: [PATCH 15/19] Fix unit tests. Backup happens at the provider level now --- .../interactive-ipynb/nativeEditorStorage.unit.test.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index 4c6a621186f0..85eb6c01489f 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -9,6 +9,7 @@ import { Matcher } from 'ts-mockito/lib/matcher/type/Matcher'; import * as typemoq from 'typemoq'; import { ConfigurationChangeEvent, EventEmitter, FileType, TextEditor, Uri } from 'vscode'; +import { CancellationToken } from 'vscode-jsonrpc'; import { DocumentManager } from '../../../client/common/application/documentManager'; import { IDocumentManager, @@ -479,8 +480,8 @@ suite('DataScience - Native Editor Storage', () => { 'a' ); - // Wait for a second so it has time to update - await sleep(1000); + // Force a backup + await storage.backup(model, CancellationToken.None); // Recreate storage = createStorage(); @@ -498,8 +499,8 @@ suite('DataScience - Native Editor Storage', () => { expect(model.isDirty).to.be.equal(false, 'Editor should not be dirty'); insertCell(0, 'a=1'); - // Wait for a second so it has time to update - await sleep(1000); + // Wait for backup + await storage.backup(model, CancellationToken.None); // Recreate storage = createStorage(); From b48c8d92f07be6f3a51408b4a4608dcdae8ac822 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 8 Jun 2020 10:46:54 -0700 Subject: [PATCH 16/19] Fix hygiene --- .../interactive-ipynb/nativeEditorStorage.unit.test.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts index 85eb6c01489f..5c672f1b42d7 100644 --- a/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts +++ b/src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts @@ -25,7 +25,6 @@ import { ConfigurationService } from '../../../client/common/configuration/servi import { CryptoUtils } from '../../../client/common/crypto'; import { IFileSystem } from '../../../client/common/platform/types'; import { IConfigurationService, ICryptoUtils, IDisposable, IExtensionContext } from '../../../client/common/types'; -import { sleep } from '../../../client/common/utils/async'; import { EXTENSION_ROOT_DIR } from '../../../client/constants'; import { IEditorContentChange, From 23329a6c13fcecb404fd20e351fd570428487349 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 8 Jun 2020 11:11:21 -0700 Subject: [PATCH 17/19] Fix untitled to work for old provider too --- .../interactive-ipynb/notebookStorageProvider.ts | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts index e2381a48ef41..c6f231ebf15a 100644 --- a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts +++ b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts @@ -4,6 +4,7 @@ 'use strict'; import { inject, injectable } from 'inversify'; +import * as os from 'os'; import { EventEmitter, Uri } from 'vscode'; import { CancellationToken } from 'vscode-jsonrpc'; import { IDisposable, IDisposableRegistry } from '../../common/types'; @@ -84,11 +85,16 @@ export class NotebookStorageProvider implements INotebookStorageProvider { } private async getNextNewNotebookUri(): Promise { - // Just use the current counter. Counter will be incremented after actually opening a file. + // Because of this bug here: + // https://github.com/microsoft/vscode/issues/93441 + // We can't create 'untitled' files anymore. The untitled scheme will just be ignored. + // Instead we need to create untitled files in the temp folder and force a saveas whenever they're + // saved. + + // However if there are files already on disk, we should be able to overwrite them because + // they will only ever be used by 'open' editors. So just use the current counter for our untitled count. const fileName = `${DataScience.untitledNotebookFileName()}-${NotebookStorageProvider.untitledCounter}.ipynb`; - const fileUri = Uri.file(fileName); - // Turn this back into an untitled - return fileUri.with({ scheme: 'untitled', path: fileName }); + return Uri.parse(`untitled:///${os.tmpdir}/${fileName}`); } private trackModel(model: INotebookModel): INotebookModel { From 496b8bee697bb84486b5978bbf9653b1826b9750 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 8 Jun 2020 12:57:31 -0700 Subject: [PATCH 18/19] Some feedback and attempt to fix URI problem on linux/mac --- data/.vscode/settings.json | 2 +- src/client/datascience/common.ts | 20 ++++++++++++++++++- .../interactive-ipynb/nativeEditorProvider.ts | 14 ++----------- .../notebookStorageProvider.ts | 18 ++++------------- 4 files changed, 26 insertions(+), 28 deletions(-) diff --git a/data/.vscode/settings.json b/data/.vscode/settings.json index 97e5bc3cb0d6..99acc159fcaa 100644 --- a/data/.vscode/settings.json +++ b/data/.vscode/settings.json @@ -1,3 +1,3 @@ { - "python.pythonPath": "C:\\Users\\rchiodo.REDMOND\\AppData\\Local\\Continuum\\miniconda3\\envs\\jupyter\\python.exe" + "python.pythonPath": "/usr/bin/python3" } diff --git a/src/client/datascience/common.ts b/src/client/datascience/common.ts index ed8f4e600064..a87ff374bcc3 100644 --- a/src/client/datascience/common.ts +++ b/src/client/datascience/common.ts @@ -2,8 +2,11 @@ // Licensed under the MIT License. 'use strict'; import type { nbformat } from '@jupyterlab/coreutils'; -import { Memento } from 'vscode'; +import * as os from 'os'; +import * as path from 'path'; +import { Memento, Uri } from 'vscode'; import { splitMultilineString } from '../../datascience-ui/common'; +import { DataScience } from '../common/utils/localize'; import { noop } from '../common/utils/misc'; import { traceError, traceInfo } from '../logging'; import { Settings } from './constants'; @@ -128,3 +131,18 @@ export function translateKernelLanguageToMonaco(kernelLanguage: string): string } return kernelLanguage.toLowerCase(); } + +export function generateNewNotebookUri(counter: number): Uri { + // Because of this bug here: + // https://github.com/microsoft/vscode/issues/93441 + // We can't create 'untitled' files anymore. The untitled scheme will just be ignored. + // Instead we need to create untitled files in the temp folder and force a saveas whenever they're + // saved. + + // However if there are files already on disk, we should be able to overwrite them because + // they will only ever be used by 'open' editors. So just use the current counter for our untitled count. + const fileName = `${DataScience.untitledNotebookFileName()}-${counter}.ipynb`; + const filePath = Uri.file(path.join(os.tmpdir(), fileName)); + // Turn this back into an untitled + return filePath.with({ scheme: 'untitled', path: filePath.fsPath }); +} diff --git a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts index fe8625aa6764..0b82dc08e0fb 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorProvider.ts @@ -2,7 +2,6 @@ // Licensed under the MIT License. 'use strict'; import { inject, injectable } from 'inversify'; -import * as os from 'os'; import * as uuid from 'uuid/v4'; import { Disposable, Event, EventEmitter, Uri, WebviewPanel } from 'vscode'; import { CancellationToken } from 'vscode-languageclient'; @@ -25,9 +24,9 @@ import { IDisposableRegistry } from '../../common/types'; import { createDeferred } from '../../common/utils/async'; -import { DataScience } from '../../common/utils/localize'; import { IServiceContainer } from '../../ioc/types'; import { captureTelemetry, sendTelemetryEvent } from '../../telemetry'; +import { generateNewNotebookUri } from '../common'; import { Telemetry } from '../constants'; import { NotebookModelChange } from '../interactive-common/interactiveWindowTypes'; import { INotebookEditor, INotebookEditorProvider, INotebookModel } from '../types'; @@ -283,15 +282,6 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit } private getNextNewNotebookUri(): Uri { - // Because of this bug here: - // https://github.com/microsoft/vscode/issues/93441 - // We can't create 'untitled' files anymore. The untitled scheme will just be ignored. - // Instead we need to create untitled files in the temp folder and force a saveas whenever they're - // saved. - - // However if there are files already on disk, we should be able to overwrite them because - // they will only ever be used by 'open' editors. So just use the current counter for our untitled count. - const fileName = `${DataScience.untitledNotebookFileName()}-${this.untitledCounter}.ipynb`; - return Uri.parse(`untitled:///${os.tmpdir}/${fileName}`); + return generateNewNotebookUri(this.untitledCounter); } } diff --git a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts index c6f231ebf15a..ecb000feab12 100644 --- a/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts +++ b/src/client/datascience/interactive-ipynb/notebookStorageProvider.ts @@ -4,11 +4,10 @@ 'use strict'; import { inject, injectable } from 'inversify'; -import * as os from 'os'; import { EventEmitter, Uri } from 'vscode'; import { CancellationToken } from 'vscode-jsonrpc'; import { IDisposable, IDisposableRegistry } from '../../common/types'; -import { DataScience } from '../../common/utils/localize'; +import { generateNewNotebookUri } from '../common'; import { INotebookModel, INotebookStorage } from '../types'; import { getNextUntitledCounter } from './nativeEditorStorage'; @@ -78,23 +77,14 @@ export class NotebookStorageProvider implements INotebookStorageProvider { public async createNew(contents?: string): Promise { // Create a new URI for the dummy file using our root workspace path - const uri = await this.getNextNewNotebookUri(); + const uri = this.getNextNewNotebookUri(); // Always skip loading from the hot exit file. When creating a new file we want a new file. return this.load(uri, contents, true); } - private async getNextNewNotebookUri(): Promise { - // Because of this bug here: - // https://github.com/microsoft/vscode/issues/93441 - // We can't create 'untitled' files anymore. The untitled scheme will just be ignored. - // Instead we need to create untitled files in the temp folder and force a saveas whenever they're - // saved. - - // However if there are files already on disk, we should be able to overwrite them because - // they will only ever be used by 'open' editors. So just use the current counter for our untitled count. - const fileName = `${DataScience.untitledNotebookFileName()}-${NotebookStorageProvider.untitledCounter}.ipynb`; - return Uri.parse(`untitled:///${os.tmpdir}/${fileName}`); + private getNextNewNotebookUri(): Uri { + return generateNewNotebookUri(NotebookStorageProvider.untitledCounter); } private trackModel(model: INotebookModel): INotebookModel { From 615caf5e363924575da7e62a21ea671acdc1fe83 Mon Sep 17 00:00:00 2001 From: Rich Chiodo Date: Mon, 8 Jun 2020 13:38:37 -0700 Subject: [PATCH 19/19] Review feedback --- src/client/common/application/customEditorService.ts | 3 ++- src/client/common/experiments/groups.ts | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/client/common/application/customEditorService.ts b/src/client/common/application/customEditorService.ts index 85f657605e1b..f6bcffca0f92 100644 --- a/src/client/common/application/customEditorService.ts +++ b/src/client/common/application/customEditorService.ts @@ -6,6 +6,7 @@ import * as path from 'path'; import * as vscode from 'vscode'; import { DataScience } from '../../common/utils/localize'; +import { traceError } from '../../logging'; import { EXTENSION_ROOT_DIR, UseCustomEditorApi } from '../constants'; import { IFileSystem } from '../platform/types'; import { noop } from '../utils/misc'; @@ -21,7 +22,7 @@ export class CustomEditorService implements ICustomEditorService { ) { // Double check the package json has the necessary entries for contributing a custom editor if (this.useCustomEditorApi && !appEnvironment.packageJson.contributes?.customEditors) { - this.rewritePackageJson().ignoreErrors(); + this.rewritePackageJson().catch((e) => traceError(`Error rewriting package json: `, e)); } } diff --git a/src/client/common/experiments/groups.ts b/src/client/common/experiments/groups.ts index 862d163e0a62..dde206554c7a 100644 --- a/src/client/common/experiments/groups.ts +++ b/src/client/common/experiments/groups.ts @@ -104,7 +104,7 @@ export enum DeprecatePythonPath { } /* - * Experiment to check whether the extension should deprecate `python.pythonPath` setting + * Experiment to turn on custom editor API support. */ export enum CustomEditorSupport { control = 'CustomEditorSupport - control',