From 39f0d26deb29be4ee1b7a3d694870ebbfcbae498 Mon Sep 17 00:00:00 2001 From: Sandeep Somavarapu Date: Wed, 14 Dec 2022 14:51:09 +0100 Subject: [PATCH] Fix #121804 (#169128) --- .../configuration/common/configuration.ts | 13 ++++++++++- .../api/browser/mainThreadConfiguration.ts | 2 +- .../preferences/browser/settingsEditor2.ts | 2 +- .../browser/configurationService.ts | 12 +++++----- .../common/configurationEditing.ts | 20 +++++----------- .../configuration/common/jsonEditing.ts | 5 ---- .../common/jsonEditingService.ts | 23 ++++++------------- .../test/browser/configurationService.test.ts | 14 +++++------ .../abstractWorkspaceEditingService.ts | 8 ------- 9 files changed, 40 insertions(+), 59 deletions(-) diff --git a/src/vs/platform/configuration/common/configuration.ts b/src/vs/platform/configuration/common/configuration.ts index 0afd5a8707cd8..5b9046d746ebe 100644 --- a/src/vs/platform/configuration/common/configuration.ts +++ b/src/vs/platform/configuration/common/configuration.ts @@ -99,6 +99,17 @@ export interface IConfigurationValue { readonly overrideIdentifiers?: string[]; } +export interface IConfigurationUpdateOptions { + /** + * If `true`, do not notifies the error to user by showing the message box. Default is `false`. + */ + donotNotifyError?: boolean; + /** + * How to handle dirty file when updating the configuration. + */ + handleDirtyFile?: 'save' | 'revert'; +} + export interface IConfigurationService { readonly _serviceBrand: undefined; @@ -140,7 +151,7 @@ export interface IConfigurationService { updateValue(key: string, value: any): Promise; updateValue(key: string, value: any, target: ConfigurationTarget): Promise; updateValue(key: string, value: any, overrides: IConfigurationOverrides | IConfigurationUpdateOverrides): Promise; - updateValue(key: string, value: any, overrides: IConfigurationOverrides | IConfigurationUpdateOverrides, target: ConfigurationTarget, donotNotifyError?: boolean): Promise; + updateValue(key: string, value: any, overrides: IConfigurationOverrides | IConfigurationUpdateOverrides, target: ConfigurationTarget, options?: IConfigurationUpdateOptions): Promise; inspect(key: string, overrides?: IConfigurationOverrides): IConfigurationValue>; diff --git a/src/vs/workbench/api/browser/mainThreadConfiguration.ts b/src/vs/workbench/api/browser/mainThreadConfiguration.ts index de0630df1a180..deb82c813659d 100644 --- a/src/vs/workbench/api/browser/mainThreadConfiguration.ts +++ b/src/vs/workbench/api/browser/mainThreadConfiguration.ts @@ -77,7 +77,7 @@ export class MainThreadConfiguration implements MainThreadConfigurationShape { : scopeToLanguage === false ? { resource: overrides.resource } : overrides.overrideIdentifier && overriddenValue !== undefined ? overrides : { resource: overrides.resource }; - return this.configurationService.updateValue(key, value, overrides, configurationTarget, true); + return this.configurationService.updateValue(key, value, overrides, configurationTarget, { donotNotifyError: true }); } private deriveConfigurationTarget(key: string, overrides: IConfigurationOverrides): ConfigurationTarget { diff --git a/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts b/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts index 8df8742a5f9fa..57cfac5b1701e 100644 --- a/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts +++ b/src/vs/workbench/contrib/preferences/browser/settingsEditor2.ts @@ -1078,7 +1078,7 @@ export class SettingsEditor2 extends EditorPane { value = undefined; } - return this.configurationService.updateValue(key, value, overrides, configurationTarget) + return this.configurationService.updateValue(key, value, overrides, configurationTarget, { handleDirtyFile: 'save' }) .then(() => { const query = this.searchWidget.getValue(); if (query.includes(`@${MODIFIED_SETTING_TAG}`)) { diff --git a/src/vs/workbench/services/configuration/browser/configurationService.ts b/src/vs/workbench/services/configuration/browser/configurationService.ts index 3ae0db4640b12..2bfa1e5fa5abc 100644 --- a/src/vs/workbench/services/configuration/browser/configurationService.ts +++ b/src/vs/workbench/services/configuration/browser/configurationService.ts @@ -12,7 +12,7 @@ import { Queue, Barrier, runWhenIdle, Promises } from 'vs/base/common/async'; import { IJSONContributionRegistry, Extensions as JSONExtensions } from 'vs/platform/jsonschemas/common/jsonContributionRegistry'; import { IWorkspaceContextService, Workspace as BaseWorkspace, WorkbenchState, IWorkspaceFolder, IWorkspaceFoldersChangeEvent, WorkspaceFolder, toWorkspaceFolder, isWorkspaceFolder, IWorkspaceFoldersWillChangeEvent, IEmptyWorkspaceIdentifier, ISingleFolderWorkspaceIdentifier, isSingleFolderWorkspaceIdentifier, isWorkspaceIdentifier, IWorkspaceIdentifier, IAnyWorkspaceIdentifier } from 'vs/platform/workspace/common/workspace'; import { ConfigurationModel, ConfigurationChangeEvent, mergeChanges } from 'vs/platform/configuration/common/configurationModels'; -import { IConfigurationChangeEvent, ConfigurationTarget, IConfigurationOverrides, isConfigurationOverrides, IConfigurationData, IConfigurationValue, IConfigurationChange, ConfigurationTargetToString, IConfigurationUpdateOverrides, isConfigurationUpdateOverrides, IConfigurationService } from 'vs/platform/configuration/common/configuration'; +import { IConfigurationChangeEvent, ConfigurationTarget, IConfigurationOverrides, isConfigurationOverrides, IConfigurationData, IConfigurationValue, IConfigurationChange, ConfigurationTargetToString, IConfigurationUpdateOverrides, isConfigurationUpdateOverrides, IConfigurationService, IConfigurationUpdateOptions } from 'vs/platform/configuration/common/configuration'; import { IPolicyConfiguration, NullPolicyConfiguration, PolicyConfiguration } from 'vs/platform/configuration/common/configurations'; import { Configuration } from 'vs/workbench/services/configuration/common/configurationModels'; import { FOLDER_CONFIG_FOLDER_NAME, defaultSettingsSchemaId, userSettingsSchemaId, workspaceSettingsSchemaId, folderSettingsSchemaId, IConfigurationCache, machineSettingsSchemaId, LOCAL_MACHINE_SCOPES, IWorkbenchConfigurationService, RestrictedSettings, PROFILE_SCOPES, LOCAL_MACHINE_PROFILE_SCOPES, profileSettingsSchemaId } from 'vs/workbench/services/configuration/common/configuration'; @@ -330,8 +330,8 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat updateValue(key: string, value: any): Promise; updateValue(key: string, value: any, overrides: IConfigurationOverrides | IConfigurationUpdateOverrides): Promise; updateValue(key: string, value: any, target: ConfigurationTarget): Promise; - updateValue(key: string, value: any, overrides: IConfigurationOverrides | IConfigurationUpdateOverrides, target: ConfigurationTarget, donotNotifyError?: boolean): Promise; - async updateValue(key: string, value: any, arg3?: any, arg4?: any, donotNotifyError?: any): Promise { + updateValue(key: string, value: any, overrides: IConfigurationOverrides | IConfigurationUpdateOverrides, target: ConfigurationTarget, options?: IConfigurationUpdateOptions): Promise; + async updateValue(key: string, value: any, arg3?: any, arg4?: any, options?: any): Promise { const overrides: IConfigurationUpdateOverrides | undefined = isConfigurationUpdateOverrides(arg3) ? arg3 : isConfigurationOverrides(arg3) ? { resource: arg3.resource, overrideIdentifiers: arg3.overrideIdentifier ? [arg3.overrideIdentifier] : undefined } : undefined; const target: ConfigurationTarget | undefined = overrides ? arg4 : arg3; @@ -355,7 +355,7 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat } } - await Promises.settled(targets.map(target => this.writeConfigurationValue(key, value, target, overrides, donotNotifyError))); + await Promises.settled(targets.map(target => this.writeConfigurationValue(key, value, target, overrides, options))); } async reloadConfiguration(target?: ConfigurationTarget | IWorkspaceFolder): Promise { @@ -971,7 +971,7 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat return validWorkspaceFolders; } - private async writeConfigurationValue(key: string, value: any, target: ConfigurationTarget, overrides: IConfigurationUpdateOverrides | undefined, donotNotifyError: boolean): Promise { + private async writeConfigurationValue(key: string, value: any, target: ConfigurationTarget, overrides: IConfigurationUpdateOverrides | undefined, options?: IConfigurationUpdateOverrides): Promise { if (!this.instantiationService) { throw new Error('Cannot write configuration because the configuration service is not yet ready to accept writes.'); } @@ -998,7 +998,7 @@ export class WorkspaceService extends Disposable implements IWorkbenchConfigurat // Use same instance of ConfigurationEditing to make sure all writes go through the same queue this.configurationEditing = this.configurationEditing ?? this.instantiationService.createInstance(ConfigurationEditing, (await this.remoteAgentService.getEnvironment())?.settingsPath ?? null); - await this.configurationEditing.writeConfiguration(editableConfigurationTarget, { key, value }, { scopes: overrides, donotNotifyError }); + await this.configurationEditing.writeConfiguration(editableConfigurationTarget, { key, value }, { scopes: overrides, ...options }); switch (editableConfigurationTarget) { case EditableConfigurationTarget.USER_LOCAL: if (this.applicationConfiguration && this.configurationRegistry.getConfigurationProperties()[key].scope === ConfigurationScope.APPLICATION) { diff --git a/src/vs/workbench/services/configuration/common/configurationEditing.ts b/src/vs/workbench/services/configuration/common/configurationEditing.ts index 73ca3d0bc65e2..33919697053ac 100644 --- a/src/vs/workbench/services/configuration/common/configurationEditing.ts +++ b/src/vs/workbench/services/configuration/common/configurationEditing.ts @@ -12,7 +12,7 @@ import { Edit, FormattingOptions } from 'vs/base/common/jsonFormatter'; import { Registry } from 'vs/platform/registry/common/platform'; import { IWorkspaceContextService, WorkbenchState } from 'vs/platform/workspace/common/workspace'; import { ITextFileService } from 'vs/workbench/services/textfile/common/textfiles'; -import { IConfigurationService, IConfigurationUpdateOverrides } from 'vs/platform/configuration/common/configuration'; +import { IConfigurationService, IConfigurationUpdateOptions, IConfigurationUpdateOverrides } from 'vs/platform/configuration/common/configuration'; import { FOLDER_SETTINGS_PATH, WORKSPACE_STANDALONE_CONFIGURATIONS, TASKS_CONFIGURATION_KEY, LAUNCH_CONFIGURATION_KEY, USER_STANDALONE_CONFIGURATIONS, TASKS_DEFAULT, FOLDER_SCOPES } from 'vs/workbench/services/configuration/common/configuration'; import { FileOperationError, FileOperationResult, IFileService } from 'vs/platform/files/common/files'; import { IResolvedTextEditorModel, ITextModelService } from 'vs/editor/common/services/resolverService'; @@ -114,11 +114,7 @@ export interface IConfigurationValue { value: any; } -export interface IConfigurationEditingOptions { - /** - * If `true`, do not notifies the error to user by showing the message box. Default is `false`. - */ - donotNotifyError?: boolean; +export interface IConfigurationEditingOptions extends IConfigurationUpdateOptions { /** * Scope of configuration to be written into. */ @@ -139,10 +135,6 @@ interface IConfigurationEditOperation extends IConfigurationValue { workspaceStandAloneConfigurationKey?: string; } -interface ConfigurationEditingOptions extends IConfigurationEditingOptions { - handleDirtyFile?: 'save' | 'revert'; -} - export class ConfigurationEditing { public _serviceBrand: undefined; @@ -181,7 +173,7 @@ export class ConfigurationEditing { }); } - private async doWriteConfiguration(operation: IConfigurationEditOperation, options: ConfigurationEditingOptions): Promise { + private async doWriteConfiguration(operation: IConfigurationEditOperation, options: IConfigurationEditingOptions): Promise { await this.validate(operation.target, operation, !options.handleDirtyFile, options.scopes || {}); const resource: URI = operation.resource!; const reference = await this.resolveModelReference(resource); @@ -193,7 +185,7 @@ export class ConfigurationEditing { } } - private async updateConfiguration(operation: IConfigurationEditOperation, model: ITextModel, formattingOptions: FormattingOptions, options: ConfigurationEditingOptions): Promise { + private async updateConfiguration(operation: IConfigurationEditOperation, model: ITextModel, formattingOptions: FormattingOptions, options: IConfigurationEditingOptions): Promise { if (this.hasParseErrors(model.getValue(), operation)) { throw this.toConfigurationEditingError(ConfigurationEditingErrorCode.ERROR_INVALID_CONFIGURATION, operation.target, operation); } @@ -301,7 +293,7 @@ export class ConfigurationEditing { label: nls.localize('saveAndRetry', "Save and Retry"), run: () => { const key = operation.key ? `${operation.workspaceStandAloneConfigurationKey}.${operation.key}` : operation.workspaceStandAloneConfigurationKey!; - this.writeConfiguration(operation.target, { key, value: operation.value }, { handleDirtyFile: 'save', scopes }); + this.writeConfiguration(operation.target, { key, value: operation.value }, { handleDirtyFile: 'save', scopes }); } }, { @@ -313,7 +305,7 @@ export class ConfigurationEditing { this.notificationService.prompt(Severity.Error, error.message, [{ label: nls.localize('saveAndRetry', "Save and Retry"), - run: () => this.writeConfiguration(operation.target, { key: operation.key, value: operation.value }, { handleDirtyFile: 'save', scopes }) + run: () => this.writeConfiguration(operation.target, { key: operation.key, value: operation.value }, { handleDirtyFile: 'save', scopes }) }, { label: nls.localize('open', "Open Settings"), diff --git a/src/vs/workbench/services/configuration/common/jsonEditing.ts b/src/vs/workbench/services/configuration/common/jsonEditing.ts index b5dc176eb0a6c..ea4e426b1c4a1 100644 --- a/src/vs/workbench/services/configuration/common/jsonEditing.ts +++ b/src/vs/workbench/services/configuration/common/jsonEditing.ts @@ -11,11 +11,6 @@ export const IJSONEditingService = createDecorator('jsonEdi export const enum JSONEditingErrorCode { - /** - * Error when trying to write and save to the file while it is dirty in the editor. - */ - ERROR_FILE_DIRTY, - /** * Error when trying to write to a file that contains JSON errors. */ diff --git a/src/vs/workbench/services/configuration/common/jsonEditingService.ts b/src/vs/workbench/services/configuration/common/jsonEditingService.ts index e906a376240e8..31ca52ef0f0e5 100644 --- a/src/vs/workbench/services/configuration/common/jsonEditingService.ts +++ b/src/vs/workbench/services/configuration/common/jsonEditingService.ts @@ -34,26 +34,26 @@ export class JSONEditingService implements IJSONEditingService { this.queue = new Queue(); } - write(resource: URI, values: IJSONValue[], save: boolean): Promise { - return Promise.resolve(this.queue.queue(() => this.doWriteConfiguration(resource, values, save))); // queue up writes to prevent race conditions + write(resource: URI, values: IJSONValue[]): Promise { + return Promise.resolve(this.queue.queue(() => this.doWriteConfiguration(resource, values))); // queue up writes to prevent race conditions } - private async doWriteConfiguration(resource: URI, values: IJSONValue[], save: boolean): Promise { - const reference = await this.resolveAndValidate(resource, save); + private async doWriteConfiguration(resource: URI, values: IJSONValue[]): Promise { + const reference = await this.resolveAndValidate(resource, true); try { - await this.writeToBuffer(reference.object.textEditorModel, values, save); + await this.writeToBuffer(reference.object.textEditorModel, values); } finally { reference.dispose(); } } - private async writeToBuffer(model: ITextModel, values: IJSONValue[], save: boolean): Promise { + private async writeToBuffer(model: ITextModel, values: IJSONValue[]): Promise { let hasEdits: boolean = false; for (const value of values) { const edit = this.getEdits(model, value)[0]; hasEdits = this.applyEditsToBuffer(edit, model); } - if (hasEdits && save) { + if (hasEdits) { return this.textFileService.save(model.uri); } } @@ -113,12 +113,6 @@ export class JSONEditingService implements IJSONEditingService { return this.reject>(JSONEditingErrorCode.ERROR_INVALID_FILE); } - // Target cannot be dirty if not writing into buffer - if (checkDirty && this.textFileService.isDirty(resource)) { - reference.dispose(); - return this.reject>(JSONEditingErrorCode.ERROR_FILE_DIRTY); - } - return reference; } @@ -133,9 +127,6 @@ export class JSONEditingService implements IJSONEditingService { case JSONEditingErrorCode.ERROR_INVALID_FILE: { return nls.localize('errorInvalidFile', "Unable to write into the file. Please open the file to correct errors/warnings in the file and try again."); } - case JSONEditingErrorCode.ERROR_FILE_DIRTY: { - return nls.localize('errorFileDirty', "Unable to write into the file because the file has unsaved changes. Please save the file and try again."); - } } } } diff --git a/src/vs/workbench/services/configuration/test/browser/configurationService.test.ts b/src/vs/workbench/services/configuration/test/browser/configurationService.test.ts index c37183e8f48bc..3e65b199b551f 100644 --- a/src/vs/workbench/services/configuration/test/browser/configurationService.test.ts +++ b/src/vs/workbench/services/configuration/test/browser/configurationService.test.ts @@ -1277,12 +1277,12 @@ suite('WorkspaceConfigurationService - Folder', () => { })); test('update application setting into workspace configuration in a workspace is not supported', () => { - return testObject.updateValue('configurationService.folder.applicationSetting', 'workspaceValue', {}, ConfigurationTarget.WORKSPACE, true) + return testObject.updateValue('configurationService.folder.applicationSetting', 'workspaceValue', {}, ConfigurationTarget.WORKSPACE, { donotNotifyError: true }) .then(() => assert.fail('Should not be supported'), (e) => assert.strictEqual(e.code, ConfigurationEditingErrorCode.ERROR_INVALID_WORKSPACE_CONFIGURATION_APPLICATION)); }); test('update machine setting into workspace configuration in a workspace is not supported', () => { - return testObject.updateValue('configurationService.folder.machineSetting', 'workspaceValue', {}, ConfigurationTarget.WORKSPACE, true) + return testObject.updateValue('configurationService.folder.machineSetting', 'workspaceValue', {}, ConfigurationTarget.WORKSPACE, { donotNotifyError: true }) .then(() => assert.fail('Should not be supported'), (e) => assert.strictEqual(e.code, ConfigurationEditingErrorCode.ERROR_INVALID_WORKSPACE_CONFIGURATION_MACHINE)); }); @@ -1949,7 +1949,7 @@ suite('WorkspaceConfigurationService-Multiroot', () => { assert.strictEqual(testObject.getValue('configurationService.workspace.testNewApplicationSetting', { resource: workspaceContextService.getWorkspace().folders[0].uri }), 'userValue'); })); - test('application settings are not read from workspace folder after defaults are registered', () => runWithFakedTimers({ useFakeTimers: true }, async () => { + test('machine settings are not read from workspace folder after defaults are registered', () => runWithFakedTimers({ useFakeTimers: true }, async () => { await fileService.writeFile(userDataProfileService.currentProfile.settingsResource, VSBuffer.fromString('{ "configurationService.workspace.testNewMachineSetting": "userValue" }')); await fileService.writeFile(workspaceContextService.getWorkspace().folders[0].toResource('.vscode/settings.json'), VSBuffer.fromString('{ "configurationService.workspace.testNewMachineSetting": "workspaceFolderValue" }')); await testObject.reloadConfiguration(); @@ -2261,12 +2261,12 @@ suite('WorkspaceConfigurationService-Multiroot', () => { })); test('update application setting into workspace configuration in a workspace is not supported', () => { - return testObject.updateValue('configurationService.workspace.applicationSetting', 'workspaceValue', {}, ConfigurationTarget.WORKSPACE, true) + return testObject.updateValue('configurationService.workspace.applicationSetting', 'workspaceValue', {}, ConfigurationTarget.WORKSPACE, { donotNotifyError: true }) .then(() => assert.fail('Should not be supported'), (e) => assert.strictEqual(e.code, ConfigurationEditingErrorCode.ERROR_INVALID_WORKSPACE_CONFIGURATION_APPLICATION)); }); test('update machine setting into workspace configuration in a workspace is not supported', () => { - return testObject.updateValue('configurationService.workspace.machineSetting', 'workspaceValue', {}, ConfigurationTarget.WORKSPACE, true) + return testObject.updateValue('configurationService.workspace.machineSetting', 'workspaceValue', {}, ConfigurationTarget.WORKSPACE, { donotNotifyError: true }) .then(() => assert.fail('Should not be supported'), (e) => assert.strictEqual(e.code, ConfigurationEditingErrorCode.ERROR_INVALID_WORKSPACE_CONFIGURATION_MACHINE)); }); @@ -2341,14 +2341,14 @@ suite('WorkspaceConfigurationService-Multiroot', () => { test('update launch configuration in a workspace', () => runWithFakedTimers({ useFakeTimers: true }, async () => { const workspace = workspaceContextService.getWorkspace(); - await testObject.updateValue('launch', { 'version': '1.0.0', configurations: [{ 'name': 'myLaunch' }] }, { resource: workspace.folders[0].uri }, ConfigurationTarget.WORKSPACE, true); + await testObject.updateValue('launch', { 'version': '1.0.0', configurations: [{ 'name': 'myLaunch' }] }, { resource: workspace.folders[0].uri }, ConfigurationTarget.WORKSPACE, { donotNotifyError: true }); assert.deepStrictEqual(testObject.getValue('launch'), { 'version': '1.0.0', configurations: [{ 'name': 'myLaunch' }] }); })); test('update tasks configuration in a workspace', () => runWithFakedTimers({ useFakeTimers: true }, async () => { const workspace = workspaceContextService.getWorkspace(); const tasks = { 'version': '2.0.0', tasks: [{ 'label': 'myTask' }] }; - await testObject.updateValue('tasks', tasks, { resource: workspace.folders[0].uri }, ConfigurationTarget.WORKSPACE, true); + await testObject.updateValue('tasks', tasks, { resource: workspace.folders[0].uri }, ConfigurationTarget.WORKSPACE, { donotNotifyError: true }); assert.deepStrictEqual(testObject.getValue(TasksSchemaProperties.Tasks), tasks); })); diff --git a/src/vs/workbench/services/workspaces/browser/abstractWorkspaceEditingService.ts b/src/vs/workbench/services/workspaces/browser/abstractWorkspaceEditingService.ts index 47a2646ef4fd0..c20195d6b061d 100644 --- a/src/vs/workbench/services/workspaces/browser/abstractWorkspaceEditingService.ts +++ b/src/vs/workbench/services/workspaces/browser/abstractWorkspaceEditingService.ts @@ -331,9 +331,6 @@ export abstract class AbstractWorkspaceEditingService implements IWorkspaceEditi case JSONEditingErrorCode.ERROR_INVALID_FILE: this.onInvalidWorkspaceConfigurationFileError(); break; - case JSONEditingErrorCode.ERROR_FILE_DIRTY: - this.onWorkspaceConfigurationFileDirtyError(); - break; default: this.notificationService.error(error.message); } @@ -344,11 +341,6 @@ export abstract class AbstractWorkspaceEditingService implements IWorkspaceEditi this.askToOpenWorkspaceConfigurationFile(message); } - private onWorkspaceConfigurationFileDirtyError(): void { - const message = localize('errorWorkspaceConfigurationFileDirty', "Unable to write into workspace configuration file because the file has unsaved changes. Please save it and try again."); - this.askToOpenWorkspaceConfigurationFile(message); - } - private askToOpenWorkspaceConfigurationFile(message: string): void { this.notificationService.prompt(Severity.Error, message, [{