From fc9a929dd0f5ab5029c8c9a01e0e221892364a74 Mon Sep 17 00:00:00 2001 From: Sandeep Somavarapu Date: Mon, 15 May 2017 16:19:31 +0200 Subject: [PATCH] Fix #19161 --- src/vs/workbench/electron-browser/actions.ts | 2 +- .../node/configurationEditingService.ts | 63 +++++++++++++------ .../node/configurationEditingService.test.ts | 24 ++++++- 3 files changed, 67 insertions(+), 22 deletions(-) diff --git a/src/vs/workbench/electron-browser/actions.ts b/src/vs/workbench/electron-browser/actions.ts index c21c6700f29..b5b380afb6a 100644 --- a/src/vs/workbench/electron-browser/actions.ts +++ b/src/vs/workbench/electron-browser/actions.ts @@ -246,7 +246,7 @@ export abstract class BaseZoomAction extends Action { browser.setZoomLevel(webFrame.getZoomLevel(), /*isTrusted*/false); }; - this.configurationEditingService.writeConfiguration(target, { key: BaseZoomAction.SETTING_KEY, value: level }).done(() => applyZoom(), error => applyZoom()); + this.configurationEditingService.writeConfiguration(target, { key: BaseZoomAction.SETTING_KEY, value: level }, { donotNotifyError: true }).done(() => applyZoom(), error => applyZoom()); } } diff --git a/src/vs/workbench/services/configuration/node/configurationEditingService.ts b/src/vs/workbench/services/configuration/node/configurationEditingService.ts index 70d1426b1f5..a2ace0b1b2c 100644 --- a/src/vs/workbench/services/configuration/node/configurationEditingService.ts +++ b/src/vs/workbench/services/configuration/node/configurationEditingService.ts @@ -42,6 +42,10 @@ interface IValidationResult { exists?: boolean; } +interface ConfigurationEditingOptions extends IConfigurationEditingOptions { + force?: boolean; +} + export class ConfigurationEditingService implements IConfigurationEditingService { public _serviceBrand: any; @@ -66,16 +70,17 @@ export class ConfigurationEditingService implements IConfigurationEditingService return this.queue.queue(() => this.doWriteConfiguration(target, value, options) // queue up writes to prevent race conditions .then(() => null, error => { - return options.donotNotifyError ? TPromise.wrapError(error) : this.onError(error, target); + return options.donotNotifyError ? TPromise.wrapError(error) : this.onError(error, target, value); })); } - private doWriteConfiguration(target: ConfigurationTarget, value: IConfigurationValue, options: IConfigurationEditingOptions): TPromise { + private doWriteConfiguration(target: ConfigurationTarget, value: IConfigurationValue, options: ConfigurationEditingOptions): TPromise { const operation = this.getConfigurationEditOperation(target, value); - return this.resolveAndValidate(target, operation, !options.donotSave) - .then(reference => this.writeToBuffer(reference.object.textEditorModel, operation, !options.donotSave) - .then(() => reference.dispose())); + const checkDirtyConfiguration = !(options.force || options.donotSave); + const saveConfiguration = options.force || !options.donotSave; + return this.resolveAndValidate(target, operation, checkDirtyConfiguration) + .then(reference => this.writeToBuffer(reference.object.textEditorModel, operation, saveConfiguration)); } private writeToBuffer(model: editorCommon.IModel, operation: IConfigurationEditOperation, save: boolean): TPromise { @@ -101,17 +106,13 @@ export class ConfigurationEditingService implements IConfigurationEditingService return false; } - private onError(error: IConfigurationEditingError, target: ConfigurationTarget): TPromise { + private onError(error: IConfigurationEditingError, target: ConfigurationTarget, value: IConfigurationValue): TPromise { switch (error.code) { case ConfigurationEditingErrorCode.ERROR_INVALID_CONFIGURATION: + this.onInvalidConfigurationError(error, target); + break; case ConfigurationEditingErrorCode.ERROR_CONFIGURATION_FILE_DIRTY: - this.choiceService.choose(Severity.Error, error.message, [nls.localize('open', "Open Settings"), nls.localize('close', "Close")], 1) - .then(option => { - switch (option) { - case 0: - this.openSettings(target); - } - }); + this.onConfigurationFileDirtyError(error, target, value); break; default: this.messageService.show(Severity.Error, error.message); @@ -119,6 +120,30 @@ export class ConfigurationEditingService implements IConfigurationEditingService return TPromise.wrapError(error); } + private onInvalidConfigurationError(error: IConfigurationEditingError, target: ConfigurationTarget): void { + this.choiceService.choose(Severity.Error, error.message, [nls.localize('open', "Open Settings"), nls.localize('close', "Close")], 1) + .then(option => { + switch (option) { + case 0: + this.openSettings(target); + } + }); + } + + private onConfigurationFileDirtyError(error: IConfigurationEditingError, target: ConfigurationTarget, value: IConfigurationValue): void { + this.choiceService.choose(Severity.Error, error.message, [nls.localize('saveAndRetry', "Save Settings and Retry"), nls.localize('open', "Open Settings"), nls.localize('close', "Close")], 2) + .then(option => { + switch (option) { + case 0: + this.writeConfiguration(target, value, { force: true }); + break; + case 1: + this.openSettings(target); + break; + } + }); + } + private openSettings(target: ConfigurationTarget): void { this.commandService.executeCommand(ConfigurationTarget.USER === target ? 'workbench.action.openGlobalSettings' : 'workbench.action.openWorkspaceSettings'); } @@ -216,18 +241,18 @@ export class ConfigurationEditingService implements IConfigurationEditingService return this.wrapError(ConfigurationEditingErrorCode.ERROR_NO_WORKSPACE_OPENED, target); } - // Target cannot be dirty if not writing into buffer - const resource = operation.resource; - if (checkDirty && this.textFileService.isDirty(resource)) { - return this.wrapError(ConfigurationEditingErrorCode.ERROR_CONFIGURATION_FILE_DIRTY, target); - } - return this.resolveModelReference(operation.resource) .then(reference => { const model = reference.object.textEditorModel; + if (this.hasParseErrors(model, operation)) { return this.wrapError(ConfigurationEditingErrorCode.ERROR_INVALID_CONFIGURATION, target); } + + // Target cannot be dirty if not writing into buffer + if (checkDirty && this.textFileService.isDirty(operation.resource)) { + return this.wrapError(ConfigurationEditingErrorCode.ERROR_CONFIGURATION_FILE_DIRTY, target); + } return reference; }); } diff --git a/src/vs/workbench/services/configuration/test/node/configurationEditingService.test.ts b/src/vs/workbench/services/configuration/test/node/configurationEditingService.test.ts index 3a53e2a1de2..a444ccfb9cf 100644 --- a/src/vs/workbench/services/configuration/test/node/configurationEditingService.test.ts +++ b/src/vs/workbench/services/configuration/test/node/configurationEditingService.test.ts @@ -5,6 +5,7 @@ 'use strict'; +import * as sinon from 'sinon'; import assert = require('assert'); import os = require('os'); import path = require('path'); @@ -55,12 +56,13 @@ class SettingsTestEnvironmentService extends EnvironmentService { suite('ConfigurationEditingService', () => { - let instantiationService; + let instantiationService: TestInstantiationService; let testObject: ConfigurationEditingService; let parentDir; let workspaceDir; let globalSettingsFile; let workspaceSettingsDir; + let choiceService; suiteSetup(() => { const configurationRegistry = Registry.as(ConfigurationExtensions.Configuration); @@ -126,7 +128,7 @@ suite('ConfigurationEditingService', () => { instantiationService.stub(ITextFileService, instantiationService.createInstance(TestTextFileService)); instantiationService.stub(ITextModelResolverService, instantiationService.createInstance(TextModelResolverService)); instantiationService.stub(IBackupFileService, new TestBackupFileService()); - instantiationService.stub(IChoiceService, { + choiceService = instantiationService.stub(IChoiceService, { choose: (severity, message, options, cancelId): TPromise => { return TPromise.as(cancelId); } @@ -197,6 +199,24 @@ suite('ConfigurationEditingService', () => { (error: IConfigurationEditingError) => assert.equal(error.code, ConfigurationEditingErrorCode.ERROR_CONFIGURATION_FILE_DIRTY)); }); + test('dirty error is not thrown if not asked to save', () => { + instantiationService.stub(ITextFileService, 'isDirty', true); + return testObject.writeConfiguration(ConfigurationTarget.USER, { key: 'configurationEditing.service.testSetting', value: 'value' }, { donotSave: true }) + .then(() => null, error => assert.fail('Should not fail.')); + }); + + test('do not notify error', () => { + instantiationService.stub(ITextFileService, 'isDirty', true); + const target = sinon.stub(); + instantiationService.stubPromise(IChoiceService, 'choose', target); + return testObject.writeConfiguration(ConfigurationTarget.USER, { key: 'configurationEditing.service.testSetting', value: 'value' }, { donotNotifyError: true }) + .then(() => assert.fail('Should fail with ERROR_CONFIGURATION_FILE_DIRTY error.'), + (error: IConfigurationEditingError) => { + assert.equal(false, target.calledOnce); + assert.equal(error.code, ConfigurationEditingErrorCode.ERROR_CONFIGURATION_FILE_DIRTY); + }); + }); + test('write one setting - empty file', () => { return testObject.writeConfiguration(ConfigurationTarget.USER, { key: 'configurationEditing.service.testSetting', value: 'value' }) .then(() => { -- GitLab