Fix #19161

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());
 	}
 }
 

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<void> {
+	private doWriteConfiguration(target: ConfigurationTarget, value: IConfigurationValue, options: ConfigurationEditingOptions): TPromise<void> {
 		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<any> {
@@ -101,17 +106,13 @@ export class ConfigurationEditingService implements IConfigurationEditingService
 		return false;
 	}
 
-	private onError(error: IConfigurationEditingError, target: ConfigurationTarget): TPromise<IConfigurationEditingError> {
+	private onError(error: IConfigurationEditingError, target: ConfigurationTarget, value: IConfigurationValue): TPromise<IConfigurationEditingError> {
 		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, <ConfigurationEditingOptions>{ 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;
 			});
 	}

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 = <IConfigurationRegistry>Registry.as(ConfigurationExtensions.Configuration);
@@ -126,7 +128,7 @@ suite('ConfigurationEditingService', () => {
 		instantiationService.stub(ITextFileService, instantiationService.createInstance(TestTextFileService));
 		instantiationService.stub(ITextModelResolverService, <ITextModelResolverService>instantiationService.createInstance(TextModelResolverService));
 		instantiationService.stub(IBackupFileService, new TestBackupFileService());
-		instantiationService.stub(IChoiceService, {
+		choiceService = instantiationService.stub(IChoiceService, {
 			choose: (severity, message, options, cancelId): TPromise<number> => {
 				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(() => {