From a4e06dc83ba8acc35f4fc04dbe8289eacc6422af Mon Sep 17 00:00:00 2001 From: Benjamin Pasero Date: Wed, 7 Sep 2016 12:45:07 +0200 Subject: [PATCH] move disposeUnusedTextFileModels into model manager and add tests --- src/vs/test/utils/servicesTestUtils.ts | 7 +- .../common/editor/editorStacksModel.ts | 2 +- .../parts/files/browser/fileTracker.ts | 20 +----- .../editors/textFileEditorModelManager.ts | 68 +++++++++++++++++- src/vs/workbench/parts/files/common/files.ts | 2 +- .../parts/files/common/textFileService.ts | 8 ++- .../files/electron-browser/textFileService.ts | 4 +- .../textFileEditorModelManager.test.ts | 69 ++++++++++++++++++- .../test/node/configurationService.test.ts | 21 ------ 9 files changed, 150 insertions(+), 51 deletions(-) diff --git a/src/vs/test/utils/servicesTestUtils.ts b/src/vs/test/utils/servicesTestUtils.ts index cbda2a90f06..a65f5da97ac 100644 --- a/src/vs/test/utils/servicesTestUtils.ts +++ b/src/vs/test/utils/servicesTestUtils.ts @@ -42,6 +42,7 @@ import {IModeService} from 'vs/editor/common/services/modeService'; import {IWorkbenchEditorService} from 'vs/workbench/services/editor/common/editorService'; import {ITextFileService} from 'vs/workbench/parts/files/common/files'; import {IHistoryService} from 'vs/workbench/services/history/common/history'; +import {IInstantiationService} from 'vs/platform/instantiation/common/instantiation'; export const TestWorkspace: IWorkspace = { resource: URI.file('C:\\testWorkspace'), @@ -103,9 +104,10 @@ export class TestTextFileService extends TextFileService { @IWorkbenchEditorService editorService: IWorkbenchEditorService, @IEditorGroupService editorGroupService: IEditorGroupService, @IFileService fileService: IFileService, - @IUntitledEditorService untitledEditorService: IUntitledEditorService + @IUntitledEditorService untitledEditorService: IUntitledEditorService, + @IInstantiationService instantiationService: IInstantiationService ) { - super(lifecycleService, contextService, configurationService, telemetryService, editorGroupService, editorService, fileService, untitledEditorService); + super(lifecycleService, contextService, configurationService, telemetryService, editorGroupService, editorService, fileService, untitledEditorService, instantiationService); } public setPromptPath(path: string): void { @@ -339,6 +341,7 @@ export class TestEditorGroupService implements IEditorGroupService { let services = new ServiceCollection(); services.set(IStorageService, new TestStorageService()); + services.set(IConfigurationService, new TestConfigurationService()); services.set(IWorkspaceContextService, new TestContextService()); const lifecycle = new TestLifecycleService(); services.set(ILifecycleService, lifecycle); diff --git a/src/vs/workbench/common/editor/editorStacksModel.ts b/src/vs/workbench/common/editor/editorStacksModel.ts index 41767e63e97..44c9391e79d 100644 --- a/src/vs/workbench/common/editor/editorStacksModel.ts +++ b/src/vs/workbench/common/editor/editorStacksModel.ts @@ -115,7 +115,7 @@ export class EditorGroup implements IEditorGroup { } private onConfigurationUpdated(config: IWorkbenchEditorConfiguration): void { - this.editorOpenPositioning = config.workbench.editor.openPositioning; + this.editorOpenPositioning = config && config.workbench && config.workbench.editor && config.workbench.editor.openPositioning; } public get id(): GroupIdentifier { diff --git a/src/vs/workbench/parts/files/browser/fileTracker.ts b/src/vs/workbench/parts/files/browser/fileTracker.ts index 8a9869f0450..a1ed70be41f 100644 --- a/src/vs/workbench/parts/files/browser/fileTracker.ts +++ b/src/vs/workbench/parts/files/browser/fileTracker.ts @@ -91,7 +91,6 @@ export class FileTracker implements IWorkbenchContribution { } private onEditorsChanged(): void { - this.disposeUnusedTextFileModels(); this.handleOutOfWorkspaceWatchers(); } @@ -214,7 +213,7 @@ export class FileTracker implements IWorkbenchContribution { return true; // ok boss }) - .forEach(model => this.textFileService.models.dispose(model.getResource())); + .forEach(model => this.textFileService.models.disposeModel(model.getResource())); // Update inputs that got updated const editors = this.editorService.getVisibleEditors(); @@ -394,7 +393,7 @@ export class FileTracker implements IWorkbenchContribution { }); // Clean up model if any - this.textFileService.models.dispose(resource); + this.textFileService.models.disposeModel(resource); } private containsResource(input: FileEditorInput, resource: URI): boolean; @@ -411,21 +410,6 @@ export class FileTracker implements IWorkbenchContribution { return false; } - private disposeUnusedTextFileModels(): void { - - // To not grow our text file model cache infinitly, we dispose models that - // are not showing up in any opened editor. - - // Get all cached file models - this.textFileService.models.getAll() - - // Only take text file models and remove those that are under working files or opened - .filter(model => !this.stacks.isOpen(model.getResource()) && this.canDispose(model)) - - // Dispose - .forEach(model => this.textFileService.models.dispose(model.getResource())); - } - private canDispose(textModel: ITextFileEditorModel): boolean { if (!textModel) { return false; // we need data! diff --git a/src/vs/workbench/parts/files/common/editors/textFileEditorModelManager.ts b/src/vs/workbench/parts/files/common/editors/textFileEditorModelManager.ts index 55e238ce447..8b8e4d127c7 100644 --- a/src/vs/workbench/parts/files/common/editors/textFileEditorModelManager.ts +++ b/src/vs/workbench/parts/files/common/editors/textFileEditorModelManager.ts @@ -8,17 +8,42 @@ import URI from 'vs/base/common/uri'; import {TextFileEditorModel} from 'vs/workbench/parts/files/common/editors/textFileEditorModel'; import {ITextFileEditorModelManager} from 'vs/workbench/parts/files/common/files'; import {dispose, IDisposable} from 'vs/base/common/lifecycle'; +import {IEditorGroupService} from 'vs/workbench/services/group/common/groupService'; +import {ModelState, ITextFileEditorModel} from 'vs/workbench/parts/files/common/files'; +import {ILifecycleService} from 'vs/platform/lifecycle/common/lifecycle'; export class TextFileEditorModelManager implements ITextFileEditorModelManager { + + private toUnbind: IDisposable[]; + private mapResourceToDisposeListener: { [resource: string]: IDisposable; }; private mapResourcePathToModel: { [resource: string]: TextFileEditorModel; }; - constructor() { + constructor( + @ILifecycleService private lifecycleService: ILifecycleService, + + @IEditorGroupService private editorGroupService: IEditorGroupService + ) { + this.toUnbind = []; + this.mapResourcePathToModel = Object.create(null); this.mapResourceToDisposeListener = Object.create(null); + + this.registerListeners(); } - public dispose(resource: URI): void { + private registerListeners(): void { + this.toUnbind.push(this.editorGroupService.onEditorsChanged(() => this.onEditorsChanged())); + + // Lifecycle + this.lifecycleService.onShutdown(this.dispose, this); + } + + private onEditorsChanged(): void { + this.disposeUnusedModels(); + } + + public disposeModel(resource: URI): void { const model = this.get(resource); if (model) { if (model.isDirty()) { @@ -76,4 +101,43 @@ export class TextFileEditorModelManager implements ITextFileEditorModelManager { dispose(keys.map(k => this.mapResourceToDisposeListener[k])); this.mapResourceToDisposeListener = Object.create(null); } + + private canDispose(textModel: ITextFileEditorModel): boolean { + if (!textModel) { + return false; // we need data! + } + + if (textModel.isDisposed()) { + return false; // already disposed + } + + if (textModel.textEditorModel && textModel.textEditorModel.isAttachedToEditor()) { + return false; // never dispose when attached to editor + } + + if (textModel.getState() !== ModelState.SAVED) { + return false; // never dispose unsaved models + } + + return true; + } + + private disposeUnusedModels(): void { + + // To not grow our text file model cache infinitly, we dispose models that + // are not showing up in any opened editor. + + // Get all cached file models + this.getAll() + + // Only take text file models and remove those that are under working files or opened + .filter(model => !this.editorGroupService.getStacksModel().isOpen(model.getResource()) && this.canDispose(model)) + + // Dispose + .forEach(model => this.disposeModel(model.getResource())); + } + + public dispose(): void { + this.toUnbind = dispose(this.toUnbind); + } } \ No newline at end of file diff --git a/src/vs/workbench/parts/files/common/files.ts b/src/vs/workbench/parts/files/common/files.ts index d6e52ac5f55..928270ff16b 100644 --- a/src/vs/workbench/parts/files/common/files.ts +++ b/src/vs/workbench/parts/files/common/files.ts @@ -277,7 +277,7 @@ export interface IRawTextContent extends IBaseStat { export interface ITextFileEditorModelManager { - dispose(resource: URI): void; + disposeModel(resource: URI): void; get(resource: URI): ITextFileEditorModel; diff --git a/src/vs/workbench/parts/files/common/textFileService.ts b/src/vs/workbench/parts/files/common/textFileService.ts index 575d79d0f27..8420515a88d 100644 --- a/src/vs/workbench/parts/files/common/textFileService.ts +++ b/src/vs/workbench/parts/files/common/textFileService.ts @@ -25,6 +25,7 @@ import {IUntitledEditorService} from 'vs/workbench/services/untitled/common/unti import {UntitledEditorModel} from 'vs/workbench/common/editor/untitledEditorModel'; import {BinaryEditorModel} from 'vs/workbench/common/editor/binaryEditorModel'; import {TextFileEditorModelManager} from 'vs/workbench/parts/files/common/editors/textFileEditorModelManager'; +import {IInstantiationService} from 'vs/platform/instantiation/common/instantiation'; /** * The workbench file service implementation implements the raw file service spec and adds additional methods on top. @@ -51,11 +52,12 @@ export abstract class TextFileService implements ITextFileService { @IEditorGroupService private editorGroupService: IEditorGroupService, @IWorkbenchEditorService private editorService: IWorkbenchEditorService, @IFileService protected fileService: IFileService, - @IUntitledEditorService private untitledEditorService: IUntitledEditorService + @IUntitledEditorService private untitledEditorService: IUntitledEditorService, + @IInstantiationService private instantiationService: IInstantiationService ) { this.listenerToUnbind = []; this._onAutoSaveConfigurationChange = new Emitter(); - this._models = new TextFileEditorModelManager(); + this._models = this.instantiationService.createInstance(TextFileEditorModelManager); const configuration = this.configurationService.getConfiguration(); this.onConfigurationChange(configuration); @@ -481,7 +483,7 @@ export abstract class TextFileService implements ITextFileService { clients.forEach(input => input.dispose()); // Model - this._models.dispose(model.getResource()); + this._models.disposeModel(model.getResource()); // store as successful revert mapResourceToResult[model.getResource().toString()].success = true; diff --git a/src/vs/workbench/parts/files/electron-browser/textFileService.ts b/src/vs/workbench/parts/files/electron-browser/textFileService.ts index cee16cd1414..8fd9c242d3d 100644 --- a/src/vs/workbench/parts/files/electron-browser/textFileService.ts +++ b/src/vs/workbench/parts/files/electron-browser/textFileService.ts @@ -28,6 +28,7 @@ import {IModelService} from 'vs/editor/common/services/modelService'; import {ModelBuilder} from 'vs/editor/node/model/modelBuilder'; import product from 'vs/platform/product'; import {IEnvironmentService} from 'vs/platform/environment/common/environment'; +import {IInstantiationService} from 'vs/platform/instantiation/common/instantiation'; export class TextFileService extends AbstractTextFileService { @@ -38,6 +39,7 @@ export class TextFileService extends AbstractTextFileService { @IFileService fileService: IFileService, @IUntitledEditorService untitledEditorService: IUntitledEditorService, @ILifecycleService lifecycleService: ILifecycleService, + @IInstantiationService instantiationService: IInstantiationService, @ITelemetryService telemetryService: ITelemetryService, @IConfigurationService configurationService: IConfigurationService, @IModeService private modeService: IModeService, @@ -47,7 +49,7 @@ export class TextFileService extends AbstractTextFileService { @IModelService private modelService: IModelService, @IEnvironmentService private environmentService: IEnvironmentService ) { - super(lifecycleService, contextService, configurationService, telemetryService, editorGroupService, editorService, fileService, untitledEditorService); + super(lifecycleService, contextService, configurationService, telemetryService, editorGroupService, editorService, fileService, untitledEditorService, instantiationService); } public resolveTextContent(resource: URI, options?: IResolveContentOptions): TPromise { diff --git a/src/vs/workbench/parts/files/test/browser/textFileEditorModelManager.test.ts b/src/vs/workbench/parts/files/test/browser/textFileEditorModelManager.test.ts index a9eb75af0b2..3c872a9b890 100644 --- a/src/vs/workbench/parts/files/test/browser/textFileEditorModelManager.test.ts +++ b/src/vs/workbench/parts/files/test/browser/textFileEditorModelManager.test.ts @@ -7,13 +7,36 @@ import * as assert from 'assert'; import URI from 'vs/base/common/uri'; +import {TestInstantiationService} from 'vs/test/utils/instantiationTestUtils'; import {TextFileEditorModelManager} from 'vs/workbench/parts/files/common/editors/textFileEditorModelManager'; import {EditorModel} from 'vs/workbench/common/editor'; +import {join} from 'vs/base/common/paths'; +import {workbenchInstantiationService, TestEditorGroupService} from 'vs/test/utils/servicesTestUtils'; +import {IEditorGroupService} from 'vs/workbench/services/group/common/groupService'; +import {FileEditorInput} from 'vs/workbench/parts/files/common/editors/fileEditorInput'; +import {TextFileEditorModel} from 'vs/workbench/parts/files/common/editors/textFileEditorModel'; + +class ServiceAccessor { + constructor(@IEditorGroupService public editorGroupService: TestEditorGroupService) { + } +} + +function toResource(path) { + return URI.file(join('C:\\', path)); +} suite('Files - TextFileEditorModelManager', () => { - test('add, remove, clear', function () { - const manager = new TextFileEditorModelManager(); + let instantiationService: TestInstantiationService; + let accessor: ServiceAccessor; + + setup(() => { + instantiationService = workbenchInstantiationService(); + accessor = instantiationService.createInstance(ServiceAccessor); + }); + + test('add, remove, clear, get, getAll', function () { + const manager = instantiationService.createInstance(TextFileEditorModelManager); const model1 = new EditorModel(); const model2 = new EditorModel(); @@ -52,4 +75,46 @@ suite('Files - TextFileEditorModelManager', () => { result = manager.getAll(); assert.strictEqual(0, result.length); }); + + test('removed from cache when model disposed', function () { + const manager = instantiationService.createInstance(TextFileEditorModelManager); + + const model1 = new EditorModel(); + const model2 = new EditorModel(); + const model3 = new EditorModel(); + + manager.add(URI.file('/test.html'), model1); + manager.add(URI.file('/some/other.html'), model2); + manager.add(URI.file('/some/this.txt'), model3); + + assert.strictEqual(manager.get(URI.file('/test.html')), model1); + + model1.dispose(); + assert(!manager.get(URI.file('/test.html'))); + }); + + test('disposes model when not open anymore', function () { + const manager:TextFileEditorModelManager = instantiationService.createInstance(TextFileEditorModelManager); + + const resource = toResource('/path/index.txt'); + + const model:TextFileEditorModel = instantiationService.createInstance(TextFileEditorModel, resource, 'utf8'); + manager.add(resource, model); + + const input = instantiationService.createInstance(FileEditorInput, resource, 'text/plain', void 0); + + const stacks = accessor.editorGroupService.getStacksModel(); + const group = stacks.openGroup('group', true); + group.openEditor(input); + + accessor.editorGroupService.fireChange(); + + assert.ok(!model.isDisposed()); + + group.closeEditor(input); + accessor.editorGroupService.fireChange(); + assert.ok(model.isDisposed()); + + manager.dispose(); + }); }); \ No newline at end of file diff --git a/src/vs/workbench/services/configuration/test/node/configurationService.test.ts b/src/vs/workbench/services/configuration/test/node/configurationService.test.ts index 2ccbea49fdc..88b6bed4a9e 100644 --- a/src/vs/workbench/services/configuration/test/node/configurationService.test.ts +++ b/src/vs/workbench/services/configuration/test/node/configurationService.test.ts @@ -185,27 +185,6 @@ suite('WorkspaceConfigurationService - Node', () => { }); }); - test('global change triggers event', (done: () => void) => { - createWorkspace((workspaceDir, globalSettingsFile, cleanUp) => { - return createService(workspaceDir, globalSettingsFile).then(service => { - fs.writeFileSync(globalSettingsFile, '{ "testworkbench.editor.icons": false }'); - service.reloadConfiguration().then(() => { - service.onDidUpdateConfiguration(event => { - const config = service.getConfiguration<{ testworkbench: { editor: { icons: boolean } } }>(); - assert.equal(config.testworkbench.editor.icons, true); - assert.equal(event.config.testworkbench.editor.icons, true); - - service.dispose(); - - cleanUp(done); - }); - - fs.writeFileSync(globalSettingsFile, '{ "testworkbench.editor.icons": true }'); - }); - }); - }); - }); - test('workspace change triggers event', (done: () => void) => { createWorkspace((workspaceDir, globalSettingsFile, cleanUp) => { const workspaceContextService = new WorkspaceContextService({ resource: URI.file(workspaceDir) });