Skip to content

Commit c3af16d

Browse files
rchiodorchiodo
authored
Fix save on close (#13567)
* Pass model through command instead of URI in order to use directly * Add a test to verify we don't regress * Finish fixing tests * Add news entry * Fix unit tests
1 parent 6c09b63 commit c3af16d

File tree

13 files changed

+286
-65
lines changed

13 files changed

+286
-65
lines changed

‎news/2 Fixes/11711.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
Fix saving during close and auto backup to actually save a notebook.

‎src/client/common/application/commands.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -195,8 +195,8 @@ export interface ICommandNameArgumentTypeMapping extends ICommandNameWithoutArgu
195195
[DSCommands.SetJupyterKernel]: [KernelConnectionMetadata, Uri, undefined | Uri];
196196
[DSCommands.SwitchJupyterKernel]: [ISwitchKernelOptions | undefined];
197197
[DSCommands.SelectJupyterCommandLine]: [undefined | Uri];
198-
[DSCommands.SaveNotebookNonCustomEditor]: [Uri];
199-
[DSCommands.SaveAsNotebookNonCustomEditor]: [Uri, Uri];
198+
[DSCommands.SaveNotebookNonCustomEditor]: [INotebookModel];
199+
[DSCommands.SaveAsNotebookNonCustomEditor]: [INotebookModel, Uri];
200200
[DSCommands.OpenNotebookNonCustomEditor]: [Uri];
201201
[DSCommands.GatherQuality]: [string];
202202
[DSCommands.LatestExtension]: [string];

‎src/client/datascience/gather/gatherLogger.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import { inject, injectable } from 'inversify';
33
import cloneDeep = require('lodash/cloneDeep');
44
import { extensions } from 'vscode';
55
import { concatMultilineString } from '../../../datascience-ui/common';
6-
import { traceError } from '../../common/logger';
6+
import { traceError, traceInfo } from '../../common/logger';
77
import { IConfigurationService } from '../../common/types';
88
import { noop } from '../../common/utils/misc';
99
import { sendTelemetryEvent } from '../../telemetry';
@@ -69,7 +69,11 @@ export class GatherLogger implements IGatherLogger {
6969
}
7070
}
7171
const api = ext.exports;
72-
this.gather = api.getGatherProvider();
72+
try {
73+
this.gather = api.getGatherProvider();
74+
} catch {
75+
traceInfo(`Gather not installed`);
76+
}
7377
}
7478
}
7579
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -269,7 +269,7 @@ export class NativeEditorOldWebView extends NativeEditor {
269269
}
270270
try {
271271
if (!this.isUntitled) {
272-
await this.commandManager.executeCommand(Commands.SaveNotebookNonCustomEditor, this.model?.file);
272+
await this.commandManager.executeCommand(Commands.SaveNotebookNonCustomEditor, this.model);
273273
this.savedEvent.fire(this);
274274
return;
275275
}
@@ -295,7 +295,7 @@ export class NativeEditorOldWebView extends NativeEditor {
295295
if (fileToSaveTo) {
296296
await this.commandManager.executeCommand(
297297
Commands.SaveAsNotebookNonCustomEditor,
298-
this.model.file,
298+
this.model,
299299
fileToSaveTo
300300
);
301301
this.savedEvent.fire(this);

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

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -83,7 +83,7 @@ export class NativeEditorProviderOld extends NativeEditorProvider {
8383
@inject(IWorkspaceService) workspace: IWorkspaceService,
8484
@inject(IConfigurationService) configuration: IConfigurationService,
8585
@inject(ICustomEditorService) customEditorService: ICustomEditorService,
86-
@inject(IDataScienceFileSystem) private fs: IDataScienceFileSystem,
86+
@inject(IDataScienceFileSystem) fs: IDataScienceFileSystem,
8787
@inject(IDocumentManager) private documentManager: IDocumentManager,
8888
@inject(ICommandManager) private readonly cmdManager: ICommandManager,
8989
@inject(IDataScienceErrorHandler) private dataScienceErrorHandler: IDataScienceErrorHandler,
@@ -98,7 +98,8 @@ export class NativeEditorProviderOld extends NativeEditorProvider {
9898
configuration,
9999
customEditorService,
100100
storage,
101-
notebookProvider
101+
notebookProvider,
102+
fs
102103
);
103104

104105
// No live share sync required as open document from vscode will give us our contents.
@@ -107,21 +108,18 @@ export class NativeEditorProviderOld extends NativeEditorProvider {
107108
this.documentManager.onDidChangeActiveTextEditor(this.onDidChangeActiveTextEditorHandler.bind(this))
108109
);
109110
this.disposables.push(
110-
this.cmdManager.registerCommand(Commands.SaveNotebookNonCustomEditor, async (resource: Uri) => {
111-
const customDocument = this.customDocuments.get(resource.fsPath);
112-
if (customDocument) {
113-
await this.saveCustomDocument(customDocument, new CancellationTokenSource().token);
114-
}
111+
this.cmdManager.registerCommand(Commands.SaveNotebookNonCustomEditor, async (model: INotebookModel) => {
112+
await this.storage.save(model, new CancellationTokenSource().token);
115113
})
116114
);
117115
this.disposables.push(
118116
this.cmdManager.registerCommand(
119117
Commands.SaveAsNotebookNonCustomEditor,
120-
async (resource: Uri, targetResource: Uri) => {
121-
const customDocument = this.customDocuments.get(resource.fsPath);
118+
async (model: INotebookModel, targetResource: Uri) => {
119+
await this.storage.saveAs(model, targetResource);
120+
const customDocument = this.customDocuments.get(model.file.fsPath);
122121
if (customDocument) {
123-
await this.saveCustomDocumentAs(customDocument, targetResource);
124-
this.customDocuments.delete(resource.fsPath);
122+
this.customDocuments.delete(model.file.fsPath);
125123
this.customDocuments.set(targetResource.fsPath, { ...customDocument, uri: targetResource });
126124
}
127125
}

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

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,8 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit
108108
@inject(IConfigurationService) protected readonly configuration: IConfigurationService,
109109
@inject(ICustomEditorService) private customEditorService: ICustomEditorService,
110110
@inject(INotebookStorageProvider) protected readonly storage: INotebookStorageProvider,
111-
@inject(INotebookProvider) private readonly notebookProvider: INotebookProvider
111+
@inject(INotebookProvider) private readonly notebookProvider: INotebookProvider,
112+
@inject(IDataScienceFileSystem) protected readonly fs: IDataScienceFileSystem
112113
) {
113114
traceInfo(`id is ${this._id}`);
114115

@@ -214,14 +215,18 @@ export class NativeEditorProvider implements INotebookEditorProvider, CustomEdit
214215
public async loadModel(file: Uri, contents?: string, backupId?: string): Promise<INotebookModel>;
215216
// tslint:disable-next-line: no-any
216217
public async loadModel(file: Uri, contents?: string, options?: any): Promise<INotebookModel> {
217-
// Every time we load a new untitled file, up the counter past the max value for this counter
218-
this.untitledCounter = getNextUntitledCounter(file, this.untitledCounter);
218+
// Get the model that may match this file
219+
let model = [...this.models.values()].find((m) => this.fs.arePathsSame(m.file, file));
220+
if (!model) {
221+
// 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);
219223

220-
// Load our model from our storage object.
221-
const model = await this.storage.getOrCreateModel(file, contents, options);
224+
// Load our model from our storage object.
225+
model = await this.storage.getOrCreateModel(file, contents, options);
222226

223-
// Make sure to listen to events on the model
224-
this.trackModel(model);
227+
// Make sure to listen to events on the model
228+
this.trackModel(model);
229+
}
225230
return model;
226231
}
227232

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

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ import {
2626
import detectIndent = require('detect-indent');
2727
import { VSCodeNotebookModel } from './vscNotebookModel';
2828

29-
const KeyPrefix = 'notebook-storage-';
29+
export const KeyPrefix = 'notebook-storage-';
3030
const NotebookTransferKey = 'notebook-transfered';
3131

3232
export function getNextUntitledCounter(file: Uri | undefined, currentValue: number): number {
@@ -178,21 +178,29 @@ export class NativeEditorStorage implements INotebookStorage {
178178
// Keep track of the time when this data was saved.
179179
// This way when we retrieve the data we can compare it against last modified date of the file.
180180
const specialContents = contents ? JSON.stringify({ contents, lastModifiedTimeMs: Date.now() }) : undefined;
181-
return this.writeToStorage(filePath, specialContents, cancelToken);
181+
return this.writeToStorage(model.file, filePath, specialContents, cancelToken);
182182
}
183183

184184
private async clearHotExit(file: Uri, backupId?: string): Promise<void> {
185185
const key = backupId || this.getStaticStorageKey(file);
186186
const filePath = this.getHashedFileName(key);
187-
await this.writeToStorage(filePath);
187+
await this.writeToStorage(undefined, filePath);
188188
}
189189

190-
private async writeToStorage(filePath: string, contents?: string, cancelToken?: CancellationToken): Promise<void> {
190+
private async writeToStorage(
191+
owningFile: Uri | undefined,
192+
filePath: string,
193+
contents?: string,
194+
cancelToken?: CancellationToken
195+
): Promise<void> {
191196
try {
192197
if (!cancelToken?.isCancellationRequested) {
193198
if (contents) {
194199
await this.fs.createLocalDirectory(path.dirname(filePath));
195200
if (!cancelToken?.isCancellationRequested) {
201+
if (owningFile) {
202+
this.trustService.trustNotebook(owningFile, contents).ignoreErrors();
203+
}
196204
await this.fs.writeLocalFile(filePath, contents);
197205
}
198206
} else {
@@ -374,6 +382,8 @@ export class NativeEditorStorage implements INotebookStorage {
374382
if (isNotebookTrusted) {
375383
model.trust();
376384
}
385+
} else {
386+
model.trust();
377387
}
378388

379389
return model;
@@ -407,9 +417,10 @@ export class NativeEditorStorage implements INotebookStorage {
407417
}
408418

409419
private async getStoredContentsFromFile(file: Uri, key: string): Promise<string | undefined> {
420+
const filePath = this.getHashedFileName(key);
410421
try {
411422
// Use this to read from the extension global location
412-
const contents = await this.fs.readLocalFile(file.fsPath);
423+
const contents = await this.fs.readLocalFile(filePath);
413424
const data = JSON.parse(contents);
414425
// Check whether the file has been modified since the last time the contents were saved.
415426
if (data && data.lastModifiedTimeMs && file.scheme === 'file') {

‎src/datascience-ui/interactive-common/redux/store.ts

Lines changed: 40 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@ import { forceLoad } from '../transforms';
3131
import { isAllowedAction, isAllowedMessage, postActionToExtension } from './helpers';
3232
import { generatePostOfficeSendReducer } from './postOffice';
3333
import { generateMonacoReducer, IMonacoState } from './reducers/monaco';
34+
import { CommonActionType } from './reducers/types';
3435
import { generateVariableReducer, IVariableState } from './reducers/variables';
3536

3637
function generateDefaultState(
@@ -109,19 +110,21 @@ function createSendInfoMiddleware(): Redux.Middleware<{}, IStore> {
109110
}
110111

111112
// If cell vm count changed or selected cell changed, send the message
112-
const currentSelection = getSelectedAndFocusedInfo(afterState.main);
113-
if (
114-
prevState.main.cellVMs.length !== afterState.main.cellVMs.length ||
115-
getSelectedAndFocusedInfo(prevState.main).selectedCellId !== currentSelection.selectedCellId ||
116-
prevState.main.undoStack.length !== afterState.main.undoStack.length ||
117-
prevState.main.redoStack.length !== afterState.main.redoStack.length
118-
) {
119-
postActionToExtension({ queueAction: store.dispatch }, InteractiveWindowMessages.SendInfo, {
120-
cellCount: afterState.main.cellVMs.length,
121-
undoCount: afterState.main.undoStack.length,
122-
redoCount: afterState.main.redoStack.length,
123-
selectedCell: currentSelection.selectedCellId
124-
});
113+
if (!action.type || action.type !== CommonActionType.UNMOUNT) {
114+
const currentSelection = getSelectedAndFocusedInfo(afterState.main);
115+
if (
116+
prevState.main.cellVMs.length !== afterState.main.cellVMs.length ||
117+
getSelectedAndFocusedInfo(prevState.main).selectedCellId !== currentSelection.selectedCellId ||
118+
prevState.main.undoStack.length !== afterState.main.undoStack.length ||
119+
prevState.main.redoStack.length !== afterState.main.redoStack.length
120+
) {
121+
postActionToExtension({ queueAction: store.dispatch }, InteractiveWindowMessages.SendInfo, {
122+
cellCount: afterState.main.cellVMs.length,
123+
undoCount: afterState.main.undoStack.length,
124+
redoCount: afterState.main.redoStack.length,
125+
selectedCell: currentSelection.selectedCellId
126+
});
127+
}
125128
}
126129
return res;
127130
};
@@ -159,21 +162,26 @@ function createTestMiddleware(): Redux.Middleware<{}, IStore> {
159162
});
160163
};
161164

162-
// Special case for focusing a cell
163-
const previousSelection = getSelectedAndFocusedInfo(prevState.main);
164-
const currentSelection = getSelectedAndFocusedInfo(afterState.main);
165-
if (previousSelection.focusedCellId !== currentSelection.focusedCellId && currentSelection.focusedCellId) {
166-
// Send async so happens after render state changes (so our enzyme wrapper is up to date)
167-
sendMessage(InteractiveWindowMessages.FocusedCellEditor, { cellId: action.payload.cellId });
168-
}
169-
if (previousSelection.selectedCellId !== currentSelection.selectedCellId && currentSelection.selectedCellId) {
170-
// Send async so happens after render state changes (so our enzyme wrapper is up to date)
171-
sendMessage(InteractiveWindowMessages.SelectedCell, { cellId: action.payload.cellId });
172-
}
173-
// Special case for unfocusing a cell
174-
if (previousSelection.focusedCellId !== currentSelection.focusedCellId && !currentSelection.focusedCellId) {
175-
// Send async so happens after render state changes (so our enzyme wrapper is up to date)
176-
sendMessage(InteractiveWindowMessages.UnfocusedCellEditor);
165+
if (!action.type || action.type !== CommonActionType.UNMOUNT) {
166+
// Special case for focusing a cell
167+
const previousSelection = getSelectedAndFocusedInfo(prevState.main);
168+
const currentSelection = getSelectedAndFocusedInfo(afterState.main);
169+
if (previousSelection.focusedCellId !== currentSelection.focusedCellId && currentSelection.focusedCellId) {
170+
// Send async so happens after render state changes (so our enzyme wrapper is up to date)
171+
sendMessage(InteractiveWindowMessages.FocusedCellEditor, { cellId: action.payload.cellId });
172+
}
173+
if (
174+
previousSelection.selectedCellId !== currentSelection.selectedCellId &&
175+
currentSelection.selectedCellId
176+
) {
177+
// Send async so happens after render state changes (so our enzyme wrapper is up to date)
178+
sendMessage(InteractiveWindowMessages.SelectedCell, { cellId: action.payload.cellId });
179+
}
180+
// Special case for unfocusing a cell
181+
if (previousSelection.focusedCellId !== currentSelection.focusedCellId && !currentSelection.focusedCellId) {
182+
// Send async so happens after render state changes (so our enzyme wrapper is up to date)
183+
sendMessage(InteractiveWindowMessages.UnfocusedCellEditor);
184+
}
177185
}
178186

179187
// Indicate settings updates
@@ -218,7 +226,10 @@ function createTestMiddleware(): Redux.Middleware<{}, IStore> {
218226
sendMessage(InteractiveWindowMessages.ExecutionRendered);
219227
}
220228

221-
if (!action.type || action.type !== InteractiveWindowMessages.FinishCell) {
229+
if (
230+
!action.type ||
231+
(action.type !== InteractiveWindowMessages.FinishCell && action.type !== CommonActionType.UNMOUNT)
232+
) {
222233
// Might be a non finish but still update cells (like an undo or a delete)
223234
const prevFinished = prevState.main.cellVMs
224235
.filter((c) => c.cell.state === CellState.finished || c.cell.state === CellState.error)

‎src/test/datascience/interactive-ipynb/nativeEditorStorage.unit.test.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -283,7 +283,9 @@ suite('DataScience - Native Editor Storage', () => {
283283
when(executionProvider.serverStarted).thenReturn(serverStartedEvent.event);
284284

285285
when(trustService.isNotebookTrusted(anything(), anything())).thenReturn(Promise.resolve(true));
286-
when(trustService.trustNotebook(anything(), anything())).thenReturn(Promise.resolve());
286+
when(trustService.trustNotebook(anything(), anything())).thenCall(() => {
287+
return Promise.resolve();
288+
});
287289

288290
testIndex += 1;
289291
when(crypto.createHash(anything(), 'string')).thenReturn(`${testIndex}`);
@@ -351,7 +353,7 @@ suite('DataScience - Native Editor Storage', () => {
351353
context.object,
352354
globalMemento,
353355
localMemento,
354-
trustService,
356+
instance(trustService),
355357
new NotebookModelFactory(false)
356358
);
357359

‎src/test/datascience/mountedWebView.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import {
77
IWebPanelOptions,
88
WebPanelMessage
99
} from '../../client/common/application/types';
10-
import { traceInfo } from '../../client/common/logger';
10+
import { traceError, traceInfo } from '../../client/common/logger';
1111
import { IDisposable } from '../../client/common/types';
1212
import { createDeferred } from '../../client/common/utils/async';
1313
import { InteractiveWindowMessages } from '../../client/datascience/interactive-common/interactiveWindowTypes';
@@ -236,6 +236,9 @@ export class MountedWebView implements IMountedWebView, IDisposable {
236236
}
237237
}
238238
private postMessageToWebPanel(msg: any) {
239+
if (this.disposed && !msg.type.startsWith(`DISPATCHED`)) {
240+
traceError(`Posting to disposed mount.`);
241+
}
239242
if (this.webPanelListener) {
240243
this.webPanelListener.onMessage(msg.type, msg.payload);
241244
} else {

0 commit comments

Comments
 (0)