From 35abf6c2815f345a4425e016e4485e6b45afc461 Mon Sep 17 00:00:00 2001 From: Joyce Er Date: Tue, 14 Jul 2020 09:51:58 -0700 Subject: [PATCH 1/4] Preserve json layout in getContent --- src/client/datascience/notebookStorage/baseModel.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/client/datascience/notebookStorage/baseModel.ts b/src/client/datascience/notebookStorage/baseModel.ts index 89fbaf7c5bad..452534c162e5 100644 --- a/src/client/datascience/notebookStorage/baseModel.ts +++ b/src/client/datascience/notebookStorage/baseModel.ts @@ -196,12 +196,8 @@ export abstract class BaseNotebookModel implements INotebookModel { this.ensureNotebookJson(); // Reuse our original json except for the cells. - const json = { - cells: this.cells.map((c) => pruneCell(c.data)), - metadata: this.notebookJson.metadata, - nbformat: this.notebookJson.nbformat, - nbformat_minor: this.notebookJson.nbformat_minor - }; + const json = { ...this.notebookJson }; + json.cells = this.cells.map((c) => pruneCell(c.data)); return JSON.stringify(json, null, this.indentAmount); } } From 83b4d496e8372bed226b6c8e05fcf85f40f606fc Mon Sep 17 00:00:00 2001 From: Joyce Er Date: Tue, 14 Jul 2020 11:19:05 -0700 Subject: [PATCH 2/4] Use getContent for checking isNotebookTrusted --- .../interactive-ipynb/nativeEditorStorage.ts | 31 ++++++++++++------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 10f2e29eb8ed..1eaf7d42c690 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -348,18 +348,9 @@ export class NativeEditorStorage implements INotebookStorage { } const pythonNumber = json ? await this.extractPythonMainVersion(json) : 3; - /* As an optimization, we don't call trustNotebook for hot exit, since our hot exit backup code gets called by VS - Code whenever the notebook model changes. This means it's called very often, perhaps even as often as autosave. - Instead, when loading a file that is dirty, we check if the actual file contents on disk are trusted. If so, we treat - the dirty contents as trusted as well. */ - const contentsToCheck = isInitiallyDirty && trueContents !== undefined ? trueContents : contents; - const isTrusted = - contents === undefined || isUntitledFile(file) - ? true // If no contents or untitled, this is a newly created file, so it should be trusted - : await this.trustService.isNotebookTrusted(file, contentsToCheck!); - return this.factory.createModel( + const model = this.factory.createModel( { - trusted: isTrusted, + trusted: true, file, cells: remapped, notebookJson: json, @@ -369,6 +360,24 @@ export class NativeEditorStorage implements INotebookStorage { }, forVSCodeNotebook ); + + /* As an optimization, we don't call trustNotebook for hot exit, since our hot exit backup code gets called by VS + Code whenever the notebook model changes. This means it's called very often, perhaps even as often as autosave. + Instead, when loading a file that is dirty, we check if the actual file contents on disk are trusted. If so, we treat + the dirty contents as trusted as well. */ + if (contents !== undefined && !isUntitledFile(file)) { + // If no contents or untitled, this is a newly created file, so it should be trusted + const contentsToCheck = isInitiallyDirty && trueContents !== undefined ? trueContents : model.getContent(); + model.update({ + source: 'user', + kind: 'updateTrust', + oldDirty: model.isDirty, + newDirty: model.isDirty, + isNotebookTrusted: await this.trustService.isNotebookTrusted(file, contentsToCheck) + }); + } + + return model; } private getStaticStorageKey(file: Uri): string { From 3ac4b9f6852403642a2334eb3536cfd6f4f391b0 Mon Sep 17 00:00:00 2001 From: Joyce Er Date: Tue, 14 Jul 2020 11:28:03 -0700 Subject: [PATCH 3/4] Fix comments --- .../interactive-ipynb/nativeEditorStorage.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 1eaf7d42c690..5e1d39f3984b 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -361,12 +361,12 @@ export class NativeEditorStorage implements INotebookStorage { forVSCodeNotebook ); - /* As an optimization, we don't call trustNotebook for hot exit, since our hot exit backup code gets called by VS - Code whenever the notebook model changes. This means it's called very often, perhaps even as often as autosave. - Instead, when loading a file that is dirty, we check if the actual file contents on disk are trusted. If so, we treat - the dirty contents as trusted as well. */ + // If no contents or untitled, this is a newly created file, so the model should remain trusted if (contents !== undefined && !isUntitledFile(file)) { - // If no contents or untitled, this is a newly created file, so it should be trusted + /* As an optimization, we don't call trustNotebook for hot exit, since our hot exit backup code gets called by VS + Code whenever the notebook model changes. This means it's called very often, perhaps even as often as autosave. + Instead, when loading a file that is dirty, we check if the actual file contents on disk are trusted. If so, we treat + the dirty contents as trusted as well. */ const contentsToCheck = isInitiallyDirty && trueContents !== undefined ? trueContents : model.getContent(); model.update({ source: 'user', From f6afa36e2efde69bfd55980695dca28705d0e780 Mon Sep 17 00:00:00 2001 From: Joyce Er Date: Tue, 14 Jul 2020 14:46:39 -0700 Subject: [PATCH 4/4] PR feedback --- .../interactive-ipynb/nativeEditorStorage.ts | 32 +++++++++---------- 1 file changed, 15 insertions(+), 17 deletions(-) diff --git a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts index 5e1d39f3984b..d9600bf8f483 100644 --- a/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts +++ b/src/client/datascience/interactive-ipynb/nativeEditorStorage.ts @@ -282,10 +282,10 @@ export class NativeEditorStorage implements INotebookStorage { const dirtyContents = skipDirtyContents ? undefined : await this.getStoredContents(file, backupId); if (dirtyContents) { // This means we're dirty. Indicate dirty and load from this content - return this.loadContents(file, dirtyContents, true, contents, forVSCodeNotebook); + return this.loadContents(file, dirtyContents, true, forVSCodeNotebook); } else { // Load without setting dirty - return this.loadContents(file, contents, undefined, undefined, forVSCodeNotebook); + return this.loadContents(file, contents, undefined, forVSCodeNotebook); } } catch (ex) { // May not exist at this time. Should always have a single cell though @@ -308,7 +308,6 @@ export class NativeEditorStorage implements INotebookStorage { file: Uri, contents: string | undefined, isInitiallyDirty = false, - trueContents?: string, forVSCodeNotebook?: boolean ) { // tslint:disable-next-line: no-any @@ -361,20 +360,19 @@ export class NativeEditorStorage implements INotebookStorage { forVSCodeNotebook ); - // If no contents or untitled, this is a newly created file, so the model should remain trusted - if (contents !== undefined && !isUntitledFile(file)) { - /* As an optimization, we don't call trustNotebook for hot exit, since our hot exit backup code gets called by VS - Code whenever the notebook model changes. This means it's called very often, perhaps even as often as autosave. - Instead, when loading a file that is dirty, we check if the actual file contents on disk are trusted. If so, we treat - the dirty contents as trusted as well. */ - const contentsToCheck = isInitiallyDirty && trueContents !== undefined ? trueContents : model.getContent(); - model.update({ - source: 'user', - kind: 'updateTrust', - oldDirty: model.isDirty, - newDirty: model.isDirty, - isNotebookTrusted: await this.trustService.isNotebookTrusted(file, contentsToCheck) - }); + // If no contents or untitled, this is a newly created file + // If dirty, that means it's been edited before in our extension + if (contents !== undefined && !isUntitledFile(file) && !isInitiallyDirty) { + const isNotebookTrusted = await this.trustService.isNotebookTrusted(file, model.getContent()); + if (isNotebookTrusted !== model.isTrusted) { + model.update({ + source: 'user', + kind: 'updateTrust', + oldDirty: model.isDirty, + newDirty: model.isDirty, + isNotebookTrusted + }); + } } return model;