Skip to content

Commit e08825b

Browse files
DonJayamanneDonJayamanne
authored
Remove work arounds for SaveAs of Native Notebooks (microsoft#13236)
1 parent 62a8194 commit e08825b

9 files changed

Lines changed: 14 additions & 58 deletions

File tree

‎news/2 Fixes/13235.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Handle `Save As` of preview Notebooks.

‎src/client/datascience/interactive-common/notebookProvider.ts‎

Lines changed: 1 addition & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,6 @@ import { IDisposableRegistry, Resource } from '../../common/types';
1111
import { noop } from '../../common/utils/misc';
1212
import { Identifiers } from '../constants';
1313
import { KernelSpecInterpreter } from '../jupyter/kernels/kernelSelector';
14-
import { INotebookStorageProvider } from '../notebookStorage/notebookStorageProvider';
1514
import {
1615
ConnectNotebookProviderOptions,
1716
GetNotebookOptions,
@@ -43,10 +42,8 @@ export class NotebookProvider implements INotebookProvider {
4342
@inject(IDisposableRegistry) private readonly disposables: IDisposableRegistry,
4443
@inject(IRawNotebookProvider) private readonly rawNotebookProvider: IRawNotebookProvider,
4544
@inject(IJupyterNotebookProvider) private readonly jupyterNotebookProvider: IJupyterNotebookProvider,
46-
@inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService,
47-
@inject(INotebookStorageProvider) storageProvider: INotebookStorageProvider
45+
@inject(IWorkspaceService) private readonly workspaceService: IWorkspaceService
4846
) {
49-
disposables.push(storageProvider.onSavedAs(this.onSavedAs, this));
5047
this.rawNotebookProvider
5148
.supported()
5249
.then((b) => (this._type = b ? 'raw' : 'jupyter'))
@@ -183,13 +180,4 @@ export class NotebookProvider implements INotebookProvider {
183180
// If promise fails, then remove the promise from cache.
184181
promise.catch(removeFromCache);
185182
}
186-
187-
private async onSavedAs(e: { new: Uri; old: Uri }) {
188-
// Swap the Uris when a notebook is saved as a different file.
189-
const notebookPromise = this.notebooks.get(e.old.toString());
190-
if (notebookPromise) {
191-
this.notebooks.set(e.new.toString(), notebookPromise);
192-
this.notebooks.delete(e.old.toString());
193-
}
194-
}
195183
}

‎src/client/datascience/notebook/contentProvider.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import type {
1414
NotebookDocumentContentChangeEvent,
1515
NotebookDocumentOpenContext
1616
} from 'vscode-proposed';
17-
import { ICommandManager } from '../../common/application/types';
1817
import { MARKDOWN_LANGUAGE } from '../../common/constants';
1918
import { DataScience } from '../../common/utils/localize';
2019
import { captureTelemetry, sendTelemetryEvent, setSharedProperty } from '../../telemetry';
@@ -42,7 +41,6 @@ export class NotebookContentProvider implements INotebookContentProvider {
4241
}
4342
constructor(
4443
@inject(INotebookStorageProvider) private readonly notebookStorage: INotebookStorageProvider,
45-
@inject(ICommandManager) private readonly commandManager: ICommandManager,
4644
@inject(NotebookEditorCompatibilitySupport)
4745
private readonly compatibilitySupport: NotebookEditorCompatibilitySupport
4846
) {}
@@ -85,11 +83,7 @@ export class NotebookContentProvider implements INotebookContentProvider {
8583
if (cancellation.isCancellationRequested) {
8684
return;
8785
}
88-
if (model.isUntitled) {
89-
await this.commandManager.executeCommand('workbench.action.files.saveAs', document.uri);
90-
} else {
91-
await this.notebookStorage.save(model, cancellation);
92-
}
86+
await this.notebookStorage.save(model, cancellation);
9387
}
9488

9589
public async saveNotebookAs(

‎src/client/datascience/notebook/notebookEditorProvider.ts‎

Lines changed: 0 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import { inject, injectable } from 'inversify';
77
import { Event, EventEmitter, Uri } from 'vscode';
88
import type { NotebookDocument, NotebookEditor as VSCodeNotebookEditor } from 'vscode-proposed';
99
import { IApplicationShell, ICommandManager, IVSCodeNotebook } from '../../common/application/types';
10-
import { UseVSCodeNotebookEditorApi } from '../../common/constants';
1110
import '../../common/extensions';
1211

1312
import { IConfigurationService, IDisposableRegistry } from '../../common/types';
@@ -69,7 +68,6 @@ export class NotebookEditorProvider implements INotebookEditorProvider {
6968
@inject(IApplicationShell) private readonly appShell: IApplicationShell,
7069
@inject(IStatusProvider) private readonly statusProvider: IStatusProvider,
7170
@inject(IServiceContainer) private readonly serviceContainer: IServiceContainer,
72-
@inject(UseVSCodeNotebookEditorApi) useVSCodeNotebookEditorApi: boolean,
7371
@inject(IDataScienceFileSystem) private readonly fs: IDataScienceFileSystem
7472
) {
7573
this.disposables.push(this.vscodeNotebook.onDidOpenNotebookDocument(this.onDidOpenNotebookDocument, this));
@@ -86,21 +84,6 @@ export class NotebookEditorProvider implements INotebookEditorProvider {
8684
}
8785
})
8886
);
89-
90-
// Swap the uris.
91-
this.disposables.push(
92-
this.storage.onSavedAs((e) => {
93-
// We are interested in this ONLY if we have a VS Code NotebookEditor opened or if we belong to the nb experiment.
94-
if (!useVSCodeNotebookEditorApi && !this.vscodeNotebook.notebookDocuments.length) {
95-
return;
96-
}
97-
const savedEditor = this.notebookEditorsByUri.get(e.old.toString());
98-
if (savedEditor) {
99-
this.notebookEditorsByUri.delete(e.old.toString());
100-
this.notebookEditorsByUri.set(e.new.toString(), savedEditor);
101-
}
102-
})
103-
);
10487
}
10588

10689
public async open(file: Uri): Promise<INotebookEditor> {

‎src/client/datascience/notebookStorage/nativeEditorStorage.ts‎

Lines changed: 5 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type { nbformat } from '@jupyterlab/coreutils';
22
import { inject, injectable, named } from 'inversify';
33
import * as path from 'path';
44
import * as uuid from 'uuid/v4';
5-
import { CancellationToken, Event, EventEmitter, Memento, Uri } from 'vscode';
5+
import { CancellationToken, Memento, Uri } from 'vscode';
66
import { createCodeCell } from '../../../datascience-ui/common/cellFactory';
77
import { traceError } from '../../common/logger';
88
import { isFileNotFoundError } from '../../common/platform/errors';
@@ -24,6 +24,7 @@ import {
2424

2525
// tslint:disable-next-line:no-require-imports no-var-requires
2626
import detectIndent = require('detect-indent');
27+
import { VSCodeNotebookModel } from './vscNotebookModel';
2728

2829
const KeyPrefix = 'notebook-storage-';
2930
const NotebookTransferKey = 'notebook-transfered';
@@ -53,11 +54,6 @@ export function getNextUntitledCounter(file: Uri | undefined, currentValue: numb
5354

5455
@injectable()
5556
export class NativeEditorStorage implements INotebookStorage {
56-
public get onSavedAs(): Event<{ new: Uri; old: Uri }> {
57-
return this.savedAs.event;
58-
}
59-
private readonly savedAs = new EventEmitter<{ new: Uri; old: Uri }>();
60-
6157
// Keep track of if we are backing up our file already
6258
private backingUp = false;
6359
// If backup requests come in while we are already backing up save the most recent one here
@@ -119,13 +115,15 @@ export class NativeEditorStorage implements INotebookStorage {
119115
}
120116

121117
public async saveAs(model: INotebookModel, file: Uri): Promise<void> {
122-
const old = model.file;
123118
const contents = model.getContent();
124119
const parallelize = [this.fs.writeFile(file, contents)];
125120
if (model.isTrusted) {
126121
parallelize.push(this.trustService.trustNotebook(file, contents));
127122
}
128123
await Promise.all(parallelize);
124+
if (model instanceof VSCodeNotebookModel) {
125+
return;
126+
}
129127
model.update({
130128
source: 'user',
131129
kind: 'saveAs',
@@ -134,7 +132,6 @@ export class NativeEditorStorage implements INotebookStorage {
134132
target: file,
135133
sourceUri: model.file
136134
});
137-
this.savedAs.fire({ new: file, old });
138135
}
139136
public async backup(model: INotebookModel, cancellation: CancellationToken, backupId?: string): Promise<void> {
140137
// If we are already backing up, save this request replacing any other previous requests

‎src/client/datascience/notebookStorage/notebookStorageProvider.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import { IDisposable, IDisposableRegistry } from '../../common/types';
1010
import { generateNewNotebookUri } from '../common';
1111
import { INotebookModel, INotebookStorage } from '../types';
1212
import { getNextUntitledCounter } from './nativeEditorStorage';
13+
import { VSCodeNotebookModel } from './vscNotebookModel';
1314

1415
// tslint:disable-next-line:no-require-imports no-var-requires
1516

@@ -32,14 +33,16 @@ export class NotebookStorageProvider implements INotebookStorageProvider {
3233
@inject(IDisposableRegistry) disposables: IDisposableRegistry
3334
) {
3435
disposables.push(this);
35-
disposables.push(storage.onSavedAs((e) => this._savedAs.fire(e)));
3636
}
3737
public async save(model: INotebookModel, cancellation: CancellationToken) {
3838
await this.storage.save(model, cancellation);
3939
}
4040
public async saveAs(model: INotebookModel, targetResource: Uri) {
4141
const oldUri = model.file;
4242
await this.storage.saveAs(model, targetResource);
43+
if (model instanceof VSCodeNotebookModel) {
44+
return;
45+
}
4346
this.trackModel(model);
4447
this.storageAndModels.delete(oldUri.toString());
4548
this.storageAndModels.set(targetResource.toString(), Promise.resolve(model));

‎src/client/datascience/types.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1104,7 +1104,6 @@ export interface INotebookModel {
11041104
export const INotebookStorage = Symbol('INotebookStorage');
11051105

11061106
export interface INotebookStorage {
1107-
readonly onSavedAs: Event<{ new: Uri; old: Uri }>;
11081107
generateBackupId(model: INotebookModel): string;
11091108
save(model: INotebookModel, cancellation: CancellationToken): Promise<void>;
11101109
saveAs(model: INotebookModel, targetResource: Uri): Promise<void>;

‎src/test/datascience/interactive-common/notebookProvider.unit.test.ts‎

Lines changed: 1 addition & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@ import * as vscode from 'vscode';
77
import { IWorkspaceService } from '../../../client/common/application/types';
88
import { IDataScienceSettings, IDisposableRegistry, IPythonSettings } from '../../../client/common/types';
99
import { NotebookProvider } from '../../../client/datascience/interactive-common/notebookProvider';
10-
import { INotebookStorageProvider } from '../../../client/datascience/notebookStorage/notebookStorageProvider';
1110
import { IJupyterNotebookProvider, INotebook, IRawNotebookProvider } from '../../../client/datascience/types';
1211

1312
function Uri(filename: string): vscode.Uri {
@@ -42,7 +41,6 @@ suite('DataScience - NotebookProvider', () => {
4241
// Set up our settings
4342
pythonSettings = mock<IPythonSettings>();
4443
dataScienceSettings = mock<IDataScienceSettings>();
45-
const storageProvider = mock<INotebookStorageProvider>();
4644
when(pythonSettings.datascience).thenReturn(instance(dataScienceSettings));
4745
when(workspaceService.hasWorkspaceFolders).thenReturn(false);
4846
when(dataScienceSettings.jupyterServerURI).thenReturn('local');
@@ -53,8 +51,7 @@ suite('DataScience - NotebookProvider', () => {
5351
instance(disposableRegistry),
5452
instance(rawNotebookProvider),
5553
instance(jupyterNotebookProvider),
56-
instance(workspaceService),
57-
instance(storageProvider)
54+
instance(workspaceService)
5855
);
5956
});
6057

‎src/test/datascience/notebook/contentProvider.unit.test.ts‎

Lines changed: 1 addition & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,6 @@ import { Uri } from 'vscode';
1010
const vscodeNotebookEnums = require('vscode') as typeof import('vscode-proposed');
1111
import type { NotebookContentProvider as VSCodeNotebookContentProvider } from 'vscode-proposed';
1212
import { NotebookCellData } from '../../../../typings/vscode-proposed';
13-
import { ICommandManager } from '../../../client/common/application/types';
1413
import { MARKDOWN_LANGUAGE, PYTHON_LANGUAGE } from '../../../client/common/constants';
1514
import { NotebookContentProvider } from '../../../client/datascience/notebook/contentProvider';
1615
import { NotebookEditorCompatibilitySupport } from '../../../client/datascience/notebook/notebookEditorCompatibilitySupport';
@@ -23,15 +22,10 @@ suite('DataScience - NativeNotebook ContentProvider', () => {
2322
const fileUri = Uri.file('a.ipynb');
2423
setup(async () => {
2524
storageProvider = mock<INotebookStorageProvider>();
26-
const commandManager = mock<ICommandManager>();
2725
const compatSupport = mock(NotebookEditorCompatibilitySupport);
2826
when(compatSupport.canOpenWithOurNotebookEditor(anything())).thenReturn(true);
2927
when(compatSupport.canOpenWithVSCodeNotebookEditor(anything())).thenReturn(true);
30-
contentProvider = new NotebookContentProvider(
31-
instance(storageProvider),
32-
instance(commandManager),
33-
instance(compatSupport)
34-
);
28+
contentProvider = new NotebookContentProvider(instance(storageProvider), instance(compatSupport));
3529
});
3630
[true, false].forEach((isNotebookTrusted) => {
3731
suite(isNotebookTrusted ? 'Trusted Notebook' : 'Un-trusted notebook', () => {

0 commit comments

Comments
 (0)