From d1800d49fb79b547c6b9eb76ee215d9ef7f972d5 Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Wed, 1 Mar 2017 07:47:18 +0100 Subject: [PATCH] File models get disposed when contents change on disk (fixes #20901) --- .../test/node/processes/processes.test.ts | 2 +- .../files/common/editors/fileEditorTracker.ts | 38 +++++++++++++++++-- .../test/browser/fileEditorTracker.test.ts | 28 ++++++++++++++ .../common/textFileEditorModelManager.ts | 21 ---------- .../test/textFileEditorModelManager.test.ts | 19 ---------- 5 files changed, 63 insertions(+), 45 deletions(-) diff --git a/src/vs/base/test/node/processes/processes.test.ts b/src/vs/base/test/node/processes/processes.test.ts index fc22a4f093b62..77278a62d6487 100644 --- a/src/vs/base/test/node/processes/processes.test.ts +++ b/src/vs/base/test/node/processes/processes.test.ts @@ -25,7 +25,7 @@ function fork(id: string): cp.ChildProcess { } suite('Processes', () => { - test('pasero buffered sending - simple data', function (done: () => void) { + test('buffered sending - simple data', function (done: () => void) { if (process.env['VSCODE_PID']) { return done(); // TODO@Ben find out why test fails when run from within VS Code } diff --git a/src/vs/workbench/parts/files/common/editors/fileEditorTracker.ts b/src/vs/workbench/parts/files/common/editors/fileEditorTracker.ts index 7e5a6325049bc..506f6834e98e1 100644 --- a/src/vs/workbench/parts/files/common/editors/fileEditorTracker.ts +++ b/src/vs/workbench/parts/files/common/editors/fileEditorTracker.ts @@ -12,13 +12,14 @@ import { IEditor, IEditorViewState, isCommonCodeEditor } from 'vs/editor/common/ import { IEditor as IBaseEditor } from 'vs/platform/editor/common/editor'; import { toResource, EditorInput, IEditorStacksModel, SideBySideEditorInput, IEditorGroup } from 'vs/workbench/common/editor'; import { BINARY_FILE_EDITOR_ID } from 'vs/workbench/parts/files/common/files'; -import { ITextFileService, ModelState } from 'vs/workbench/services/textfile/common/textfiles'; +import { ITextFileService, ModelState, ITextFileEditorModel } from 'vs/workbench/services/textfile/common/textfiles'; import { FileOperationEvent, FileOperation, IFileService, FileChangeType, FileChangesEvent } from 'vs/platform/files/common/files'; import { FileEditorInput } from 'vs/workbench/parts/files/common/editors/fileEditorInput'; import { IEditorGroupService } from 'vs/workbench/services/group/common/groupService'; import { ILifecycleService } from 'vs/platform/lifecycle/common/lifecycle'; import { IWorkbenchEditorService } from 'vs/workbench/services/editor/common/editorService'; import { IDisposable, dispose } from 'vs/base/common/lifecycle'; +import { distinct } from 'vs/base/common/arrays'; export class FileEditorTracker implements IWorkbenchContribution { @@ -77,8 +78,8 @@ export class FileEditorTracker implements IWorkbenchContribution { private onFileChanges(e: FileChangesEvent): void { - // Handle updates to visible editors - this.handleUpdatesToVisibleEditors(e); + // Handle updates + this.handleUpdates(e); // Handle deletes if (e.gotDeleted()) { @@ -193,7 +194,33 @@ export class FileEditorTracker implements IWorkbenchContribution { return void 0; } - private handleUpdatesToVisibleEditors(e: FileChangesEvent) { + private handleUpdates(e: FileChangesEvent): void { + + // Collect distinct (saved) models to update. + // + // Note: we also consider the added event because it could be that a file was added + // and updated right after. + const modelsToUpdate = distinct([...e.getUpdated(), ...e.getAdded()] + .map(u => this.textFileService.models.get(u.resource)) + .filter(model => model && model.getState() === ModelState.SAVED), m => m.getResource().toString()); + + // Handle updates to visible editors specially to preserve view state + const visibleModels = this.handleUpdatesToVisibleEditors(e); + + // Handle updates to remaining models that are not visible + modelsToUpdate.forEach(model => { + if (visibleModels.indexOf(model) >= 0) { + return; // already updated + } + + // Load model to update + model.load().done(null, errors.onUnexpectedError); + }); + } + + private handleUpdatesToVisibleEditors(e: FileChangesEvent): ITextFileEditorModel[] { + const models: ITextFileEditorModel[] = []; + const editors = this.editorService.getVisibleEditors(); editors.forEach(editor => { let input = editor.input; @@ -213,6 +240,7 @@ export class FileEditorTracker implements IWorkbenchContribution { // Text file: check for last save time if (textModel) { + models.push(textModel); // We only ever update models that are in good saved state if (textModel.getState() === ModelState.SAVED) { @@ -239,6 +267,8 @@ export class FileEditorTracker implements IWorkbenchContribution { } } }); + + return models; } private isEditorShowingPath(editor: IBaseEditor, resource: URI): boolean { diff --git a/src/vs/workbench/parts/files/test/browser/fileEditorTracker.test.ts b/src/vs/workbench/parts/files/test/browser/fileEditorTracker.test.ts index 19cf83dfbf42d..e12cb168e0cd2 100644 --- a/src/vs/workbench/parts/files/test/browser/fileEditorTracker.test.ts +++ b/src/vs/workbench/parts/files/test/browser/fileEditorTracker.test.ts @@ -17,6 +17,7 @@ import { IEditorGroupService } from 'vs/workbench/services/group/common/groupSer import { EditorStacksModel } from 'vs/workbench/common/editor/editorStacksModel'; import { ITextFileService } from 'vs/workbench/services/textfile/common/textfiles'; import { FileOperation, FileOperationEvent, FileChangesEvent, FileChangeType, IFileService } from 'vs/platform/files/common/files'; +import { TextFileEditorModel } from 'vs/workbench/services/textfile/common/textFileEditorModel'; function toResource(path) { return URI.file(join('C:\\', new Buffer(this.test.fullTitle()).toString('base64'), path)); @@ -75,6 +76,8 @@ suite('Files - FileEditorTracker', () => { const to = toResource.call(this, '/foo/barfoo/change.js'); accessor.fileService.fireAfterOperation(new FileOperationEvent(resource, FileOperation.MOVE, to)); assert.ok(input.isDisposed()); + + tracker.dispose(); }); test('disposes when resource gets deleted - remote file changes', function () { @@ -105,5 +108,30 @@ suite('Files - FileEditorTracker', () => { accessor.fileService.fireFileChanges(new FileChangesEvent([{ resource: parent, type: FileChangeType.DELETED }])); assert.ok(input.isDisposed()); + + tracker.dispose(); + }); + + test('file change event updates model', function (done) { + const tracker = instantiationService.createInstance(FileEditorTracker); + + const resource = toResource.call(this, '/path/index.txt'); + + accessor.textFileService.models.loadOrCreate(resource).then((model: TextFileEditorModel) => { + model.textEditorModel.setValue('Super Good'); + assert.equal(model.getValue(), 'Super Good'); + + model.save().then(() => { + + // change event (watcher) + accessor.fileService.fireFileChanges(new FileChangesEvent([{ resource, type: FileChangeType.UPDATED }])); + + assert.equal(model.getValue(), 'Hello Html'); + + tracker.dispose(); + + done(); + }); + }); }); }); \ No newline at end of file diff --git a/src/vs/workbench/services/textfile/common/textFileEditorModelManager.ts b/src/vs/workbench/services/textfile/common/textFileEditorModelManager.ts index 22dc6e92a6b8a..5121ccc14f0be 100644 --- a/src/vs/workbench/services/textfile/common/textFileEditorModelManager.ts +++ b/src/vs/workbench/services/textfile/common/textFileEditorModelManager.ts @@ -17,10 +17,6 @@ import { FileOperation, FileOperationEvent, FileChangesEvent, IFileService } fro export class TextFileEditorModelManager implements ITextFileEditorModelManager { - // Delay in ms that we wait at minimum before we update a model from a file change event. - // This reduces the chance that a save from the client triggers an update of the editor. - private static FILE_CHANGE_UPDATE_DELAY = 2000; - private toUnbind: IDisposable[]; private _onModelDisposed: Emitter; @@ -116,23 +112,6 @@ export class TextFileEditorModelManager implements ITextFileEditorModelManager { e.getDeleted().forEach(deleted => { this.disposeModelIfPossible(deleted.resource); }); - - // Dispose models that got changed and are not visible. We do this because otherwise - // cached file models will be stale from the contents on disk. - e.getUpdated() - .map(u => this.get(u.resource)) - .filter(model => { - if (!model) { - return false; - } - - if (Date.now() - model.getLastSaveAttemptTime() < TextFileEditorModelManager.FILE_CHANGE_UPDATE_DELAY) { - return false; // this is a weak check to see if the change came from outside the editor or not - } - - return true; // ok boss - }) - .forEach(model => this.disposeModelIfPossible(model.getResource())); } private canDispose(textModel: ITextFileEditorModel): boolean { diff --git a/src/vs/workbench/services/textfile/test/textFileEditorModelManager.test.ts b/src/vs/workbench/services/textfile/test/textFileEditorModelManager.test.ts index ab1a653ca2d72..0e10b09c0681e 100644 --- a/src/vs/workbench/services/textfile/test/textFileEditorModelManager.test.ts +++ b/src/vs/workbench/services/textfile/test/textFileEditorModelManager.test.ts @@ -238,25 +238,6 @@ suite('Files - TextFileEditorModelManager', () => { manager.dispose(); }); - test('file change event dispose model if happening > 2 second after last save', function () { - const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager); - - const resource = toResource('/path/index.txt'); - - const model: TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, resource, 'utf8'); - manager.add(resource, model); - - assert.ok(!model.isDisposed()); - - // change event (watcher) - accessor.fileService.fireFileChanges(new FileChangesEvent([{ resource, type: FileChangeType.UPDATED }])); - - assert.ok(model.isDisposed()); - assert.ok(!accessor.modelService.getModel(model.getResource())); - - manager.dispose(); - }); - test('file change event does NOT dispose model if happening < 2 second after last save', function (done) { const manager: TestTextFileEditorModelManager = instantiationService.createInstance(TestTextFileEditorModelManager);