Skip to content

Commit f881e03

Browse files
rchiodorchiodo
committed
Fix backup storage by looking at the options correctly (#13983)
* Fix backup storage by looking at the options correctly * Fix backup by being more explicit * Only linux tests are failing. Hopefully fix them
1 parent 0489ddf commit f881e03

16 files changed

+93
-144
lines changed

‎news/2 Fixes/13981.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Backup on custom editors is being ignored.

‎src/client/datascience/export/exportUtil.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ export class ExportUtil {
5757
await this.jupyterExporter.exportToFile(cells, tempFile.filePath, false);
5858
const newPath = path.join(tempDir.path, '.ipynb');
5959
await this.fs.copyLocal(tempFile.filePath, newPath);
60-
model = await this.notebookStorage.getOrCreateModel(Uri.file(newPath));
60+
model = await this.notebookStorage.getOrCreateModel({ file: Uri.file(newPath) });
6161
} finally {
6262
tempFile.dispose();
6363
tempDir.dispose();
@@ -67,7 +67,7 @@ export class ExportUtil {
6767
}
6868

6969
public async removeSvgs(source: Uri) {
70-
const model = await this.notebookStorage.getOrCreateModel(source);
70+
const model = await this.notebookStorage.getOrCreateModel({ file: source });
7171

7272
const newCells: ICell[] = [];
7373
for (const cell of model.cells) {

‎src/client/datascience/interactive-ipynb/nativeEditorProviderOld.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -299,7 +299,7 @@ export class NativeEditorProviderOld extends NativeEditorProvider {
299299
this.activeEditors.set(e.file.fsPath, e);
300300

301301
// Remove backup storage
302-
this.loadModel(Uri.file(oldPath))
302+
this.loadModel({ file: Uri.file(oldPath) })
303303
.then((m) => this.storage.deleteBackup(m))
304304
.ignoreErrors();
305305
}
@@ -347,7 +347,7 @@ export class NativeEditorProviderOld extends NativeEditorProvider {
347347
* I.e. document is already opened in a VSC Notebook.
348348
*/
349349
private async isDocumentOpenedInVSCodeNotebook(document: TextDocument): Promise<boolean> {
350-
const model = await this.loadModel(document.uri);
350+
const model = await this.loadModel({ file: document.uri });
351351
// This is temporary code.
352352
return model instanceof VSCodeNotebookModel;
353353
}

‎src/client/datascience/interactive-ipynb/trustCommandHandler.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ export class TrustCommandHandler implements IExtensionSingleActivationService {
4242
return;
4343
}
4444

45-
const model = await this.storageProvider.getOrCreateModel(uri);
45+
const model = await this.storageProvider.getOrCreateModel({ file: uri });
4646
if (model.isTrusted) {
4747
return;
4848
}

‎src/client/datascience/interactive-window/interactiveWindow.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,7 +256,7 @@ export class InteractiveWindow extends InteractiveBase implements IInteractiveWi
256256
break;
257257

258258
case InteractiveWindowMessages.ExportNotebookAs:
259-
this.handleMessage(message, payload, this.exportAs);
259+
this.handleMessage(message, payload, this.exportAs.bind);
260260
break;
261261

262262
case InteractiveWindowMessages.HasCellResponse:

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

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -70,9 +70,12 @@ export class NotebookContentProvider implements INotebookContentProvider {
7070
};
7171
}
7272
// If there's no backup id, then skip loading dirty contents.
73-
const model = await (openContext.backupId
74-
? this.notebookStorage.getOrCreateModel(uri, undefined, openContext.backupId, true)
75-
: this.notebookStorage.getOrCreateModel(uri, undefined, true, true));
73+
const model = await this.notebookStorage.getOrCreateModel({
74+
file: uri,
75+
backupId: openContext.backupId,
76+
isNative: true,
77+
skipLoadingDirtyContents: openContext.backupId === undefined
78+
});
7679
if (!(model instanceof VSCodeNotebookModel)) {
7780
throw new Error('Incorrect NotebookModel, expected VSCodeNotebookModel');
7881
}
@@ -82,7 +85,7 @@ export class NotebookContentProvider implements INotebookContentProvider {
8285
}
8386
@captureTelemetry(Telemetry.Save, undefined, true)
8487
public async saveNotebook(document: NotebookDocument, cancellation: CancellationToken) {
85-
const model = await this.notebookStorage.getOrCreateModel(document.uri, undefined, undefined, true);
88+
const model = await this.notebookStorage.getOrCreateModel({ file: document.uri, isNative: true });
8689
if (cancellation.isCancellationRequested) {
8790
return;
8891
}
@@ -94,7 +97,7 @@ export class NotebookContentProvider implements INotebookContentProvider {
9497
document: NotebookDocument,
9598
cancellation: CancellationToken
9699
): Promise<void> {
97-
const model = await this.notebookStorage.getOrCreateModel(document.uri, undefined, undefined, true);
100+
const model = await this.notebookStorage.getOrCreateModel({ file: document.uri, isNative: true });
98101
if (!cancellation.isCancellationRequested) {
99102
await this.notebookStorage.saveAs(model, targetResource);
100103
}
@@ -104,7 +107,7 @@ export class NotebookContentProvider implements INotebookContentProvider {
104107
_context: NotebookDocumentBackupContext,
105108
cancellation: CancellationToken
106109
): Promise<NotebookDocumentBackup> {
107-
const model = await this.notebookStorage.getOrCreateModel(document.uri, undefined, undefined, true);
110+
const model = await this.notebookStorage.getOrCreateModel({ file: document.uri, isNative: true });
108111
const id = this.notebookStorage.generateBackupId(model);
109112
await this.notebookStorage.backup(model, cancellation, id);
110113
return {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -145,7 +145,7 @@ export class NotebookEditorProvider implements INotebookEditorProvider {
145145
return;
146146
}
147147
const uri = doc.uri;
148-
const model = await this.storage.getOrCreateModel(uri, undefined, undefined, true);
148+
const model = await this.storage.getOrCreateModel({ file: uri, isNative: true });
149149
if (model instanceof VSCodeNotebookModel) {
150150
model.associateNotebookDocument(doc);
151151
}

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

Lines changed: 18 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import {
5151
IJupyterDebugger,
5252
IJupyterVariableDataProviderFactory,
5353
IJupyterVariables,
54+
IModelLoadOptions,
5455
INotebookEditor,
5556
INotebookEditorProvider,
5657
INotebookExporter,
@@ -128,30 +129,34 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit
128129
context: CustomDocumentOpenContext, // This has info about backups. right now we use our own data.
129130
_cancellation: CancellationToken
130131
): Promise<CustomDocument> {
131-
const model = await this.loadModel(uri, undefined, context.backupId);
132+
const model = await this.loadModel({
133+
file: uri,
134+
backupId: context.backupId,
135+
skipLoadingDirtyContents: context.backupId === undefined
136+
});
132137
return {
133138
uri,
134139
dispose: () => model.dispose()
135140
};
136141
}
137142
public async saveCustomDocument(document: CustomDocument, cancellation: CancellationToken): Promise<void> {
138-
const model = await this.loadModel(document.uri);
143+
const model = await this.loadModel({ file: document.uri });
139144
return this.storage.save(model, cancellation);
140145
}
141146
public async saveCustomDocumentAs(document: CustomDocument, targetResource: Uri): Promise<void> {
142-
const model = await this.loadModel(document.uri);
147+
const model = await this.loadModel({ file: document.uri });
143148
return this.storage.saveAs(model, targetResource);
144149
}
145150
public async revertCustomDocument(document: CustomDocument, cancellation: CancellationToken): Promise<void> {
146-
const model = await this.loadModel(document.uri);
151+
const model = await this.loadModel({ file: document.uri });
147152
return this.storage.revert(model, cancellation);
148153
}
149154
public async backupCustomDocument(
150155
document: CustomDocument,
151156
_context: CustomDocumentBackupContext,
152157
cancellation: CancellationToken
153158
): Promise<CustomDocumentBackup> {
154-
const model = await this.loadModel(document.uri);
159+
const model = await this.loadModel({ file: document.uri });
155160
const id = this.storage.generateBackupId(model);
156161
await this.storage.backup(model, cancellation, id);
157162
return {
@@ -167,7 +172,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit
167172

168173
public async resolveCustomDocument(document: CustomDocument): Promise<void> {
169174
this.customDocuments.set(document.uri.fsPath, document);
170-
await this.loadModel(document.uri);
175+
await this.loadModel({ file: document.uri });
171176
}
172177

173178
public async open(file: Uri): Promise<INotebookEditor> {
@@ -199,30 +204,26 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit
199204
}
200205

201206
@captureTelemetry(Telemetry.CreateNewNotebook, undefined, false)
202-
public async createNew(contents?: string, title?: string): Promise<INotebookEditor> {
207+
public async createNew(possibleContents?: string, title?: string): Promise<INotebookEditor> {
203208
// Create a new URI for the dummy file using our root workspace path
204209
const uri = this.getNextNewNotebookUri(title);
205210

206211
// Set these contents into the storage before the file opens. Make sure not
207212
// load from the memento storage though as this is an entirely brand new file.
208-
await this.loadModel(uri, contents, true);
213+
await this.loadModel({ file: uri, possibleContents, skipLoadingDirtyContents: true });
209214

210215
return this.open(uri);
211216
}
212217

213-
public async loadModel(file: Uri, contents?: string, skipDirtyContents?: boolean): Promise<INotebookModel>;
214-
// tslint:disable-next-line: unified-signatures
215-
public async loadModel(file: Uri, contents?: string, backupId?: string): Promise<INotebookModel>;
216-
// tslint:disable-next-line: no-any
217-
public async loadModel(file: Uri, contents?: string, options?: any): Promise<INotebookModel> {
218+
public async loadModel(options: IModelLoadOptions): Promise<INotebookModel> {
218219
// Get the model that may match this file
219-
let model = [...this.models.values()].find((m) => this.fs.arePathsSame(m.file, file));
220+
let model = [...this.models.values()].find((m) => this.fs.arePathsSame(m.file, options.file));
220221
if (!model) {
221222
// Every time we load a new untitled file, up the counter past the max value for this counter
222-
this.untitledCounter = getNextUntitledCounter(file, this.untitledCounter);
223+
this.untitledCounter = getNextUntitledCounter(options.file, this.untitledCounter);
223224

224225
// Load our model from our storage object.
225-
model = await this.storage.getOrCreateModel(file, contents, options);
226+
model = await this.storage.getOrCreateModel(options);
226227

227228
// Make sure to listen to events on the model
228229
this.trackModel(model);
@@ -273,7 +274,7 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit
273274
protected async loadNotebookEditor(resource: Uri, panel?: WebviewPanel) {
274275
try {
275276
// Get the model
276-
const model = await this.loadModel(resource);
277+
const model = await this.loadModel({ file: resource });
277278

278279
// Load it (should already be visible)
279280
return this.createNotebookEditor(model, panel);

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

Lines changed: 27 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
CellState,
1818
IDataScienceFileSystem,
1919
IJupyterExecution,
20+
IModelLoadOptions,
2021
INotebookModel,
2122
INotebookStorage,
2223
ITrustService
@@ -75,27 +76,8 @@ export class NativeEditorStorage implements INotebookStorage {
7576
public get(_file: Uri): INotebookModel | undefined {
7677
return undefined;
7778
}
78-
public getOrCreateModel(
79-
file: Uri,
80-
possibleContents?: string,
81-
backupId?: string,
82-
forVSCodeNotebook?: boolean
83-
): Promise<INotebookModel>;
84-
public getOrCreateModel(
85-
file: Uri,
86-
possibleContents?: string,
87-
// tslint:disable-next-line: unified-signatures
88-
skipDirtyContents?: boolean,
89-
forVSCodeNotebook?: boolean
90-
): Promise<INotebookModel>;
91-
public getOrCreateModel(
92-
file: Uri,
93-
possibleContents?: string,
94-
// tslint:disable-next-line: no-any
95-
options?: any,
96-
forVSCodeNotebook?: boolean
97-
): Promise<INotebookModel> {
98-
return this.loadFromFile(file, possibleContents, options, forVSCodeNotebook);
79+
public getOrCreateModel(options: IModelLoadOptions): Promise<INotebookModel> {
80+
return this.loadFromFile(options);
9981
}
10082
public async save(model: INotebookModel, _cancellation: CancellationToken): Promise<void> {
10183
const contents = model.getContent();
@@ -154,8 +136,8 @@ export class NativeEditorStorage implements INotebookStorage {
154136
}
155137

156138
public async revert(model: INotebookModel, _cancellation: CancellationToken): Promise<void> {
157-
// Revert to what is in the hot exit file
158-
await this.loadFromFile(model.file);
139+
// Revert to what is in the real file. This is only used for the custom editor
140+
await this.loadFromFile({ file: model.file, skipLoadingDirtyContents: true });
159141
}
160142

161143
public async deleteBackup(model: INotebookModel, backupId: string): Promise<void> {
@@ -253,54 +235,44 @@ export class NativeEditorStorage implements INotebookStorage {
253235
noop();
254236
}
255237
}
256-
private loadFromFile(
257-
file: Uri,
258-
possibleContents?: string,
259-
backupId?: string,
260-
forVSCodeNotebook?: boolean
261-
): Promise<INotebookModel>;
262-
private loadFromFile(
263-
file: Uri,
264-
possibleContents?: string,
265-
// tslint:disable-next-line: unified-signatures
266-
skipDirtyContents?: boolean,
267-
forVSCodeNotebook?: boolean
268-
): Promise<INotebookModel>;
269-
private async loadFromFile(
270-
file: Uri,
271-
possibleContents?: string,
272-
options?: boolean | string,
273-
forVSCodeNotebook?: boolean
274-
): Promise<INotebookModel> {
238+
private async loadFromFile(options: IModelLoadOptions): Promise<INotebookModel> {
275239
try {
276240
// Attempt to read the contents if a viable file
277-
const contents = NativeEditorStorage.isUntitledFile(file) ? possibleContents : await this.fs.readFile(file);
241+
const contents = NativeEditorStorage.isUntitledFile(options.file)
242+
? options.possibleContents
243+
: await this.fs.readFile(options.file);
278244

279-
const skipDirtyContents = typeof options === 'boolean' ? options : !!options;
280-
// Use backupId provided, else use static storage key.
281-
const backupId =
282-
typeof options === 'string' ? options : skipDirtyContents ? undefined : this.getStaticStorageKey(file);
245+
// Get backup id from the options if available.
246+
const backupId = options.backupId ? options.backupId : this.getStaticStorageKey(options.file);
283247

284248
// If skipping dirty contents, delete the dirty hot exit file now
285-
if (skipDirtyContents) {
286-
await this.clearHotExit(file, backupId);
249+
if (options.skipLoadingDirtyContents) {
250+
await this.clearHotExit(options.file, backupId);
287251
}
288252

289253
// See if this file was stored in storage prior to shutdown
290-
const dirtyContents = skipDirtyContents ? undefined : await this.getStoredContents(file, backupId);
254+
const dirtyContents = options.skipLoadingDirtyContents
255+
? undefined
256+
: await this.getStoredContents(options.file, backupId);
291257
if (dirtyContents) {
292258
// This means we're dirty. Indicate dirty and load from this content
293-
return this.loadContents(file, dirtyContents, true, forVSCodeNotebook);
259+
return this.loadContents(options.file, dirtyContents, true, options.isNative);
294260
} else {
295261
// Load without setting dirty
296-
return this.loadContents(file, contents, undefined, forVSCodeNotebook);
262+
return this.loadContents(options.file, contents, undefined, options.isNative);
297263
}
298264
} catch (ex) {
299265
// May not exist at this time. Should always have a single cell though
300-
traceError(`Failed to load notebook file ${file.toString()}`, ex);
266+
traceError(`Failed to load notebook file ${options.file.toString()}`, ex);
301267
return this.factory.createModel(
302-
{ trusted: true, file, cells: [], crypto: this.crypto, globalMemento: this.globalStorage },
303-
forVSCodeNotebook
268+
{
269+
trusted: true,
270+
file: options.file,
271+
cells: [],
272+
crypto: this.crypto,
273+
globalMemento: this.globalStorage
274+
},
275+
options.isNative
304276
);
305277
}
306278
}

0 commit comments

Comments
 (0)