Skip to content

Commit f6629ff

Browse files
rchiodorchiodo
committed
Fix storage not being used (#11649)
* Fix storage not being used * Add disposable to storage so it won't write after shutdown * Fix dirty title * Hack to get tests to pass * Another way to get run all to not interfere
1 parent 2e4b2ed commit f6629ff

File tree

6 files changed

+136
-28
lines changed

6 files changed

+136
-28
lines changed

‎news/2 Fixes/11557.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
When VS quits, make sure to save contents of notebook for next reopen.

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

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,11 @@ export class NativeEditorOldWebView extends NativeEditor {
139139
// Update our title to match
140140
this.setTitle(path.basename(model.file.fsPath));
141141

142+
// Update dirty if model started out that way
143+
if (this.model?.isDirty) {
144+
this.setDirty().ignoreErrors();
145+
}
146+
142147
// Show ourselves
143148
await this.show();
144149
this.model?.changed(() => {

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

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,14 @@ import { concatMultilineStringInput, splitMultilineString } from '../../../datas
88
import { createCodeCell } from '../../../datascience-ui/common/cellFactory';
99
import { traceError } from '../../common/logger';
1010
import { IFileSystem } from '../../common/platform/types';
11-
import { GLOBAL_MEMENTO, ICryptoUtils, IExtensionContext, IMemento, WORKSPACE_MEMENTO } from '../../common/types';
11+
import {
12+
GLOBAL_MEMENTO,
13+
ICryptoUtils,
14+
IDisposableRegistry,
15+
IExtensionContext,
16+
IMemento,
17+
WORKSPACE_MEMENTO
18+
} from '../../common/types';
1219
import { noop } from '../../common/utils/misc';
1320
import { PythonInterpreter } from '../../interpreter/contracts';
1421
import { Identifiers, KnownNotebookLanguages, Telemetry } from '../constants';
@@ -19,6 +26,8 @@ import { CellState, ICell, IJupyterExecution, IJupyterKernelSpec, INotebookModel
1926

2027
// tslint:disable-next-line:no-require-imports no-var-requires
2128
import detectIndent = require('detect-indent');
29+
import { IDisposable } from 'monaco-editor';
30+
import { IWorkspaceService } from '../../common/application/types';
2231
import { sendTelemetryEvent } from '../../telemetry';
2332
import { pruneCell } from '../common';
2433
// tslint:disable-next-line:no-require-imports no-var-requires
@@ -35,7 +44,7 @@ interface INativeEditorStorageState {
3544
}
3645

3746
@injectable()
38-
export class NativeEditorStorage implements INotebookModel, INotebookStorage {
47+
export class NativeEditorStorage implements INotebookModel, INotebookStorage, IDisposable {
3948
public get isDirty(): boolean {
4049
return this._state.changeCount !== this._state.saveChangeCount;
4150
}
@@ -67,15 +76,24 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage {
6776
};
6877
private indentAmount: string = ' ';
6978
private debouncedWriteToStorage = debounce(this.writeToStorage.bind(this), 250);
79+
private disposed = false;
7080

7181
constructor(
7282
@inject(IJupyterExecution) private jupyterExecution: IJupyterExecution,
7383
@inject(IFileSystem) private fileSystem: IFileSystem,
7484
@inject(ICryptoUtils) private crypto: ICryptoUtils,
7585
@inject(IExtensionContext) private context: IExtensionContext,
7686
@inject(IMemento) @named(GLOBAL_MEMENTO) private globalStorage: Memento,
77-
@inject(IMemento) @named(WORKSPACE_MEMENTO) private localStorage: Memento
78-
) {}
87+
@inject(IMemento) @named(WORKSPACE_MEMENTO) private localStorage: Memento,
88+
@inject(IWorkspaceService) private workspaceService: IWorkspaceService,
89+
@inject(IDisposableRegistry) disposableRegistry: IDisposableRegistry
90+
) {
91+
disposableRegistry.push(this);
92+
}
93+
94+
public dispose() {
95+
this.disposed = true;
96+
}
7997

8098
public async load(file: Uri, possibleContents?: string): Promise<INotebookModel> {
8199
// Reload our cells
@@ -141,9 +159,21 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage {
141159
// Write but debounced (wait at least 250 ms)
142160
return this.debouncedWriteToStorage(filePath, specialContents, cancelToken);
143161
}
162+
163+
private async saveToStorage(): Promise<void> {
164+
// Skip doing this if auto save is enabled or is untitled
165+
const filesConfig = this.workspaceService.getConfiguration('files', this.file);
166+
const autoSave = filesConfig.get('autoSave', 'off');
167+
if (autoSave === 'off' && !this.isUntitled) {
168+
// Write but debounced (wait at least 250 ms)
169+
return this.storeContentsInHotExitFile(undefined);
170+
}
171+
}
172+
144173
private async writeToStorage(filePath: string, contents?: string, cancelToken?: CancellationToken): Promise<void> {
145174
try {
146-
if (!cancelToken?.isCancellationRequested) {
175+
// Only write to storage if not disposed.
176+
if (!cancelToken?.isCancellationRequested && !this.disposed) {
147177
if (contents) {
148178
await this.fileSystem.createDirectory(path.dirname(filePath));
149179
if (!cancelToken?.isCancellationRequested) {
@@ -198,6 +228,10 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage {
198228
// Forward onto our listeners if necessary
199229
if (changed || this.isDirty !== oldDirty) {
200230
this._changedEmitter.fire({ ...change, newDirty: this.isDirty, oldDirty });
231+
if (this.isDirty) {
232+
// Save to temp storage so we don't lose the file if the user exits VS code
233+
this.saveToStorage().ignoreErrors();
234+
}
201235
}
202236
// Slightly different for the event we send to VS code. Skip version and file changes. Only send user events.
203237
if (
@@ -451,6 +485,7 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage {
451485
const dirtyContents = await this.getStoredContents();
452486
if (dirtyContents) {
453487
// This means we're dirty. Indicate dirty and load from this content
488+
this._state.saveChangeCount = -1; // Indicates dirty
454489
this.loadContents(dirtyContents);
455490
} else {
456491
// Load without setting dirty
@@ -621,8 +656,8 @@ export class NativeEditorStorage implements INotebookModel, INotebookStorage {
621656
if (data && !this.isUntitled && data.contents) {
622657
return data.contents;
623658
}
624-
} catch {
625-
noop();
659+
} catch (exc) {
660+
traceError(`Exception reading from temporary storage for ${key}`, exc);
626661
}
627662
}
628663

‎src/test/datascience/dataScienceIocContainer.ts

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,8 @@
33
//tslint:disable:trailing-comma no-any
44
import * as child_process from 'child_process';
55
import { ReactWrapper } from 'enzyme';
6+
import * as fs from 'fs-extra';
7+
import * as glob from 'glob';
68
import { interfaces } from 'inversify';
79
import * as os from 'os';
810
import * as path from 'path';
@@ -24,6 +26,7 @@ import {
2426
import * as vsls from 'vsls/vscode';
2527
import { KernelDaemonPool } from '../../client/datascience/kernel-launcher/kernelDaemonPool';
2628

29+
import { promisify } from 'util';
2730
import { LanguageServerExtensionActivationService } from '../../client/activation/activationService';
2831
import { LanguageServerDownloader } from '../../client/activation/common/downloader';
2932
import { JediExtensionActivator } from '../../client/activation/jedi';
@@ -479,6 +482,18 @@ export class DataScienceIocContainer extends UnitTestIocContainer {
479482
}
480483

481484
public async dispose(): Promise<void> {
485+
try {
486+
// Make sure to delete any temp files written by native editor storage
487+
const globPr = promisify(glob);
488+
const tempLocation = this.serviceManager.get<IExtensionContext>(IExtensionContext).globalStoragePath;
489+
const tempFiles = await globPr(`${tempLocation}/*.ipynb`);
490+
if (tempFiles && tempFiles.length) {
491+
await Promise.all(tempFiles.map((t) => fs.remove(t)));
492+
}
493+
} catch (exc) {
494+
// tslint:disable-next-line: no-console
495+
console.log(`Exception on cleanup: ${exc}`);
496+
}
482497
await this.asyncRegistry.dispose();
483498
await super.dispose();
484499
this.disposed = true;

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

Lines changed: 58 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import {
1111
ConfigurationChangeEvent,
1212
ConfigurationTarget,
1313
EventEmitter,
14+
FileType,
1415
TextEditor,
1516
Uri,
1617
WorkspaceConfiguration
@@ -31,6 +32,7 @@ import { ConfigurationService } from '../../../client/common/configuration/servi
3132
import { CryptoUtils } from '../../../client/common/crypto';
3233
import { IFileSystem } from '../../../client/common/platform/types';
3334
import { IConfigurationService, ICryptoUtils, IDisposable, IExtensionContext } from '../../../client/common/types';
35+
import { sleep } from '../../../client/common/utils/async';
3436
import { EXTENSION_ROOT_DIR } from '../../../client/constants';
3537
import {
3638
IEditorContentChange,
@@ -87,7 +89,7 @@ class MockWorkspaceConfiguration implements WorkspaceConfiguration {
8789
}
8890

8991
// tslint:disable: max-func-body-length
90-
suite('Data Science - Native Editor Storage', () => {
92+
suite('DataScience - Native Editor Storage', () => {
9193
let workspace: IWorkspaceService;
9294
let configService: IConfigurationService;
9395
let fileSystem: typemoq.IMock<IFileSystem>;
@@ -321,6 +323,7 @@ suite('Data Science - Native Editor Storage', () => {
321323

322324
let listener: IWebPanelMessageListener;
323325
const webPanel = mock(WebPanel);
326+
const startTime = Date.now();
324327
class WebPanelCreateMatcher extends Matcher {
325328
public match(value: any) {
326329
listener = value.listener;
@@ -351,16 +354,26 @@ suite('Data Science - Native Editor Storage', () => {
351354
.returns((_a1) => {
352355
return Promise.resolve(lastWriteFileValue);
353356
});
357+
fileSystem
358+
.setup((f) => f.stat(typemoq.It.isAny()))
359+
.returns((_a1) => {
360+
return Promise.resolve({ mtime: startTime, type: FileType.File, ctime: startTime, size: 100 });
361+
});
362+
storage = createStorage();
363+
});
354364

355-
storage = new NativeEditorStorage(
365+
function createStorage() {
366+
return new NativeEditorStorage(
356367
instance(executionProvider),
357368
fileSystem.object, // Use typemoq so can save values in returns
358369
instance(crypto),
359370
context.object,
360371
globalMemento,
361-
localMemento
372+
localMemento,
373+
instance(workspace),
374+
[]
362375
);
363-
});
376+
}
364377

365378
teardown(() => {
366379
globalMemento.clear();
@@ -480,6 +493,47 @@ suite('Data Science - Native Editor Storage', () => {
480493
expect(cells).to.be.lengthOf(1);
481494
});
482495

496+
test('Editing a file and closing will keep contents', async () => {
497+
await filesConfig?.update('autoSave', 'off');
498+
499+
await storage.load(baseUri);
500+
expect(storage.isDirty).to.be.equal(false, 'Editor should not be dirty');
501+
editCell(
502+
[
503+
{
504+
range: {
505+
startLineNumber: 2,
506+
startColumn: 1,
507+
endLineNumber: 2,
508+
endColumn: 1
509+
},
510+
rangeOffset: 4,
511+
rangeLength: 0,
512+
text: 'a',
513+
position: {
514+
lineNumber: 1,
515+
column: 1
516+
}
517+
}
518+
],
519+
storage.cells[1],
520+
'a'
521+
);
522+
523+
// Wait for a second so it has time to update
524+
await sleep(1000);
525+
526+
// Recreate
527+
storage = createStorage();
528+
await storage.load(baseUri);
529+
530+
const cells = storage.cells;
531+
expect(cells).to.be.lengthOf(3);
532+
expect(cells[1].id).to.be.match(/NotebookImport#1/);
533+
expect(concatMultilineStringInput(cells[1].data.source)).to.be.equals('b=2\nab');
534+
expect(storage.isDirty).to.be.equal(true, 'Editor should be dirty');
535+
});
536+
483537
test('Opening file with local storage but no global will still open with old contents', async () => {
484538
await filesConfig?.update('autoSave', 'off');
485539
// This test is really for making sure when a user upgrades to a new extension, we still have their old storage

‎src/test/datascience/nativeEditor.functional.test.tsx

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,9 @@ import {
8383
use(chaiAsPromised);
8484

8585
// tslint:disable:max-func-body-length trailing-comma no-any no-multiline-string
86+
async function updateFileConfig(ioc: DataScienceIocContainer, key: string, value: any) {
87+
return ioc.get<IWorkspaceService>(IWorkspaceService).getConfiguration('file').update(key, value);
88+
}
8689

8790
suite('DataScience Native Editor', () => {
8891
const originalPlatform = window.navigator.platform;
@@ -641,6 +644,8 @@ df.head()`;
641644
runMountedTest(
642645
'RunAllCells',
643646
async (wrapper) => {
647+
// Make sure we don't write to storage for the notebook. It messes up other tests
648+
await updateFileConfig(ioc, 'autoSave', 'onFocusChange');
644649
addMockData(ioc, 'print(1)\na=1', 1);
645650
addMockData(ioc, 'a=a+1\nprint(a)', 2);
646651
addMockData(ioc, 'print(a+1)', 3);
@@ -2122,17 +2127,10 @@ df.head()`;
21222127
await addCell(wrapper, ioc, 'a', false);
21232128
}
21242129

2125-
async function updateFileConfig(key: string, value: any) {
2126-
return ioc
2127-
.get<IWorkspaceService>(IWorkspaceService)
2128-
.getConfiguration('file')
2129-
.update(key, value);
2130-
}
2131-
21322130
test('Auto save notebook every 1s', async () => {
21332131
// Configure notebook to save automatically ever 1s.
2134-
await updateFileConfig('autoSave', 'afterDelay');
2135-
await updateFileConfig('autoSaveDelay', 1_000);
2132+
await updateFileConfig(ioc, 'autoSave', 'afterDelay');
2133+
await updateFileConfig(ioc, 'autoSaveDelay', 1_000);
21362134
ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath);
21372135

21382136
/**
@@ -2165,8 +2163,8 @@ df.head()`;
21652163

21662164
test('File saved with same format', async () => {
21672165
// Configure notebook to save automatically ever 1s.
2168-
await updateFileConfig('autoSave', 'afterDelay');
2169-
await updateFileConfig('autoSaveDelay', 2_000);
2166+
await updateFileConfig(ioc, 'autoSave', 'afterDelay');
2167+
await updateFileConfig(ioc, 'autoSaveDelay', 2_000);
21702168

21712169
ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath);
21722170
const notebookFileContents = await fs.readFile(notebookFile.filePath, 'utf8');
@@ -2190,8 +2188,8 @@ df.head()`;
21902188
const notebookFileContents = await fs.readFile(notebookFile.filePath, 'utf8');
21912189

21922190
// Configure notebook to to never save.
2193-
await updateFileConfig('autoSave', 'off');
2194-
await updateFileConfig('autoSaveDelay', 1_000);
2191+
await updateFileConfig(ioc, 'autoSave', 'off');
2192+
await updateFileConfig(ioc, 'autoSaveDelay', 1_000);
21952193

21962194
// Update the settings and wait for the component to receive it and process it.
21972195
const promise = waitForMessage(ioc, InteractiveWindowMessages.SettingsUpdated);
@@ -2231,7 +2229,7 @@ df.head()`;
22312229
await dirtyPromise;
22322230

22332231
// Configure notebook to save when active editor changes.
2234-
await updateFileConfig('autoSave', 'onFocusChange');
2232+
await updateFileConfig(ioc, 'autoSave', 'onFocusChange');
22352233
ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath);
22362234

22372235
// Now that the notebook is dirty, change the active editor.
@@ -2263,7 +2261,7 @@ df.head()`;
22632261
await dirtyPromise;
22642262

22652263
// Configure notebook to save when window state changes.
2266-
await updateFileConfig('autoSave', 'onWindowChange');
2264+
await updateFileConfig(ioc, 'autoSave', 'onWindowChange');
22672265
ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath);
22682266

22692267
// Now that the notebook is dirty, change the active editor.
@@ -2289,7 +2287,7 @@ df.head()`;
22892287
await dirtyPromise;
22902288

22912289
// Configure notebook to save when active editor changes.
2292-
await updateFileConfig('autoSave', configSetting);
2290+
await updateFileConfig(ioc, 'autoSave', configSetting);
22932291
ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath);
22942292

22952293
// Now that the notebook is dirty, send notification about changes to window state.
@@ -2321,7 +2319,7 @@ df.head()`;
23212319
await dirtyPromise;
23222320

23232321
// Configure notebook to save when active editor changes.
2324-
await updateFileConfig('autoSave', 'onFocusChange');
2322+
await updateFileConfig(ioc, 'autoSave', 'onFocusChange');
23252323
ioc.forceSettingsChanged(undefined, ioc.getSettings().pythonPath);
23262324

23272325
// Force a view state change

0 commit comments

Comments
 (0)