From bc0f81bd0545af1eb0282870a95b0f156811b48e Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Mon, 13 Jan 2020 11:42:13 +0100 Subject: [PATCH] debt - cleanup file model a bit --- .../textfile/common/saveSequenzializer.ts | 95 +++++++++++++++ .../textfile/common/textFileEditorModel.ts | 110 ++---------------- .../textfile/test/saveSequenzializer.test.ts | 90 ++++++++++++++ .../textfile/test/textFileEditorModel.test.ts | 81 +------------ .../test/textFileEditorModelManager.test.ts | 11 ++ .../common/textResourcePropertiesService.ts | 0 src/vs/workbench/workbench.common.main.ts | 2 +- 7 files changed, 206 insertions(+), 183 deletions(-) create mode 100644 src/vs/workbench/services/textfile/common/saveSequenzializer.ts create mode 100644 src/vs/workbench/services/textfile/test/saveSequenzializer.test.ts rename src/vs/workbench/services/{textfile => textresourceProperties}/common/textResourcePropertiesService.ts (100%) diff --git a/src/vs/workbench/services/textfile/common/saveSequenzializer.ts b/src/vs/workbench/services/textfile/common/saveSequenzializer.ts new file mode 100644 index 00000000000..2b7cc0c8282 --- /dev/null +++ b/src/vs/workbench/services/textfile/common/saveSequenzializer.ts @@ -0,0 +1,95 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +interface IPendingSave { + versionId: number; + promise: Promise; +} + +interface ISaveOperation { + promise: Promise; + promiseResolve: () => void; + promiseReject: (error: Error) => void; + run: () => Promise; +} + +export class SaveSequentializer { + private _pendingSave?: IPendingSave; + private _nextSave?: ISaveOperation; + + hasPendingSave(versionId?: number): boolean { + if (!this._pendingSave) { + return false; + } + + if (typeof versionId === 'number') { + return this._pendingSave.versionId === versionId; + } + + return !!this._pendingSave; + } + + get pendingSave(): Promise | undefined { + return this._pendingSave ? this._pendingSave.promise : undefined; + } + + setPending(versionId: number, promise: Promise): Promise { + this._pendingSave = { versionId, promise }; + + promise.then(() => this.donePending(versionId), () => this.donePending(versionId)); + + return promise; + } + + private donePending(versionId: number): void { + if (this._pendingSave && versionId === this._pendingSave.versionId) { + + // only set pending to done if the promise finished that is associated with that versionId + this._pendingSave = undefined; + + // schedule the next save now that we are free if we have any + this.triggerNextSave(); + } + } + + private triggerNextSave(): void { + if (this._nextSave) { + const saveOperation = this._nextSave; + this._nextSave = undefined; + + // Run next save and complete on the associated promise + saveOperation.run().then(saveOperation.promiseResolve, saveOperation.promiseReject); + } + } + + setNext(run: () => Promise): Promise { + + // this is our first next save, so we create associated promise with it + // so that we can return a promise that completes when the save operation + // has completed. + if (!this._nextSave) { + let promiseResolve: () => void; + let promiseReject: (error: Error) => void; + const promise = new Promise((resolve, reject) => { + promiseResolve = resolve; + promiseReject = reject; + }); + + this._nextSave = { + run, + promise, + promiseResolve: promiseResolve!, + promiseReject: promiseReject! + }; + } + + // we have a previous next save, just overwrite it + else { + this._nextSave.run = run; + } + + return this._nextSave.promise; + } +} diff --git a/src/vs/workbench/services/textfile/common/textFileEditorModel.ts b/src/vs/workbench/services/textfile/common/textFileEditorModel.ts index 22c6313771f..8293240037e 100644 --- a/src/vs/workbench/services/textfile/common/textFileEditorModel.ts +++ b/src/vs/workbench/services/textfile/common/textFileEditorModel.ts @@ -13,7 +13,6 @@ import { EncodingMode, IRevertOptions, SaveReason } from 'vs/workbench/common/ed import { BaseTextEditorModel } from 'vs/workbench/common/editor/textEditorModel'; import { IBackupFileService } from 'vs/workbench/services/backup/common/backup'; import { IFileService, FileOperationError, FileOperationResult, FileChangesEvent, FileChangeType, IFileStatWithMetadata, ETAG_DISABLED, FileSystemProviderCapabilities } from 'vs/platform/files/common/files'; -import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; import { IModeService } from 'vs/editor/common/services/modeService'; import { IModelService } from 'vs/editor/common/services/modelService'; import { timeout } from 'vs/base/common/async'; @@ -24,8 +23,9 @@ import { isEqual, basename } from 'vs/base/common/resources'; import { onUnexpectedError } from 'vs/base/common/errors'; import { IWorkingCopyService } from 'vs/workbench/services/workingCopy/common/workingCopyService'; import { IFilesConfigurationService } from 'vs/workbench/services/filesConfiguration/common/filesConfigurationService'; +import { SaveSequentializer } from 'vs/workbench/services/textfile/common/saveSequenzializer'; -export interface IBackupMetaData { +interface IBackupMetaData { mtime: number; ctime: number; size: number; @@ -80,7 +80,6 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil @IModeService modeService: IModeService, @IModelService modelService: IModelService, @IFileService private readonly fileService: IFileService, - @IInstantiationService private readonly instantiationService: IInstantiationService, @ITextFileService private readonly textFileService: ITextFileService, @IBackupFileService private readonly backupFileService: IBackupFileService, @ILogService private readonly logService: ILogService, @@ -755,7 +754,12 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil // Prepare handler if (!TextFileEditorModel.saveErrorHandler) { - TextFileEditorModel.setSaveErrorHandler(this.instantiationService.createInstance(DefaultSaveErrorHandler)); + const notificationService = this.notificationService; + TextFileEditorModel.setSaveErrorHandler({ + onSaveError(error: Error, model: TextFileEditorModel): void { + notificationService.error(nls.localize('genericSaveError', "Failed to save '{0}': {1}", basename(model.resource), toErrorMessage(error, false))); + } + }); } // Handle @@ -887,102 +891,4 @@ export class TextFileEditorModel extends BaseTextEditorModel implements ITextFil } } -interface IPendingSave { - versionId: number; - promise: Promise; -} - -interface ISaveOperation { - promise: Promise; - promiseResolve: () => void; - promiseReject: (error: Error) => void; - run: () => Promise; -} - -export class SaveSequentializer { - private _pendingSave?: IPendingSave; - private _nextSave?: ISaveOperation; - - hasPendingSave(versionId?: number): boolean { - if (!this._pendingSave) { - return false; - } - - if (typeof versionId === 'number') { - return this._pendingSave.versionId === versionId; - } - - return !!this._pendingSave; - } - - get pendingSave(): Promise | undefined { - return this._pendingSave ? this._pendingSave.promise : undefined; - } - - setPending(versionId: number, promise: Promise): Promise { - this._pendingSave = { versionId, promise }; - - promise.then(() => this.donePending(versionId), () => this.donePending(versionId)); - - return promise; - } - - private donePending(versionId: number): void { - if (this._pendingSave && versionId === this._pendingSave.versionId) { - - // only set pending to done if the promise finished that is associated with that versionId - this._pendingSave = undefined; - - // schedule the next save now that we are free if we have any - this.triggerNextSave(); - } - } - - private triggerNextSave(): void { - if (this._nextSave) { - const saveOperation = this._nextSave; - this._nextSave = undefined; - - // Run next save and complete on the associated promise - saveOperation.run().then(saveOperation.promiseResolve, saveOperation.promiseReject); - } - } - - setNext(run: () => Promise): Promise { - - // this is our first next save, so we create associated promise with it - // so that we can return a promise that completes when the save operation - // has completed. - if (!this._nextSave) { - let promiseResolve: () => void; - let promiseReject: (error: Error) => void; - const promise = new Promise((resolve, reject) => { - promiseResolve = resolve; - promiseReject = reject; - }); - - this._nextSave = { - run, - promise, - promiseResolve: promiseResolve!, - promiseReject: promiseReject! - }; - } - - // we have a previous next save, just overwrite it - else { - this._nextSave.run = run; - } - - return this._nextSave.promise; - } -} - -class DefaultSaveErrorHandler implements ISaveErrorHandler { - - constructor(@INotificationService private readonly notificationService: INotificationService) { } - onSaveError(error: Error, model: TextFileEditorModel): void { - this.notificationService.error(nls.localize('genericSaveError', "Failed to save '{0}': {1}", basename(model.resource), toErrorMessage(error, false))); - } -} diff --git a/src/vs/workbench/services/textfile/test/saveSequenzializer.test.ts b/src/vs/workbench/services/textfile/test/saveSequenzializer.test.ts new file mode 100644 index 00000000000..c85a38bd8b3 --- /dev/null +++ b/src/vs/workbench/services/textfile/test/saveSequenzializer.test.ts @@ -0,0 +1,90 @@ +/*--------------------------------------------------------------------------------------------- + * Copyright (c) Microsoft Corporation. All rights reserved. + * Licensed under the MIT License. See License.txt in the project root for license information. + *--------------------------------------------------------------------------------------------*/ + +import * as assert from 'assert'; +import { timeout } from 'vs/base/common/async'; +import { SaveSequentializer } from 'vs/workbench/services/textfile/common/saveSequenzializer'; + +suite('Files - SaveSequentializer', () => { + + test('SaveSequentializer - pending basics', async function () { + const sequentializer = new SaveSequentializer(); + + assert.ok(!sequentializer.hasPendingSave()); + assert.ok(!sequentializer.hasPendingSave(2323)); + assert.ok(!sequentializer.pendingSave); + + // pending removes itself after done + await sequentializer.setPending(1, Promise.resolve()); + assert.ok(!sequentializer.hasPendingSave()); + assert.ok(!sequentializer.hasPendingSave(1)); + assert.ok(!sequentializer.pendingSave); + + // pending removes itself after done (use timeout) + sequentializer.setPending(2, timeout(1)); + assert.ok(sequentializer.hasPendingSave()); + assert.ok(sequentializer.hasPendingSave(2)); + assert.ok(!sequentializer.hasPendingSave(1)); + assert.ok(sequentializer.pendingSave); + + await timeout(2); + assert.ok(!sequentializer.hasPendingSave()); + assert.ok(!sequentializer.hasPendingSave(2)); + assert.ok(!sequentializer.pendingSave); + }); + + test('SaveSequentializer - pending and next (finishes instantly)', async function () { + const sequentializer = new SaveSequentializer(); + + let pendingDone = false; + sequentializer.setPending(1, timeout(1).then(() => { pendingDone = true; return; })); + + // next finishes instantly + let nextDone = false; + const res = sequentializer.setNext(() => Promise.resolve(null).then(() => { nextDone = true; return; })); + + await res; + assert.ok(pendingDone); + assert.ok(nextDone); + }); + + test('SaveSequentializer - pending and next (finishes after timeout)', async function () { + const sequentializer = new SaveSequentializer(); + + let pendingDone = false; + sequentializer.setPending(1, timeout(1).then(() => { pendingDone = true; return; })); + + // next finishes after timeout + let nextDone = false; + const res = sequentializer.setNext(() => timeout(1).then(() => { nextDone = true; return; })); + + await res; + assert.ok(pendingDone); + assert.ok(nextDone); + }); + + test('SaveSequentializer - pending and multiple next (last one wins)', async function () { + const sequentializer = new SaveSequentializer(); + + let pendingDone = false; + sequentializer.setPending(1, timeout(1).then(() => { pendingDone = true; return; })); + + // next finishes after timeout + let firstDone = false; + let firstRes = sequentializer.setNext(() => timeout(2).then(() => { firstDone = true; return; })); + + let secondDone = false; + let secondRes = sequentializer.setNext(() => timeout(3).then(() => { secondDone = true; return; })); + + let thirdDone = false; + let thirdRes = sequentializer.setNext(() => timeout(4).then(() => { thirdDone = true; return; })); + + await Promise.all([firstRes, secondRes, thirdRes]); + assert.ok(pendingDone); + assert.ok(!firstDone); + assert.ok(!secondDone); + assert.ok(thirdDone); + }); +}); diff --git a/src/vs/workbench/services/textfile/test/textFileEditorModel.test.ts b/src/vs/workbench/services/textfile/test/textFileEditorModel.test.ts index 5d0f0212cad..a3b647521f7 100644 --- a/src/vs/workbench/services/textfile/test/textFileEditorModel.test.ts +++ b/src/vs/workbench/services/textfile/test/textFileEditorModel.test.ts @@ -6,7 +6,7 @@ import * as assert from 'assert'; import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation'; import { EncodingMode } from 'vs/workbench/common/editor'; -import { TextFileEditorModel, SaveSequentializer } from 'vs/workbench/services/textfile/common/textFileEditorModel'; +import { TextFileEditorModel } from 'vs/workbench/services/textfile/common/textFileEditorModel'; import { ITextFileService, ModelState, StateChange, snapshotToString } from 'vs/workbench/services/textfile/common/textfiles'; import { workbenchInstantiationService, TestTextFileService, createFileInput, TestFileService } from 'vs/workbench/test/workbenchTestServices'; import { toResource } from 'vs/base/test/common/utils'; @@ -475,83 +475,4 @@ suite('Files - TextFileEditorModel', () => { await model.save(); model.dispose(); }); - - test('SaveSequentializer - pending basics', async function () { - const sequentializer = new SaveSequentializer(); - - assert.ok(!sequentializer.hasPendingSave()); - assert.ok(!sequentializer.hasPendingSave(2323)); - assert.ok(!sequentializer.pendingSave); - - // pending removes itself after done - await sequentializer.setPending(1, Promise.resolve()); - assert.ok(!sequentializer.hasPendingSave()); - assert.ok(!sequentializer.hasPendingSave(1)); - assert.ok(!sequentializer.pendingSave); - - // pending removes itself after done (use timeout) - sequentializer.setPending(2, timeout(1)); - assert.ok(sequentializer.hasPendingSave()); - assert.ok(sequentializer.hasPendingSave(2)); - assert.ok(!sequentializer.hasPendingSave(1)); - assert.ok(sequentializer.pendingSave); - - await timeout(2); - assert.ok(!sequentializer.hasPendingSave()); - assert.ok(!sequentializer.hasPendingSave(2)); - assert.ok(!sequentializer.pendingSave); - }); - - test('SaveSequentializer - pending and next (finishes instantly)', async function () { - const sequentializer = new SaveSequentializer(); - - let pendingDone = false; - sequentializer.setPending(1, timeout(1).then(() => { pendingDone = true; return; })); - - // next finishes instantly - let nextDone = false; - const res = sequentializer.setNext(() => Promise.resolve(null).then(() => { nextDone = true; return; })); - - await res; - assert.ok(pendingDone); - assert.ok(nextDone); - }); - - test('SaveSequentializer - pending and next (finishes after timeout)', async function () { - const sequentializer = new SaveSequentializer(); - - let pendingDone = false; - sequentializer.setPending(1, timeout(1).then(() => { pendingDone = true; return; })); - - // next finishes after timeout - let nextDone = false; - const res = sequentializer.setNext(() => timeout(1).then(() => { nextDone = true; return; })); - - await res; - assert.ok(pendingDone); - assert.ok(nextDone); - }); - - test('SaveSequentializer - pending and multiple next (last one wins)', async function () { - const sequentializer = new SaveSequentializer(); - - let pendingDone = false; - sequentializer.setPending(1, timeout(1).then(() => { pendingDone = true; return; })); - - // next finishes after timeout - let firstDone = false; - let firstRes = sequentializer.setNext(() => timeout(2).then(() => { firstDone = true; return; })); - - let secondDone = false; - let secondRes = sequentializer.setNext(() => timeout(3).then(() => { secondDone = true; return; })); - - let thirdDone = false; - let thirdRes = sequentializer.setNext(() => timeout(4).then(() => { thirdDone = true; return; })); - - await Promise.all([firstRes, secondRes, thirdRes]); - assert.ok(pendingDone); - assert.ok(!firstDone); - assert.ok(!secondDone); - assert.ok(thirdDone); - }); }); diff --git a/src/vs/workbench/services/textfile/test/textFileEditorModelManager.test.ts b/src/vs/workbench/services/textfile/test/textFileEditorModelManager.test.ts index 4145c7f6f8f..8c9e2e98670 100644 --- a/src/vs/workbench/services/textfile/test/textFileEditorModelManager.test.ts +++ b/src/vs/workbench/services/textfile/test/textFileEditorModelManager.test.ts @@ -141,11 +141,18 @@ suite('Files - TextFileEditorModelManager', () => { const resource1 = toResource.call(this, '/path/index.txt'); const resource2 = toResource.call(this, '/path/other.txt'); + let loadedCounter = 0; let dirtyCounter = 0; let revertedCounter = 0; let savedCounter = 0; let encodingCounter = 0; + manager.onModelLoaded(e => { + if (e.resource.toString() === resource1.toString()) { + loadedCounter++; + } + }); + manager.onModelDirty(e => { if (e.resource.toString() === resource1.toString()) { dirtyCounter++; @@ -171,10 +178,14 @@ suite('Files - TextFileEditorModelManager', () => { }); const model1 = await manager.loadOrCreate(resource1, { encoding: 'utf8' }); + assert.equal(loadedCounter, 1); + accessor.fileService.fireFileChanges(new FileChangesEvent([{ resource: resource1, type: FileChangeType.DELETED }])); accessor.fileService.fireFileChanges(new FileChangesEvent([{ resource: resource1, type: FileChangeType.ADDED }])); const model2 = await manager.loadOrCreate(resource2, { encoding: 'utf8' }); + assert.equal(loadedCounter, 2); + model1.textEditorModel!.setValue('changed'); model1.updatePreferredEncoding('utf16'); diff --git a/src/vs/workbench/services/textfile/common/textResourcePropertiesService.ts b/src/vs/workbench/services/textresourceProperties/common/textResourcePropertiesService.ts similarity index 100% rename from src/vs/workbench/services/textfile/common/textResourcePropertiesService.ts rename to src/vs/workbench/services/textresourceProperties/common/textResourcePropertiesService.ts diff --git a/src/vs/workbench/workbench.common.main.ts b/src/vs/workbench/workbench.common.main.ts index c437848af7e..ab49435a19b 100644 --- a/src/vs/workbench/workbench.common.main.ts +++ b/src/vs/workbench/workbench.common.main.ts @@ -71,7 +71,7 @@ import 'vs/workbench/services/history/browser/history'; import 'vs/workbench/services/activity/browser/activityService'; import 'vs/workbench/services/keybinding/browser/keybindingService'; import 'vs/workbench/services/untitled/common/untitledTextEditorService'; -import 'vs/workbench/services/textfile/common/textResourcePropertiesService'; +import 'vs/workbench/services/textresourceProperties/common/textResourcePropertiesService'; import 'vs/workbench/services/mode/common/workbenchModeService'; import 'vs/workbench/services/commands/common/commandService'; import 'vs/workbench/services/themes/browser/workbenchThemeService'; -- GitLab